From 436ed61ed57a269de42d8bcc924e6b1ad0932617 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 8 Aug 2016 15:36:58 +0100 Subject: [PATCH 01/22] Remove helper method declaration The method `require_account` does not exist so there's no need for it to be declared a helper method. --- app/controllers/concerns/authentication.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 97a514191..717383aec 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -16,7 +16,7 @@ # before_filter :login_required, :except => [:index, :show] module Authentication def self.included(controller) - controller.send :helper_method, :current_account, :current_user, :logged_in?, :redirect_to_target_or_default, :require_admin + controller.send :helper_method, :current_user, :logged_in?, :redirect_to_target_or_default, :require_admin end def current_session From 3f2cf379791bc14d2478b1057de53bc07c0a33bd Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 8 Aug 2016 15:53:09 +0100 Subject: [PATCH 02/22] Don't expose controller-only methods to views Only methods that are likely to be used in views should be exposed as helper methods - before action methods and redirects will never be used. --- app/controllers/concerns/authentication.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 717383aec..df87ff0f3 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -16,7 +16,7 @@ # before_filter :login_required, :except => [:index, :show] module Authentication def self.included(controller) - controller.send :helper_method, :current_user, :logged_in?, :redirect_to_target_or_default, :require_admin + controller.send :helper_method, :current_user, :logged_in? end def current_session From fe038191402b2f8165c4cb83905b6b4391aaadc2 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 8 Aug 2016 17:14:13 +0100 Subject: [PATCH 03/22] Use AS::Concern style Migrate to using the ActiveSupport::Concern style so that we can use an included block to define our helper methods instead of using send. --- app/controllers/concerns/authentication.rb | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index df87ff0f3..e59bc9dab 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -1,22 +1,8 @@ -# This module is included in your application controller which makes -# several methods available to all controllers and views. Here's a -# common example you might add to your application layout file. -# -# <% if logged_in? %> -# Welcome <%=h current_account.username %>! Not you? -# <%= link_to "Log out", logout_path %> -# <% else %> -# <%= link_to "Sign up", signup_path %> or -# <%= link_to "log in", login_path %>. -# <% end %> -# -# You can also restrict unregistered users from accessing a controller using -# a before filter. For example. -# -# before_filter :login_required, :except => [:index, :show] module Authentication - def self.included(controller) - controller.send :helper_method, :current_user, :logged_in? + extend ActiveSupport::Concern + + included do + helper_method :current_user, :logged_in? end def current_session From 7e9779c6318ed2171b6b724c18a1b46b563181cc Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 8 Aug 2016 17:27:32 +0100 Subject: [PATCH 04/22] Remove unused method --- app/controllers/concerns/authentication.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index e59bc9dab..6ec4fc15f 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -19,14 +19,6 @@ def logged_in? current_user end - def login_required - unless logged_in? - flash[:error] = "You must first log in or sign up before accessing this page." - store_target_location - redirect_to admin_login_url - end - end - def redirect_to_target_or_default redirect_to(session[:return_to] || admin_root_url) session[:return_to] = nil From 4ef54d44f9c36efbff97e286cbda4af930ef0e85 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 8 Aug 2016 17:52:46 +0100 Subject: [PATCH 05/22] Move before_action declaration to included block --- app/controllers/admin/admin_controller.rb | 1 - app/controllers/concerns/authentication.rb | 1 + config/initializers/delayed_web.rb | 1 - 3 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/admin/admin_controller.rb b/app/controllers/admin/admin_controller.rb index 622180cf8..60d4837e0 100644 --- a/app/controllers/admin/admin_controller.rb +++ b/app/controllers/admin/admin_controller.rb @@ -1,7 +1,6 @@ class Admin::AdminController < ApplicationController include Authentication - before_action :require_admin_and_check_for_password_change before_action :do_not_cache layout 'admin' diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 6ec4fc15f..e85aecfad 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -2,6 +2,7 @@ module Authentication extend ActiveSupport::Concern included do + before_action :require_admin_and_check_for_password_change helper_method :current_user, :logged_in? end diff --git a/config/initializers/delayed_web.rb b/config/initializers/delayed_web.rb index 0c993f8eb..fe181dccd 100644 --- a/config/initializers/delayed_web.rb +++ b/config/initializers/delayed_web.rb @@ -6,7 +6,6 @@ # Authenticate our delayed job web interface Delayed::Web::ApplicationController.class_eval do include Authentication - before_filter :require_admin_and_check_for_password_change def admin_login_url main_app.admin_login_url From c3485969acfacb9798c74393ebfd1725446ca902 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 8 Aug 2016 18:06:23 +0100 Subject: [PATCH 06/22] Remove flash[:error] block from public messages template The app doesn't generate flash[:error] messages for public consumption so there's no need to have this code in the messages template. --- app/views/application/_messages.html.erb | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/views/application/_messages.html.erb b/app/views/application/_messages.html.erb index 113214f41..076bbe1f4 100644 --- a/app/views/application/_messages.html.erb +++ b/app/views/application/_messages.html.erb @@ -1,6 +1,3 @@ <% if flash[:notice] %>

<%= flash[:notice] %>

<% end %> -<% if flash[:error] %> -

<%= flash[:error] %>

