Merge branch 'feature/log-ldap-to-application-log' into 'master'

Log LDAP blocking/unblocking events to application log

See merge request !8042

changelogs/unreleased/feature-log-ldap-to-application-log.yml

+---
+title: Log LDAP blocking/unblocking events to application log
+merge_request: 8042
+author: Markus Koller

doc/administration/auth/ldap.md

@@ -298,8 +298,11 @@ LDAP server please double-check the LDAP `port` and `method` settings used by
 GitLab. Common combinations are `method: 'plain'` and `port: 389`, OR
 `method: 'ssl'` and `port: 636`.
 
-### Login with valid credentials rejected
+### Troubleshooting
 
-If there is an unexpected error while authenticating the user with the LDAP
-backend, the login is rejected and details about the error are logged to
+If a user account is blocked or unblocked due to the LDAP configuration, a
+message will be logged to `application.log`.
+
+If there is an unexpected error during an LDAP lookup (configuration error,
+timeout), the login is rejected and a message will be logged to
 `production.log`.

lib/gitlab/ldap/access.rb

@@ -34,21 +34,21 @@ module Gitlab
       def allowed?
         if ldap_user
           unless ldap_config.active_directory
-            user.activate if user.ldap_blocked?
+            unblock_user(user, 'is available again') if user.ldap_blocked?
             return true
           end
 
           # Block user in GitLab if he/she was blocked in AD
           if Gitlab::LDAP::Person.disabled_via_active_directory?(user.ldap_identity.extern_uid, adapter)
-            user.ldap_block
+            block_user(user, 'is disabled in Active Directory')
             false
           else
-            user.activate if user.ldap_blocked?
+            unblock_user(user, 'is not disabled anymore') if user.ldap_blocked?
             true
           end
         else
           # Block the user if they no longer exist in LDAP/AD
-          user.ldap_block
+          block_user(user, 'does not exist anymore')
           false
         end
       end
@@ -64,6 +64,24 @@ module Gitlab
       def ldap_user
         @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
       end
+
+      def block_user(user, reason)
+        user.ldap_block
+
+        Gitlab::AppLogger.info(
+          "LDAP account \"#{user.ldap_identity.extern_uid}\" #{reason}, " +
+          "blocking Gitlab user \"#{user.name}\" (#{user.email})"
+        )
+      end
+
+      def unblock_user(user, reason)
+        user.activate
+
+        Gitlab::AppLogger.info(
+          "LDAP account \"#{user.ldap_identity.extern_uid}\" #{reason}, " +
+          "unblocking Gitlab user \"#{user.name}\" (#{user.email})"
+        )
+      end
     end
   end
 end

spec/lib/gitlab/ldap/access_spec.rb

@@ -15,9 +15,9 @@ describe Gitlab::LDAP::Access, lib: true do
       it { is_expected.to be_falsey }
 
       it 'should block user in GitLab' do
+        expect(access).to receive(:block_user).with(user, 'does not exist anymore')
+
         access.allowed?
-        expect(user).to be_blocked
-        expect(user).to be_ldap_blocked
       end
     end
 
@@ -34,9 +34,9 @@ describe Gitlab::LDAP::Access, lib: true do
         it { is_expected.to be_falsey }
 
         it 'blocks user in GitLab' do
+          expect(access).to receive(:block_user).with(user, 'is disabled in Active Directory')
+
           access.allowed?
-          expect(user).to be_blocked
-          expect(user).to be_ldap_blocked
         end
       end
 
@@ -53,7 +53,10 @@ describe Gitlab::LDAP::Access, lib: true do
           end
 
           it 'does not unblock user in GitLab' do
+            expect(access).not_to receive(:unblock_user)
+
             access.allowed?
+
             expect(user).to be_blocked
             expect(user).not_to be_ldap_blocked # this block is handled by omniauth not by our internal logic
           end
@@ -65,8 +68,9 @@ describe Gitlab::LDAP::Access, lib: true do
           end
 
           it 'unblocks user in GitLab' do
+            expect(access).to receive(:unblock_user).with(user, 'is not disabled anymore')
+
             access.allowed?
-            expect(user).not_to be_blocked
           end
         end
       end
@@ -87,9 +91,9 @@ describe Gitlab::LDAP::Access, lib: true do
           it { is_expected.to be_falsey }
 
           it 'blocks user in GitLab' do
+            expect(access).to receive(:block_user).with(user, 'does not exist anymore')
+
             access.allowed?
-            expect(user).to be_blocked
-            expect(user).to be_ldap_blocked
           end
         end
 
@@ -99,11 +103,54 @@ describe Gitlab::LDAP::Access, lib: true do
           end
 
           it 'unblocks the user if it exists' do
+            expect(access).to receive(:unblock_user).with(user, 'is available again')
+
             access.allowed?
-            expect(user).not_to be_blocked
           end
         end
       end
     end
   end
+
+  describe '#block_user' do
+    before do
+      user.activate
+      allow(Gitlab::AppLogger).to receive(:info)
+
+      access.block_user user, 'reason'
+    end
+
+    it 'blocks the user' do
+      expect(user).to be_blocked
+      expect(user).to be_ldap_blocked
+    end
+
+    it 'logs the reason' do
+      expect(Gitlab::AppLogger).to have_received(:info).with(
+        "LDAP account \"123456\" reason, " +
+        "blocking Gitlab user \"#{user.name}\" (#{user.email})"
+      )
+    end
+  end
+
+  describe '#unblock_user' do
+    before do
+      user.ldap_block
+      allow(Gitlab::AppLogger).to receive(:info)
+
+      access.unblock_user user, 'reason'
+    end
+
+    it 'activates the user' do
+      expect(user).not_to be_blocked
+      expect(user).not_to be_ldap_blocked
+    end
+
+    it 'logs the reason' do
+      Gitlab::AppLogger.info(
+        "LDAP account \"123456\" reason, " +
+        "unblocking Gitlab user \"#{user.name}\" (#{user.email})"
+      )
+    end
+  end
 end