Merge branch 'patch/fix-ldap-unblock-user-logic' into 'master'

Unblocks user when active_directory is disabled and it can be found

We implemented a specific block state to handle user blocking that originates from LDAP filtering rules / directory state in !2242.

That introduced a regression in LDAP authentication when Active Directory support was disabled. You could have a scenario where the user would not be temporarily found (like a filtering rule), that would mark the user as `ldap_blocked`, but will never unblock it automatically when that state changed.

Fixes #14253, #13179, #13259, #13959

See merge request !3550

3 files changed, 31 insertions, 21 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 78635d3ab78..fe1cc494db1 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,30 +1,12 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
-v 8.7.0 (unreleased)
-  - Don't attempt to look up an avatar in repo if repo directory does not exist (Stan hu)
-  - All images in discussions and wikis now link to their source files !3464 (Connor Shea).
-  - Improved Markdown rendering performance !3389 (Yorick Peterse)
-  - Don't attempt to look up an avatar in repo if repo directory does not exist (Stan Hu)
-  - Preserve time notes/comments have been updated at when moving issue
-  - Make HTTP(s) label consistent on clone bar (Stan Hu)
-  - Expose label description in API (Mariusz Jachimowicz)
-  - Allow back dating on issues when created through the API
-  - Fix avatar stretching by providing a cropping feature
-  - Add links to CI setup documentation from project settings and builds pages
-  - Handle nil descriptions in Slack issue messages (Stan Hu)
-  - Add default scope to projects to exclude projects pending deletion
-  - Implement 'Groups View' as an option for dashboard preferences !3379 (Elias W.)
-  - Implement 'TODOs View' as an option for dashboard preferences !3379 (Elias W.)
-  - Gracefully handle notes on deleted commits in merge requests (Stan Hu)
-  - Fall back to `In-Reply-To` and `References` headers when sub-addressing is not available (David Padilla)
-  - Remove "Congratulations!" tweet button on newly-created project. (Connor Shea)
-
 v 8.6.5
   - Fix importing from GitHub Enterprise. !3529
   - Perform the language detection after updating merge requests in `GitPushService`, leading to faster visual feedback for the end-user. !3533
   - Check permissions when user attempts to import members from another project. !3535
   - Only update repository language if it is not set to improve performance. !3556
   - Return status code 303 after a branch DELETE operation to avoid project deletion (Stan Hu). !3583
+  - Unblock user when active_directory is disabled and it can be found !3550
   - Fix a 2FA authentication spoofing vulnerability.
 
 v 8.6.4
diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index da4435c7308..f2b649e50a2 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -33,7 +33,10 @@ module Gitlab
 
       def allowed?
         if ldap_user
-          return true unless ldap_config.active_directory
+          unless ldap_config.active_directory
+            user.activate 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)
diff --git a/spec/lib/gitlab/ldap/access_spec.rb b/spec/lib/gitlab/ldap/access_spec.rb
index 32a19bf344b..f5b66b8156f 100644
--- a/spec/lib/gitlab/ldap/access_spec.rb
+++ b/spec/lib/gitlab/ldap/access_spec.rb
@@ -33,7 +33,7 @@ describe Gitlab::LDAP::Access, lib: true do
 
         it { is_expected.to be_falsey }
 
-        it 'should block user in GitLab' do
+        it 'blocks user in GitLab' do
           access.allowed?
           expect(user).to be_blocked
           expect(user).to be_ldap_blocked
@@ -78,6 +78,31 @@ describe Gitlab::LDAP::Access, lib: true do
         end
 
         it { is_expected.to be_truthy }
+
+        context 'when user cannot be found' do
+          before do
+            allow(Gitlab::LDAP::Person).to receive(:find_by_dn).and_return(nil)
+          end
+
+          it { is_expected.to be_falsey }
+
+          it 'blocks user in GitLab' do
+            access.allowed?
+            expect(user).to be_blocked
+            expect(user).to be_ldap_blocked
+          end
+        end
+
+        context 'when user was previously ldap_blocked' do
+          before do
+            user.ldap_block
+          end
+
+          it 'unblocks the user if it exists' do
+            access.allowed?
+            expect(user).not_to be_blocked
+          end
+        end
       end
     end
   end