-<% end %> From 5d5d6fd3c88559c2e9fe4b734ccce9f14bb76381 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 8 Aug 2016 18:08:36 +0100 Subject: [PATCH 07/22] Switch from flash[:error] to flash[:alert] Using flash[:alert] is more in line with 'The Rails Way' in that you can pass it to redirect and it will automatically be assigned. --- app/controllers/admin/admin_users_controller.rb | 4 ++-- app/controllers/concerns/authentication.rb | 11 ++++------- spec/controllers/admin/admin_users_controller_spec.rb | 2 +- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 88cbb8c4a..54071a9d9 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -38,9 +38,9 @@ def destroy # only destroy if user is not the logged in user and there are at least 2 users if @user == current_user - flash[:error] = "You are not allowed to delete yourself!" + flash[:alert] = "You are not allowed to delete yourself!" elsif AdminUser.count < 2 - flash[:error] = "There needs to be at least 1 admin user" + flash[:alert] = "There needs to be at least 1 admin user" else @user.destroy end diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index e85aecfad..71d59d4c4 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -27,24 +27,21 @@ def redirect_to_target_or_default def require_admin unless current_user - flash[:error] = "You must be logged in as an administrator to view this page." - redirect_to admin_login_url + redirect_to admin_login_url, alert: "You must be logged in as an administrator to view this page." end end def require_admin_and_check_for_password_change if current_user.nil? - flash[:error] = "You must be logged in as an administrator to view this page." - redirect_to admin_login_url + redirect_to admin_login_url, alert: "You must be logged in as an administrator to view this page." elsif current_user.has_to_change_password? - flash[:error] = "Please change your password before continuing" - redirect_to edit_admin_profile_url(current_user) + redirect_to edit_admin_profile_url(current_user), alert: "Please change your password before continuing" end end def require_sysadmin unless current_user && current_user.is_a_sysadmin? - flash[:error] = "You must be logged in as a system administrator to view this page." + flash[:alert] = "You must be logged in as a system administrator to view this page." if current_user.is_a_moderator? redirect_to admin_root_url diff --git a/spec/controllers/admin/admin_users_controller_spec.rb b/spec/controllers/admin/admin_users_controller_spec.rb index b3f7ff470..dbf4b6b89 100644 --- a/spec/controllers/admin/admin_users_controller_spec.rb +++ b/spec/controllers/admin/admin_users_controller_spec.rb @@ -210,7 +210,7 @@ def do_update delete :destroy, :id => user.to_param expect(AdminUser.exists?(user.id)).to be_truthy expect(response).to redirect_to(:action => :index) - expect(flash[:error]).to eq "You are not allowed to delete yourself!" + expect(flash[:alert]).to eq "You are not allowed to delete yourself!" end end end From 6a15e6d059f8984f2d52612d5f699f0de9c40d99 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 8 Aug 2016 18:38:04 +0100 Subject: [PATCH 08/22] Refactor before action filters to be more granular Having a single monolithic before action that does everthing makes it harder to skip the right functionality when editing your profile or signing in so by splitting it up into two parts we can only override the parts we need to. --- app/controllers/admin/profile_controller.rb | 3 +-- .../admin/user_sessions_controller.rb | 3 ++- app/controllers/concerns/authentication.rb | 20 +++++++------------ 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/app/controllers/admin/profile_controller.rb b/app/controllers/admin/profile_controller.rb index 781941f48..436d4359e 100644 --- a/app/controllers/admin/profile_controller.rb +++ b/app/controllers/admin/profile_controller.rb @@ -1,6 +1,5 @@ class Admin::ProfileController < Admin::AdminController - skip_before_filter :require_admin_and_check_for_password_change - before_filter :require_admin + skip_before_filter :check_for_password_change def edit diff --git a/app/controllers/admin/user_sessions_controller.rb b/app/controllers/admin/user_sessions_controller.rb index 93ead7bcb..51b54fa37 100644 --- a/app/controllers/admin/user_sessions_controller.rb +++ b/app/controllers/admin/user_sessions_controller.rb @@ -1,5 +1,6 @@ class Admin::UserSessionsController < Admin::AdminController - skip_before_filter :require_admin_and_check_for_password_change + skip_before_filter :require_admin, only: [:new, :create] + skip_before_filter :check_for_password_change def new @user_session = AdminUserSession.new diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 71d59d4c4..e2bebfe99 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -2,7 +2,9 @@ module Authentication extend ActiveSupport::Concern included do - before_action :require_admin_and_check_for_password_change + before_action :require_admin + before_action :check_for_password_change + helper_method :current_user, :logged_in? end @@ -31,23 +33,15 @@ def require_admin end end - def require_admin_and_check_for_password_change - if current_user.nil? - redirect_to admin_login_url, alert: "You must be logged in as an administrator to view this page." - elsif current_user.has_to_change_password? + def check_for_password_change + if current_user.has_to_change_password? redirect_to edit_admin_profile_url(current_user), alert: "Please change your password before continuing" end end def require_sysadmin - unless current_user && current_user.is_a_sysadmin? - flash[:alert] = "You must be logged in as a system administrator to view this page." - - if current_user.is_a_moderator? - redirect_to admin_root_url - else - redirect_to admin_login_url - end + unless current_user.is_a_sysadmin? + redirect_to admin_root_url, alert: "You must be logged in as a system administrator to view this page." end end From ebb56215ded62d7524c90b58947e2c1056edbb55 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Mon, 8 Aug 2016 18:54:25 +0100 Subject: [PATCH 09/22] Use idiomatic redirect with :notice message --- app/controllers/admin/admin_users_controller.rb | 6 ++---- app/controllers/admin/petition_details_controller.rb | 3 +-- app/controllers/admin/user_sessions_controller.rb | 3 +-- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index 54071a9d9..eea394aa2 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -12,8 +12,7 @@ def new def create @user = AdminUser.create(admin_user_params) if @user.save - flash[:notice] = "User was successfully created" - redirect_to admin_admin_users_url + redirect_to admin_admin_users_url, notice: "User was successfully created" else render :action => 'new' end @@ -26,8 +25,7 @@ def edit def update @user = AdminUser.find(params[:id]) if @user.update_attributes(admin_user_params) - flash[:notice] = "User was successfully updated" - redirect_to admin_admin_users_url + redirect_to admin_admin_users_url, notice: "User was successfully updated" else render :action => 'edit' end diff --git a/app/controllers/admin/petition_details_controller.rb b/app/controllers/admin/petition_details_controller.rb index a27400c8a..bd398c273 100644 --- a/app/controllers/admin/petition_details_controller.rb +++ b/app/controllers/admin/petition_details_controller.rb @@ -7,8 +7,7 @@ def show def update if @petition.update_attributes(petition_params) - flash[:notice] = 'Petition has been successfully updated' - redirect_to [:admin, @petition] + redirect_to [:admin, @petition], notice: "Petition has been successfully updated" else render :show end diff --git a/app/controllers/admin/user_sessions_controller.rb b/app/controllers/admin/user_sessions_controller.rb index 51b54fa37..69dbdefd9 100644 --- a/app/controllers/admin/user_sessions_controller.rb +++ b/app/controllers/admin/user_sessions_controller.rb @@ -24,8 +24,7 @@ def create def destroy current_session.destroy if logged_in? - flash[:notice] = "You have been logged out." - redirect_to admin_login_url + redirect_to admin_login_url, notice: "You have been logged out." end end From 07234ae705259319c82c7bc6fa53a1ddd0f4efd4 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 9 Aug 2016 22:06:33 +0100 Subject: [PATCH 10/22] Move admin flash messages to locale file To streamline the translation of the I18n keys override redirect_to so that symbol keys are translated. If substitution is required then pass an array with the last element being the substitution hash. Also override render so that we can use :alert and :notice in a similar fashion to how redirect_to uses them with the exception that they go into flash.now and not the standard flash. As part of this some logic around admin users was pushed down into the model instead of the controller so that we can use model based I18n. --- app/controllers/admin/admin_controller.rb | 3 +- .../admin/admin_users_controller.rb | 42 +-- .../admin/debate_outcomes_controller.rb | 4 +- .../admin/government_response_controller.rb | 4 +- .../admin/invalidations_controller.rb | 32 +-- .../admin/petition_details_controller.rb | 2 +- .../admin/petition_emails_controller.rb | 12 +- app/controllers/admin/petitions_controller.rb | 2 +- app/controllers/admin/profile_controller.rb | 31 +-- .../admin/rate_limits_controller.rb | 2 +- .../admin/schedule_debate_controller.rb | 4 +- app/controllers/admin/searches_controller.rb | 2 +- .../admin/signatures_controller.rb | 12 +- .../admin/user_sessions_controller.rb | 17 +- app/controllers/concerns/authentication.rb | 6 +- app/controllers/concerns/flash_i18n.rb | 30 +++ app/controllers/concerns/flash_render.rb | 23 ++ app/models/admin_user.rb | 61 ++++- app/models/admin_user_session.rb | 9 + app/views/admin/admin_users/index.html.erb | 8 +- config/initializers/delayed_web.rb | 4 +- config/locales/activerecord.en-GB.yml | 5 + config/locales/admin.en-GB.yml | 48 ++++ features/admin/admin_access.feature | 9 +- .../admin/admin_controller_spec.rb | 238 +++++++++++++++++ spec/models/admin_user_session_spec.rb | 51 ++++ spec/models/admin_user_spec.rb | 249 ++++++++++++++++-- 27 files changed, 787 insertions(+), 123 deletions(-) create mode 100644 app/controllers/concerns/flash_i18n.rb create mode 100644 app/controllers/concerns/flash_render.rb create mode 100644 spec/controllers/admin/admin_controller_spec.rb create mode 100644 spec/models/admin_user_session_spec.rb diff --git a/app/controllers/admin/admin_controller.rb b/app/controllers/admin/admin_controller.rb index 60d4837e0..6d941aeb2 100644 --- a/app/controllers/admin/admin_controller.rb +++ b/app/controllers/admin/admin_controller.rb @@ -1,5 +1,5 @@ class Admin::AdminController < ApplicationController - include Authentication + include Authentication, FlashI18n, FlashRender before_action :do_not_cache @@ -7,5 +7,4 @@ class Admin::AdminController < ApplicationController def index end - end diff --git a/app/controllers/admin/admin_users_controller.rb b/app/controllers/admin/admin_users_controller.rb index eea394aa2..78fbecf2f 100644 --- a/app/controllers/admin/admin_users_controller.rb +++ b/app/controllers/admin/admin_users_controller.rb @@ -1,8 +1,17 @@ class Admin::AdminUsersController < Admin::AdminController before_filter :require_sysadmin + before_filter :find_user, only: %i[edit update destroy] + + rescue_from AdminUser::CannotDeleteCurrentUser do + redirect_to admin_admin_users_url, alert: :user_is_current_user + end + + rescue_from AdminUser::MustBeAtLeastOneAdminUser do + redirect_to admin_admin_users_url, alert: :user_count_is_too_low + end def index - @users = AdminUser.by_name.paginate(:page => params[:page], :per_page => 50) + @users = AdminUser.by_name.paginate(page: params[:page], per_page: 50) end def new @@ -10,43 +19,40 @@ def new end def create - @user = AdminUser.create(admin_user_params) + @user = AdminUser.new(admin_user_params) + if @user.save - redirect_to admin_admin_users_url, notice: "User was successfully created" + redirect_to admin_admin_users_url, notice: :user_created else - render :action => 'new' + render :new end end def edit - @user = AdminUser.find(params[:id]) end def update - @user = AdminUser.find(params[:id]) - if @user.update_attributes(admin_user_params) - redirect_to admin_admin_users_url, notice: "User was successfully updated" + if @user.update(admin_user_params) + redirect_to admin_admin_users_url, notice: :user_updated else - render :action => 'edit' + render :edit end end def destroy - @user = AdminUser.find(params[:id]) - - # only destroy if user is not the logged in user and there are at least 2 users - if @user == current_user - flash[:alert] = "You are not allowed to delete yourself!" - elsif AdminUser.count < 2 - flash[:alert] = "There needs to be at least 1 admin user" + if @user.destroy(current_user: current_user) + redirect_to admin_admin_users_url, notice: :user_deleted else - @user.destroy + redirect_to admin_admin_users_url, alert: :user_not_deleted end - redirect_to admin_admin_users_url end protected + def find_user + @user = AdminUser.find(params[:id]) + end + def admin_user_params params. require(:admin_user). diff --git a/app/controllers/admin/debate_outcomes_controller.rb b/app/controllers/admin/debate_outcomes_controller.rb index 0e81cfc12..9a74a8d1b 100644 --- a/app/controllers/admin/debate_outcomes_controller.rb +++ b/app/controllers/admin/debate_outcomes_controller.rb @@ -15,9 +15,9 @@ def update if @debate_outcome.update(debate_outcome_params) if send_email_to_petitioners? EmailDebateOutcomesJob.run_later_tonight(petition: @petition) - message = 'Email will be sent overnight' + message = :email_sent_overnight else - message = 'Updated debate outcome successfully' + message = :debate_outcome_updated end redirect_to [:admin, @petition], notice: message diff --git a/app/controllers/admin/government_response_controller.rb b/app/controllers/admin/government_response_controller.rb index 60583ba4e..3afdcdb3d 100644 --- a/app/controllers/admin/government_response_controller.rb +++ b/app/controllers/admin/government_response_controller.rb @@ -15,9 +15,9 @@ def update if @government_response.update(government_response_params) if send_email_to_petitioners? EmailThresholdResponseJob.run_later_tonight(petition: @petition) - message = 'Email will be sent overnight' + message = :email_sent_overnight else - message = 'Updated government response successfully' + message = :government_response_updated end redirect_to [:admin, @petition], notice: message diff --git a/app/controllers/admin/invalidations_controller.rb b/app/controllers/admin/invalidations_controller.rb index 2ba932e68..60082a0ed 100644 --- a/app/controllers/admin/invalidations_controller.rb +++ b/app/controllers/admin/invalidations_controller.rb @@ -18,7 +18,7 @@ def new def create if @invalidation.save - redirect_to admin_invalidations_url, notice: "Invalidation created successfully" + redirect_to admin_invalidations_url, notice: :invalidation_created else respond_to do |format| format.html { render :new } @@ -32,44 +32,44 @@ def edit format.html end else - redirect_to_index_url notice: "Can't edit invalidations that aren't pending" + redirect_to_index_url notice: :invalidation_cant_be_edited end end def update if @invalidation.pending? if @invalidation.update(invalidation_params) - redirect_to admin_invalidations_url, notice: "Invalidation updated successfully" + redirect_to admin_invalidations_url, notice: :invalidation_updated else respond_to do |format| format.html { render :edit } end end else - redirect_to_index_url notice: "Can't edit invalidations that aren't pending" + redirect_to_index_url notice: :invalidation_cant_be_edited end end def destroy if @invalidation.started? - redirect_to_index_url notice: "Can't remove invalidations that have started" + redirect_to_index_url notice: :invalidation_cant_be_removed else if @invalidation.destroy - redirect_to_index_url notice: "Invalidation removed successfully" + redirect_to_index_url notice: :invalidation_removed else - redirect_to_index_url alert: "Invalidation could not be removed - please contact support" + redirect_to_index_url alert: :invalidation_not_removed end end end def cancel if @invalidation.completed? - redirect_to_index_url notice: "Can't cancel invalidations that have completed" + redirect_to_index_url notice: :invalidation_cant_be_cancelled else if @invalidation.cancel! - redirect_to_index_url notice: "Invalidation cancelled successfully" + redirect_to_index_url notice: :invalidation_cancelled else - redirect_to_index_url alert: "Invalidation could not be cancelled - please contact support" + redirect_to_index_url alert: :invalidation_not_cancelled end end end @@ -77,24 +77,24 @@ def cancel def count if @invalidation.pending? if @invalidation.count! - redirect_to_index_url notice: "Counted the matching signatures for invalidation #{@invalidation.summary.inspect}" + redirect_to_index_url notice: [:invalidation_counted, summary: @invalidation.summary.inspect] else - redirect_to_index_url alert: "Invalidation could not be counted - please contact support" + redirect_to_index_url alert: :invalidation_not_counted end else - redirect_to_index_url notice: "Can't count invalidations that aren't pending" + redirect_to_index_url notice: :invalidation_cant_be_counted end end def start if @invalidation.pending? if @invalidation.start! - redirect_to_index_url notice: "Enqueued the invalidation #{@invalidation.summary.inspect}" + redirect_to_index_url notice: [:invalidation_started, summary: @invalidation.summary.inspect] else - redirect_to_index_url alert: "Invalidation could not be enqueued - please contact support" + redirect_to_index_url alert: :invalidation_not_started end else - redirect_to_index_url notice: "Can't start invalidations that aren't pending" + redirect_to_index_url notice: :invalidation_cant_be_started end end diff --git a/app/controllers/admin/petition_details_controller.rb b/app/controllers/admin/petition_details_controller.rb index bd398c273..9d456a0b0 100644 --- a/app/controllers/admin/petition_details_controller.rb +++ b/app/controllers/admin/petition_details_controller.rb @@ -7,7 +7,7 @@ def show def update if @petition.update_attributes(petition_params) - redirect_to [:admin, @petition], notice: "Petition has been successfully updated" + redirect_to [:admin, @petition], notice: :petition_updated else render :show end diff --git a/app/controllers/admin/petition_emails_controller.rb b/app/controllers/admin/petition_emails_controller.rb index ca4422ab6..bae3625c5 100644 --- a/app/controllers/admin/petition_emails_controller.rb +++ b/app/controllers/admin/petition_emails_controller.rb @@ -12,9 +12,9 @@ def create if @email.update(email_params) if send_email_to_petitioners? schedule_email_petitioners_job - message = 'Email will be sent overnight' + message = :email_sent_overnight else - message = 'Created other parliamentary business successfully' + message = :petition_email_created end redirect_to [:admin, @petition], notice: message @@ -30,9 +30,9 @@ def update if @email.update(email_params) if send_email_to_petitioners? schedule_email_petitioners_job - message = 'Email will be sent overnight' + message = :email_sent_overnight else - message = 'Updated other parliamentary business successfully' + message = :petition_email_updated end redirect_to [:admin, @petition], notice: message @@ -43,9 +43,9 @@ def update def destroy if @email.destroy - message = 'Deleted other parliamentary business successfully' + message = :petition_email_deleted else - message = 'Unable to delete other parliamentary business - please contact support' + message = :petition_email_not_deleted end redirect_to [:admin, @petition], notice: message diff --git a/app/controllers/admin/petitions_controller.rb b/app/controllers/admin/petitions_controller.rb index 6e2346e3d..2e1f404e8 100644 --- a/app/controllers/admin/petitions_controller.rb +++ b/app/controllers/admin/petitions_controller.rb @@ -26,7 +26,7 @@ def update_scheduled_debate_date fetch_petition_for_scheduled_debate_date if @petition.update(update_scheduled_debate_date_params) EmailDebateScheduledJob.run_later_tonight(petition: @petition) - redirect_to admin_petition_url(@petition), notice: "Email will be sent overnight" + redirect_to admin_petition_url(@petition), notice: :email_sent_overnight else render :edit_scheduled_debate_date end diff --git a/app/controllers/admin/profile_controller.rb b/app/controllers/admin/profile_controller.rb index 436d4359e..36cbe9efa 100644 --- a/app/controllers/admin/profile_controller.rb +++ b/app/controllers/admin/profile_controller.rb @@ -2,34 +2,19 @@ class Admin::ProfileController < Admin::AdminController skip_before_filter :check_for_password_change def edit - end def update - # reset attributes that could be forcing a user to change their password - current_user.password_changed_at = Time.current - current_user.force_password_reset = false - - update_params = admin_user_params_for_update - - if ! current_user.valid_password?(current_password) - current_user.errors.add(:current_password, "is incorrect") - elsif current_password == params[:admin_user][:password] - current_user.errors.add(:password, "is the same as the current password") - elsif update_params[:password].present? and current_user.update_attributes(update_params) - flash[:notice] = "Password was successfully updated" - redirect_to admin_root_url and return + if current_user.update_with_password(admin_user_params) + redirect_to admin_root_url, notice: :password_updated + else + render :edit end - render :edit - end - - def current_password - params.require(:admin_user).fetch(:current_password, '') end - def admin_user_params_for_update - params. - require(:admin_user). - permit(:password, :password_confirmation) + def admin_user_params + params.require(:admin_user).permit( + :current_password, :password, :password_confirmation + ) end end diff --git a/app/controllers/admin/rate_limits_controller.rb b/app/controllers/admin/rate_limits_controller.rb index 4a0d783fb..77abd60f6 100644 --- a/app/controllers/admin/rate_limits_controller.rb +++ b/app/controllers/admin/rate_limits_controller.rb @@ -10,7 +10,7 @@ def edit def update if @rate_limit.update(rate_limit_params) - redirect_to edit_admin_rate_limits_url, notice: "Rate limits updated successfully" + redirect_to edit_admin_rate_limits_url, notice: :rate_limits_updated else respond_to do |format| format.html { render :edit } diff --git a/app/controllers/admin/schedule_debate_controller.rb b/app/controllers/admin/schedule_debate_controller.rb index 379dc789f..9ee8569b4 100644 --- a/app/controllers/admin/schedule_debate_controller.rb +++ b/app/controllers/admin/schedule_debate_controller.rb @@ -10,9 +10,9 @@ def update if @petition.update_attributes(params_for_update) if send_email_to_petitioners? EmailDebateScheduledJob.run_later_tonight(petition: @petition) - message = 'Email will be sent overnight' + message = :email_sent_overnight else - message = 'Updated the scheduled debate date successfully' + message = :debate_date_updated end redirect_to [:admin, @petition], notice: message diff --git a/app/controllers/admin/searches_controller.rb b/app/controllers/admin/searches_controller.rb index 500ed3357..bc93fec50 100644 --- a/app/controllers/admin/searches_controller.rb +++ b/app/controllers/admin/searches_controller.rb @@ -42,7 +42,7 @@ def find_petition_by_id begin redirect_to admin_petition_url(Petition.find(query.to_i)) rescue ActiveRecord::RecordNotFound - redirect_to admin_petitions_url, alert: "Cannot find petition with id: #{query}" + redirect_to admin_petitions_url, alert: [:petition_not_found, query: query.inspect] end end diff --git a/app/controllers/admin/signatures_controller.rb b/app/controllers/admin/signatures_controller.rb index 8c2e27dda..8ddd53c77 100644 --- a/app/controllers/admin/signatures_controller.rb +++ b/app/controllers/admin/signatures_controller.rb @@ -4,28 +4,28 @@ class Admin::SignaturesController < Admin::AdminController def validate begin @signature.validate! - redirect_to admin_search_url(q: params[:q]), notice: "Signature validated successfully" + redirect_to admin_search_url(q: params[:q]), notice: :signature_validated rescue StandardError => e Appsignal.send_exception e - redirect_to admin_search_url(q: params[:q]), alert: "Signature could not be validated - please contact support" + redirect_to admin_search_url(q: params[:q]), alert: :signature_not_validated end end def invalidate begin @signature.invalidate! - redirect_to admin_search_url(q: params[:q]), notice: "Signature invalidated successfully" + redirect_to admin_search_url(q: params[:q]), notice: :signature_invalidated rescue StandardError => e Appsignal.send_exception e - redirect_to admin_search_url(q: @signature.email), alert: "Signature could not be invalidated - please contact support" + redirect_to admin_search_url(q: @signature.email), alert: :signature_not_invalidated end end def destroy if @signature.destroy - redirect_to admin_search_url(q: params[:q]), notice: "Signature removed successfully" + redirect_to admin_search_url(q: params[:q]), notice: :signature_deleted else - redirect_to admin_search_url(q: params[:q]), alert: "Signature could not be removed - please contact support" + redirect_to admin_search_url(q: params[:q]), alert: :signature_not_deleted end end diff --git a/app/controllers/admin/user_sessions_controller.rb b/app/controllers/admin/user_sessions_controller.rb index 69dbdefd9..80f73683f 100644 --- a/app/controllers/admin/user_sessions_controller.rb +++ b/app/controllers/admin/user_sessions_controller.rb @@ -8,23 +8,20 @@ def new def create @user_session = AdminUserSession.new(params[:admin_user_session]) + if @user_session.save redirect_to_target_or_default - - # if failed logins are above the specified level, then authlogic disables account - # so we need to display appropriate error message - elsif @user_session.errors[:base].size > 0 - flash.now[:alert] = @user_session.errors[:base][0] - render :new + elsif @user_session.last_login_attempt? + render :new, alert: :last_login + elsif @user_session.being_brute_force_protected? + render :new, alert: :disabled_login else - flash.now[:alert] = "Invalid email/password combination" - render :new + render :new, alert: :invalid_login end end def destroy current_session.destroy if logged_in? - redirect_to admin_login_url, notice: "You have been logged out." + redirect_to admin_login_url, notice: :logged_out end end - diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index e2bebfe99..1e4e39760 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -29,19 +29,19 @@ def redirect_to_target_or_default def require_admin unless current_user - redirect_to admin_login_url, alert: "You must be logged in as an administrator to view this page." + redirect_to admin_login_url, alert: :admin_required end end def check_for_password_change if current_user.has_to_change_password? - redirect_to edit_admin_profile_url(current_user), alert: "Please change your password before continuing" + redirect_to edit_admin_profile_url(current_user), alert: :change_password end end def require_sysadmin unless current_user.is_a_sysadmin? - redirect_to admin_root_url, alert: "You must be logged in as a system administrator to view this page." + redirect_to admin_root_url, alert: :sysadmin_required end end diff --git a/app/controllers/concerns/flash_i18n.rb b/app/controllers/concerns/flash_i18n.rb new file mode 100644 index 000000000..4330d1e21 --- /dev/null +++ b/app/controllers/concerns/flash_i18n.rb @@ -0,0 +1,30 @@ +module FlashI18n + protected + + def redirect_to(url, options = {}) + self.class._flash_types.each do |flash_type| + if options.key?(flash_type) + options[flash_type] = translate_flash(options[flash_type]) + end + end + + if other_flashes = options[:flash] + other_flashes.each do |key, value| + other_flashes[key] = translate_flash(value) + end + end + + super(url, options) + end + + def translate_flash(key) + if Array === key + options = key.extract_options! + I18n.t(key.first, { scope: :"admin.flash" }.merge(options)) + elsif Symbol === key + I18n.t(key, scope: :"admin.flash") + else + key + end + end +end diff --git a/app/controllers/concerns/flash_render.rb b/app/controllers/concerns/flash_render.rb new file mode 100644 index 000000000..57eb2df5d --- /dev/null +++ b/app/controllers/concerns/flash_render.rb @@ -0,0 +1,23 @@ +module FlashRender + extend ActiveSupport::Concern + + include FlashI18n + + def render(options = {}, locals = {}, &block) + flash_options = Hash === options ? options : locals + + self.class._flash_types.each do |flash_type| + if value = flash_options.delete(flash_type) + flash.now[flash_type] = translate_flash(value) + end + end + + if other_flashes = flash_options.delete(:flash) + other_flashes.each do |key, value| + flash.now[key] = translate_flash(value) + end + end + + super(options, locals, &block) + end +end diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index 50cbe621a..5a20de522 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -5,16 +5,20 @@ class AdminUser < ActiveRecord::Base ROLES = [SYSADMIN_ROLE, MODERATOR_ROLE] PASSWORD_REGEX = /\A.*(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[\W_]).*\z/ + class CannotDeleteCurrentUser < RuntimeError; end + class MustBeAtLeastOneAdminUser < RuntimeError; end + acts_as_authentic do |config| - config.merge_validates_length_of_password_field_options :minimum => 8 + config.check_passwords_against_database = true config.ignore_blank_passwords = true - config.merge_validates_uniqueness_of_email_field_options :case_sensitive => false + config.require_password_confirmation = true - # Add conditions to the default validations to tidy up output. - config.merge_validates_format_of_email_field_options :unless => Proc.new { |user| user.email.blank? } - config.merge_validates_length_of_email_field_options :unless => Proc.new { |user| user.email.blank? } - config.merge_validates_length_of_password_field_options :unless => Proc.new { |user| user.password.blank? } - config.merge_validates_confirmation_of_password_field_options :unless => Proc.new { |user| user.password.blank? } + config.merge_validates_length_of_password_field_options minimum: 8 + config.merge_validates_uniqueness_of_email_field_options case_sensitive: false + config.merge_validates_format_of_email_field_options unless: ->(u) { u.email.blank? } + config.merge_validates_length_of_email_field_options unless: ->(u) { u.email.blank? } + config.merge_validates_length_of_password_field_options unless: ->(u) { u.password.blank? } + config.merge_validates_confirmation_of_password_field_options unless: ->(u) { u.password.blank? } end # = Validations = @@ -23,11 +27,54 @@ class AdminUser < ActiveRecord::Base validates_format_of :password, with: PASSWORD_REGEX, allow_blank: true validates_inclusion_of :role, in: ROLES + # = Callbacks = + before_update if: :crypted_password_changed? do + self.force_password_reset = false + self.password_changed_at = Time.current + end + # = Finders = scope :by_name, -> { order(:last_name, :first_name) } scope :by_role, ->(role) { where(role: role).order(:id) } # = Methods = + def current_password + defined?(@current_password) ? @current_password : nil + end + + def current_password=(value) + @current_password = value + end + + def update_with_password(attrs) + if attrs[:password].blank? + attrs.delete(:password) + attrs.delete(:password_confirmation) if attrs[:password_confirmation].blank? + end + + self.attributes = attrs + self.valid? + + if current_password.blank? + errors.add(:current_password, :blank) + elsif !valid_password?(current_password) + errors.add(:current_password, :invalid) + elsif current_password == password + errors.add(:password, :taken) + end + + errors.empty? && save(validate: false) + end + + def destroy(current_user: nil) + if self == current_user + raise CannotDeleteCurrentUser, "Cannot delete current user" + elsif self.class.count < 2 + raise MustBeAtLeastOneAdminUser, "There must be at least one admin user" + else + super() + end + end def name "#{last_name}, #{first_name}" diff --git a/app/models/admin_user_session.rb b/app/models/admin_user_session.rb index 20103296c..da56a06ed 100644 --- a/app/models/admin_user_session.rb +++ b/app/models/admin_user_session.rb @@ -2,4 +2,13 @@ class AdminUserSession < Authlogic::Session::Base allow_http_basic_auth false consecutive_failed_logins_limit AdminUser::DISABLED_LOGIN_COUNT + def last_login_attempt? + failed_login_count == consecutive_failed_logins_limit - 1 + end + + private + + def failed_login_count + attempted_record.present? ? attempted_record.failed_login_count : 0 + end end diff --git a/app/views/admin/admin_users/index.html.erb b/app/views/admin/admin_users/index.html.erb index 7e7070639..cff2fb66e 100644 --- a/app/views/admin/admin_users/index.html.erb +++ b/app/views/admin/admin_users/index.html.erb @@ -14,7 +14,13 @@ <%= mail_to user.email %> <%= user.role %> <%= user.account_disabled ? 'Yes' : '' %> - <%= cms_delete_link user, :url => admin_admin_user_path(user) %> + + <% if user != current_user %> + <%= cms_delete_link user, :url => admin_admin_user_path(user) %> + <% else %> +   + <% end %> + <%- end -%> <%= will_paginate(@users) if @users.any? %> diff --git a/config/initializers/delayed_web.rb b/config/initializers/delayed_web.rb index fe181dccd..606121018 100644 --- a/config/initializers/delayed_web.rb +++ b/config/initializers/delayed_web.rb @@ -5,7 +5,9 @@ # Authenticate our delayed job web interface Delayed::Web::ApplicationController.class_eval do - include Authentication + include Authentication, FlashI18n + + protected def admin_login_url main_app.admin_login_url diff --git a/config/locales/activerecord.en-GB.yml b/config/locales/activerecord.en-GB.yml index b8e1a9e7e..a5fd1d631 100644 --- a/config/locales/activerecord.en-GB.yml +++ b/config/locales/activerecord.en-GB.yml @@ -14,8 +14,13 @@ en-GB: models: admin_user: attributes: + current_password: + invalid: "Current password is incorrect" password: invalid: "Password must contain at least one digit, a lower and upper case letter and a special character" + taken: "Password is the same as the current password" + password_confirmation: + confirmation: "Password confirmation doesn't match password" role: inclusion: "Role '%{value}' is invalid" diff --git a/config/locales/admin.en-GB.yml b/config/locales/admin.en-GB.yml index f7fd701f8..b96e009e8 100644 --- a/config/locales/admin.en-GB.yml +++ b/config/locales/admin.en-GB.yml @@ -27,3 +27,51 @@ en-GB: enqueued: "Enqueued invalidations (%{quantity})" pending: "Pending invalidations (%{quantity})" running: "Running invalidations (%{quantity})" + + flash: + admin_required: "You must be logged in as an administrator to view this page" + change_password: "Please change your password before continuing" + debate_date_updated: "Updated the scheduled debate date successfully" + debate_outcome_updated: "Updated debate outcome successfully" + email_sent_overnight: "Email will be sent overnight" + government_response_updated: "Updated government response successfully" + invalidation_cant_be_cancelled: "Can't cancel invalidations that have completed" + invalidation_cant_be_counted: "Can't count invalidations that aren't pending" + invalidation_cant_be_edited: "Can't edit invalidations that aren't pending" + invalidation_cant_be_removed: "Can't remove invalidations that have started" + invalidation_cant_be_started: "Can't start invalidations that aren't pending" + invalidation_not_cancelled: "Invalidation could not be cancelled - please contact support" + invalidation_not_counted: "Invalidation could not be counted - please contact support" + invalidation_not_removed: "Invalidation could not be removed - please contact support" + invalidation_not_started: "Invalidation could not be enqueued - please contact support" + invalidation_cancelled: "Invalidation cancelled successfully" + invalidation_counted: "Counted the matching signatures for invalidation %{summary}" + invalidation_created: "Invalidation created successfully" + invalidation_removed: "Invalidation removed successfully" + invalidation_started: "Enqueued the invalidation %{summary}" + invalidation_updated: "Invalidation updated successfully" + last_login: "You have one more attempt before your account is temporarily disabled" + disabled_login: "Failed login limit exceeded, your account has been temporarily disabled" + invalid_login: "Invalid email/password combination" + logged_out: "You have been logged out" + password_updated: "Password was successfully updated" + petition_email_created: "Created other parliamentary business successfully" + petition_email_updated: "Updated other parliamentary business successfully" + petition_email_deleted: "Deleted other parliamentary business successfully" + petition_email_not_deleted: "Unable to delete other parliamentary business - please contact support" + petition_not_found: "Cannot find petition with id: %{query}" + rate_limits_updated: "Rate limits updated successfully" + signature_validated: "Signature validated successfully" + signature_not_validated: "Signature could not be validated - please contact support" + signature_invalidated: "Signature invalidated successfully" + signature_not_invalidated: "Signature could not be invalidated - please contact support" + signature_deleted: "Signature removed successfully" + signature_not_deleted: "Signature could not be removed - please contact support" + sysadmin_required: "You must be logged in as a system administrator to view this page" + petition_updated: "Petition has been successfully updated" + user_created: "User was successfully created" + user_updated: "User was successfully updated" + user_deleted: "User was successfully deleted" + user_not_deleted: "User could not be deleted - please contact support" + user_is_current_user: "You are not allowed to delete yourself!" + user_count_is_too_low: "There needs to be at least one admin user" diff --git a/features/admin/admin_access.feature b/features/admin/admin_access.feature index c7ab0ad56..9ce8db7a7 100644 --- a/features/admin/admin_access.feature +++ b/features/admin/admin_access.feature @@ -52,11 +52,16 @@ Feature: Restricted access to the admin console Scenario: 5 failed logins disables an account Given a moderator user exists with email: "admin@example.com", password: "Letmein1!", password_confirmation: "Letmein1!" And I go to the admin login page - And I try the password "wrong trousers" 5 times in a row + And I try the password "wrong trousers" 3 times in a row And I fill in "Email" with "admin@example.com" And I fill in "Password" with "wrong trousers" And I press "Sign in" - Then I should see "Consecutive failed logins limit exceeded, account has been temporarily disabled." + Then I should see "You have one more attempt before your account is temporarily disabled" + And should not see "Logout" + And I fill in "Email" with "admin@example.com" + And I fill in "Password" with "wrong trousers" + And I press "Sign in" + Then I should see "Failed login limit exceeded, your account has been temporarily disabled" And should not see "Logout" Scenario: 5 failed logins with an email address containing a wildcard does not disable an account diff --git a/spec/controllers/admin/admin_controller_spec.rb b/spec/controllers/admin/admin_controller_spec.rb new file mode 100644 index 000000000..f71ef2ec9 --- /dev/null +++ b/spec/controllers/admin/admin_controller_spec.rb @@ -0,0 +1,238 @@ +require 'rails_helper' + +RSpec.describe Admin::AdminController, type: :controller do + let(:user) { FactoryGirl.create(:moderator_user) } + + before do + login_as(user) + end + + describe "flash translation" do + let(:i18n_args) { [i18n_key, i18n_options] } + + context "when using :alert in redirect_to" do + controller do + def index + redirect_to "/", alert: :update_failed + end + end + + let(:i18n_key) { :update_failed } + let(:i18n_options) { { scope: :"admin.flash" } } + let(:i18n_response) { "Update failed" } + + before do + expect(I18n).to receive(:t).with(*i18n_args).and_return(i18n_response) + end + + it "translates the key" do + get :index + expect(flash[:alert]).to eq("Update failed") + end + end + + context "when using :notice in redirect_to" do + controller do + def index + redirect_to "/", notice: :update_succeeded + end + end + + let(:i18n_key) { :update_succeeded } + let(:i18n_options) { { scope: :"admin.flash" } } + let(:i18n_response) { "Update succeeded" } + + before do + expect(I18n).to receive(:t).with(*i18n_args).and_return(i18n_response) + end + + it "translates the key" do + get :index + expect(flash[:notice]).to eq("Update succeeded") + end + end + + context "when using :flash in redirect_to" do + controller do + def index + redirect_to "/", flash: { error: :update_failed } + end + end + + let(:i18n_key) { :update_failed } + let(:i18n_options) { { scope: :"admin.flash" } } + let(:i18n_response) { "Update failed" } + + before do + expect(I18n).to receive(:t).with(*i18n_args).and_return(i18n_response) + end + + it "translates the key" do + get :index + expect(flash[:error]).to eq("Update failed") + end + end + + context "when using substitution" do + controller do + def index + redirect_to "/", notice: [:search_failed, query: "foo"] + end + end + + let(:i18n_key) { :search_failed } + let(:i18n_options) { { scope: :"admin.flash", query: "foo" } } + let(:i18n_response) { "No petition that matches 'foo'" } + + before do + expect(I18n).to receive(:t).with(*i18n_args).and_return(i18n_response) + end + + it "translates the key" do + get :index + expect(flash[:notice]).to eq("No petition that matches 'foo'") + end + end + + context "when using a string" do + controller do + def index + redirect_to "/", notice: "Update succeeded" + end + end + + before do + expect(I18n).not_to receive(:t) + end + + it "translates the key" do + get :index + expect(flash[:notice]).to eq("Update succeeded") + end + end + end + + describe "flash rendering" do + context "when using :alert in render :action" do + controller do + def index + render :index, alert: "Login failed" + end + end + + it "sets flash.now" do + get :index + expect(flash[:alert]).to eq("Login failed") + end + end + + context "when using :notice in render :action" do + controller do + def index + render :index, notice: "Login succeeded" + end + end + + it "sets flash.now" do + get :index + expect(flash[:notice]).to eq("Login succeeded") + end + end + + context "when using :flash in render :action" do + controller do + def index + render :index, flash: { error: "Login failed" } + end + end + + it "sets flash.now" do + get :index + expect(flash[:error]).to eq("Login failed") + end + end + + context "when using :alert in render options" do + controller do + def index + render action: "index", alert: "Login failed" + end + end + + it "sets flash.now" do + get :index + expect(flash[:alert]).to eq("Login failed") + end + end + + context "when using :notice in render options" do + controller do + def index + render action: "index", notice: "Login succeeded" + end + end + + it "sets flash.now" do + get :index + expect(flash[:notice]).to eq("Login succeeded") + end + end + + context "when using :flash in render options" do + controller do + def index + render action: "index", flash: { error: "Login failed" } + end + end + + it "sets flash.now" do + get :index + expect(flash[:error]).to eq("Login failed") + end + end + + context "when using render with flash translation" do + controller do + def index + render :index, alert: :login_failed + end + end + + let(:i18n_args) { [i18n_key, i18n_options] } + let(:i18n_key) { :login_failed } + let(:i18n_options) { { scope: :"admin.flash" } } + let(:i18n_response) { "Login failed" } + + before do + expect(I18n).to receive(:t).with(*i18n_args).and_return(i18n_response) + end + + it "sets flash.now" do + get :index + expect(flash[:alert]).to eq("Login failed") + end + end + + context "when using render with flash translation and substitution" do + controller do + def index + render :index, notice: [:search_failed, query: "foo"] + end + end + + let(:i18n_args) { [i18n_key, i18n_options] } + let(:i18n_key) { :search_failed } + let(:i18n_options) { { scope: :"admin.flash", query: "foo" } } + let(:i18n_response) { "No petition that matches 'foo'" } + + before do + expect(I18n).to receive(:t).with(*i18n_args).and_return(i18n_response) + end + + it "sets flash.now" do + get :index + expect(flash[:notice]).to eq("No petition that matches 'foo'") + end + end + end +end diff --git a/spec/models/admin_user_session_spec.rb b/spec/models/admin_user_session_spec.rb new file mode 100644 index 000000000..2413d8a57 --- /dev/null +++ b/spec/models/admin_user_session_spec.rb @@ -0,0 +1,51 @@ +require 'rails_helper' + +RSpec.describe AdminUserSession do + describe "#last_login_attempt?" do + before do + Authlogic::Session::Base.controller = Authlogic::TestCase::MockController.new + FactoryGirl.create(:moderator_user, email: email) + end + + let(:email) { "admin@petition.parliament.uk" } + let(:params) { { email: email, password: "password" } } + let(:user_session) { described_class.new(params) } + let(:user) { AdminUser.find_by!(email: email) } + + context "when there are no failed login attempts" do + before do + user.update_columns(failed_login_count: 0) + user_session.save + end + + it "returns false" do + expect(user_session.attempted_record).to be_present + expect(user_session.last_login_attempt?).to be false + end + end + + context "when there are 3 failed login attempts" do + before do + user.update_columns(failed_login_count: 3) + user_session.save + end + + it "returns true" do + expect(user_session.attempted_record).to be_present + expect(user_session.last_login_attempt?).to be true + end + end + + context "when there are 4 failed login attempts" do + before do + user.update_columns(failed_login_count: 4) + user_session.save + end + + it "returns false" do + expect(user_session.attempted_record).to be_present + expect(user_session.last_login_attempt?).to be false + end + end + end +end diff --git a/spec/models/admin_user_spec.rb b/spec/models/admin_user_spec.rb index 23a92a7f2..c33abdaf6 100644 --- a/spec/models/admin_user_spec.rb +++ b/spec/models/admin_user_spec.rb @@ -1,19 +1,18 @@ require 'rails_helper' RSpec.describe AdminUser, type: :model do - - context "behaviours" do + describe "behaviours" do it { expect(AdminUser.respond_to?(:acts_as_authentic)).to be_truthy } end - context "defaults" do + describe "defaults" do it "force_password_reset should default to true" do u = AdminUser.new expect(u.force_password_reset).to be_truthy end end - context "validations" do + describe "validations" do it { is_expected.to validate_presence_of(:password) } it { is_expected.to validate_presence_of(:email) } it { is_expected.to validate_presence_of(:first_name) } @@ -57,31 +56,245 @@ end end - context "scopes" do + describe "scopes" do before :each do @user1 = FactoryGirl.create(:sysadmin_user, :first_name => 'Ronald', :last_name => 'Reagan') @user2 = FactoryGirl.create(:moderator_user, :first_name => 'Bill', :last_name => 'Clinton') end - context "by_name" do + describe ".by_name" do it "should return admin users by name" do expect(AdminUser.by_name).to eq([@user2, @user1]) end end - context "by_role" do + describe ".by_role" do it "should return moderator users" do - expect(AdminUser.by_role(AdminUser::MODERATOR_ROLE)).to eq([@user2]) end + expect(AdminUser.by_role(AdminUser::MODERATOR_ROLE)).to eq([@user2]) + end end end - context "methods" do - it "should return a user's name" do - user = FactoryGirl.create(:moderator_user, :first_name => 'Jo', :last_name => 'Public') - expect(user.name).to eq('Public, Jo') + describe "instance methods" do + describe "#update_with_password" do + let!(:user) do + FactoryGirl.create( + :sysadmin_user, + password: "Testing!23", + password_confirmation: "Testing!23", + password_changed_at: nil, + force_password_reset: true + ) + end + + let(:params) do + { + current_password: current_password, password: password, + password_confirmation: password_confirmation + } + end + + let(:current_password) { "Testing!23" } + let(:password) { "NewP4ssword!" } + let(:password_confirmation) { "NewP4ssword!" } + + context "when the new password is valid" do + it "returns true" do + expect(user.update_with_password(params)).to be_truthy + end + + it "changes the crypted_password field" do + expect { + user.update_with_password(params) + }.to change { + user.reload.crypted_password + } + end + + it "sets the timestamp for when the password was changed" do + expect { + user.update_with_password(params) + }.to change { + user.reload.password_changed_at + }.from(nil).to(be_within(1.second).of(Time.current)) + end + + it "clears the force password reset flag" do + expect { + user.update_with_password(params) + }.to change { + user.reload.force_password_reset + }.from(true).to(false) + end + end + + context "when the current password is missing" do + let(:current_password) { "" } + + it "returns false" do + expect(user.update_with_password(params)).to be_falsey + end + + it "adds an error" do + user.update_with_password(params) + expect(user.errors[:current_password]).to eq(["Current password can't be blank"]) + end + + it "doesn't clear the force password reset flag" do + user.update_with_password(params) + expect(user.force_password_reset).to be true + end + + it "doesn't set the password_changed_at timestamp" do + user.update_with_password(params) + expect(user.password_changed_at).to be_nil + end + end + + context "when the current password is incorrect" do + let(:current_password) { "L3tme!n" } + + it "returns false" do + expect(user.update_with_password(params)).to be_falsey + end + + it "adds an error" do + user.update_with_password(params) + expect(user.errors[:current_password]).to eq(["Current password is incorrect"]) + end + + it "doesn't clear the force password reset flag" do + user.update_with_password(params) + expect(user.force_password_reset).to be true + end + + it "doesn't set the password_changed_at timestamp" do + user.update_with_password(params) + expect(user.password_changed_at).to be_nil + end + end + + context "when the new password is the same as the old password" do + let(:password) { current_password } + let(:password_confirmation) { current_password } + + it "returns false" do + expect(user.update_with_password(params)).to be_falsey + end + + it "adds an error" do + user.update_with_password(params) + expect(user.errors[:password]).to eq(["Password is the same as the current password"]) + end + + it "doesn't clear the force password reset flag" do + user.update_with_password(params) + expect(user.force_password_reset).to be true + end + + it "doesn't set the password_changed_at timestamp" do + user.update_with_password(params) + expect(user.password_changed_at).to be_nil + end + end + + context "when the new password is invalid" do + let(:password) { "password" } + let(:password_confirmation) { "password" } + + it "returns false" do + expect(user.update_with_password(params)).to be_falsey + end + + it "adds an error" do + user.update_with_password(params) + expect(user.errors[:password]).to eq(["Password must contain at least one digit, a lower and upper case letter and a special character"]) + end + + it "doesn't clear the force password reset flag" do + user.update_with_password(params) + expect(user.force_password_reset).to be true + end + + it "doesn't set the password_changed_at timestamp" do + user.update_with_password(params) + expect(user.password_changed_at).to be_nil + end + end + + context "when the new password doesn't match the confirmation" do + let(:password) { "L3tme!n1" } + let(:password_confirmation) { "L3tme!n2" } + + it "returns false" do + expect(user.update_with_password(params)).to be_falsey + end + + it "adds an error" do + user.update_with_password(params) + expect(user.errors[:password_confirmation]).to eq(["Password confirmation doesn't match password"]) + end + + it "doesn't clear the force password reset flag" do + user.update_with_password(params) + expect(user.force_password_reset).to be true + end + + it "doesn't set the password_changed_at timestamp" do + user.update_with_password(params) + expect(user.password_changed_at).to be_nil + end + end + end + + describe "#destroy" do + context "when there is no current user and there is more than one" do + let!(:user_1) { FactoryGirl.create(:sysadmin_user) } + let!(:user_2) { FactoryGirl.create(:sysadmin_user) } + + it "returns true" do + expect(user_1.destroy(current_user: nil)).to be_truthy + end + end + + context "when the user is not current and there is more than one" do + let!(:user_1) { FactoryGirl.create(:sysadmin_user) } + let!(:user_2) { FactoryGirl.create(:sysadmin_user) } + + it "returns true" do + expect(user_1.destroy(current_user: user_2)).to be_truthy + end + end + + context "when the current user is itself" do + let!(:user) { FactoryGirl.create(:sysadmin_user) } + + it "raises an AdminUser::CannotDeleteCurrentUser error" do + expect { + user.destroy(current_user: user.reload) + }.to raise_error(AdminUser::CannotDeleteCurrentUser) + end + end + + context "when there is only one user left" do + let!(:user) { FactoryGirl.create(:sysadmin_user) } + + it "raises an AdminUser::MustBeAtLeastOneAdminUser error" do + expect { + user.destroy(current_user: nil) + }.to raise_error(AdminUser::MustBeAtLeastOneAdminUser) + end + end + end + + describe "#name" do + it "should return a user's name" do + user = FactoryGirl.create(:moderator_user, :first_name => 'Jo', :last_name => 'Public') + expect(user.name).to eq('Public, Jo') + end end - context "is_a_sysadmin?" do + describe "#is_a_sysadmin?" do it "should return true when user is a sysadmin" do user = FactoryGirl.create(:admin_user, :role => 'sysadmin') expect(user.is_a_sysadmin?).to be_truthy @@ -93,7 +306,7 @@ end end - context "is_a_moderator?" do + describe "#is_a_moderator?" do it "should return true when user is a moderator user" do user = FactoryGirl.create(:admin_user, :role => 'moderator') expect(user.is_a_moderator?).to be_truthy @@ -105,7 +318,7 @@ end end - context "has_to_change_password?" do + describe "#has_to_change_password?" do it "should be true when force_reset_password is true" do user = FactoryGirl.create(:moderator_user, :force_password_reset => true) expect(user.has_to_change_password?).to be_truthy @@ -127,7 +340,7 @@ end end - context "can_take_petitions_down?" do + describe "#can_take_petitions_down?" do it "is true if the user is a sysadmin" do user = FactoryGirl.create(:admin_user, :role => 'sysadmin') expect(user.can_take_petitions_down?).to be_truthy @@ -139,7 +352,7 @@ end end - context "account_disabled" do + describe "#account_disabled" do it "should return true when user has tried to login 5 times unsuccessfully" do user = FactoryGirl.create(:moderator_user) user.failed_login_count = 5 @@ -159,7 +372,7 @@ end end - context "account_disabled=" do + describe "#account_disabled=" do it "should set the failed login count to 5 when true" do u = FactoryGirl.create(:moderator_user) u.account_disabled = true From 4f5586d4d9554d432bb9adab6249a7f76f43efdf Mon Sep 17 00:00:00 2001 From: Andrew White Date: Wed, 5 Oct 2016 16:01:32 +0100 Subject: [PATCH 11/22] Reset persistence token on login/logout To minimise the possibility of an admin user credentials cookie being replayed after a MITM SSL attack reset them on login/logout. --- app/models/admin_user_session.rb | 8 ++ .../admin_user_persistence_token_spec.rb | 73 +++++++++++++++++++ spec/support/forgery_protection.rb | 25 +++++++ 3 files changed, 106 insertions(+) create mode 100644 spec/requests/admin_user_persistence_token_spec.rb create mode 100644 spec/support/forgery_protection.rb diff --git a/app/models/admin_user_session.rb b/app/models/admin_user_session.rb index da56a06ed..ee1379386 100644 --- a/app/models/admin_user_session.rb +++ b/app/models/admin_user_session.rb @@ -2,6 +2,14 @@ class AdminUserSession < Authlogic::Session::Base allow_http_basic_auth false consecutive_failed_logins_limit AdminUser::DISABLED_LOGIN_COUNT + before_save do + record.reset_persistence_token! + end + + before_destroy do + record.reset_persistence_token! + end + def last_login_attempt? failed_login_count == consecutive_failed_logins_limit - 1 end diff --git a/spec/requests/admin_user_persistence_token_spec.rb b/spec/requests/admin_user_persistence_token_spec.rb new file mode 100644 index 000000000..70fcb80a8 --- /dev/null +++ b/spec/requests/admin_user_persistence_token_spec.rb @@ -0,0 +1,73 @@ +require 'rails_helper' + +RSpec.describe "admin user persistence token", type: :request, csrf: false do + let(:user_attributes) do + { + first_name: "System", + last_name: "Administrator", + email: "admin@petition.parliament.uk", + password: "L3tme1n!", + password_confirmation: "L3tme1n!" + } + end + + let(:login_params) do + { email: "admin@petition.parliament.uk", password: "L3tme1n!" } + end + + before do + FactoryGirl.create(:sysadmin_user, user_attributes) + end + + def new_browser + open_session do |s| + s.reset! + s.host! "moderate.petition.parliament.uk" + s.https! + end + end + + context "when a new session is created" do + it "logs out existing sessions" do + s1 = new_browser + s1.post "/admin/user_sessions", admin_user_session: login_params + + expect(s1.response.status).to eq(302) + expect(s1.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin") + + s2 = new_browser + s2.post "/admin/user_sessions", admin_user_session: login_params + + expect(s2.response.status).to eq(302) + expect(s2.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin") + + s1.get("/admin") + + expect(s1.response.status).to eq(302) + expect(s1.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin/login") + end + end + + context "when a session is destroyed" do + it "resets the persistence token" do + s1 = new_browser + s1.post "/admin/user_sessions", admin_user_session: login_params + + expect(s1.response.status).to eq(302) + expect(s1.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin") + + s2 = new_browser + s2.cookies["admin_user_credentials"] = s1.cookies["admin_user_credentials"] + + s1.get("/admin/logout") + + expect(s1.response.status).to eq(302) + expect(s1.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin/login") + + s2.get("/admin") + + expect(s2.response.status).to eq(302) + expect(s2.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin/login") + end + end +end diff --git a/spec/support/forgery_protection.rb b/spec/support/forgery_protection.rb new file mode 100644 index 000000000..a531b497f --- /dev/null +++ b/spec/support/forgery_protection.rb @@ -0,0 +1,25 @@ +RSpec.configure do |config| + mod = Module.new do + def with_forgery_protection(on_or_off, &block) + begin + current = ActionController::Base.allow_forgery_protection + ActionController::Base.allow_forgery_protection = on_or_off + yield + ensure + ActionController::Base.allow_forgery_protection = current + end + end + end + + config.include(mod, type: :request) + + config.around(:each, type: :request) do |example| + if example.metadata.key?(:csrf) + with_forgery_protection(example.metadata[:csrf]) do + example.run + end + else + example.run + end + end +end From cc3cd9c1bd5a3a644d6e0bf07eb3061f0df1e451 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Thu, 6 Oct 2016 22:47:32 +0100 Subject: [PATCH 12/22] Automatically logout admin sessions after 30 minutes --- app/models/admin_user.rb | 1 + app/models/admin_user_session.rb | 1 + ...5752_add_last_request_at_to_admin_users.rb | 5 ++++ db/structure.sql | 5 +++- spec/models/admin_user_spec.rb | 24 +++++++++++++++++++ 5 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20161006095752_add_last_request_at_to_admin_users.rb diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index 5a20de522..ddd8ce16e 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -11,6 +11,7 @@ class MustBeAtLeastOneAdminUser < RuntimeError; end acts_as_authentic do |config| config.check_passwords_against_database = true config.ignore_blank_passwords = true + config.logged_in_timeout = 30.minutes config.require_password_confirmation = true config.merge_validates_length_of_password_field_options minimum: 8 diff --git a/app/models/admin_user_session.rb b/app/models/admin_user_session.rb index ee1379386..e540cac13 100644 --- a/app/models/admin_user_session.rb +++ b/app/models/admin_user_session.rb @@ -1,6 +1,7 @@ class AdminUserSession < Authlogic::Session::Base allow_http_basic_auth false consecutive_failed_logins_limit AdminUser::DISABLED_LOGIN_COUNT + logout_on_timeout true before_save do record.reset_persistence_token! diff --git a/db/migrate/20161006095752_add_last_request_at_to_admin_users.rb b/db/migrate/20161006095752_add_last_request_at_to_admin_users.rb new file mode 100644 index 000000000..9404987e7 --- /dev/null +++ b/db/migrate/20161006095752_add_last_request_at_to_admin_users.rb @@ -0,0 +1,5 @@ +class AddLastRequestAtToAdminUsers < ActiveRecord::Migration + def change + add_column :admin_users, :last_request_at, :datetime + end +end diff --git a/db/structure.sql b/db/structure.sql index a99aad48d..35111eacf 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -51,7 +51,8 @@ CREATE TABLE admin_users ( force_password_reset boolean DEFAULT true, password_changed_at timestamp without time zone, created_at timestamp without time zone, - updated_at timestamp without time zone + updated_at timestamp without time zone, + last_request_at timestamp without time zone ); @@ -1797,3 +1798,5 @@ INSERT INTO schema_migrations (version) VALUES ('20160822064645'); INSERT INTO schema_migrations (version) VALUES ('20160910054223'); +INSERT INTO schema_migrations (version) VALUES ('20161006095752'); + diff --git a/spec/models/admin_user_spec.rb b/spec/models/admin_user_spec.rb index c33abdaf6..15a1a8f9c 100644 --- a/spec/models/admin_user_spec.rb +++ b/spec/models/admin_user_spec.rb @@ -1,6 +1,30 @@ require 'rails_helper' RSpec.describe AdminUser, type: :model do + describe "schema" do + it { is_expected.to have_db_column(:email).of_type(:string).with_options(null: false) } + it { is_expected.to have_db_column(:persistence_token).of_type(:string) } + it { is_expected.to have_db_column(:crypted_password).of_type(:string) } + it { is_expected.to have_db_column(:password_salt).of_type(:string) } + it { is_expected.to have_db_column(:login_count).of_type(:integer).with_options(default: 0) } + it { is_expected.to have_db_column(:failed_login_count).of_type(:integer).with_options(default: 0) } + it { is_expected.to have_db_column(:current_login_at).of_type(:datetime) } + it { is_expected.to have_db_column(:last_login_at).of_type(:datetime) } + it { is_expected.to have_db_column(:current_login_ip).of_type(:string) } + it { is_expected.to have_db_column(:last_login_ip).of_type(:string) } + it { is_expected.to have_db_column(:role).of_type(:string).with_options(limit: 10, null: false) } + it { is_expected.to have_db_column(:force_password_reset).of_type(:boolean).with_options(default: true) } + it { is_expected.to have_db_column(:password_changed_at).of_type(:datetime) } + it { is_expected.to have_db_column(:created_at).of_type(:datetime) } + it { is_expected.to have_db_column(:updated_at).of_type(:datetime) } + it { is_expected.to have_db_column(:last_request_at).of_type(:datetime) } + end + + describe "indexes" do + it { is_expected.to have_db_index([:email]).unique } + it { is_expected.to have_db_index([:last_name, :first_name]) } + end + describe "behaviours" do it { expect(AdminUser.respond_to?(:acts_as_authentic)).to be_truthy } end From 6fa07843e78ed38782fe9742a325fcc74686557c Mon Sep 17 00:00:00 2001 From: Andrew White Date: Thu, 6 Oct 2016 22:50:22 +0100 Subject: [PATCH 13/22] Make login timeout configurable --- app/models/admin_user.rb | 2 +- app/models/site.rb | 10 +++++++++ ...0161006101123_add_login_timeout_to_site.rb | 5 +++++ db/structure.sql | 5 ++++- spec/models/site_spec.rb | 21 ++++++++++++++++++- 5 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20161006101123_add_login_timeout_to_site.rb diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index ddd8ce16e..40fc5771a 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -11,7 +11,7 @@ class MustBeAtLeastOneAdminUser < RuntimeError; end acts_as_authentic do |config| config.check_passwords_against_database = true config.ignore_blank_passwords = true - config.logged_in_timeout = 30.minutes + config.logged_in_timeout = Site.login_timeout config.require_password_confirmation = true config.merge_validates_length_of_password_field_options minimum: 8 diff --git a/app/models/site.rb b/app/models/site.rb index 9dddb111d..be69c7076 100644 --- a/app/models/site.rb +++ b/app/models/site.rb @@ -92,6 +92,10 @@ def protected? instance.protected? end + def login_timeout + instance.login_timeout + end + def reload Thread.current[:__site__] = nil end @@ -111,6 +115,7 @@ def defaults password: default_password, enabled: default_enabled, protected: default_protected, + login_timeout: default_login_timeout, petition_duration: default_petition_duration, minimum_number_of_sponsors: default_minimum_number_of_sponsors, maximum_number_of_sponsors: default_maximum_number_of_sponsors, @@ -198,6 +203,10 @@ def default_protected !ENV.fetch('SITE_PROTECTED', '0').to_i.zero? end + def default_login_timeout + ENV.fetch('SITE_LOGIN_TIMEOUT', '1800').to_i + end + def default_petition_duration ENV.fetch('PETITION_DURATION', '6').to_i end @@ -345,6 +354,7 @@ def closed_at_for_opening(time = Time.current) validates :threshold_for_debate, presence: true, numericality: { only_integer: true } validates :username, presence: true, length: { maximum: 30 }, if: :protected? validates :password, length: { maximum: 30 }, confirmation: true, if: :protected? + validates :login_timeout, presence: true, numericality: { only_integer: true } validate if: :protected? do errors.add(:password, :blank) unless password_digest? diff --git a/db/migrate/20161006101123_add_login_timeout_to_site.rb b/db/migrate/20161006101123_add_login_timeout_to_site.rb new file mode 100644 index 000000000..8516577a7 --- /dev/null +++ b/db/migrate/20161006101123_add_login_timeout_to_site.rb @@ -0,0 +1,5 @@ +class AddLoginTimeoutToSite < ActiveRecord::Migration + def change + add_column :sites, :login_timeout, :integer, null: false, default: 1800 + end +end diff --git a/db/structure.sql b/db/structure.sql index 35111eacf..25b730cc6 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -748,7 +748,8 @@ CREATE TABLE sites ( updated_at timestamp without time zone NOT NULL, feedback_email character varying(100) DEFAULT '"Petitions: UK Government and Parliament" '::character varying NOT NULL, moderate_url character varying(50) DEFAULT 'https://moderate.petition.parliament.uk'::character varying NOT NULL, - last_petition_created_at timestamp without time zone + last_petition_created_at timestamp without time zone, + login_timeout integer DEFAULT 1800 NOT NULL ); @@ -1800,3 +1801,5 @@ INSERT INTO schema_migrations (version) VALUES ('20160910054223'); INSERT INTO schema_migrations (version) VALUES ('20161006095752'); +INSERT INTO schema_migrations (version) VALUES ('20161006101123'); + diff --git a/spec/models/site_spec.rb b/spec/models/site_spec.rb index afa06867e..76afca20e 100644 --- a/spec/models/site_spec.rb +++ b/spec/models/site_spec.rb @@ -20,6 +20,7 @@ it { is_expected.to have_db_column(:updated_at).of_type(:datetime).with_options(null: false) } it { is_expected.to have_db_column(:feedback_email).of_type(:string).with_options(limit: 100, default: '"Petitions: UK Government and Parliament" ') } it { is_expected.to have_db_column(:last_petition_created_at).of_type(:datetime).with_options(null: true, default: nil) } + it { is_expected.to have_db_column(:login_timeout).of_type(:integer).with_options(null: false, default: 1800) } end describe "validations" do @@ -32,6 +33,7 @@ it { is_expected.to validate_presence_of(:threshold_for_moderation) } it { is_expected.to validate_presence_of(:threshold_for_response) } it { is_expected.to validate_presence_of(:threshold_for_debate) } + it { is_expected.to validate_presence_of(:login_timeout) } it { is_expected.to validate_length_of(:title).is_at_most(50) } it { is_expected.to validate_length_of(:url).is_at_most(50) } @@ -40,7 +42,7 @@ %w[ petition_duration minimum_number_of_sponsors maximum_number_of_sponsors - threshold_for_moderation threshold_for_response threshold_for_debate + threshold_for_moderation threshold_for_response threshold_for_debate login_timeout ].each do |attribute| describe attribute do let(:errors) { subject.errors[attribute] } @@ -155,6 +157,11 @@ expect(Site.protected?).to eq(true) end + it "delegates login_timeout to the instance" do + expect(site).to receive(:login_timeout).and_return(900) + expect(Site.login_timeout).to eq(900) + end + it "delegates petition_duration to the instance" do expect(site).to receive(:petition_duration).and_return(3) expect(Site.petition_duration).to eq(3) @@ -380,6 +387,18 @@ end end + describe "for login_timeout" do + it "defaults to 1800" do + allow(ENV).to receive(:fetch).with("SITE_LOGIN_TIMEOUT", '1800').and_return("1800") + expect(defaults[:login_timeout]).to eq(1800) + end + + it "can be overridden with the SITE_LOGIN_TIMEOUT environment variable" do + allow(ENV).to receive(:fetch).with("SITE_LOGIN_TIMEOUT", '1800').and_return("900") + expect(defaults[:login_timeout]).to eq(900) + end + end + describe "for petition_duration" do it "defaults to 6" do allow(ENV).to receive(:fetch).with("PETITION_DURATION", '6').and_return("6") From 49cebc07ab26e4ee8e670d23da7c0b9e46a03c5f Mon Sep 17 00:00:00 2001 From: Andrew White Date: Thu, 6 Oct 2016 23:30:58 +0100 Subject: [PATCH 14/22] Add spec for automatic logout after specified time period --- spec/requests/login_timeout_spec.rb | 41 +++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 spec/requests/login_timeout_spec.rb diff --git a/spec/requests/login_timeout_spec.rb b/spec/requests/login_timeout_spec.rb new file mode 100644 index 000000000..724184dce --- /dev/null +++ b/spec/requests/login_timeout_spec.rb @@ -0,0 +1,41 @@ +require 'rails_helper' + +RSpec.describe "login timeout", type: :request, csrf: false do + let(:user_attributes) do + { + first_name: "System", + last_name: "Administrator", + email: "admin@petition.parliament.uk", + password: "L3tme1n!", + password_confirmation: "L3tme1n!" + } + end + + let(:login_params) do + { email: "admin@petition.parliament.uk", password: "L3tme1n!" } + end + + let!(:user) { FactoryGirl.create(:sysadmin_user, user_attributes) } + + before do + host! "moderate.petition.parliament.uk" + https! + end + + it "logs out automatically after a certain time period" do + Site.instance.update(login_timeout: 600) + + travel_to 2.minutes.ago do + post "/admin/user_sessions", admin_user_session: login_params + expect(response).to redirect_to("/admin") + end + + get "/admin" + expect(response).to be_successful + + travel_to 10.minutes.from_now do + get "/admin" + expect(response).to redirect_to("/admin/login") + end + end +end From 06912f0922066caefba4d52f2b2ce88586311843 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 7 Oct 2016 07:07:21 +0100 Subject: [PATCH 15/22] Destroy stale sessions Since the login timeout is now configurable we should logout any stale sessions so that they don't become fresh again should the login timeout be increased at any point. --- app/controllers/concerns/authentication.rb | 11 +++++++ .../admin_user_persistence_token_spec.rb | 31 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 1e4e39760..f101b49e0 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -2,6 +2,9 @@ module Authentication extend ActiveSupport::Concern included do + before_action :set_login_timeout + before_action :logout_stale_session + before_action :require_admin before_action :check_for_password_change @@ -50,4 +53,12 @@ def require_sysadmin def store_target_location session[:return_to] = request.fullpath end + + def set_login_timeout + AdminUser.logged_in_timeout = Site.login_timeout + end + + def logout_stale_session + current_session.destroy if current_session && current_session.stale? + end end diff --git a/spec/requests/admin_user_persistence_token_spec.rb b/spec/requests/admin_user_persistence_token_spec.rb index 70fcb80a8..ad512bccc 100644 --- a/spec/requests/admin_user_persistence_token_spec.rb +++ b/spec/requests/admin_user_persistence_token_spec.rb @@ -70,4 +70,35 @@ def new_browser expect(s2.response.headers["Location"]).to eq("https://moderate.petition.parliament.uk/admin/login") end end + + context "when a session is stale" do + before do + host! "moderate.petition.parliament.uk" + https! + end + + it "resets the persistence token" do + Site.instance.update(login_timeout: 600) + + travel_to 5.minutes.ago do + post "/admin/user_sessions", admin_user_session: login_params + expect(response).to redirect_to("/admin") + end + + get "/admin" + expect(response).to be_successful + + travel_to 15.minutes.from_now do + get "/admin" + expect(response).to redirect_to("/admin/login") + end + + Site.instance.update(login_timeout: 1800) + + travel_to 15.minutes.from_now do + get "/admin" + expect(response).to redirect_to("/admin/login") + end + end + end end From 4e8d02832ad082701aa8cb3fbd98ef39ac1c037c Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 7 Oct 2016 16:12:09 +0100 Subject: [PATCH 16/22] Login inside do_patch method Because the auto logout gets confused by all the time travel. --- spec/controllers/admin/government_response_controller_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/controllers/admin/government_response_controller_spec.rb b/spec/controllers/admin/government_response_controller_spec.rb index 5afede784..034141377 100644 --- a/spec/controllers/admin/government_response_controller_spec.rb +++ b/spec/controllers/admin/government_response_controller_spec.rb @@ -105,6 +105,8 @@ context 'when clicking the Email button' do def do_patch(overrides = {}) + login_as(user) + params = { petition_id: petition.id, government_response: government_response_attributes, @@ -354,6 +356,8 @@ def do_patch(overrides = {}) context 'when clicking the Save button' do def do_patch(overrides = {}) + login_as(user) + params = { petition_id: petition.id, government_response: government_response_attributes, From b569cf2f5cae36ea0f62e01cb9467fffe1e022dd Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 7 Oct 2016 16:15:15 +0100 Subject: [PATCH 17/22] Protect against logging out stale sessions If logout gets called on a stale session then the record is present at stale_record and not at record. --- app/models/admin_user_session.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/admin_user_session.rb b/app/models/admin_user_session.rb index e540cac13..2d5083d90 100644 --- a/app/models/admin_user_session.rb +++ b/app/models/admin_user_session.rb @@ -8,7 +8,11 @@ class AdminUserSession < Authlogic::Session::Base end before_destroy do - record.reset_persistence_token! + if stale? + stale_record.reset_persistence_token! + else + record.reset_persistence_token! + end end def last_login_attempt? From cf714fd71ec11686530816fef6d2de829bba995a Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 7 Oct 2016 16:31:28 +0100 Subject: [PATCH 18/22] Tidy up authentication helper for controller tests --- spec/support/authentication.rb | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/spec/support/authentication.rb b/spec/support/authentication.rb index 2a4b1406d..3f737e20f 100644 --- a/spec/support/authentication.rb +++ b/spec/support/authentication.rb @@ -1,11 +1,17 @@ require 'authlogic/test_case' -include Authlogic::TestCase -def login_as(user) - activate_authlogic - AdminUserSession.create(user) -end +RSpec.configure do |config| + mod = Module.new do + def login_as(user) + activate_authlogic + AdminUserSession.create(user) + end + + def http_authentication(username, password) + request.env['HTTP_AUTHORIZATION'] = ActionController::HttpAuthentication::Basic.encode_credentials(username, password) + end + end -def http_authentication(username, password) - request.env['HTTP_AUTHORIZATION'] = ActionController::HttpAuthentication::Basic.encode_credentials(username, password) + config.include(mod, type: :controller) + config.include(Authlogic::TestCase, type: :controller) end From d8a5130a4bc732b0c1cac2ea7fa74dcaabd3edd6 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 7 Oct 2016 16:32:07 +0100 Subject: [PATCH 19/22] Increase time travelled to avoid intermittent test failures --- spec/requests/login_timeout_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/login_timeout_spec.rb b/spec/requests/login_timeout_spec.rb index 724184dce..98f1ca83b 100644 --- a/spec/requests/login_timeout_spec.rb +++ b/spec/requests/login_timeout_spec.rb @@ -33,7 +33,7 @@ get "/admin" expect(response).to be_successful - travel_to 10.minutes.from_now do + travel_to 15.minutes.from_now do get "/admin" expect(response).to redirect_to("/admin/login") end From a1ac9dc8e231fe7719d500d81f073a5c9450ca0a Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 7 Oct 2016 16:33:08 +0100 Subject: [PATCH 20/22] Add logout warning functionality So that a moderator isn't surprised when their session times out on the server add a two minute warning with the option to continue for another thirty minutes. --- app/assets/javascripts/admin.js | 1 + app/assets/javascripts/auto_logout.js | 44 +++++++++++++++++++ .../stylesheets/petitions/admin/_logout.scss | 30 +++++++++++++ app/assets/stylesheets/site-admin.scss | 1 + .../admin/user_sessions_controller.rb | 15 ++++++- app/controllers/concerns/authentication.rb | 2 +- app/models/admin_user.rb | 8 ++++ app/models/admin_user_session.rb | 4 ++ app/views/admin/shared/_footer.html.erb | 19 ++++++++ .../user_sessions/continue.json.jbuilder | 1 + .../admin/user_sessions/status.json.jbuilder | 5 +++ app/views/layouts/admin.html.erb | 1 + config/routes.rb | 3 ++ spec/models/admin_user_spec.rb | 14 ++++++ 14 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 app/assets/javascripts/auto_logout.js create mode 100644 app/assets/stylesheets/petitions/admin/_logout.scss create mode 100644 app/views/admin/shared/_footer.html.erb create mode 100644 app/views/admin/user_sessions/continue.json.jbuilder create mode 100644 app/views/admin/user_sessions/status.json.jbuilder diff --git a/app/assets/javascripts/admin.js b/app/assets/javascripts/admin.js index 407fb6273..6d47e8e73 100644 --- a/app/assets/javascripts/admin.js +++ b/app/assets/javascripts/admin.js @@ -1,3 +1,4 @@ //= require jquery //= require jquery_ujs +//= require auto_logout //= require autosubmit_selects diff --git a/app/assets/javascripts/auto_logout.js b/app/assets/javascripts/auto_logout.js new file mode 100644 index 000000000..6fb80dda2 --- /dev/null +++ b/app/assets/javascripts/auto_logout.js @@ -0,0 +1,44 @@ +(function ($) { + 'use strict'; + + $.fn.autoLogout = function() { + var $html = this; + var $continue = $html.find('#logout-warning-continue'); + var $logout = $html.find('#logout-warning-logout'); + var INTERVAL = 10000; + var WARNING_TIME = 120; + + var AutoLogout = { + continueClicked: function(e) { + $.getJSON('/admin/continue.json', AutoLogout.processContinue); + }, + + logoutClicked: function(e) { + window.location = '/admin/logout'; + }, + + processContinue: function(data) { + $html.hide(); + }, + + processStatus: function(data) { + if (data.time_remaining == 0) { + window.location = '/admin/logout'; + } else if (data.time_remaining <= WARNING_TIME) { + $html.show(); + } else { + $html.hide(); + } + }, + + checkStatus: function() { + $.getJSON('/admin/status.json', AutoLogout.processStatus); + } + }; + + $continue.on('click', AutoLogout.continueClicked); + $logout.on('click', AutoLogout.logoutClicked); + + window.setInterval(AutoLogout.checkStatus, INTERVAL); + } +})(jQuery); diff --git a/app/assets/stylesheets/petitions/admin/_logout.scss b/app/assets/stylesheets/petitions/admin/_logout.scss new file mode 100644 index 000000000..7691e5cc6 --- /dev/null +++ b/app/assets/stylesheets/petitions/admin/_logout.scss @@ -0,0 +1,30 @@ +#logout-warning { + width: 100%; + height: 100%; + position: fixed; + top: 0; + left: 0; + background-color: rgba(0, 0, 0, 0.5); + display: none; + + .logout-warning-modal { + background-color: #FFF; + margin: 10% auto 0; + width: 40%; + padding: 24px; + box-shadow: 0px 5px 20px 10px rgba(0, 0, 0, 0.3); + } + + .logout-warning-message { + margin: 0 0 24px 0; + } + + .logout-warning-actions { + margin: 0; + + button { + font-size: 16px; + margin-bottom: 2px; + } + } +} diff --git a/app/assets/stylesheets/site-admin.scss b/app/assets/stylesheets/site-admin.scss index cb5779814..48f148625 100644 --- a/app/assets/stylesheets/site-admin.scss +++ b/app/assets/stylesheets/site-admin.scss @@ -45,6 +45,7 @@ @import "petitions/admin/meta"; @import "petitions/admin/actions"; @import "petitions/admin/list"; +@import "petitions/admin/logout"; @import "petitions/admin/tables"; // Petition admin views diff --git a/app/controllers/admin/user_sessions_controller.rb b/app/controllers/admin/user_sessions_controller.rb index 80f73683f..f28a1006e 100644 --- a/app/controllers/admin/user_sessions_controller.rb +++ b/app/controllers/admin/user_sessions_controller.rb @@ -1,5 +1,5 @@ class Admin::UserSessionsController < Admin::AdminController - skip_before_filter :require_admin, only: [:new, :create] + skip_before_filter :require_admin, only: [:new, :create, :destroy, :status] skip_before_filter :check_for_password_change def new @@ -24,4 +24,17 @@ def destroy current_session.destroy if logged_in? redirect_to admin_login_url, notice: :logged_out end + + def status + end + + def continue + current_user.touch(:last_request_at) + end + + private + + def last_request_update_allowed? + action_name != 'status' + end end diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index f101b49e0..4ed2e8447 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -8,7 +8,7 @@ module Authentication before_action :require_admin before_action :check_for_password_change - helper_method :current_user, :logged_in? + helper_method :current_user, :current_session, :logged_in? end def current_session diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index 40fc5771a..c7b803dec 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -112,4 +112,12 @@ def account_disabled def account_disabled=(flag) self.failed_login_count = (flag == "0" or !flag) ? 0 : DISABLED_LOGIN_COUNT end + + def elapsed_time(now = Time.current) + (now - last_request_at).floor + end + + def time_remaining(now = Time.current) + [Site.login_timeout - elapsed_time(now), 0].max + end end diff --git a/app/models/admin_user_session.rb b/app/models/admin_user_session.rb index 2d5083d90..f4476d737 100644 --- a/app/models/admin_user_session.rb +++ b/app/models/admin_user_session.rb @@ -19,6 +19,10 @@ def last_login_attempt? failed_login_count == consecutive_failed_logins_limit - 1 end + def time_remaining + record ? record.time_remaining : 0 + end + private def failed_login_count diff --git a/app/views/admin/shared/_footer.html.erb b/app/views/admin/shared/_footer.html.erb new file mode 100644 index 000000000..a3e2baa4c --- /dev/null +++ b/app/views/admin/shared/_footer.html.erb @@ -0,0 +1,19 @@ +<% if logged_in? %> +
+
+

