Move email cleaning to authentication level

app/controllers/sessions_controller.rb

@@ -4,12 +4,14 @@ require_dependency 'google_plus_api'
 require_dependency 'oauth/github/config'
 require_dependency 'carto/saml_service'
 require_dependency 'carto/username_proposer'
+require_dependency 'carto/email_cleaner'
 
 require_relative '../../lib/user_account_creator'
 require_relative '../../lib/cartodb/stats/authentication'
 
 class SessionsController < ApplicationController
   include LoginHelper
+  include Carto::EmailCleaner
 
   layout 'frontend'
   ssl_required :new, :create, :destroy, :show, :unauthenticated, :account_token_authentication_error,
@@ -206,7 +208,7 @@ class SessionsController < ApplicationController
   end
 
   def username_from_user_by_email(email)
-    ::User.where(email: email).first.try(:username)
+    ::User.where(email: clean_email(email)).first.try(:username)
   end
 
   def ldap_strategy_username

app/models/user.rb

@@ -21,6 +21,7 @@ require_dependency 'carto/helpers/has_connector_configuration'
 require_dependency 'carto/helpers/batch_queries_statement_timeout'
 require_dependency 'carto/user_authenticator'
 require_dependency 'carto/helpers/billing_cycle'
+require_dependency 'carto/email_cleaner'
 require_dependency 'carto/visualization'
 
 class User < Sequel::Model
@@ -33,6 +34,7 @@ class User < Sequel::Model
   include Carto::HasConnectorConfiguration
   include Carto::BatchQueriesStatementTimeout
   include Carto::BillingCycle
+  include Carto::EmailCleaner
   extend Carto::UserAuthenticator
 
   self.strict_param_setting = false
@@ -228,7 +230,7 @@ class User < Sequel::Model
 
   ## Callbacks
   def before_validation
-    self.email = self.email.to_s.strip.downcase
+    self.email = clean_email(email)
     self.geocoding_quota ||= DEFAULT_GEOCODING_QUOTA
     self.here_isolines_quota ||= DEFAULT_HERE_ISOLINES_QUOTA
     self.obs_snapshot_quota ||= DEFAULT_OBS_SNAPSHOT_QUOTA

config/initializers/warden.rb

@@ -1,4 +1,5 @@
 require_dependency 'carto/user_authenticator'
+require_dependency 'carto/email_cleaner'
 
 Rails.configuration.middleware.use RailsWarden::Manager do |manager|
   manager.default_strategies :password, :api_authentication
@@ -30,6 +31,7 @@ end
 
 Warden::Strategies.add(:password) do
   include Carto::UserAuthenticator
+  include Carto::EmailCleaner
   include LoginEventTrigger
 
   def valid_password_strategy_for_user(user)
@@ -38,7 +40,7 @@ Warden::Strategies.add(:password) do
 
   def authenticate!
     if params[:email] && params[:password]
-      if (user = authenticate(params[:email], params[:password]))
+      if (user = authenticate(clean_email(params[:email]), params[:password]))
         if user.enabled? && valid_password_strategy_for_user(user)
           trigger_login_event(user)
 
@@ -109,6 +111,7 @@ end
 
 Warden::Strategies.add(:github_oauth) do
   include LoginEventTrigger
+  include Carto::EmailCleaner
 
   def valid_github_oauth_strategy_for_user(user)
     user.organization.nil? || user.organization.auth_github_enabled
@@ -120,7 +123,7 @@ Warden::Strategies.add(:github_oauth) do
       github_id = github_api.id
       user = User.where(github_user_id: github_id).first
       unless user
-        user = User.where(email: github_api.email, github_user_id: nil).first
+        user = User.where(email: clean_email(github_api.email), github_user_id: nil).first
         if user && valid_github_oauth_strategy_for_user(user)
           user.github_user_id = github_id
           user.save
@@ -243,6 +246,7 @@ end
 
 Warden::Strategies.add(:saml) do
   include LoginEventTrigger
+  include Carto::EmailCleaner
 
   def organization_from_request
     subdomain = CartoDB.extract_subdomain(request)
@@ -261,7 +265,7 @@ Warden::Strategies.add(:saml) do
     organization = organization_from_request
     saml_service = Carto::SamlService.new(organization)
 
-    email = saml_service.get_user_email(params[:SAMLResponse])
+    email = clean_email(saml_service.get_user_email(params[:SAMLResponse]))
     user = organization.users.where(email: email).first
 
     if user

lib/carto/email_cleaner.rb

@@ -0,0 +1,5 @@
+module Carto::EmailCleaner
+  def clean_email(email)
+    email.strip.downcase
+  end
+end

lib/carto/saml_service.rb

@@ -18,7 +18,7 @@ module Carto
 
     def get_user_email(saml_response_param)
       response = get_saml_response(saml_response_param)
-      response.is_valid? && email_from_saml_response(response).try(:strip).try(:downcase)
+      response.is_valid? && email_from_saml_response(response)
     rescue OneLogin::RubySaml::ValidationError
       debug_response("Invalid SAML response", response)
     end

lib/google_plus_api.rb

@@ -1,8 +1,10 @@
 require_dependency 'google_plus_api_user_data'
 require_dependency 'google_plus_config'
 require_relative 'carto/http/client'
+require_dependency 'carto/email_cleaner'
 
 class GooglePlusAPI
+  include Carto::EmailCleaner
 
   def get_user_data(access_token)
     response = request_user_data(access_token)
@@ -21,7 +23,7 @@ class GooglePlusAPI
   def get_user(access_token)
     google_user_data = GooglePlusAPI.new.get_user_data(access_token)
     # INFO: we assume if a user is queried at a CartoDB instance, user is local
-    google_user_data.present? ? ::User.where(email: google_user_data.email).first : false
+    google_user_data.present? ? ::User.where(email: clean_email(google_user_data.email)).first : false
   end
 
   def request_user_data(access_token)
@@ -35,5 +37,3 @@ class GooglePlusAPI
   end
 
 end
-
-

spec/lib/carto/saml_service_spec.rb

@@ -63,13 +63,6 @@ describe Carto::SamlService do
 
         user.delete
       end
-
-      it 'returns a downcased user email (matches User.before_validation behaviour)' do
-        response_mock.stubs(:is_valid?).returns(true)
-        response_mock.stubs(:attributes).returns(saml_config[:email_attribute] => ' Juan.Perez@example.com')
-
-        service.get_user_email(saml_response_param_mock).should eq 'juan.perez@example.com'
-      end
     end
 
     def create_test_saml_user

spec/requests/sessions_controller_spec.rb

@@ -362,6 +362,19 @@ describe SessionsController do
       response.status.should == 403
     end
 
+    it "authenticates users with casing differences in email" do
+      Carto::SamlService.any_instance.stubs(:enabled?).returns(true)
+      Carto::SamlService.any_instance.stubs(:get_user_email).returns(@user.email.upcase)
+
+      post create_session_url(user_domain: user_domain, SAMLResponse: 'xx')
+
+      response.status.should eq 302
+
+      # Double check authentication is correct
+      get response.redirect_url
+      response.status.should eq 200
+    end
+
     describe 'SAML logout' do
       it 'calls SamlService#sp_logout_request from user-initiated logout' do
         stub_saml_service(@user)