Refactor OAuth refactorings to CE

parent 9c824888
...@@ -15,15 +15,17 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -15,15 +15,17 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
error.to_s.humanize if error error.to_s.humanize if error
end end
def ldap
# We only find ourselves here # We only find ourselves here
# if the authentication to LDAP was successful. # if the authentication to LDAP was successful.
@user = Gitlab::LDAP::User.find_or_create(oauth) def ldap
@user.remember_me = true if @user.persisted? @user = Gitlab::LDAP::User.new(oauth)
@user.save if @user.changed? # will also save new users
gl_user = @user.gl_user
gl_user.remember_me = true if @user.persisted?
# Do additional LDAP checks for the user filter and EE features # Do additional LDAP checks for the user filter and EE features
if Gitlab::LDAP::Access.allowed?(@user) if Gitlab::LDAP::Access.allowed?(gl_user)
sign_in_and_redirect(@user) sign_in_and_redirect(gl_user)
else else
flash[:alert] = "Access denied for your LDAP account." flash[:alert] = "Access denied for your LDAP account."
redirect_to new_user_session_path redirect_to new_user_session_path
...@@ -46,24 +48,17 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -46,24 +48,17 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
current_user.save current_user.save
redirect_to profile_path redirect_to profile_path
else else
@user = Gitlab::OAuth::User.find(oauth) @user = Gitlab::OAuth::User.new(oauth)
# Create user if does not exist if Gitlab.config.omniauth['allow_single_sign_on'] && @user.new?
# and allow_single_sign_on is true @user.save
if Gitlab.config.omniauth['allow_single_sign_on'] && !@user
@user, errors = Gitlab::OAuth::User.create(oauth)
end end
if @user && !errors if @user.valid?
sign_in_and_redirect(@user) sign_in_and_redirect(@user.gl_user)
else else
if errors error_message = @user.gl_user.errors.map{ |attribute, message| "#{attribute} #{message}" }.join(", ")
error_message = errors.map{ |attribute, message| "#{attribute} #{message}" }.join(", ")
redirect_to omniauth_error_path(oauth['provider'], error: error_message) and return redirect_to omniauth_error_path(oauth['provider'], error: error_message) and return
else
flash[:notice] = "There's no such user!"
end
redirect_to new_user_session_path
end end
end end
end end
......
...@@ -10,22 +10,6 @@ module Gitlab ...@@ -10,22 +10,6 @@ module Gitlab
module LDAP module LDAP
class User < Gitlab::OAuth::User class User < Gitlab::OAuth::User
class << self class << self
def find_or_create(auth_hash)
self.auth_hash = auth_hash
find(auth_hash) || find_and_connect_by_email(auth_hash) || create(auth_hash)
end
def find_and_connect_by_email(auth_hash)
self.auth_hash = auth_hash
user = model.find_by(email: self.auth_hash.email)
if user
user.update_attributes(extern_uid: auth_hash.uid, provider: auth_hash.provider)
Gitlab::AppLogger.info("(LDAP) Updating legacy LDAP user #{self.auth_hash.email} with extern_uid => #{auth_hash.uid}")
return user
end
end
def authenticate(login, password) def authenticate(login, password)
# Check user against LDAP backend if user is not authenticated # Check user against LDAP backend if user is not authenticated
# Only check with valid login and password to prevent anonymous bind results # Only check with valid login and password to prevent anonymous bind results
...@@ -44,10 +28,18 @@ module Gitlab ...@@ -44,10 +28,18 @@ module Gitlab
@adapter ||= OmniAuth::LDAP::Adaptor.new(ldap_conf) @adapter ||= OmniAuth::LDAP::Adaptor.new(ldap_conf)
end end
protected def user_filter(login)
filter = Net::LDAP::Filter.eq(adapter.uid, login)
# Apply LDAP user filter if present
if ldap_conf['user_filter'].present?
user_filter = Net::LDAP::Filter.construct(ldap_conf['user_filter'])
filter = Net::LDAP::Filter.join(filter, user_filter)
end
filter
end
def find_by_uid_and_provider def ldap_conf
find_by_uid(auth_hash.uid) Gitlab.config.ldap
end end
def find_by_uid(uid) def find_by_uid(uid)
...@@ -58,24 +50,39 @@ module Gitlab ...@@ -58,24 +50,39 @@ module Gitlab
def provider def provider
'ldap' 'ldap'
end end
end
def raise_error(message) def initialize(auth_hash)
raise OmniAuth::Error, "(LDAP) " + message super
update_user_attributes
end end
def ldap_conf # instance methods
Gitlab.config.ldap def gl_user
@gl_user ||= find_by_uid_and_provider || find_by_email || build_new_user
end end
def user_filter(login) def find_by_uid_and_provider
filter = Net::LDAP::Filter.eq(adapter.uid, login) # LDAP distinguished name is case-insensitive
# Apply LDAP user filter if present model.
if ldap_conf['user_filter'].present? where(provider: auth_hash.provider).
user_filter = Net::LDAP::Filter.construct(ldap_conf['user_filter']) where('lower(extern_uid) = ?', auth_hash.uid.downcase).last
filter = Net::LDAP::Filter.join(filter, user_filter)
end end
filter
def find_by_email
model.find_by(email: auth_hash.email)
end
def update_user_attributes
gl_user.attributes = {
extern_uid: auth_hash.uid,
provider: auth_hash.provider,
email: auth_hash.email
}
end end
def changed?
gl_user.changed?
end end
def needs_blocking? def needs_blocking?
......
...@@ -21,7 +21,7 @@ module Gitlab ...@@ -21,7 +21,7 @@ module Gitlab
end end
def name def name
(info.name || full_name).to_s.force_encoding('utf-8') (info.try(:name) || full_name).to_s.force_encoding('utf-8')
end end
def full_name def full_name
......
...@@ -6,55 +6,52 @@ ...@@ -6,55 +6,52 @@
module Gitlab module Gitlab
module OAuth module OAuth
class User class User
class << self attr_accessor :auth_hash, :gl_user
attr_reader :auth_hash
def find(auth_hash) def initialize(auth_hash)
self.auth_hash = auth_hash self.auth_hash = auth_hash
find_by_uid_and_provider
end end
def create(auth_hash) def persisted?
user = new(auth_hash) gl_user.persisted?
user.save_and_trigger_callbacks
end end
def model def new?
::User !gl_user.persisted?
end end
def auth_hash=(auth_hash) def valid?
@auth_hash = AuthHash.new(auth_hash) gl_user.valid?
end end
protected def save
def find_by_uid_and_provider gl_user.save!
model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last log.info "(OAuth) saving user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}"
end gl_user.block if needs_blocking?
end
# Instance methods gl_user
attr_accessor :auth_hash, :user rescue ActiveRecord::RecordInvalid => e
log.info "(OAuth) Error saving user: #{gl_user.errors.full_messages}"
return self, e.record.errors
end
def initialize(auth_hash) def gl_user
self.auth_hash = auth_hash @user ||= find_by_uid_and_provider || build_new_user
self.user = self.class.model.new(user_attributes)
user.skip_confirmation!
end end
protected
def auth_hash=(auth_hash) def auth_hash=(auth_hash)
@auth_hash = AuthHash.new(auth_hash) @auth_hash = AuthHash.new(auth_hash)
end end
def save_and_trigger_callbacks def find_by_uid_and_provider
user.save! model.where(provider: auth_hash.provider, extern_uid: auth_hash.uid).last
log.info "(OAuth) Creating user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}" end
user.block if needs_blocking?
user def build_new_user
rescue ActiveRecord::RecordInvalid => e model.new(user_attributes).tap do |user|
log.info "(OAuth) Email #{e.record.errors[:email]}. Username #{e.record.errors[:username]}" user.skip_confirmation!
return nil, e.record.errors end
end end
def user_attributes def user_attributes
...@@ -80,6 +77,10 @@ module Gitlab ...@@ -80,6 +77,10 @@ module Gitlab
def needs_blocking? def needs_blocking?
Gitlab.config.omniauth['block_auto_created_users'] Gitlab.config.omniauth['block_auto_created_users']
end end
def model
::User
end
end end
end end
end end
require 'spec_helper' require 'spec_helper'
describe Gitlab::LDAP::User do describe Gitlab::LDAP::User do
let(:gl_user) { Gitlab::LDAP::User } let(:gl_user) { Gitlab::LDAP::User.new(auth_hash) }
let(:info) do let(:info) do
double( {
name: 'John', name: 'John',
email: 'john@example.com', email: 'john@example.com',
nickname: 'john' nickname: 'john'
) }
end end
before { Gitlab.config.stub(omniauth: {}) } let(:auth_hash) do
double(uid: 'my-uid', provider: 'ldap', info: double(info))
describe :find_or_create do
let(:auth) do
double(info: info, provider: 'ldap', uid: 'my-uid')
end end
describe :find_or_create do
it "finds the user if already existing" do it "finds the user if already existing" do
existing_user = create(:user, extern_uid: 'my-uid', provider: 'ldap') existing_user = create(:user, extern_uid: 'my-uid', provider: 'ldap')
expect{ gl_user.find_or_create(auth) }.to_not change{ User.count } expect{ gl_user.save }.to_not change{ User.count }
end end
it "connects to existing non-ldap user if the email matches" do it "connects to existing non-ldap user if the email matches" do
existing_user = create(:user, email: 'john@example.com') existing_user = create(:user, email: 'john@example.com')
expect{ gl_user.find_or_create(auth) }.to_not change{ User.count } expect{ gl_user.save }.to_not change{ User.count }
existing_user.reload existing_user.reload
expect(existing_user.extern_uid).to eql 'my-uid' expect(existing_user.extern_uid).to eql 'my-uid'
...@@ -32,7 +30,7 @@ describe Gitlab::LDAP::User do ...@@ -32,7 +30,7 @@ describe Gitlab::LDAP::User do
end end
it "creates a new user if not found" do it "creates a new user if not found" do
expect{ gl_user.find_or_create(auth) }.to change{ User.count }.by(1) expect{ gl_user.save }.to change{ User.count }.by(1)
end end
end end
......
require 'spec_helper'
describe Gitlab::OAuth::AuthHash do
let(:auth_hash) do
Gitlab::OAuth::AuthHash.new(double({
provider: 'twitter',
uid: uid,
info: double(info_hash)
}))
end
let(:uid) { 'my-uid' }
let(:email) { 'my-email@example.com' }
let(:nickname) { 'my-nickname' }
let(:info_hash) {
{
email: email,
nickname: nickname,
name: 'John',
first_name: "John",
last_name: "Who"
}
}
context "defaults" do
it { expect(auth_hash.provider).to eql 'twitter' }
it { expect(auth_hash.uid).to eql uid }
it { expect(auth_hash.email).to eql email }
it { expect(auth_hash.username).to eql nickname }
it { expect(auth_hash.name).to eql "John" }
it { expect(auth_hash.password).to_not be_empty }
end
context "email not provided" do
before { info_hash.delete(:email) }
it "generates a temp email" do
expect( auth_hash.email).to start_with('temp-email-for-oauth')
end
end
context "username not provided" do
before { info_hash.delete(:nickname) }
it "takes the first part of the email as username" do
expect( auth_hash.username ).to eql "my-email"
end
end
context "name not provided" do
before { info_hash.delete(:name) }
it "concats first and lastname as the name" do
expect( auth_hash.name ).to eql "John Who"
end
end
end
\ No newline at end of file
require 'spec_helper' require 'spec_helper'
describe Gitlab::OAuth::User do describe Gitlab::OAuth::User do
let(:gl_auth) { Gitlab::OAuth::User } let(:oauth_user) { Gitlab::OAuth::User.new(auth_hash) }
let(:info) do let(:gl_user) { oauth_user.gl_user }
double( let(:uid) { 'my-uid' }
let(:provider) { 'my-provider' }
let(:auth_hash) { double(uid: uid, provider: provider, info: double(info_hash)) }
let(:info_hash) do
{
nickname: 'john', nickname: 'john',
name: 'John', name: 'John',
email: 'john@mail.com' email: 'john@mail.com'
) }
end end
before do describe :persisted? do
Gitlab.config.stub(omniauth: {})
end
describe :find do
let!(:existing_user) { create(:user, extern_uid: 'my-uid', provider: 'my-provider') } let!(:existing_user) { create(:user, extern_uid: 'my-uid', provider: 'my-provider') }
it "finds an existing user based on uid and provider (facebook)" do it "finds an existing user based on uid and provider (facebook)" do
auth = double(info: double(name: 'John'), uid: 'my-uid', provider: 'my-provider') auth = double(info: double(name: 'John'), uid: 'my-uid', provider: 'my-provider')
assert gl_auth.find(auth) expect( oauth_user.persisted? ).to be_true
end end
it "finds an existing user based on nested uid and provider" do it "returns false if use is not found in database" do
auth = double(info: info, uid: 'my-uid', provider: 'my-provider') auth_hash.stub(uid: 'non-existing')
assert gl_auth.find(auth) expect( oauth_user.persisted? ).to be_false
end end
end end
describe :create do describe :save do
it "should create user from LDAP" do context "LDAP" do
auth = double(info: info, uid: 'my-uid', provider: 'ldap') let(:provider) { 'ldap' }
user = gl_auth.create(auth) it "creates a user from LDAP" do
oauth_user.save
user.should be_valid expect(gl_user).to be_valid
user.extern_uid.should == auth.uid expect(gl_user.extern_uid).to eql uid
user.provider.should == 'ldap' expect(gl_user.provider).to eql 'ldap'
end end
it "should create user from Omniauth" do
auth = double(info: info, uid: 'my-uid', provider: 'twitter')
user = gl_auth.create(auth)
user.should be_valid
user.extern_uid.should == auth.uid
user.provider.should == 'twitter'
end end
it "should apply defaults to user" do context "twitter" do
auth = double(info: info, uid: 'my-uid', provider: 'ldap') let(:provider) { 'twitter' }
user = gl_auth.create(auth)
user.should be_valid
user.projects_limit.should == Gitlab.config.gitlab.default_projects_limit
user.can_create_group.should == Gitlab.config.gitlab.default_can_create_group
end
it "Set a temp email address if not provided (like twitter does)" do it "creates a user from Omniauth" do
info = double( oauth_user.save
uid: 'my-uid',
nickname: 'john',
name: 'John'
)
auth = double(info: info, uid: 'my-uid', provider: 'my-provider')
user = gl_auth.create(auth) expect(gl_user).to be_valid
expect(user.email).to_not be_empty expect(gl_user.extern_uid).to eql uid
expect(gl_user.provider).to eql 'twitter'
end end
it 'generates a username if non provided (google)' do
info = double(
uid: 'my-uid',
name: 'John',
email: 'john@example.com'
)
auth = double(info: info, uid: 'my-uid', provider: 'my-provider')
user = gl_auth.create(auth)
expect(user.username).to eql 'john'
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment