Merge branch '36808-link-smartcard-ldap-certs-to-users' into 'master'

Link users to new Smartcards certificate ldap identities on login

Closes #36808

See merge request gitlab-org/gitlab!20470

ee/app/controllers/smartcard_controller.rb

@@ -21,7 +21,7 @@ class SmartcardController < ApplicationController
 
   def sign_in_with(certificate)
     user = certificate.find_or_create_user
-    unless user
+    unless user&.persisted?
       flash[:alert] = _('Failed to signing using smartcard authentication')
       redirect_to new_user_session_path(port: Gitlab.config.gitlab.port)
 

ee/changelogs/unreleased/36808-link-smartcard-ldap-certs-to-users.yml

+---
+title: Link user accounts to new Smartcards certificate ldap identities on login
+merge_request: 20470
+author:
+type: fixed

ee/lib/gitlab/auth/smartcard/ldap_certificate.rb

@@ -22,7 +22,12 @@ module Gitlab
         end
 
         def create_identity_for_existing_user
-          # TODO: create new identity for existing users as part of https://gitlab.com/gitlab-org/gitlab/issues/36808
+          user = User.find_by_email(ldap_user.email.first)
+
+          return if user.nil? || user.ldap_user?
+
+          create_ldap_certificate_identity_for(user)
+          user
         end
 
         def create_user
@@ -41,6 +46,10 @@ module Gitlab
           Users::CreateService.new(nil, user_params).execute(skip_authorization: true)
         end
 
+        def create_ldap_certificate_identity_for(user)
+          user.identities.create(provider: @provider, extern_uid: ldap_user.dn)
+        end
+
         def adapter
           @adapter ||= Gitlab::Auth::LDAP::Adapter.new(@provider)
         end

ee/spec/lib/gitlab/auth/smartcard/ldap_certificate_spec.rb

@@ -25,7 +25,6 @@ describe Gitlab::Auth::Smartcard::LDAPCertificate do
       entry['mail'] = ldap_person_email
     end
   end
-  let(:ldap_person) { ::Gitlab::Auth::LDAP::Person.new(ldap_entry, ldap_provider) }
 
   before do
     allow(described_class).to(
@@ -41,7 +40,7 @@ describe Gitlab::Auth::Smartcard::LDAPCertificate do
   describe '#find_or_create_user' do
     subject { described_class.new(ldap_provider, certificate_header).find_or_create_user }
 
-    context 'user already exists' do
+    context 'user and smartcard ldap certificate already exists' do
       let(:user) { create(:user) }
 
       before do
@@ -59,6 +58,35 @@ describe Gitlab::Auth::Smartcard::LDAPCertificate do
       end
     end
 
+    context 'user exists but it is using a new ldap certificate' do
+      let(:ldap_person_email) { user.email }
+      let_it_be(:user) { create(:user) }
+
+      it 'finds existing user' do
+        expect(subject).to eql(user)
+      end
+
+      it 'does create new user identity' do
+        expect { subject }.to change { user.identities.count }.by(1)
+      end
+
+      context 'user already has a different ldap certificate identity' do
+        before do
+          create(:identity, { provider: 'ldapmain',
+                              extern_uid: 'old_subject_ldap_dn',
+                              user: user })
+        end
+
+        it "doesn't create a new identity" do
+          expect { subject }.not_to change { Identity.count }
+        end
+
+        it "doesn't create a new user" do
+          expect { subject }.not_to change { User.count }
+        end
+      end
+    end
+
     context 'user does not exist' do
       let(:user) { create(:user) }
 

ee/spec/requests/smartcard_controller_spec.rb

@@ -157,12 +157,13 @@ describe SmartcardController, type: :request do
     end
 
     let(:ldap_connection) { instance_double(::Net::LDAP) }
+    let(:ldap_email) { 'john.doe@example.com' }
     let(:ldap_entry) do
       Net::LDAP::Entry.new.tap do |entry|
         entry['dn'] = subject_ldap_dn
         entry['uid'] = 'john doe'
         entry['cn'] = 'John Doe'
-        entry['mail'] = 'john.doe@example.com'
+        entry['mail'] = ldap_email
       end
     end
     let(:ldap_user_search_scope) { 'dc=example,dc=com' }
@@ -205,17 +206,37 @@ describe SmartcardController, type: :request do
     end
 
     context 'user already exists' do
-      before do
-        user = create(:user)
+      let_it_be(:user) { create(:user) }
+
+      it 'finds existing user' do
         create(:identity, { provider: 'ldapmain',
                             extern_uid: subject_ldap_dn,
                             user: user })
-      end
 
-      it 'finds existing user' do
         expect { subject }.not_to change { User.count }
+
         expect(request.env['warden']).to be_authenticated
       end
+
+      context "user has a different identity" do
+        let(:ldap_email) { user.email }
+
+        before do
+          create(:identity, { provider: 'ldapmain',
+                              extern_uid: 'different_identity_dn',
+                              user: user })
+        end
+
+        it "doesn't login a user" do
+          subject
+
+          expect(request.env['warden']).not_to be_authenticated
+        end
+
+        it "doesn't create a new user entry either" do
+          expect { subject }.not_to change { User.count }
+        end
+      end
     end
   end
 end