diff --git a/Gemfile b/Gemfile index bc052b8d6c..62415219ca 100644 --- a/Gemfile +++ b/Gemfile @@ -68,6 +68,7 @@ gem 'gibbon', '1.1.4' gem 'virtus', '1.0.5' gem 'uuidtools', '2.1.5' gem 'cartodb-common', git: 'https://github.com/cartodb/cartodb-common.git', branch: 'master' +gem 'email_address', '~> 0.1.11' # Markdown gem 'redcarpet', '3.3.3' diff --git a/Gemfile.lock b/Gemfile.lock index 91a4a4520c..e31812d202 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -135,6 +135,9 @@ GEM faraday (~> 0.9, ~> 0.8) oauth2 (~> 1.1) ejs (1.1.1) + email_address (0.1.11) + netaddr (~> 2.0) + simpleidn equalizer (0.0.11) erubis (2.7.0) ethon (0.8.1) @@ -230,6 +233,7 @@ GEM multipart-post (2.0.0) net-ldap (0.16.0) net-telnet (0.1.1) + netaddr (2.0.3) nokogiri (1.8.5) mini_portile2 (~> 2.3.0) oauth (0.4.7) @@ -367,6 +371,8 @@ GEM simplecov simplecov-rcov (0.2.3) simplecov (>= 0.4.1) + simpleidn (0.1.1) + unf (~> 0.1.4) sinatra (1.4.6) rack (~> 1.4) rack-protection (~> 1.4) @@ -403,6 +409,9 @@ GEM tzinfo (1.2.5) thread_safe (~> 0.1) uber (0.1.0) + unf (0.1.4) + unf_ext + unf_ext (0.0.7.6) unicorn (4.8.2) kgio (~> 2.6) rack @@ -448,6 +457,7 @@ DEPENDENCIES delorean dropbox_api (= 0.1.6) ejs (~> 1.1.1) + email_address (~> 0.1.11) execjs (~> 0.4) factory_girl_rails (~> 4.0.0) fake_net_ldap! diff --git a/NEWS.md b/NEWS.md index 19994f9535..8a7d9721ea 100644 --- a/NEWS.md +++ b/NEWS.md @@ -14,6 +14,7 @@ sudo make install - Add number of employees and use case to user profile ([#14966](https://github.com/CartoDB/cartodb/pull/14966)) ### Bug fixes / enhancements +- Stricter email domain validation ([#15030](https://github.com/CartoDB/cartodb/pull/15030)) - Add more columns to oauth_app ([#15015](https://github.com/CartoDB/cartodb/issues/15015)) - Redirect viewer users to shared visualizations page, and show shared visualizations in Home ([CartoDB/support#2032](https://github.com/CartoDB/support/issues/2032)) - Fix user presenter ([#15033](https://github.com/CartoDB/cartodb/pull/15033)) diff --git a/app/models/user.rb b/app/models/user.rb index 93dd63f322..353cee2f5d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,6 +1,7 @@ # encoding: UTF-8 require 'cartodb/per_request_sequel_cache' require 'cartodb-common' +require 'email_address' require_relative './user/user_decorator' require_relative './user/oauths' require_relative './synchronization/synchronization_oauth' @@ -159,6 +160,14 @@ class User < Sequel::Model ## Validations def validate super + validate_username + validate_email + validate_password + validate_organization + validate_quotas + end + + def validate_username validates_presence :username validates_unique :username validates_format /\A[a-z0-9\-]+\z/, :username, message: "must only contain lowercase letters, numbers and the dash (-) symbol" @@ -166,24 +175,32 @@ class User < Sequel::Model validates_format /[a-z0-9]{1}\z/, :username, message: "must end with alphanumeric chars" validates_max_length 63, :username errors.add(:name, 'is taken') if name_exists_in_organizations? + end + def validate_email validates_presence :email - validates_unique :email, :message => 'is already taken' - validates_format EmailAddressValidator::Regexp::ADDR_SPEC, :email, :message => 'is not a valid address' + validates_unique :email, message: 'is already taken' + errors.add(:email, EmailAddress.error(email)) unless EmailAddress.valid?(email) + end + def validate_password validates_presence :password if new? && crypted_password.blank? if new? || (password.present? && !@new_password.present?) errors.add(:password, "is not confirmed") unless password == password_confirmation end validate_password_change + end + def validate_organization if organization.present? organization_validation elsif org_admin errors.add(:org_admin, "cannot be set for non-organization user") end + end + def validate_quotas errors.add(:geocoding_quota, "cannot be nil") if geocoding_quota.nil? errors.add(:here_isolines_quota, "cannot be nil") if here_isolines_quota.nil? errors.add(:obs_snapshot_quota, "cannot be nil") if obs_snapshot_quota.nil? diff --git a/config/initializers/email_address.rb b/config/initializers/email_address.rb new file mode 100644 index 0000000000..90e4ab1b0e --- /dev/null +++ b/config/initializers/email_address.rb @@ -0,0 +1,21 @@ +EmailAddress::Config.configure(local_format: :conventional) + +class EmailValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + return unless value + + if value.is_a?(Array) + value.each { |v| validate_value(record, attribute, v) } + else + validate_value(record, attribute, value) + end + end + + private + + def validate_value(record, attribute, value) + unless EmailAddress.valid?(value) + record.errors[attribute] << (options[:message] || "#{value} is not a valid email") + end + end +end diff --git a/config/initializers/rfc822.rb b/config/initializers/rfc822.rb deleted file mode 100644 index 5512cdf9f6..0000000000 --- a/config/initializers/rfc822.rb +++ /dev/null @@ -1,44 +0,0 @@ -# coding: UTF-8 - -module EmailAddressValidator - module Regexp - ATEXT = /[A-Za-z0-9!#\$%&'\*\+\-\/=\?\^_`\{\|\}\~]/ - DOT_ATOM = /(?:#{ATEXT})+(?:\.(?:#{ATEXT})+)*/ - - TEXT = /[\x01-\x09\x0B\x0C\x0E-\x7F]/ - QTEXT = /[\x01-\x08\x0B\x0C\x0E-\x1F\x21\x23-\x5B\x5D-\x7E]/ - QUOTED_PAIR = /\\#{TEXT}/ - QCONTENT = /(?:#{QTEXT}|#{QUOTED_PAIR})/ - QUOTED_STRING = /"(?:\s*#{QCONTENT})*\s*"/ - - DTEXT = /[\x01-\x08\x0B\x0C\x0E-\x1F\x21-\x5A\x5E-\x7E]/ - DCONTENT = /(?:#{DTEXT}|#{QUOTED_PAIR})/ - DOMAIN_LITERAL = /\[(?:\s*#{DCONTENT})*\s*\]/ - DOMAIN = /(?:#{DOT_ATOM}|#{DOMAIN_LITERAL})/ - - LOCAL_PART = /(?:#{DOT_ATOM}|#{QUOTED_STRING})/ - - ADDR_SPEC = /^(#{LOCAL_PART})@(#{DOMAIN})$/ - end -end - -# Taken from Rails examples, replacing with ADDR_SPEC -# the original regexp: /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i -class EmailValidator < ActiveModel::EachValidator - def validate_each(record, attribute, value) - return unless value - if value.is_a?(Array) - value.each { |v| validate_value(record, attribute, v) } - else - validate_value(record, attribute, value) - end - end - - private - - def validate_value(record, attribute, value) - unless value =~ EmailAddressValidator::Regexp::ADDR_SPEC - record.errors[attribute] << (options[:message] || "#{value} is not an email") - end - end -end diff --git a/lib/sql b/lib/sql index 32db4fd81e..afa52aa92b 160000 --- a/lib/sql +++ b/lib/sql @@ -1 +1 @@ -Subproject commit 32db4fd81efcba0be0686a0bf67f8cdb62f1133c +Subproject commit afa52aa92b1c27f6aaf12565fcbe5f79faae9917 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 14eba85364..8151f25377 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -433,6 +433,34 @@ describe User do user.dashboard_viewed?.should be_true end + describe "email validation" do + before(:all) do + EmailAddress::Config.configure(local_format: :conventional, host_validation: :mx) + end + + after(:all) do + EmailAddress::Config.configure(local_format: :conventional, host_validation: :syntax) + end + + it "disallows wrong domains" do + invalid_emails = ['pimpam@example.com', + 'pimpam@ageval.dr', + 'pimpam@qq.ocm', + 'pimpam@aa.ww', + 'pimpam@iu.eduy', + 'pimpam@gmail.como', + 'pimpam@namr.cim', + 'pimpam@buffalo.edi'] + + invalid_emails.each do |email| + user = ::User.new(email: email) + + user.valid?.should be_false + user.errors.should include :email + end + end + end + it "should validate that password is present if record is new and crypted_password is blank" do user = ::User.new user.username = "adminipop" diff --git a/spec/requests/signup_controller_spec.rb b/spec/requests/signup_controller_spec.rb index e8b1a6321f..37571f023b 100644 --- a/spec/requests/signup_controller_spec.rb +++ b/spec/requests/signup_controller_spec.rb @@ -152,7 +152,6 @@ describe SignupController do DEFAULT_QUOTA_IN_BYTES = 1000 before(:all) do - @organization.whitelisted_email_domains = ['carto.com'] @organization.default_quota_in_bytes = DEFAULT_QUOTA_IN_BYTES @organization.save @@ -160,6 +159,7 @@ describe SignupController do end before(:each) do + @organization.whitelisted_email_domains = ['carto.com'] @organization.auth_username_password_enabled = true @organization.auth_google_enabled = true @organization.strong_passwords_enabled = true diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index d9fedc6fc8..0894b933f7 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -21,6 +21,10 @@ Dir[Rails.root.join('spec/support/**/*.rb')].each { |f| require f } # Inline Resque for queue handling Resque.inline = true +# host_validation is set to support `example.com` emails in specs +# in production we do check for the existance of mx records associated to the domain +EmailAddress::Config.configure(local_format: :conventional, host_validation: :syntax) + RSpec.configure do |config| config.include SpecHelperHelpers config.include CartoDB::Factories