+ You will be logged out automatically in two minutes unless you click continue. +

+

+ + +

+
+
+ + +<% end %> diff --git a/app/views/admin/user_sessions/continue.json.jbuilder b/app/views/admin/user_sessions/continue.json.jbuilder new file mode 100644 index 000000000..f60d49986 --- /dev/null +++ b/app/views/admin/user_sessions/continue.json.jbuilder @@ -0,0 +1 @@ +json.last_request_at current_user.last_request_at diff --git a/app/views/admin/user_sessions/status.json.jbuilder b/app/views/admin/user_sessions/status.json.jbuilder new file mode 100644 index 000000000..618c77b4c --- /dev/null +++ b/app/views/admin/user_sessions/status.json.jbuilder @@ -0,0 +1,5 @@ +if current_session + json.time_remaining current_session.time_remaining +else + json.time_remaining 0 +end diff --git a/app/views/layouts/admin.html.erb b/app/views/layouts/admin.html.erb index 474759613..c265f092a 100644 --- a/app/views/layouts/admin.html.erb +++ b/app/views/layouts/admin.html.erb @@ -16,5 +16,6 @@ <%= render '/admin/shared/messages' %> <%= yield %> + <%= render '/admin/shared/footer' %> diff --git a/config/routes.rb b/config/routes.rb index de3ebd374..989450de6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -102,6 +102,9 @@ get 'logout' => 'user_sessions#destroy', :as => :logout get 'login' => 'user_sessions#new', :as => :login + + get 'continue' => 'user_sessions#continue', :as => :continue + get 'status' => 'user_sessions#status', :as => :status end end diff --git a/spec/models/admin_user_spec.rb b/spec/models/admin_user_spec.rb index 15a1a8f9c..0994622c9 100644 --- a/spec/models/admin_user_spec.rb +++ b/spec/models/admin_user_spec.rb @@ -410,5 +410,19 @@ expect(u.failed_login_count).to eq(0) end end + + describe "#elapsed_time" do + it "returns the number of seconds since the last request" do + user = FactoryGirl.build(:admin_user, last_request_at: 60.seconds.ago) + expect(user.elapsed_time).to eq(60) + end + end + + describe "#time_remaining" do + it "returns the number of seconds since the last request" do + user = FactoryGirl.build(:admin_user, last_request_at: 60.seconds.ago) + expect(user.elapsed_time).to eq(60) + end + end end end From 21b30f18e79935f24445a848f11e7eab1ebed34e Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 7 Oct 2016 16:57:27 +0100 Subject: [PATCH 21/22] Make user edit form presentable --- .../petitions/admin/views/_shared.scss | 10 ++++++++++ app/views/admin/admin_users/_form.html.erb | 19 +++++++++---------- app/views/admin/admin_users/edit.html.erb | 8 +++++--- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/app/assets/stylesheets/petitions/admin/views/_shared.scss b/app/assets/stylesheets/petitions/admin/views/_shared.scss index 5b845ab0d..b97e5c97a 100644 --- a/app/assets/stylesheets/petitions/admin/views/_shared.scss +++ b/app/assets/stylesheets/petitions/admin/views/_shared.scss @@ -63,6 +63,16 @@ margin-top: $gutter; } + .mandatory { + color: $red-50; + font-weight: bold; + font-size: 24px; + } + + .form-label-inline { + display: inline-block; + } + .button_to { display: inline-block; diff --git a/app/views/admin/admin_users/_form.html.erb b/app/views/admin/admin_users/_form.html.erb index acde05569..d1cdbdbfc 100644 --- a/app/views/admin/admin_users/_form.html.erb +++ b/app/views/admin/admin_users/_form.html.erb @@ -1,28 +1,26 @@ <%= form_for [:admin, @user] do |f| -%> - <%= f.error_messages(header_message: nil) %> - <%= form_row for: [f.object, :first_name] do %> - <%= f.label :first_name, class: 'form-label' %><%= mandatory_field %> + <%= f.label :first_name, class: 'form-label form-label-inline' %><%= mandatory_field %> <%= error_messages_for_field f.object, :first_name %> <%= f.text_field :first_name, tabindex: increment, class: 'form-control', autofocus: 'autofocus' %> <% end %> <%= form_row for: [f.object, :last_name] do %> - <%= f.label :last_name, class: 'form-label' %><%= mandatory_field %> + <%= f.label :last_name, class: 'form-label form-label-inline' %><%= mandatory_field %> <%= error_messages_for_field f.object, :last_name %> <%= f.text_field :last_name, tabindex: increment, class: 'form-control' %> <% end %> <%= form_row for: [f.object, :email] do %> - <%= f.label :email, class: 'form-label' %><%= mandatory_field %> + <%= f.label :email, class: 'form-label form-label-inline' %><%= mandatory_field %> <%= error_messages_for_field f.object, :email %> <%= f.email_field :email, tabindex: increment, class: 'form-control' %> <% end %> <%= form_row for: [f.object, :email] do %> - <%= f.label :role, class: 'form-label' %> + <%= f.label :role, class: 'form-label form-label-inline' %> <%= error_messages_for_field f.object, :role %> - <%= f.select('role', AdminUser::ROLES) %> + <%= f.select('role', AdminUser::ROLES, {}, class: 'form-control column-third') %> <% end %> <%= form_row for: [f.object, :force_password_reset] do %> @@ -37,16 +35,17 @@ <%= error_messages_for_field f.object, :account_disabled %> <% end %> - <%= field_set_tag 'Passwords' do %> + <%= field_set_tag do %> + <%= content_tag(:legend, 'Passwords', class: 'form-label-bold') %> <%= content_tag(:p, 'Note, no password needs to be entered unless you want to change it') unless f.object.new_record? %> <%= form_row for: [f.object, :password] do %> - <%= f.label :password, class: 'form-label' %><%= mandatory_field if f.object.new_record? %> + <%= f.label :password, class: 'form-label form-label-inline' %><%= mandatory_field if f.object.new_record? %> <%= error_messages_for_field f.object, :password %> <%= f.password_field :password, tabindex: increment, class: 'form-control', autocomplete: 'off' %> <% end %> <%= form_row for: [f.object, :password_confirmation] do %> - <%= f.label :password_confirmation, class: 'form-label' %><%= mandatory_field if f.object.new_record? %> + <%= f.label :password_confirmation, class: 'form-label form-label-inline' %><%= mandatory_field if f.object.new_record? %> <%= error_messages_for_field f.object, :password_confirmation %> <%= f.password_field :password_confirmation, tabindex: increment, class: 'form-control', autocomplete: 'off' %> <% end %> diff --git a/app/views/admin/admin_users/edit.html.erb b/app/views/admin/admin_users/edit.html.erb index 8b04b3dc3..3b6470a30 100644 --- a/app/views/admin/admin_users/edit.html.erb +++ b/app/views/admin/admin_users/edit.html.erb @@ -1,4 +1,6 @@

Edit user

-
- <%= render :partial => 'form' %> -
\ No newline at end of file +
+
+ <%= render :partial => 'form' %> +
+
From ed181f6028a39e9efb11aedd1f967b3b07c9a8f9 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Fri, 7 Oct 2016 16:59:10 +0100 Subject: [PATCH 22/22] Make link appear as a button for consistency --- app/views/admin/petition_details/show.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/admin/petition_details/show.html.erb b/app/views/admin/petition_details/show.html.erb index 8df48075f..5fe60b277 100644 --- a/app/views/admin/petition_details/show.html.erb +++ b/app/views/admin/petition_details/show.html.erb @@ -35,7 +35,7 @@ <%= f.submit 'Save', class: 'button' %> - <%= link_to 'Cancel', admin_petition_path(@petition) %> + <%= link_to 'Cancel', admin_petition_path(@petition), class: 'button-secondary' %> <% end %>