From 191bcb4d1b5e983583e183d8945f638604c7f0e1 Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Wed, 1 Feb 2017 13:21:28 -0500
Subject: [PATCH] Don't perform Devise trackable updates on blocked User
records
---
app/controllers/application_controller.rb | 17 +----
.../explore/application_controller.rb | 2 +-
app/controllers/help_controller.rb | 2 +-
app/controllers/koding_controller.rb | 2 +-
.../projects/uploads_controller.rb | 4 +-
app/controllers/search_controller.rb | 2 +-
app/models/user.rb | 9 +++
.../unreleased/rs-warden-blocked-users.yml | 4 ++
.../projects/uploads_controller_spec.rb | 64 +++----------------
spec/factories/users.rb | 8 +++
spec/features/login_spec.rb | 16 +++++
11 files changed, 54 insertions(+), 76 deletions(-)
create mode 100644 changelogs/unreleased/rs-warden-blocked-users.yml
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
--
GitLab