Changing OAuth lookup to be case insensitive

8 files changed, 43 insertions, 12 deletions

diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index 9612b8d8514..56baa19f864 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -54,7 +54,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     if current_user
       log_audit_event(current_user, with: :saml)
       # Update SAML identity if data has changed.
-      identity = current_user.identities.find_by(extern_uid: oauth['uid'], provider: :saml)
+      identity = current_user.identities.with_extern_uid(:saml, oauth['uid']).take
       if identity.nil?
         current_user.identities.create(extern_uid: oauth['uid'], provider: :saml)
         redirect_to profile_account_path, notice: 'Authentication method updated'
@@ -98,7 +98,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
   def handle_omniauth
     if current_user
       # Add new authentication method
-      current_user.identities.find_or_create_by(extern_uid: oauth['uid'], provider: oauth['provider'])
+      current_user.identities
+                  .with_extern_uid(oauth['provider'], oauth['uid'])
+                  .first_or_create(extern_uid: oauth['uid'])
       log_audit_event(current_user, with: oauth['provider'])
       redirect_to profile_account_path, notice: 'Authentication method updated'
     else
diff --git a/app/models/identity.rb b/app/models/identity.rb
index ac8094b610e..ff811e19f8a 100644
--- a/app/models/identity.rb
+++ b/app/models/identity.rb
@@ -1,18 +1,27 @@
 class Identity < ActiveRecord::Base
   include Sortable
   include CaseSensitivity
+
   belongs_to :user
 
   validates :provider, presence: true
-  validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider }
+  validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider, case_sensitive: false }
   validates :user_id, uniqueness: { scope: :provider }
 
+  scope :with_provider, ->(provider) { where(provider: provider) }
   scope :with_extern_uid, ->(provider, extern_uid) do
-    extern_uid = Gitlab::LDAP::Person.normalize_dn(extern_uid) if provider.starts_with?('ldap')
-    where(extern_uid: extern_uid, provider: provider)
+    iwhere(extern_uid: normalize_uid(provider, extern_uid)).with_provider(provider)
   end
 
   def ldap?
     provider.starts_with?('ldap')
   end
+
+  def self.normalize_uid(provider, uid)
+    if provider.to_s.starts_with?('ldap')
+      Gitlab::LDAP::Person.normalize_dn(uid)
+    else
+      uid.to_s
+    end
+  end
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index be8112749bf..71c34766451 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -269,8 +269,7 @@ class User < ActiveRecord::Base
     end
 
     def for_github_id(id)
-      joins(:identities)
-        .where(identities: { provider: :github, extern_uid: id.to_s })
+      joins(:identities).merge(Identity.with_extern_uid(:github, id))
     end
 
     # Find a User by their primary email or any associated secondary email
diff --git a/changelogs/unreleased/38822-oauth-search-case-insensitive.yml b/changelogs/unreleased/38822-oauth-search-case-insensitive.yml
new file mode 100644
index 00000000000..d84360b4c5c
--- /dev/null
+++ b/changelogs/unreleased/38822-oauth-search-case-insensitive.yml
@@ -0,0 +1,5 @@
+---
+title: OAuth identity lookups case-insensitive
+merge_request: 15312
+author:
+type: fixed
diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb
index 4d5c67ed892..3945df27eed 100644
--- a/lib/gitlab/ldap/user.rb
+++ b/lib/gitlab/ldap/user.rb
@@ -9,11 +9,8 @@ module Gitlab
     class User < Gitlab::OAuth::User
       class << self
         def find_by_uid_and_provider(uid, provider)
-          uid = Gitlab::LDAP::Person.normalize_dn(uid)
+          identity = ::Identity.with_extern_uid(provider, uid).take
 
-          identity = ::Identity
-            .where(provider: provider)
-            .where(extern_uid: uid).last
           identity && identity.user
         end
       end
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index b4b3b00c84d..552133234a3 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -157,7 +157,7 @@ module Gitlab
       end
 
       def find_by_uid_and_provider
-        identity = Identity.find_by(provider: auth_hash.provider, extern_uid: auth_hash.uid)
+        identity = Identity.with_extern_uid(auth_hash.provider, auth_hash.uid).take
         identity && identity.user
       end
 
diff --git a/spec/lib/gitlab/o_auth/user_spec.rb b/spec/lib/gitlab/o_auth/user_spec.rb
index c7471a21fda..2f19fb7312d 100644
--- a/spec/lib/gitlab/o_auth/user_spec.rb
+++ b/spec/lib/gitlab/o_auth/user_spec.rb
@@ -662,4 +662,13 @@ describe Gitlab::OAuth::User do
       end
     end
   end
+
+  describe '.find_by_uid_and_provider' do
+    let!(:existing_user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') }
+
+    it 'normalizes extern_uid' do
+      allow(oauth_user.auth_hash).to receive(:uid).and_return('MY-UID')
+      expect(oauth_user.find_user).to eql gl_user
+    end
+  end
 end
diff --git a/spec/models/identity_spec.rb b/spec/models/identity_spec.rb
index 3ed048744de..a45a6088831 100644
--- a/spec/models/identity_spec.rb
+++ b/spec/models/identity_spec.rb
@@ -33,5 +33,15 @@ describe Identity do
         expect(identity).to eq(ldap_identity)
       end
     end
+
+    context 'any other provider' do
+      let!(:test_entity) { create(:identity, provider: 'test_provider', extern_uid: 'test_uid') }
+
+      it 'the extern_uid lookup is case insensitive' do
+        identity = described_class.with_extern_uid('test_provider', 'TEST_UID').first
+
+        expect(identity).to eq(test_entity)
+      end
+    end
   end
 end