diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-11-17 14:24:25 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2017-11-17 14:24:25 +0000 |
commit | e68ee8af4d981cb7b83fae76c0a94059add495fb (patch) | |
tree | 6cf6af687696012525b6cb05b336bec13f049a10 | |
parent | 76b2a7caa5219662a29f0eb16f0507aac1976f33 (diff) | |
parent | c7cf68bd6ff744e044944acad586e06badc481d4 (diff) | |
download | gitlab-ce-e68ee8af4d981cb7b83fae76c0a94059add495fb.tar.gz |
Merge branch '38822-oauth-search-case-insensitive' into 'master'
Changing OAuth lookup to be case insensitive
Closes #38822
See merge request gitlab-org/gitlab-ce!15312
-rw-r--r-- | app/controllers/omniauth_callbacks_controller.rb | 6 | ||||
-rw-r--r-- | app/models/identity.rb | 15 | ||||
-rw-r--r-- | app/models/user.rb | 3 | ||||
-rw-r--r-- | changelogs/unreleased/38822-oauth-search-case-insensitive.yml | 5 | ||||
-rw-r--r-- | lib/gitlab/ldap/user.rb | 5 | ||||
-rw-r--r-- | lib/gitlab/o_auth/user.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/o_auth/user_spec.rb | 9 | ||||
-rw-r--r-- | spec/models/identity_spec.rb | 10 |
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 |