Don't perform Devise trackable updates on blocked User recordsrs-warden-blocked-users

11 files changed, 54 insertions, 76 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index bb47e2a8bf7..bf6be3d516b 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -12,7 +12,6 @@ class ApplicationController < ActionController::Base
   before_action :authenticate_user_from_private_token!
   before_action :authenticate_user!
   before_action :validate_user_service_ticket!
-  before_action :reject_blocked!
   before_action :check_password_expiration
   before_action :check_2fa_requirement
   before_action :ldap_security_check
@@ -87,22 +86,8 @@ class ApplicationController < ActionController::Base
     logger.error "\n#{exception.class.name} (#{exception.message}):\n#{application_trace.join}"
   end
 
-  def reject_blocked!
-    if current_user && current_user.blocked?
-      sign_out current_user
-      flash[:alert] = "Your account is blocked. Retry when an admin has unblocked it."
-      redirect_to new_user_session_path
-    end
-  end
-
   def after_sign_in_path_for(resource)
-    if resource.is_a?(User) && resource.respond_to?(:blocked?) && resource.blocked?
-      sign_out resource
-      flash[:alert] = "Your account is blocked. Retry when an admin has unblocked it."
-      new_user_session_path
-    else
-      stored_location_for(:redirect) || stored_location_for(resource) || root_path
-    end
+    stored_location_for(:redirect) || stored_location_for(resource) || root_path
   end
 
   def after_sign_out_path_for(resource)
diff --git a/app/controllers/explore/application_controller.rb b/app/controllers/explore/application_controller.rb
index a1ab8b99048..baf54520b9c 100644
--- a/app/controllers/explore/application_controller.rb
+++ b/app/controllers/explore/application_controller.rb
@@ -1,5 +1,5 @@
 class Explore::ApplicationController < ApplicationController
-  skip_before_action :authenticate_user!, :reject_blocked!
+  skip_before_action :authenticate_user!
 
   layout 'explore'
 end
diff --git a/app/controllers/help_controller.rb b/app/controllers/help_controller.rb
index 37feff79999..87c0f8905ff 100644
--- a/app/controllers/help_controller.rb
+++ b/app/controllers/help_controller.rb
@@ -1,5 +1,5 @@
 class HelpController < ApplicationController
-  skip_before_action :authenticate_user!, :reject_blocked!
+  skip_before_action :authenticate_user!
 
   layout 'help'
 
diff --git a/app/controllers/koding_controller.rb b/app/controllers/koding_controller.rb
index f3759b4c0ea..6b1e64ce819 100644
--- a/app/controllers/koding_controller.rb
+++ b/app/controllers/koding_controller.rb
@@ -1,5 +1,5 @@
 class KodingController < ApplicationController
-  before_action :check_integration!, :authenticate_user!, :reject_blocked!
+  before_action :check_integration!
   layout 'koding'
 
   def index
diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb
index 50ba33ed570..61686499bd3 100644
--- a/app/controllers/projects/uploads_controller.rb
+++ b/app/controllers/projects/uploads_controller.rb
@@ -1,6 +1,6 @@
 class Projects::UploadsController < Projects::ApplicationController
-  skip_before_action :reject_blocked!, :project,
-    :repository, if: -> { action_name == 'show' && image_or_video? }
+  skip_before_action :project, :repository,
+    if: -> { action_name == 'show' && image_or_video? }
 
   before_action :authorize_upload_file!, only: [:create]
 
diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb
index 6576ebd5235..612d69cf557 100644
--- a/app/controllers/search_controller.rb
+++ b/app/controllers/search_controller.rb
@@ -1,5 +1,5 @@
 class SearchController < ApplicationController
-  skip_before_action :authenticate_user!, :reject_blocked!
+  skip_before_action :authenticate_user!
 
   include SearchHelper
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 33666b4f35b..f9245cdb82a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -167,6 +167,15 @@ class User < ActiveRecord::Base
       def blocked?
         true
       end
