Skip to content
Snippets Groups Projects
Commit c7cf68bd authored by Francisco Javier López's avatar Francisco Javier López Committed by Douwe Maan
Browse files

Changing OAuth lookup to be case insensitive

parent 76b2a7ca
No related branches found
No related tags found
No related merge requests found
Loading
Loading
@@ -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'
Loading
Loading
@@ -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
Loading
Loading
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
Loading
Loading
@@ -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
Loading
Loading
---
title: OAuth identity lookups case-insensitive
merge_request: 15312
author:
type: fixed
Loading
Loading
@@ -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
Loading
Loading
Loading
Loading
@@ -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
 
Loading
Loading
Loading
Loading
@@ -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
Loading
Loading
@@ -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
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment