From 3a5ed5260b24051939575d1934ce9b8392cac09f Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Thu, 27 Nov 2014 13:34:39 +0200 Subject: [PATCH] Supporting for multiple omniauth provider for the same user --- .../omniauth_callbacks_controller.rb | 5 +- app/helpers/profile_helper.rb | 4 +- app/models/identity.rb | 2 - app/models/user.rb | 6 +- app/services/notification_service.rb | 2 +- app/views/admin/users/show.html.haml | 2 +- .../sessions/_oauth_providers.html.haml | 2 +- .../20141121161704_add_identity_table.rb | 17 ++- db/schema.rb | 102 +++++------------- features/steps/profile/profile.rb | 2 +- lib/api/entities.rb | 8 +- lib/api/users.rb | 12 ++- lib/gitlab/ldap/access.rb | 8 +- lib/gitlab/ldap/user.rb | 10 +- lib/gitlab/oauth/user.rb | 13 ++- spec/factories.rb | 22 +++- spec/lib/gitlab/ldap/user_spec.rb | 8 +- spec/lib/gitlab/oauth/user_spec.rb | 7 +- spec/models/user_spec.rb | 16 ++- spec/requests/api/users_spec.rb | 2 +- 20 files changed, 123 insertions(+), 127 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 58d0506c07d..3e984e5007a 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -51,7 +51,6 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController # Only allow properly saved users to login. if @user.persisted? && @user.valid? - # binding.pry sign_in_and_redirect(@user.gl_user) else error_message = @@ -66,8 +65,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController redirect_to omniauth_error_path(oauth['provider'], error: error_message) and return end end - rescue StandardError - flash[:notice] = "There's no such user!" + rescue ForbiddenAction => e + flash[:notice] = e.message redirect_to new_user_session_path end diff --git a/app/helpers/profile_helper.rb b/app/helpers/profile_helper.rb index 816074e0247..6480fd3886f 100644 --- a/app/helpers/profile_helper.rb +++ b/app/helpers/profile_helper.rb @@ -10,10 +10,10 @@ module ProfileHelper end def show_profile_social_tab? - enabled_social_providers.any? && !current_user.ldap_user? + enabled_social_providers.any? end def show_profile_remove_tab? - gitlab_config.signup_enabled && !current_user.ldap_user? + gitlab_config.signup_enabled end end diff --git a/app/models/identity.rb b/app/models/identity.rb index e6af93bcc50..5fb1850c30b 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -2,6 +2,4 @@ class Identity < ActiveRecord::Base belongs_to :user validates :extern_uid, allow_blank: true, uniqueness: {scope: :provider} - - scope :ldap, -> { where('provider LIKE ?', 'ldap%') } end \ No newline at end of file diff --git a/app/models/user.rb b/app/models/user.rb index 0cf0946593c..7faeef1b5b0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -406,7 +406,11 @@ class User < ActiveRecord::Base end def ldap_user? - extern_uid && provider.start_with?('ldap') + identities.exists?(["provider LIKE ? AND extern_uid IS NOT NULL", "ldap%"]) + end + + def ldap_identity + @ldap_identity ||= identities.find_by(["provider LIKE ?", "ldap%"]) end def accessible_deploy_keys diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 2b6217e2e29..d1aadd741e1 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -107,7 +107,7 @@ class NotificationService # Notify new user with email after creation def new_user(user, token = nil) # Don't email omniauth created users - mailer.new_user_email(user.id, token) unless user.extern_uid? + mailer.new_user_email(user.id, token) unless user.identities.any? end # Notify users on new note in system diff --git a/app/views/admin/users/show.html.haml b/app/views/admin/users/show.html.haml index 211d77d5185..29717aedd80 100644 --- a/app/views/admin/users/show.html.haml +++ b/app/views/admin/users/show.html.haml @@ -95,7 +95,7 @@ %li %span.light LDAP uid: %strong - = @user.extern_uid + = @user.ldap_identity.extern_uid - if @user.created_by %li diff --git a/app/views/devise/sessions/_oauth_providers.html.haml b/app/views/devise/sessions/_oauth_providers.html.haml index 15048a78063..d053c51d7ec 100644 --- a/app/views/devise/sessions/_oauth_providers.html.haml +++ b/app/views/devise/sessions/_oauth_providers.html.haml @@ -1,4 +1,4 @@ -- providers = (enabled_oauth_providers - [:ldap]) +- providers = enabled_oauth_providers.reject{|provider| provider.to_s.starts_with?('ldap')} - if providers.present? .bs-callout.bs-callout-info{:'data-no-turbolink' => 'data-no-turbolink'} %span Sign in with:   diff --git a/db/migrate/20141121161704_add_identity_table.rb b/db/migrate/20141121161704_add_identity_table.rb index 7d019c65ee1..243958039af 100644 --- a/db/migrate/20141121161704_add_identity_table.rb +++ b/db/migrate/20141121161704_add_identity_table.rb @@ -8,14 +8,25 @@ class AddIdentityTable < ActiveRecord::Migration add_index :identities, :user_id - User.where("provider is not NULL").find_each do |user| + User.where("provider IS NOT NULL").find_each do |user| execute "INSERT INTO identities(provider, extern_uid, user_id) VALUES('#{user.provider}', '#{user.extern_uid}', '#{user.id}')" end - #TODO remove user's columns extern_uid and provider + remove_column :users, :extern_uid + remove_column :users, :provider end def down -#TODO + add_column :users, :extern_uid, :string + add_column :users, :provider, :string + + User.where("id IN(SELECT user_id FROM identities)").find_each do |user| + identity = user.identities.last + user.extern_uid = identity.extern_uid + user.provider = identity.provider + user.save + end + + drop_table :identities end end diff --git a/db/schema.rb b/db/schema.rb index 34f991e5cf2..ec211901e42 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -16,15 +16,6 @@ ActiveRecord::Schema.define(version: 20141121161704) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" - create_table "appearances", force: true do |t| - t.string "title" - t.text "description" - t.string "logo" - t.integer "updated_by" - t.datetime "created_at" - t.datetime "updated_at" - end - create_table "broadcast_messages", force: true do |t| t.text "message", null: false t.datetime "starts_at" @@ -83,21 +74,6 @@ ActiveRecord::Schema.define(version: 20141121161704) do add_index "forked_project_links", ["forked_to_project_id"], name: "index_forked_project_links_on_forked_to_project_id", unique: true, using: :btree - create_table "git_hooks", force: true do |t| - t.string "force_push_regex" - t.string "delete_branch_regex" - t.string "commit_message_regex" - t.boolean "deny_delete_tag" - t.integer "project_id" - t.datetime "created_at" - t.datetime "updated_at" - t.string "username_regex" - t.string "email_regex" - t.string "author_email_regex" - t.boolean "member_check", default: false, null: false - t.string "file_name_regex" - end - create_table "identities", force: true do |t| t.string "extern_uid" t.string "provider" @@ -162,15 +138,6 @@ ActiveRecord::Schema.define(version: 20141121161704) do add_index "labels", ["project_id"], name: "index_labels_on_project_id", using: :btree - create_table "ldap_group_links", force: true do |t| - t.string "cn", null: false - t.integer "group_access", null: false - t.integer "group_id", null: false - t.datetime "created_at" - t.datetime "updated_at" - t.string "provider" - end - create_table "members", force: true do |t| t.integer "access_level", null: false t.integer "source_id", null: false @@ -250,8 +217,6 @@ ActiveRecord::Schema.define(version: 20141121161704) do t.string "type" t.string "description", default: "", null: false t.string "avatar" - t.string "ldap_cn" - t.integer "ldap_access" end add_index "namespaces", ["name"], name: "index_namespaces_on_name", using: :btree @@ -283,14 +248,6 @@ ActiveRecord::Schema.define(version: 20141121161704) do add_index "notes", ["project_id"], name: "index_notes_on_project_id", using: :btree add_index "notes", ["updated_at"], name: "index_notes_on_updated_at", using: :btree - create_table "project_group_links", force: true do |t| - t.integer "project_id", null: false - t.integer "group_id", null: false - t.datetime "created_at" - t.datetime "updated_at" - t.integer "group_access", default: 30, null: false - end - create_table "projects", force: true do |t| t.string "name" t.string "path" @@ -298,22 +255,21 @@ ActiveRecord::Schema.define(version: 20141121161704) do t.datetime "created_at" t.datetime "updated_at" t.integer "creator_id" - t.boolean "issues_enabled", default: true, null: false - t.boolean "wall_enabled", default: true, null: false - t.boolean "merge_requests_enabled", default: true, null: false - t.boolean "wiki_enabled", default: true, null: false + t.boolean "issues_enabled", default: true, null: false + t.boolean "wall_enabled", default: true, null: false + t.boolean "merge_requests_enabled", default: true, null: false + t.boolean "wiki_enabled", default: true, null: false t.integer "namespace_id" - t.string "issues_tracker", default: "gitlab", null: false + t.string "issues_tracker", default: "gitlab", null: false t.string "issues_tracker_id" - t.boolean "snippets_enabled", default: true, null: false + t.boolean "snippets_enabled", default: true, null: false t.datetime "last_activity_at" t.string "import_url" - t.integer "visibility_level", default: 0, null: false - t.boolean "archived", default: false, null: false + t.integer "visibility_level", default: 0, null: false + t.boolean "archived", default: false, null: false t.string "import_status" - t.float "repository_size", default: 0.0 - t.integer "star_count", default: 0, null: false - t.text "merge_requests_template" + t.float "repository_size", default: 0.0 + t.integer "star_count", default: 0, null: false end add_index "projects", ["creator_id"], name: "index_projects_on_creator_id", using: :btree @@ -379,12 +335,12 @@ ActiveRecord::Schema.define(version: 20141121161704) do end create_table "users", force: true do |t| - t.string "email", default: "", null: false - t.string "encrypted_password", default: "", null: false + t.string "email", default: "", null: false + t.string "encrypted_password", default: "", null: false t.string "reset_password_token" t.datetime "reset_password_sent_at" t.datetime "remember_created_at" - t.integer "sign_in_count", default: 0 + t.integer "sign_in_count", default: 0 t.datetime "current_sign_in_at" t.datetime "last_sign_in_at" t.string "current_sign_in_ip" @@ -392,35 +348,32 @@ ActiveRecord::Schema.define(version: 20141121161704) do t.datetime "created_at" t.datetime "updated_at" t.string "name" - t.boolean "admin", default: false, null: false - t.integer "projects_limit", default: 10 - t.string "skype", default: "", null: false - t.string "linkedin", default: "", null: false - t.string "twitter", default: "", null: false + t.boolean "admin", default: false, null: false + t.integer "projects_limit", default: 10 + t.string "skype", default: "", null: false + t.string "linkedin", default: "", null: false + t.string "twitter", default: "", null: false t.string "authentication_token" - t.integer "theme_id", default: 1, null: false + t.integer "theme_id", default: 1, null: false t.string "bio" - t.integer "failed_attempts", default: 0 + t.integer "failed_attempts", default: 0 t.datetime "locked_at" - t.string "extern_uid" - t.string "provider" t.string "username" - t.boolean "can_create_group", default: true, null: false - t.boolean "can_create_team", default: true, null: false + t.boolean "can_create_group", default: true, null: false + t.boolean "can_create_team", default: true, null: false t.string "state" - t.integer "color_scheme_id", default: 1, null: false - t.integer "notification_level", default: 1, null: false + t.integer "color_scheme_id", default: 1, null: false + t.integer "notification_level", default: 1, null: false t.datetime "password_expires_at" t.integer "created_by_id" + t.datetime "last_credential_check_at" t.string "avatar" t.string "confirmation_token" t.datetime "confirmed_at" t.datetime "confirmation_sent_at" t.string "unconfirmed_email" - t.boolean "hide_no_ssh_key", default: false - t.string "website_url", default: "", null: false - t.datetime "last_credential_check_at" - t.datetime "admin_email_unsubscribed_at" + t.boolean "hide_no_ssh_key", default: false + t.string "website_url", default: "", null: false end add_index "users", ["admin"], name: "index_users_on_admin", using: :btree @@ -428,7 +381,6 @@ ActiveRecord::Schema.define(version: 20141121161704) do add_index "users", ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true, using: :btree add_index "users", ["current_sign_in_at"], name: "index_users_on_current_sign_in_at", using: :btree add_index "users", ["email"], name: "index_users_on_email", unique: true, using: :btree - add_index "users", ["extern_uid", "provider"], name: "index_users_on_extern_uid_and_provider", unique: true, using: :btree add_index "users", ["name"], name: "index_users_on_name", using: :btree add_index "users", ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true, using: :btree add_index "users", ["username"], name: "index_users_on_username", using: :btree diff --git a/features/steps/profile/profile.rb b/features/steps/profile/profile.rb index 6d747b65bae..38aaadcd28d 100644 --- a/features/steps/profile/profile.rb +++ b/features/steps/profile/profile.rb @@ -170,7 +170,7 @@ class Spinach::Features::Profile < Spinach::FeatureSteps end step "I am not an ldap user" do - current_user.update_attributes(extern_uid: nil, provider: '') + current_user.identities.delete current_user.ldap_user?.should be_false end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 42e4442365d..2fea151aeb3 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -14,10 +14,14 @@ module API expose :bio, :skype, :linkedin, :twitter, :website_url end + class Identity < Grape::Entity + expose :provider, :extern_uid + end + class UserFull < User expose :email - expose :theme_id, :color_scheme_id, :extern_uid, :provider, \ - :projects_limit + expose :theme_id, :color_scheme_id, :projects_limit + expose :identities, using: Entities::Identity expose :can_create_group?, as: :can_create_group expose :can_create_project?, as: :can_create_project end diff --git a/lib/api/users.rb b/lib/api/users.rb index d07815a8a97..37b36ddcf94 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -59,10 +59,16 @@ module API post do authenticated_as_admin! required_attributes! [:email, :password, :name, :username] - attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :extern_uid, :provider, :bio, :can_create_group, :admin] + attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :projects_limit, :username, :bio, :can_create_group, :admin] user = User.build_user(attrs) admin = attrs.delete(:admin) user.admin = admin unless admin.nil? + + identity_attrs = attributes_for_keys [:provider, :extern_uid] + if identity_attrs.any? + user.identities.build(identity_attrs) + end + if user.save present user, with: Entities::UserFull else @@ -89,8 +95,6 @@ module API # twitter - Twitter account # website_url - Website url # projects_limit - Limit projects each user can create - # extern_uid - External authentication provider UID - # provider - External provider # bio - Bio # admin - User is admin - true or false (default) # can_create_group - User can create groups - true or false @@ -99,7 +103,7 @@ module API put ":id" do authenticated_as_admin! - attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :website_url, :projects_limit, :username, :extern_uid, :provider, :bio, :can_create_group, :admin] + attrs = attributes_for_keys [:email, :name, :password, :skype, :linkedin, :twitter, :website_url, :projects_limit, :username, :bio, :can_create_group, :admin] user = User.find(params[:id]) not_found!('User') unless user diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index eb2c4e48ff2..0c85acf7e69 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -8,7 +8,7 @@ module Gitlab attr_reader :adapter, :provider, :user def self.open(user, &block) - Gitlab::LDAP::Adapter.open(user.provider) do |adapter| + Gitlab::LDAP::Adapter.open(user.ldap_identity.provider) do |adapter| block.call(self.new(user, adapter)) end end @@ -28,13 +28,13 @@ module Gitlab def initialize(user, adapter=nil) @adapter = adapter @user = user - @provider = user.provider + @provider = user.ldap_identity.provider end def allowed? - if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) + if Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter) return true unless ldap_config.active_directory - !Gitlab::LDAP::Person.disabled_via_active_directory?(user.extern_uid, adapter) + !Gitlab::LDAP::Person.disabled_via_active_directory?(user.ldap_identity.extern_uid, adapter) else false end diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb index 827a33b5217..3ef494ba137 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -35,15 +35,13 @@ module Gitlab end def find_by_email - User.find_by(email: auth_hash.email) + ::User.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 - } + gl_user.email = auth_hash.email + gl_user.identities.build(provider: auth_hash.provider, extern_uid: auth_hash.uid) + gl_user end def changed? diff --git a/lib/gitlab/oauth/user.rb b/lib/gitlab/oauth/user.rb index 7c1970eb8e5..6861427864e 100644 --- a/lib/gitlab/oauth/user.rb +++ b/lib/gitlab/oauth/user.rb @@ -5,6 +5,8 @@ # module Gitlab module OAuth + class ForbiddenAction < StandardError; end + class User attr_accessor :auth_hash, :gl_user @@ -27,9 +29,11 @@ module Gitlab def save unauthorized_to_create unless gl_user - gl_user.save! if needs_blocking? + gl_user.save! gl_user.block + else + gl_user.save! end log.info "(OAuth) saving user #{auth_hash.email} from login with extern_uid => #{auth_hash.uid}" @@ -73,9 +77,10 @@ module Gitlab end def build_new_user - user = User.new(user_attributes) + user = ::User.new(user_attributes) user.skip_confirmation! user.identities.new(extern_uid: auth_hash.uid, provider: auth_hash.provider) + user end def user_attributes @@ -92,8 +97,8 @@ module Gitlab Gitlab::AppLogger end - def raise_unauthorized_to_create - raise StandardError.new("Unauthorized to create user, signup disabled for #{auth_hash.provider}") + def unauthorized_to_create + raise ForbiddenAction.new("Unauthorized to create user, signup disabled for #{auth_hash.provider}") end end end diff --git a/spec/factories.rb b/spec/factories.rb index 15899d8c3c4..58060131638 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -18,15 +18,24 @@ FactoryGirl.define do password "12345678" password_confirmation { password } confirmed_at { Time.now } - confirmation_token { nil } + confirmation_token { nil } trait :admin do admin true end - trait :ldap do - provider 'ldapmain' - extern_uid 'my-ldap-id' + factory :omniauth_user do + ignore do + extern_uid '123456' + provider 'ldapmain' + end + + after(:create) do |user, evaluator| + user.identities << create(:identity, + provider: evaluator.provider, + extern_uid: evaluator.extern_uid + ) + end end factory :admin, traits: [:admin] @@ -182,4 +191,9 @@ FactoryGirl.define do deploy_key project end + + factory :identity do + provider 'ldapmain' + extern_uid 'my-ldap-id' + end end diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb index 726c9764e3d..294ee6cbae0 100644 --- a/spec/lib/gitlab/ldap/user_spec.rb +++ b/spec/lib/gitlab/ldap/user_spec.rb @@ -15,18 +15,18 @@ describe Gitlab::LDAP::User do describe :find_or_create do it "finds the user if already existing" do - existing_user = create(:user, extern_uid: 'my-uid', provider: 'ldapmain') + existing_user = create(:omniauth_user, extern_uid: 'my-uid', provider: 'ldapmain') expect{ gl_user.save }.to_not change{ User.count } end it "connects to existing non-ldap user if the email matches" do - existing_user = create(:user, email: 'john@example.com') + existing_user = create(:omniauth_user, email: 'john@example.com') expect{ gl_user.save }.to_not change{ User.count } existing_user.reload - expect(existing_user.extern_uid).to eql 'my-uid' - expect(existing_user.provider).to eql 'ldapmain' + expect(existing_user.ldap_identity.extern_uid).to eql 'my-uid' + expect(existing_user.ldap_identity.provider).to eql 'ldapmain' end it "creates a new user if not found" do diff --git a/spec/lib/gitlab/oauth/user_spec.rb b/spec/lib/gitlab/oauth/user_spec.rb index 8a83a1b2588..88307515789 100644 --- a/spec/lib/gitlab/oauth/user_spec.rb +++ b/spec/lib/gitlab/oauth/user_spec.rb @@ -15,7 +15,7 @@ describe Gitlab::OAuth::User do end describe :persisted? do - let!(:existing_user) { create(:user, extern_uid: 'my-uid', provider: 'my-provider') } + let!(:existing_user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') } it "finds an existing user based on uid and provider (facebook)" do auth = double(info: double(name: 'John'), uid: 'my-uid', provider: 'my-provider') @@ -39,8 +39,9 @@ describe Gitlab::OAuth::User do oauth_user.save expect(gl_user).to be_valid - expect(gl_user.extern_uid).to eql uid - expect(gl_user.provider).to eql 'twitter' + identity = gl_user.identities.first + expect(identity.extern_uid).to eql uid + expect(identity.provider).to eql 'twitter' end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6d865cfc691..8be7f733a5b 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -62,6 +62,7 @@ describe User do it { should have_many(:assigned_issues).dependent(:destroy) } it { should have_many(:merge_requests).dependent(:destroy) } it { should have_many(:assigned_merge_requests).dependent(:destroy) } + it { should have_many(:identities).dependent(:destroy) } end describe "Mass assignment" do @@ -361,24 +362,29 @@ describe User do end describe :ldap_user? do - let(:user) { build(:user, :ldap) } - it "is true if provider name starts with ldap" do - user.provider = 'ldapmain' + user = create(:omniauth_user, provider: 'ldapmain') expect( user.ldap_user? ).to be_true end it "is false for other providers" do - user.provider = 'other-provider' + user = create(:omniauth_user, provider: 'other-provider') expect( user.ldap_user? ).to be_false end it "is false if no extern_uid is provided" do - user.extern_uid = nil + user = create(:omniauth_user, extern_uid: nil) expect( user.ldap_user? ).to be_false end end + describe :ldap_identity do + it "returns ldap identity" do + user = create :omniauth_user + user.ldap_identity.provider.should_not be_empty + end + end + describe '#full_website_url' do let(:user) { create(:user) } diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 113a39b870e..1ecc79ea7ef 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -33,7 +33,7 @@ describe API::API, api: true do response.status.should == 200 json_response.should be_an Array json_response.first.keys.should include 'email' - json_response.first.keys.should include 'extern_uid' + json_response.first.keys.should include 'identities' json_response.first.keys.should include 'can_create_project' end end -- GitLab