+
+      def active_for_authentication?
+        false
+      end
+
+      def inactive_message
+        "Your account has been blocked. Please contact your GitLab " \
+          "administrator if you think this is an error."
+      end
     end
   end
 
diff --git a/changelogs/unreleased/rs-warden-blocked-users.yml b/changelogs/unreleased/rs-warden-blocked-users.yml
new file mode 100644
index 00000000000..c0c23fb6f11
--- /dev/null
+++ b/changelogs/unreleased/rs-warden-blocked-users.yml
@@ -0,0 +1,4 @@
+---
+title: Don't perform Devise trackable updates on blocked User records
+merge_request: 8915
+author:
diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb
index f1c8891e87a..0347e789576 100644
--- a/spec/controllers/projects/uploads_controller_spec.rb
+++ b/spec/controllers/projects/uploads_controller_spec.rb
@@ -170,68 +170,24 @@ describe Projects::UploadsController do
             project.team << [user, :master]
           end
 
-          context "when the user is blocked" do
+          context "when the file exists" do
             before do
-              user.block
-              project.team << [user, :master]
-            end
-
-            context "when the file exists" do
-              before do
-                allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-                allow(jpg).to receive(:exists?).and_return(true)
-              end
-
-              context "when the file is an image" do
-                before do
-                  allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
-                end
-
-                it "responds with status 200" do
-                  go
-
-                  expect(response).to have_http_status(200)
-                end
-              end
-
-              context "when the file is not an image" do
-                it "redirects to the sign in page" do
-                  go
-
-                  expect(response).to redirect_to(new_user_session_path)
-                end
-              end
+              allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
+              allow(jpg).to receive(:exists?).and_return(true)
             end
 
-            context "when the file doesn't exist" do
-              it "redirects to the sign in page" do
-                go
+            it "responds with status 200" do
+              go
 
-                expect(response).to redirect_to(new_user_session_path)
-              end
+              expect(response).to have_http_status(200)
             end
           end
 
-          context "when the user isn't blocked" do
-            context "when the file exists" do
-              before do
-                allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-                allow(jpg).to receive(:exists?).and_return(true)
-              end
-
-              it "responds with status 200" do
-                go
-
-                expect(response).to have_http_status(200)
-              end
-            end
-
-            context "when the file doesn't exist" do
-              it "responds with status 404" do
-                go
+          context "when the file doesn't exist" do
+            it "responds with status 404" do
+              go
 
-                expect(response).to have_http_status(404)
-              end
+              expect(response).to have_http_status(404)
             end
           end
         end
diff --git a/spec/factories/users.rb b/spec/factories/users.rb
index c6f7869516e..1732b1a0081 100644
--- a/spec/factories/users.rb
+++ b/spec/factories/users.rb
@@ -14,6 +14,14 @@ FactoryGirl.define do
       admin true
     end
 
+    trait :blocked do
+      after(:build) { |user, _| user.block! }
+    end
+
+    trait :external do
+      external true
+    end
+
     trait :two_factor do
       two_factor_via_otp
     end
diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb
index ab7d89306db..ae609160e18 100644
--- a/spec/features/login_spec.rb
+++ b/spec/features/login_spec.rb
@@ -32,6 +32,22 @@ feature 'Login', feature: true do
     end
   end
 
+  describe 'with a blocked account' do
+    it 'prevents the user from logging in' do
+      user = create(:user, :blocked)
+
+      login_with(user)
+
+      expect(page).to have_content('Your account has been blocked.')
+    end
+
+    it 'does not update Devise trackable attributes' do
+      user = create(:user, :blocked)
+
+      expect { login_with(user) }.not_to change { user.reload.sign_in_count }
+    end
+  end
+
   describe 'with two-factor authentication' do
     def enter_code(code)
       fill_in 'user_otp_attempt', with: code