Supporting for multiple omniauth provider for the same user

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
 

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

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

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

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

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

app/views/devise/sessions/_oauth_providers.html.haml

-- 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:  

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

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"
@@ -313,7 +270,6 @@ ActiveRecord::Schema.define(version: 20141121161704) do
     t.string   "import_status"
     t.float    "repository_size",        default: 0.0
     t.integer  "star_count",             default: 0,        null: false
-    t.text     "merge_requests_template"
   end
 
   add_index "projects", ["creator_id"], name: "index_projects_on_creator_id", using: :btree
@@ -402,8 +358,6 @@ ActiveRecord::Schema.define(version: 20141121161704) do
     t.string   "bio"
     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
@@ -412,6 +366,7 @@ ActiveRecord::Schema.define(version: 20141121161704) do
     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"
@@ -419,8 +374,6 @@ ActiveRecord::Schema.define(version: 20141121161704) do
     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"
   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

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
 

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

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
 

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

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?

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

spec/factories.rb

@@ -24,9 +24,18 @@ FactoryGirl.define do
       admin true
     end
 
-    trait :ldap do
+    factory :omniauth_user do
+      ignore do
+        extern_uid '123456'
         provider 'ldapmain'
-      extern_uid 'my-ldap-id'
+      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

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

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
 

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) }
 

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