From 4322f8ea2277a3cf872a44cea34319e3de27f40d Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Tue, 19 Sep 2023 15:24:47 +0100 Subject: [PATCH 01/35] Inline partial This partial was being shared between two pages that I want to decouple. In the version inlined into `Users#edit_email_or_password`, I've swapped references to `@user` for `current_user`, because in that view a user can only ever change their own email and password. --- app/views/users/_form_errors.html.erb | 11 ----------- app/views/users/edit.html.erb | 13 ++++++++++++- app/views/users/edit_email_or_password.html.erb | 12 +++++++++++- 3 files changed, 23 insertions(+), 13 deletions(-) delete mode 100644 app/views/users/_form_errors.html.erb diff --git a/app/views/users/_form_errors.html.erb b/app/views/users/_form_errors.html.erb deleted file mode 100644 index 8b8af476b..000000000 --- a/app/views/users/_form_errors.html.erb +++ /dev/null @@ -1,11 +0,0 @@ -<% if @user.errors.count > 0 %> - -<% end %> diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index f0c3f9495..f87f3ed27 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -37,7 +37,18 @@ <%= form_for @user, :html => {:class => 'well add-top-margin'} do |f| %> - <%= render partial: "form_errors" %> + <% if @user.errors.count > 0 %> + + <% end %> + <%= render partial: "form_fields", locals: { f: f } %>
diff --git a/app/views/users/edit_email_or_password.html.erb b/app/views/users/edit_email_or_password.html.erb index ca399933d..32c9d99bf 100644 --- a/app/views/users/edit_email_or_password.html.erb +++ b/app/views/users/edit_email_or_password.html.erb @@ -12,7 +12,17 @@ <% end %> <% end %> -<%= render partial: "form_errors" %> +<% if current_user.errors.count > 0 %> + +<% end %>
From 453341e5351e84f535391294a63fa252660963ac Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Tue, 19 Sep 2023 15:52:29 +0100 Subject: [PATCH 02/35] Use the Error Summary component Adding some IDs to make the anchor links work --- .../passwords/_change_password_panel.html.erb | 3 +++ .../users/edit_email_or_password.html.erb | 20 ++++++++++--------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/app/views/devise/passwords/_change_password_panel.html.erb b/app/views/devise/passwords/_change_password_panel.html.erb index 42bda9743..84fa0aecf 100644 --- a/app/views/devise/passwords/_change_password_panel.html.erb +++ b/app/views/devise/passwords/_change_password_panel.html.erb @@ -5,6 +5,7 @@ text: "Current password" }, name: "user[current_password]", + id: "user_current_password", type: "password", autocomplete: "current-password" } %> @@ -17,6 +18,7 @@ }, hint: "Passwords must be at least 10 characters, shouldn’t include part of your email address and must be complex. Consider using whole sentences (with spaces), lyrics or phrases to make your password more memorable.", name: "user[password]", + id: "user_password", type: "password", autocomplete: "new-password", data: { @@ -33,6 +35,7 @@ }, name: "user[password_confirmation]", type: "password", + id: "user_password_confirmation", autocomplete: "new-password" } %>
diff --git a/app/views/users/edit_email_or_password.html.erb b/app/views/users/edit_email_or_password.html.erb index 32c9d99bf..24e07470c 100644 --- a/app/views/users/edit_email_or_password.html.erb +++ b/app/views/users/edit_email_or_password.html.erb @@ -13,15 +13,17 @@ <% end %> <% if current_user.errors.count > 0 %> - + <% content_for :error_summary do %> + <%= render "govuk_publishing_components/components/error_summary", { + title: "There is a problem", + items: current_user.errors.map do |error| + { + text: error.full_message, + href: "#user_#{error.attribute}", + } + end, + } %> + <% end %> <% end %>
From 81c7f23b9618afa9452a92aa8969ba8117304db1 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Tue, 19 Sep 2023 18:02:19 +0100 Subject: [PATCH 03/35] Inline string from locales I'm planning on moving this view to another controller. I don't believe we're currently aiming to localise Signon so it seems cleaner to just remove locales entries instead of updating them --- app/views/users/edit_email_or_password.html.erb | 2 +- config/locales/en.yml | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/views/users/edit_email_or_password.html.erb b/app/views/users/edit_email_or_password.html.erb index 24e07470c..d2204ddee 100644 --- a/app/views/users/edit_email_or_password.html.erb +++ b/app/views/users/edit_email_or_password.html.erb @@ -49,7 +49,7 @@
-

<%= t(".change_password") %>

+

Change your password

<%= form_for current_user, :url => update_password_user_path do |f| %> <%= render partial: "devise/passwords/change_password_panel", locals: { f: f, user: current_user, updating_password: true } %> <% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 501232e87..925891eb9 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -20,8 +20,6 @@ en: edit: change: "Save password" set_password: "Save password" - edit_email_or_password: - change_password: "Change your password" supported_permissions: form: From e869ac353e4c4d38e8a95805a28b9da499685fbf Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Wed, 20 Sep 2023 09:35:48 +0100 Subject: [PATCH 04/35] Group `alias_method`s more clearly I kept reading these aliases of `#edit?` as aliases of `#edit_email_or_password?`, purely based on their location. Maybe it's just me! --- app/policies/user_policy.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 6d26412b7..619aedb83 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -28,22 +28,21 @@ def edit? false end end - alias_method :update?, :edit? alias_method :unlock?, :edit? alias_method :suspension?, :edit? alias_method :resend?, :edit? alias_method :event_logs?, :edit? + alias_method :mandate_2sv?, :edit? + alias_method :require_2sv?, :edit? + alias_method :reset_2sv?, :edit? + alias_method :reset_two_step_verification?, :edit? def edit_email_or_password? allow_self_only end alias_method :update_email?, :edit_email_or_password? alias_method :update_password?, :edit_email_or_password? - alias_method :mandate_2sv?, :edit? - alias_method :require_2sv?, :edit? - alias_method :reset_2sv?, :edit? - alias_method :reset_two_step_verification?, :edit? def cancel_email_change? allow_self_only || edit? From ad6d785b1577cfee227dced99b48abc6d271ce08 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Tue, 19 Sep 2023 18:08:00 +0100 Subject: [PATCH 05/35] Move "Change your email or password" to /account We're making a new Account page. The new page will link to a handful of sub-pages that each handle a specific account-related task. This existing page will be used for changing your email address or password, so I've moved it into the /account namespace to hopefully make the user journey feel more cohesive. --- .../account/email_passwords_controller.rb | 14 ++++++++++++++ app/controllers/accounts_controller.rb | 2 +- app/controllers/users_controller.rb | 8 ++++---- app/policies/account/email_passwords_policy.rb | 5 +++++ app/policies/user_policy.rb | 5 ++--- .../email_passwords/show.html.erb} | 4 ++-- app/views/root/index.html.erb | 2 +- config/routes.rb | 2 +- test/controllers/users_controller_test.rb | 6 +++--- test/integration/account_test.rb | 2 +- .../account/email_passwords_policy_test.rb | 14 ++++++++++++++ test/policies/user_policy_test.rb | 2 +- 12 files changed, 49 insertions(+), 17 deletions(-) create mode 100644 app/controllers/account/email_passwords_controller.rb create mode 100644 app/policies/account/email_passwords_policy.rb rename app/views/{users/edit_email_or_password.html.erb => account/email_passwords/show.html.erb} (95%) create mode 100644 test/policies/account/email_passwords_policy_test.rb diff --git a/app/controllers/account/email_passwords_controller.rb b/app/controllers/account/email_passwords_controller.rb new file mode 100644 index 000000000..35aad3196 --- /dev/null +++ b/app/controllers/account/email_passwords_controller.rb @@ -0,0 +1,14 @@ +class Account::EmailPasswordsController < ApplicationController + layout "admin_layout" + + before_action :authenticate_user! + before_action :authorise_user + + def show; end + +private + + def authorise_user + authorize %i[account email_passwords] + end +end diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 1fcc0fa2f..73287292e 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -7,7 +7,7 @@ def show if policy(current_user).edit? redirect_to edit_user_path(current_user) else - redirect_to edit_email_or_password_user_path(current_user) + redirect_to account_email_password_path end end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a81e219e7..9bff56953 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -3,7 +3,7 @@ class UsersController < ApplicationController include UserPermissionsControllerMethods - layout "admin_layout", only: %w[index edit_email_or_password event_logs require_2sv] + layout "admin_layout", only: %w[index event_logs require_2sv] before_action :authenticate_user!, except: :show before_action :load_and_authorize_user, except: %i[index show] @@ -97,13 +97,13 @@ def update_email new_email = params[:user][:email] if current_email == new_email.strip flash[:alert] = "Nothing to update." - render :edit_email_or_password, layout: "admin_layout" + render "account/email_passwords/show", layout: "admin_layout" elsif @user.update(email: new_email) EventLog.record_email_change(@user, current_email, new_email) UserMailer.email_changed_notification(@user).deliver_later redirect_to root_path, notice: "An email has been sent to #{new_email}. Follow the link in the email to update your address." else - render :edit_email_or_password, layout: "admin_layout" + render "account/email_passwords/show", layout: "admin_layout" end end @@ -115,7 +115,7 @@ def update_password redirect_to root_path else EventLog.record_event(@user, EventLog::UNSUCCESSFUL_PASSWORD_CHANGE, ip_address: user_ip_address) - render :edit_email_or_password, layout: "admin_layout" + render "account/email_passwords/show", layout: "admin_layout" end end diff --git a/app/policies/account/email_passwords_policy.rb b/app/policies/account/email_passwords_policy.rb new file mode 100644 index 000000000..f44ad1a1f --- /dev/null +++ b/app/policies/account/email_passwords_policy.rb @@ -0,0 +1,5 @@ +class Account::EmailPasswordsPolicy < BasePolicy + def show? + current_user.present? + end +end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 619aedb83..85ab075b4 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -38,11 +38,10 @@ def edit? alias_method :reset_2sv?, :edit? alias_method :reset_two_step_verification?, :edit? - def edit_email_or_password? + def update_email? allow_self_only end - alias_method :update_email?, :edit_email_or_password? - alias_method :update_password?, :edit_email_or_password? + alias_method :update_password?, :update_email? def cancel_email_change? allow_self_only || edit? diff --git a/app/views/users/edit_email_or_password.html.erb b/app/views/account/email_passwords/show.html.erb similarity index 95% rename from app/views/users/edit_email_or_password.html.erb rename to app/views/account/email_passwords/show.html.erb index d2204ddee..c5297acdb 100644 --- a/app/views/users/edit_email_or_password.html.erb +++ b/app/views/account/email_passwords/show.html.erb @@ -30,7 +30,7 @@

Change your email address

- <%= form_for current_user, url: update_email_user_path do |f| %> + <%= form_for current_user, url: update_email_user_path(current_user) do |f| %> <%= render "govuk_publishing_components/components/input", { label: { text: "Email address" @@ -50,7 +50,7 @@

Change your password

- <%= form_for current_user, :url => update_password_user_path do |f| %> + <%= form_for current_user, :url => update_password_user_path(current_user) do |f| %> <%= render partial: "devise/passwords/change_password_panel", locals: { f: f, user: current_user, updating_password: true } %> <% end %>
diff --git a/app/views/root/index.html.erb b/app/views/root/index.html.erb index 1e358d1f4..28497998a 100644 --- a/app/views/root/index.html.erb +++ b/app/views/root/index.html.erb @@ -63,7 +63,7 @@ } %>
  • - <%= link_to "Change your email or password", edit_email_or_password_user_path(current_user), class: "govuk-link" %> + <%= link_to "Change your email or password", account_email_password_path, class: "govuk-link" %>
    • <% if current_user.has_2sv? %> diff --git a/config/routes.rb b/config/routes.rb index c9a3454ad..8b6a1c6c4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -36,7 +36,6 @@ resources :users, except: [:show] do member do - get :edit_email_or_password patch :update_email patch :update_password post :unlock @@ -57,6 +56,7 @@ get :delete end end + resource :email_password, only: [:show] end resources :batch_invitations, only: %i[new create show] do diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 3fa965f02..6d065fc51 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -117,7 +117,7 @@ def change_user_password(user_factory, new_password) setup do @user = create(:user_with_pending_email_change) sign_in @user - request.env["HTTP_REFERER"] = edit_email_or_password_user_path(@user) + request.env["HTTP_REFERER"] = account_email_password_path end should "clear the unconfirmed_email and the confirmation_token" do @@ -128,9 +128,9 @@ def change_user_password(user_factory, new_password) assert_nil @user.confirmation_token end - should "redirect to the user edit email or password page" do + should "redirect to the change email password page" do delete :cancel_email_change, params: { id: @user.id } - assert_redirected_to edit_email_or_password_user_path(@user) + assert_redirected_to account_email_password_path end end diff --git a/test/integration/account_test.rb b/test/integration/account_test.rb index 23f320ecb..6a967c6ee 100644 --- a/test/integration/account_test.rb +++ b/test/integration/account_test.rb @@ -27,7 +27,7 @@ class AccountTest < ActionDispatch::IntegrationTest visit account_path - assert_current_url edit_email_or_password_user_path(user) + assert_current_url account_email_password_path end end end diff --git a/test/policies/account/email_passwords_policy_test.rb b/test/policies/account/email_passwords_policy_test.rb new file mode 100644 index 000000000..a20ff8415 --- /dev/null +++ b/test/policies/account/email_passwords_policy_test.rb @@ -0,0 +1,14 @@ +require "test_helper" +require "support/policy_helpers" + +class Account::EmailPasswordsPolicyTest < ActiveSupport::TestCase + include PolicyHelpers + + should "allow logged in users to see show irrespective of their role" do + assert permit?(build(:user), nil, :show) + end + + should "not allow anonymous visitors to see show" do + assert forbid?(nil, nil, :show) + end +end diff --git a/test/policies/user_policy_test.rb b/test/policies/user_policy_test.rb index 79fea5cc4..b1c170ddc 100644 --- a/test/policies/user_policy_test.rb +++ b/test/policies/user_policy_test.rb @@ -14,7 +14,7 @@ class UserPolicyTest < ActiveSupport::TestCase primary_management_actions = %i[new assign_organisations] user_management_actions = %i[edit create update unlock suspension cancel_email_change resend_email_change event_logs reset_2sv mandate_2sv] - self_management_actions = %i[edit_email_or_password update_email update_password cancel_email_change resend_email_change] + self_management_actions = %i[update_email update_password cancel_email_change resend_email_change] superadmin_actions = %i[assign_role] two_step_verification_exemption_actions = %i[exempt_from_two_step_verification] From fd5038b6c1d509fefa60b1ec554fa95e2c9e66fb Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Wed, 20 Sep 2023 15:15:52 +0100 Subject: [PATCH 06/35] Move #update_email to /account namespace The ("Change your email or password") page that uses this endpoint has already moved to the /account namespace because it's becoming a sub-page of the new Account page. I'm undecided on the naming/RESTful design of this resource, so I'm waiting to take some cues from the upcoming design review in this area. For the time being I'm keeping it as I found it, but moving it into the same controller as the page that uses it. --- .../account/email_passwords_controller.rb | 15 ++++++++ app/controllers/users_controller.rb | 15 -------- .../account/email_passwords_policy.rb | 1 + app/policies/user_policy.rb | 3 +- .../account/email_passwords/show.html.erb | 2 +- config/routes.rb | 5 ++- .../email_passwords_controller_test.rb | 38 +++++++++++++++++++ test/controllers/users_controller_test.rb | 33 ---------------- test/policies/user_policy_test.rb | 2 +- 9 files changed, 60 insertions(+), 54 deletions(-) create mode 100644 test/controllers/account/email_passwords_controller_test.rb diff --git a/app/controllers/account/email_passwords_controller.rb b/app/controllers/account/email_passwords_controller.rb index 35aad3196..0c68db183 100644 --- a/app/controllers/account/email_passwords_controller.rb +++ b/app/controllers/account/email_passwords_controller.rb @@ -6,6 +6,21 @@ class Account::EmailPasswordsController < ApplicationController def show; end + def update_email + current_email = current_user.email + new_email = params[:user][:email] + if current_email == new_email.strip + flash[:alert] = "Nothing to update." + render :show + elsif current_user.update(email: new_email) + EventLog.record_email_change(current_user, current_email, new_email) + UserMailer.email_changed_notification(current_user).deliver_later + redirect_to root_path, notice: "An email has been sent to #{new_email}. Follow the link in the email to update your address." + else + render :show + end + end + private def authorise_user diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 9bff56953..1f9f1cfc1 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -92,21 +92,6 @@ def event_logs @logs = @user.event_logs.page(params[:page]).per(100) if @user end - def update_email - current_email = @user.email - new_email = params[:user][:email] - if current_email == new_email.strip - flash[:alert] = "Nothing to update." - render "account/email_passwords/show", layout: "admin_layout" - elsif @user.update(email: new_email) - EventLog.record_email_change(@user, current_email, new_email) - UserMailer.email_changed_notification(@user).deliver_later - redirect_to root_path, notice: "An email has been sent to #{new_email}. Follow the link in the email to update your address." - else - render "account/email_passwords/show", layout: "admin_layout" - end - end - def update_password if @user.update_with_password(password_params) EventLog.record_event(@user, EventLog::SUCCESSFUL_PASSWORD_CHANGE, ip_address: user_ip_address) diff --git a/app/policies/account/email_passwords_policy.rb b/app/policies/account/email_passwords_policy.rb index f44ad1a1f..bd7304dc9 100644 --- a/app/policies/account/email_passwords_policy.rb +++ b/app/policies/account/email_passwords_policy.rb @@ -2,4 +2,5 @@ class Account::EmailPasswordsPolicy < BasePolicy def show? current_user.present? end + alias_method :update_email?, :show? end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 85ab075b4..10ef82458 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -38,10 +38,9 @@ def edit? alias_method :reset_2sv?, :edit? alias_method :reset_two_step_verification?, :edit? - def update_email? + def update_password? allow_self_only end - alias_method :update_password?, :update_email? def cancel_email_change? allow_self_only || edit? diff --git a/app/views/account/email_passwords/show.html.erb b/app/views/account/email_passwords/show.html.erb index c5297acdb..73a665d44 100644 --- a/app/views/account/email_passwords/show.html.erb +++ b/app/views/account/email_passwords/show.html.erb @@ -30,7 +30,7 @@

    Change your email address

    - <%= form_for current_user, url: update_email_user_path(current_user) do |f| %> + <%= form_for current_user, url: update_email_account_email_password_path do |f| %> <%= render "govuk_publishing_components/components/input", { label: { text: "Email address" diff --git a/config/routes.rb b/config/routes.rb index 8b6a1c6c4..8e39572e2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -36,7 +36,6 @@ resources :users, except: [:show] do member do - patch :update_email patch :update_password post :unlock put :resend_email_change @@ -56,7 +55,9 @@ get :delete end end - resource :email_password, only: [:show] + resource :email_password, only: [:show] do + patch :update_email + end end resources :batch_invitations, only: %i[new create show] do diff --git a/test/controllers/account/email_passwords_controller_test.rb b/test/controllers/account/email_passwords_controller_test.rb new file mode 100644 index 000000000..e602cabe1 --- /dev/null +++ b/test/controllers/account/email_passwords_controller_test.rb @@ -0,0 +1,38 @@ +require "test_helper" + +class Account::EmailPasswordsControllerTest < ActionController::TestCase + include ActiveJob::TestHelper + + context "PUT update_email" do + setup do + @user = create(:user, email: "old@email.com") + sign_in @user + end + + context "changing an email" do + should "stage the change, send a confirmation email to the new address and email change notification to the old address" do + perform_enqueued_jobs do + put :update_email, params: { user: { email: "new@email.com" } } + + @user.reload + + assert_equal "new@email.com", @user.unconfirmed_email + assert_equal "old@email.com", @user.email + + confirmation_email = ActionMailer::Base.deliveries[-2] + assert_equal "Confirm your email change", confirmation_email.subject + assert_equal "new@email.com", confirmation_email.to.first + + email_changed_notification = ActionMailer::Base.deliveries.last + assert_match(/Your .* Signon development email address is being changed/, email_changed_notification.subject) + assert_equal "old@email.com", email_changed_notification.to.first + end + end + + should "log an event" do + put :update_email, params: { user: { email: "new@email.com" } } + assert_equal 1, EventLog.where(event_id: EventLog::EMAIL_CHANGE_INITIATED.id, uid: @user.uid, initiator_id: @user.id).count + end + end + end +end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 6d065fc51..fbde9e9c6 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -52,39 +52,6 @@ def change_user_password(user_factory, new_password) end end - context "PUT update_email" do - setup do - @user = create(:user, email: "old@email.com") - sign_in @user - end - - context "changing an email" do - should "stage the change, send a confirmation email to the new address and email change notification to the old address" do - perform_enqueued_jobs do - put :update_email, params: { id: @user.id, user: { email: "new@email.com" } } - - @user.reload - - assert_equal "new@email.com", @user.unconfirmed_email - assert_equal "old@email.com", @user.email - - confirmation_email = ActionMailer::Base.deliveries[-2] - assert_equal "Confirm your email change", confirmation_email.subject - assert_equal "new@email.com", confirmation_email.to.first - - email_changed_notification = ActionMailer::Base.deliveries.last - assert_match(/Your .* Signon development email address is being changed/, email_changed_notification.subject) - assert_equal "old@email.com", email_changed_notification.to.first - end - end - - should "log an event" do - put :update_email, params: { id: @user.id, user: { email: "new@email.com" } } - assert_equal 1, EventLog.where(event_id: EventLog::EMAIL_CHANGE_INITIATED.id, uid: @user.uid, initiator_id: @user.id).count - end - end - end - context "PUT resend_email_change" do should "send an email change confirmation email" do perform_enqueued_jobs do diff --git a/test/policies/user_policy_test.rb b/test/policies/user_policy_test.rb index b1c170ddc..1a93a4fb6 100644 --- a/test/policies/user_policy_test.rb +++ b/test/policies/user_policy_test.rb @@ -14,7 +14,7 @@ class UserPolicyTest < ActiveSupport::TestCase primary_management_actions = %i[new assign_organisations] user_management_actions = %i[edit create update unlock suspension cancel_email_change resend_email_change event_logs reset_2sv mandate_2sv] - self_management_actions = %i[update_email update_password cancel_email_change resend_email_change] + self_management_actions = %i[update_password cancel_email_change resend_email_change] superadmin_actions = %i[assign_role] two_step_verification_exemption_actions = %i[exempt_from_two_step_verification] From 9e8eb24ab7d085cf314688bf2e3b52d8075f1c2b Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Thu, 21 Sep 2023 14:16:39 +0100 Subject: [PATCH 07/35] Move #update_password to /account namespace The ("Change your email or password") page that uses this endpoint has already moved to the /account namespace because it's becoming a sub-page of the new Account page. I'm undecided on the naming/RESTful design of this resource, so I'm waiting to take some cues from the upcoming design review in this area. For the time being I'm keeping it as I found it, but moving it into the same controller as the page that uses it. --- .../account/email_passwords_controller.rb | 20 ++++++++ app/controllers/users_controller.rb | 20 -------- .../account/email_passwords_policy.rb | 1 + app/policies/user_policy.rb | 4 -- .../account/email_passwords/show.html.erb | 2 +- config/routes.rb | 2 +- .../email_passwords_controller_test.rb | 50 +++++++++++++++++++ test/controllers/users_controller_test.rb | 49 ------------------ test/policies/user_policy_test.rb | 2 +- 9 files changed, 74 insertions(+), 76 deletions(-) diff --git a/app/controllers/account/email_passwords_controller.rb b/app/controllers/account/email_passwords_controller.rb index 0c68db183..f7678ea9a 100644 --- a/app/controllers/account/email_passwords_controller.rb +++ b/app/controllers/account/email_passwords_controller.rb @@ -21,9 +21,29 @@ def update_email end end + def update_password + if current_user.update_with_password(password_params) + EventLog.record_event(current_user, EventLog::SUCCESSFUL_PASSWORD_CHANGE, ip_address: user_ip_address) + flash[:notice] = t(:updated, scope: "devise.passwords") + bypass_sign_in(current_user) + redirect_to root_path + else + EventLog.record_event(current_user, EventLog::UNSUCCESSFUL_PASSWORD_CHANGE, ip_address: user_ip_address) + render :show + end + end + private def authorise_user authorize %i[account email_passwords] end + + def password_params + params.require(:user).permit( + :current_password, + :password, + :password_confirmation, + ) + end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 1f9f1cfc1..f8d9f638e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -92,18 +92,6 @@ def event_logs @logs = @user.event_logs.page(params[:page]).per(100) if @user end - def update_password - if @user.update_with_password(password_params) - EventLog.record_event(@user, EventLog::SUCCESSFUL_PASSWORD_CHANGE, ip_address: user_ip_address) - flash[:notice] = t(:updated, scope: "devise.passwords") - bypass_sign_in(@user) - redirect_to root_path - else - EventLog.record_event(@user, EventLog::UNSUCCESSFUL_PASSWORD_CHANGE, ip_address: user_ip_address) - render "account/email_passwords/show", layout: "admin_layout" - end - end - def reset_two_step_verification @user.reset_2sv!(current_user) UserMailer.two_step_reset(@user).deliver_later @@ -165,14 +153,6 @@ def permitted_user_params @permitted_user_params ||= params.require(:user).permit(:user, :name, :email, :organisation_id, :require_2sv, :role, :skip_update_user_permissions, supported_permission_ids: []).to_h end - def password_params - params.require(:user).permit( - :current_password, - :password, - :password_confirmation, - ) - end - def filter_params params.permit( :filter, :page, :format, :"option-select-filter", diff --git a/app/policies/account/email_passwords_policy.rb b/app/policies/account/email_passwords_policy.rb index bd7304dc9..a07ce0fbf 100644 --- a/app/policies/account/email_passwords_policy.rb +++ b/app/policies/account/email_passwords_policy.rb @@ -3,4 +3,5 @@ def show? current_user.present? end alias_method :update_email?, :show? + alias_method :update_password?, :show? end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 10ef82458..e8e893572 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -38,10 +38,6 @@ def edit? alias_method :reset_2sv?, :edit? alias_method :reset_two_step_verification?, :edit? - def update_password? - allow_self_only - end - def cancel_email_change? allow_self_only || edit? end diff --git a/app/views/account/email_passwords/show.html.erb b/app/views/account/email_passwords/show.html.erb index 73a665d44..320fc1ded 100644 --- a/app/views/account/email_passwords/show.html.erb +++ b/app/views/account/email_passwords/show.html.erb @@ -50,7 +50,7 @@

    Change your password

    - <%= form_for current_user, :url => update_password_user_path(current_user) do |f| %> + <%= form_for current_user, :url => update_password_account_email_password_path do |f| %> <%= render partial: "devise/passwords/change_password_panel", locals: { f: f, user: current_user, updating_password: true } %> <% end %>
    diff --git a/config/routes.rb b/config/routes.rb index 8e39572e2..3b36ec207 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -36,7 +36,6 @@ resources :users, except: [:show] do member do - patch :update_password post :unlock put :resend_email_change delete :cancel_email_change @@ -57,6 +56,7 @@ end resource :email_password, only: [:show] do patch :update_email + patch :update_password end end diff --git a/test/controllers/account/email_passwords_controller_test.rb b/test/controllers/account/email_passwords_controller_test.rb index e602cabe1..e7013f84f 100644 --- a/test/controllers/account/email_passwords_controller_test.rb +++ b/test/controllers/account/email_passwords_controller_test.rb @@ -35,4 +35,54 @@ class Account::EmailPasswordsControllerTest < ActionController::TestCase end end end + + def change_user_password(user_factory, new_password) + original_password = "I am a very original password. Refrigerator weevil." + user = create(user_factory, password: original_password) + original_password_hash = user.encrypted_password + sign_in user + + post :update_password, + params: { + user: { + current_password: original_password, + password: new_password, + password_confirmation: new_password, + }, + } + + [user, original_password_hash] + end + + context "PUT update_password" do + should "changing passwords to something strong should succeed" do + user, orig_password = change_user_password(:user, "destabilizers842}orthophosphate") + + assert_equal "302", response.code + assert_equal root_url, response.location + + user.reload + assert_not_equal orig_password, user.encrypted_password + end + + should "changing password to something too short should fail" do + user, orig_password = change_user_password(:user, "short") + + assert_equal "200", response.code + assert_match "too short", response.body + + user.reload + assert_equal orig_password, user.encrypted_password + end + + should "changing password to something too weak should fail" do + user, orig_password = change_user_password(:user, "zymophosphate") + + assert_equal "200", response.code + assert_match "not strong enough", response.body + + user.reload + assert_equal orig_password, user.encrypted_password + end + end end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index fbde9e9c6..dc733a2f1 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -3,55 +3,6 @@ class UsersControllerTest < ActionController::TestCase include ActiveJob::TestHelper - def change_user_password(user_factory, new_password) - original_password = "I am a very original password. Refrigerator weevil." - user = create(user_factory, password: original_password) - original_password_hash = user.encrypted_password - sign_in user - - post :update_password, - params: { id: user.id, - user: { - current_password: original_password, - password: new_password, - password_confirmation: new_password, - } } - - [user, original_password_hash] - end - - context "PUT update_password" do - should "changing passwords to something strong should succeed" do - user, orig_password = change_user_password(:user, "destabilizers842}orthophosphate") - - assert_equal "302", response.code - assert_equal root_url, response.location - - user.reload - assert_not_equal orig_password, user.encrypted_password - end - - should "changing password to something too short should fail" do - user, orig_password = change_user_password(:user, "short") - - assert_equal "200", response.code - assert_match "too short", response.body - - user.reload - assert_equal orig_password, user.encrypted_password - end - - should "changing password to something too weak should fail" do - user, orig_password = change_user_password(:user, "zymophosphate") - - assert_equal "200", response.code - assert_match "not strong enough", response.body - - user.reload - assert_equal orig_password, user.encrypted_password - end - end - context "PUT resend_email_change" do should "send an email change confirmation email" do perform_enqueued_jobs do diff --git a/test/policies/user_policy_test.rb b/test/policies/user_policy_test.rb index 1a93a4fb6..15d08e163 100644 --- a/test/policies/user_policy_test.rb +++ b/test/policies/user_policy_test.rb @@ -14,7 +14,7 @@ class UserPolicyTest < ActiveSupport::TestCase primary_management_actions = %i[new assign_organisations] user_management_actions = %i[edit create update unlock suspension cancel_email_change resend_email_change event_logs reset_2sv mandate_2sv] - self_management_actions = %i[update_password cancel_email_change resend_email_change] + self_management_actions = %i[cancel_email_change resend_email_change] superadmin_actions = %i[assign_role] two_step_verification_exemption_actions = %i[exempt_from_two_step_verification] From a3f5a7922de86e20ad8764500a0ac967f929372c Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Thu, 21 Sep 2023 15:23:13 +0100 Subject: [PATCH 08/35] A separate #resend_email_change for /account Until now, Users#resend_email_change was used in two distinct scenarios: 1. Admins performing the action for other users' accounts 2. Users (of all roles) performing the action for their own account It seems as though the `if @user.normal?` conditional was actually breaking this distinction: in the case of admin users in scenario 2, they'd only ever see the flash notice intended for scenario 1. Scenario 2 is now exclusively performed from the "Change your email or password" page. That page has already moved to the /account namespace because it's becoming a sub-page of the new Account page. That page now has its own dedicated `#resend_email_change` endpoint. Scenario 1 is still performed from Users#edit. Similarly to the approach with the actions that I've just lifted-and-shifted from UsersController to Account::EmailPasswordsController, for the time being I'm keeping the naming/RESTful design as I found it and planning to rethink that after the upcoming design review. --- .../account/email_passwords_controller.rb | 9 ++++++ app/controllers/users_controller.rb | 7 +---- .../account/email_passwords_policy.rb | 1 + app/policies/user_policy.rb | 5 +--- .../account/email_passwords/show.html.erb | 2 +- config/routes.rb | 1 + .../email_passwords_controller_test.rb | 28 +++++++++++++++++++ test/controllers/users_controller_test.rb | 28 ------------------- test/policies/user_policy_test.rb | 2 +- 9 files changed, 43 insertions(+), 40 deletions(-) diff --git a/app/controllers/account/email_passwords_controller.rb b/app/controllers/account/email_passwords_controller.rb index f7678ea9a..93a403beb 100644 --- a/app/controllers/account/email_passwords_controller.rb +++ b/app/controllers/account/email_passwords_controller.rb @@ -33,6 +33,15 @@ def update_password end end + def resend_email_change + current_user.resend_confirmation_instructions + if current_user.errors.empty? + redirect_to root_path, notice: "An email has been sent to #{current_user.unconfirmed_email}. Follow the link in the email to update your address." + else + redirect_to account_email_password_path, alert: "Failed to send email change email" + end + end + private def authorise_user diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index f8d9f638e..cb2e15bb7 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -69,12 +69,7 @@ def unlock def resend_email_change @user.resend_confirmation_instructions if @user.errors.empty? - notice = if @user.normal? - "An email has been sent to #{@user.unconfirmed_email}. Follow the link in the email to update your address." - else - "Successfully resent email change email to #{@user.unconfirmed_email}" - end - redirect_to root_path, notice: + redirect_to root_path, notice: "Successfully resent email change email to #{@user.unconfirmed_email}" else redirect_to edit_user_path(@user), alert: "Failed to send email change email" end diff --git a/app/policies/account/email_passwords_policy.rb b/app/policies/account/email_passwords_policy.rb index a07ce0fbf..0bc9b56bf 100644 --- a/app/policies/account/email_passwords_policy.rb +++ b/app/policies/account/email_passwords_policy.rb @@ -4,4 +4,5 @@ def show? end alias_method :update_email?, :show? alias_method :update_password?, :show? + alias_method :resend_email_change?, :show? end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index e8e893572..dd8c48a02 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -37,15 +37,12 @@ def edit? alias_method :require_2sv?, :edit? alias_method :reset_2sv?, :edit? alias_method :reset_two_step_verification?, :edit? + alias_method :resend_email_change?, :edit? def cancel_email_change? allow_self_only || edit? end - def resend_email_change? - allow_self_only || edit? - end - def assign_role? current_user.superadmin? end diff --git a/app/views/account/email_passwords/show.html.erb b/app/views/account/email_passwords/show.html.erb index 320fc1ded..729a65180 100644 --- a/app/views/account/email_passwords/show.html.erb +++ b/app/views/account/email_passwords/show.html.erb @@ -7,7 +7,7 @@ } do %>

    We've sent an email to <%= current_user.unconfirmed_email %> with a link to confirm the change. If you haven't received this email, we can send it again:

    - <%= link_to "Resend confirmation email", resend_email_change_user_path(current_user), method: :put, class: "govuk-button app-button--no-margin" %> + <%= link_to "Resend confirmation email", resend_email_change_account_email_password_path, method: :put, class: "govuk-button app-button--no-margin" %> <%= link_to "Cancel change", cancel_email_change_user_path(current_user), method: :delete, class: "govuk-link app-link--inline" %> <% end %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index 3b36ec207..214c25756 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -57,6 +57,7 @@ resource :email_password, only: [:show] do patch :update_email patch :update_password + put :resend_email_change end end diff --git a/test/controllers/account/email_passwords_controller_test.rb b/test/controllers/account/email_passwords_controller_test.rb index e7013f84f..1f80e4888 100644 --- a/test/controllers/account/email_passwords_controller_test.rb +++ b/test/controllers/account/email_passwords_controller_test.rb @@ -85,4 +85,32 @@ def change_user_password(user_factory, new_password) assert_equal orig_password, user.encrypted_password end end + + context "PUT resend_email_change" do + should "send an email change confirmation email" do + perform_enqueued_jobs do + @user = create(:user_with_pending_email_change) + sign_in @user + + put :resend_email_change + + assert_equal "Confirm your email change", ActionMailer::Base.deliveries.last.subject + end + end + + should "use a new token if it's expired" do + perform_enqueued_jobs do + @user = create( + :user_with_pending_email_change, + confirmation_token: "old token", + confirmation_sent_at: 15.days.ago, + ) + sign_in @user + + put :resend_email_change + + assert_not_equal "old token", @user.reload.confirmation_token + end + end + end end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index dc733a2f1..5c0907c02 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -3,34 +3,6 @@ class UsersControllerTest < ActionController::TestCase include ActiveJob::TestHelper - context "PUT resend_email_change" do - should "send an email change confirmation email" do - perform_enqueued_jobs do - @user = create(:user_with_pending_email_change) - sign_in @user - - put :resend_email_change, params: { id: @user.id } - - assert_equal "Confirm your email change", ActionMailer::Base.deliveries.last.subject - end - end - - should "use a new token if it's expired" do - perform_enqueued_jobs do - @user = create( - :user_with_pending_email_change, - confirmation_token: "old token", - confirmation_sent_at: 15.days.ago, - ) - sign_in @user - - put :resend_email_change, params: { id: @user.id } - - assert_not_equal "old token", @user.reload.confirmation_token - end - end - end - context "DELETE cancel_email_change" do setup do @user = create(:user_with_pending_email_change) diff --git a/test/policies/user_policy_test.rb b/test/policies/user_policy_test.rb index 15d08e163..5f27f16b9 100644 --- a/test/policies/user_policy_test.rb +++ b/test/policies/user_policy_test.rb @@ -14,7 +14,7 @@ class UserPolicyTest < ActiveSupport::TestCase primary_management_actions = %i[new assign_organisations] user_management_actions = %i[edit create update unlock suspension cancel_email_change resend_email_change event_logs reset_2sv mandate_2sv] - self_management_actions = %i[cancel_email_change resend_email_change] + self_management_actions = %i[cancel_email_change] superadmin_actions = %i[assign_role] two_step_verification_exemption_actions = %i[exempt_from_two_step_verification] From f39cbc214057d0d74e79946f0c7ec182a8c35532 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Thu, 21 Sep 2023 15:44:34 +0100 Subject: [PATCH 09/35] Extract User#cancel_email_change! --- app/controllers/users_controller.rb | 5 ++--- app/models/user.rb | 6 ++++++ test/models/user_test.rb | 13 +++++++++++++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index cb2e15bb7..33e75a368 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -76,9 +76,8 @@ def resend_email_change end def cancel_email_change - @user.unconfirmed_email = nil - @user.confirmation_token = nil - @user.save!(validate: false) + @user.cancel_email_change! + redirect_back_or_to(root_path) end diff --git a/app/models/user.rb b/app/models/user.rb index 651a968f8..18d4a3971 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -255,6 +255,12 @@ def postpone_email_change? end end + def cancel_email_change! + self.unconfirmed_email = nil + self.confirmation_token = nil + save!(validate: false) + end + def invited_but_not_yet_accepted? invitation_sent_at.present? && invitation_accepted_at.nil? end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 3e1014c1a..9af06194b 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -173,6 +173,19 @@ def setup assert_equal "needs to be confirmed within 14 days, please request a new one", @user.errors[:email][0] end + test "#cancel_email_change!" do + original_email = "isabella.nagarkar@department.gov.uk" + user = create(:user_with_pending_email_change, email: original_email) + + assert user.pending_reconfirmation? + + user.cancel_email_change! + user.reload + + assert_not user.pending_reconfirmation? + assert_equal original_email, user.email + end + # Scopes test "web_users includes non api users" do From 9b06439a641efbff5ee9924e0e9c0c7348730429 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Thu, 21 Sep 2023 15:51:55 +0100 Subject: [PATCH 10/35] A separate #cancel_email_change for /account Until now, Users#cancel_email_change was used in two distinct scenarios: 1. Admins performing the action for other users' accounts 2. Users (of all roles) performing the action for their own account Scenario 2 is now exclusively performed from the "Change your email or password" page. That page has already moved to the /account namespace because it's becoming a sub-page of the new Account page. (Scenario 1 is still performed from Users#edit.) Similarly to the approach with the actions that I've just lifted-and-shifted from UsersController to Account::EmailPasswordsController, for the time being I'm keeping the naming/RESTful design as I found it and planning to rethink that after the upcoming design review. I'm struggling to fully understand the UserPolicy (!) but I think it was safe to remove the remaining references to `allow_self_only`, since Users#cancel_email_change was the last UsersController action left that was used in the ways described by both of the scenarios listed above ^ --- .../account/email_passwords_controller.rb | 6 ++++++ .../account/email_passwords_policy.rb | 1 + app/policies/user_policy.rb | 13 +++--------- .../account/email_passwords/show.html.erb | 2 +- config/routes.rb | 1 + .../email_passwords_controller_test.rb | 21 +++++++++++++++++++ test/controllers/users_controller_test.rb | 21 ------------------- test/policies/user_policy_test.rb | 16 -------------- 8 files changed, 33 insertions(+), 48 deletions(-) diff --git a/app/controllers/account/email_passwords_controller.rb b/app/controllers/account/email_passwords_controller.rb index 93a403beb..9f64f082a 100644 --- a/app/controllers/account/email_passwords_controller.rb +++ b/app/controllers/account/email_passwords_controller.rb @@ -42,6 +42,12 @@ def resend_email_change end end + def cancel_email_change + current_user.cancel_email_change! + + redirect_back_or_to(root_path) + end + private def authorise_user diff --git a/app/policies/account/email_passwords_policy.rb b/app/policies/account/email_passwords_policy.rb index 0bc9b56bf..fe71f35f5 100644 --- a/app/policies/account/email_passwords_policy.rb +++ b/app/policies/account/email_passwords_policy.rb @@ -5,4 +5,5 @@ def show? alias_method :update_email?, :show? alias_method :update_password?, :show? alias_method :resend_email_change?, :show? + alias_method :cancel_email_change?, :show? end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index dd8c48a02..783d035b2 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -21,9 +21,9 @@ def edit? when Roles::Admin.role_name can_manage? when Roles::SuperOrganisationAdmin.role_name - allow_self_only || (can_manage? && (record_in_own_organisation? || record_in_child_organisation?)) + can_manage? && (record_in_own_organisation? || record_in_child_organisation?) when Roles::OrganisationAdmin.role_name - allow_self_only || (can_manage? && record_in_own_organisation?) + can_manage? && record_in_own_organisation? else # 'normal' false end @@ -38,10 +38,7 @@ def edit? alias_method :reset_2sv?, :edit? alias_method :reset_two_step_verification?, :edit? alias_method :resend_email_change?, :edit? - - def cancel_email_change? - allow_self_only || edit? - end + alias_method :cancel_email_change?, :edit? def assign_role? current_user.superadmin? @@ -56,10 +53,6 @@ def exempt_from_two_step_verification? private - def allow_self_only - current_user.id == record.id - end - def can_manage? current_user.can_manage?(record) end diff --git a/app/views/account/email_passwords/show.html.erb b/app/views/account/email_passwords/show.html.erb index 729a65180..b7da9b171 100644 --- a/app/views/account/email_passwords/show.html.erb +++ b/app/views/account/email_passwords/show.html.erb @@ -8,7 +8,7 @@

    We've sent an email to <%= current_user.unconfirmed_email %> with a link to confirm the change. If you haven't received this email, we can send it again:

    <%= link_to "Resend confirmation email", resend_email_change_account_email_password_path, method: :put, class: "govuk-button app-button--no-margin" %> - <%= link_to "Cancel change", cancel_email_change_user_path(current_user), method: :delete, class: "govuk-link app-link--inline" %> + <%= link_to "Cancel change", cancel_email_change_account_email_password_path, method: :delete, class: "govuk-link app-link--inline" %> <% end %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index 214c25756..7849191b0 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -58,6 +58,7 @@ patch :update_email patch :update_password put :resend_email_change + delete :cancel_email_change end end diff --git a/test/controllers/account/email_passwords_controller_test.rb b/test/controllers/account/email_passwords_controller_test.rb index 1f80e4888..c4e7a02d4 100644 --- a/test/controllers/account/email_passwords_controller_test.rb +++ b/test/controllers/account/email_passwords_controller_test.rb @@ -113,4 +113,25 @@ def change_user_password(user_factory, new_password) end end end + + context "DELETE cancel_email_change" do + setup do + @user = create(:user_with_pending_email_change) + sign_in @user + request.env["HTTP_REFERER"] = account_email_password_path + end + + should "clear the unconfirmed_email and the confirmation_token" do + delete :cancel_email_change + + @user.reload + assert_nil @user.unconfirmed_email + assert_nil @user.confirmation_token + end + + should "redirect to the change email password page" do + delete :cancel_email_change + assert_redirected_to account_email_password_path + end + end end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index 5c0907c02..ab54c23d9 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -3,27 +3,6 @@ class UsersControllerTest < ActionController::TestCase include ActiveJob::TestHelper - context "DELETE cancel_email_change" do - setup do - @user = create(:user_with_pending_email_change) - sign_in @user - request.env["HTTP_REFERER"] = account_email_password_path - end - - should "clear the unconfirmed_email and the confirmation_token" do - delete :cancel_email_change, params: { id: @user.id } - - @user.reload - assert_nil @user.unconfirmed_email - assert_nil @user.confirmation_token - end - - should "redirect to the change email password page" do - delete :cancel_email_change, params: { id: @user.id } - assert_redirected_to account_email_password_path - end - end - context "GET show (as OAuth client application)" do setup do @application = create(:application) diff --git a/test/policies/user_policy_test.rb b/test/policies/user_policy_test.rb index 5f27f16b9..67d2ba35a 100644 --- a/test/policies/user_policy_test.rb +++ b/test/policies/user_policy_test.rb @@ -14,13 +14,11 @@ class UserPolicyTest < ActiveSupport::TestCase primary_management_actions = %i[new assign_organisations] user_management_actions = %i[edit create update unlock suspension cancel_email_change resend_email_change event_logs reset_2sv mandate_2sv] - self_management_actions = %i[cancel_email_change] superadmin_actions = %i[assign_role] two_step_verification_exemption_actions = %i[exempt_from_two_step_verification] org_admin_actions = user_management_actions - %i[create] super_org_admin_actions = user_management_actions - %i[create] - admin_actions = user_management_actions - self_management_actions context "superadmins" do should "allow for index" do @@ -256,13 +254,6 @@ class UserPolicyTest < ActiveSupport::TestCase end end - self_management_actions.each do |permission| - should "allow for #{permission} accessing their own record" do - user = create(:user) - assert permit?(user, user, permission) - end - end - superadmin_actions.each do |permission| should "not allow for #{permission}" do assert forbid?(create(:user), User, permission) @@ -275,12 +266,5 @@ class UserPolicyTest < ActiveSupport::TestCase assert forbid?(create(:user), user, permission) end end - - admin_actions.each do |permission| - should "not allow for #{permission} accessing their own record" do - user = create(:user) - assert forbid?(user, user, permission) - end - end end end From 091cfcd9d776a77e427e1215097877ac728896ff Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Wed, 27 Sep 2023 17:18:32 +0100 Subject: [PATCH 11/35] Use `redirect_to` instead of `redirect_back_or_to` We were only doing this for one of the actions in this controller, which was resulting in an inconsistent experience for users. Since there are soon going to be multiple routes into the "Change your email or password" page (i.e. the page that uses this endpoint), it might make sense to redirect back to the page that the user visited *before* that one. That's not what `redirect_back_or_to` was doing in this case, though, it was redirecting back to the "Change your email or password" page itself. Either way, it should be consistent. --- app/controllers/account/email_passwords_controller.rb | 2 +- test/controllers/account/email_passwords_controller_test.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/account/email_passwords_controller.rb b/app/controllers/account/email_passwords_controller.rb index 9f64f082a..8abb28fc8 100644 --- a/app/controllers/account/email_passwords_controller.rb +++ b/app/controllers/account/email_passwords_controller.rb @@ -45,7 +45,7 @@ def resend_email_change def cancel_email_change current_user.cancel_email_change! - redirect_back_or_to(root_path) + redirect_to root_path end private diff --git a/test/controllers/account/email_passwords_controller_test.rb b/test/controllers/account/email_passwords_controller_test.rb index c4e7a02d4..6d61132c3 100644 --- a/test/controllers/account/email_passwords_controller_test.rb +++ b/test/controllers/account/email_passwords_controller_test.rb @@ -118,7 +118,6 @@ def change_user_password(user_factory, new_password) setup do @user = create(:user_with_pending_email_change) sign_in @user - request.env["HTTP_REFERER"] = account_email_password_path end should "clear the unconfirmed_email and the confirmation_token" do From 6dba23265b8739a8ebbe8a91acc3585eaa3b7921 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Mon, 25 Sep 2023 12:10:15 +0100 Subject: [PATCH 12/35] Extract duplicate flash notice I've included tests to confirm that this is working (most useful for `#update_email` where we've switched from using `new_email` to using `current_user.unconfirmed_email`) --- .../account/email_passwords_controller.rb | 9 +++++++-- .../account/email_passwords_controller_test.rb | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/app/controllers/account/email_passwords_controller.rb b/app/controllers/account/email_passwords_controller.rb index 8abb28fc8..03612a064 100644 --- a/app/controllers/account/email_passwords_controller.rb +++ b/app/controllers/account/email_passwords_controller.rb @@ -15,7 +15,7 @@ def update_email elsif current_user.update(email: new_email) EventLog.record_email_change(current_user, current_email, new_email) UserMailer.email_changed_notification(current_user).deliver_later - redirect_to root_path, notice: "An email has been sent to #{new_email}. Follow the link in the email to update your address." + redirect_to root_path, notice: email_change_notice else render :show end @@ -36,7 +36,7 @@ def update_password def resend_email_change current_user.resend_confirmation_instructions if current_user.errors.empty? - redirect_to root_path, notice: "An email has been sent to #{current_user.unconfirmed_email}. Follow the link in the email to update your address." + redirect_to root_path, notice: email_change_notice else redirect_to account_email_password_path, alert: "Failed to send email change email" end @@ -61,4 +61,9 @@ def password_params :password_confirmation, ) end + + def email_change_notice + "An email has been sent to #{current_user.unconfirmed_email}. "\ + "Follow the link in the email to update your address." + end end diff --git a/test/controllers/account/email_passwords_controller_test.rb b/test/controllers/account/email_passwords_controller_test.rb index 6d61132c3..c3ce79c09 100644 --- a/test/controllers/account/email_passwords_controller_test.rb +++ b/test/controllers/account/email_passwords_controller_test.rb @@ -29,6 +29,12 @@ class Account::EmailPasswordsControllerTest < ActionController::TestCase end end + should "confirm the change in a flash notice" do + put :update_email, params: { user: { email: "new@email.com" } } + + assert_match(/An email has been sent to new@email\.com/, flash[:notice]) + end + should "log an event" do put :update_email, params: { user: { email: "new@email.com" } } assert_equal 1, EventLog.where(event_id: EventLog::EMAIL_CHANGE_INITIATED.id, uid: @user.uid, initiator_id: @user.id).count @@ -98,6 +104,15 @@ def change_user_password(user_factory, new_password) end end + should "confirm resend in a flash notice" do + user = create(:user_with_pending_email_change, unconfirmed_email: "new@email.com") + sign_in user + + put :resend_email_change + + assert_match(/An email has been sent to new@email\.com/, flash[:notice]) + end + should "use a new token if it's expired" do perform_enqueued_jobs do @user = create( From 218f4f85d0b13490240b39c8a55d7888c1912edb Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Mon, 25 Sep 2023 16:26:32 +0100 Subject: [PATCH 13/35] Add missing flash message --- app/controllers/account/email_passwords_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/account/email_passwords_controller.rb b/app/controllers/account/email_passwords_controller.rb index 03612a064..c77c027c8 100644 --- a/app/controllers/account/email_passwords_controller.rb +++ b/app/controllers/account/email_passwords_controller.rb @@ -45,7 +45,7 @@ def resend_email_change def cancel_email_change current_user.cancel_email_change! - redirect_to root_path + redirect_to root_path, notice: "You have cancelled your pending email address change." end private From 18ea94ee84928ba86e56c9f09adb6df6601230dd Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 22 Sep 2023 16:57:23 +0100 Subject: [PATCH 14/35] Introduce new Account page with one sub-page That "sub-page" is the "Change your email or password" page, which is linked to from a card component. I'm planning to add 4 more cards to this page in later commits. Like this first one, they'll link to a standalone sub-page each allowing users to make changes to a different aspect of their account. In this commit I've also removed an integration test instead of updating it to reflect my changes, because the important stuff to cover is now covered by the AccountTest. --- app/controllers/accounts_controller.rb | 8 ++------ app/views/accounts/show.html.erb | 28 ++++++++++++++++++++++++++ test/integration/account_test.rb | 16 +++------------ test/integration/user_link_test.rb | 16 --------------- 4 files changed, 33 insertions(+), 35 deletions(-) create mode 100644 app/views/accounts/show.html.erb delete mode 100644 test/integration/user_link_test.rb diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 73287292e..8a3448c01 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -1,13 +1,9 @@ class AccountsController < ApplicationController + layout "admin_layout" + before_action :authenticate_user! def show authorize :account_page - - if policy(current_user).edit? - redirect_to edit_user_path(current_user) - else - redirect_to account_email_password_path - end end end diff --git a/app/views/accounts/show.html.erb b/app/views/accounts/show.html.erb new file mode 100644 index 000000000..70429922b --- /dev/null +++ b/app/views/accounts/show.html.erb @@ -0,0 +1,28 @@ +<% content_for :title, "Settings" %> + +<% content_for :breadcrumbs, + render("govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "Dashboard", + url: root_path, + }, + { + title: "Settings", + } + ] + }) +%> + +<%= render "govuk_publishing_components/components/cards", { + items: [ + { + link: { + text: "Change your email or password", + path: account_email_password_path, + }, + description: "Reset your password or change your email address.", + }, + ].compact +} %> diff --git a/test/integration/account_test.rb b/test/integration/account_test.rb index 6a967c6ee..9df0bd8b2 100644 --- a/test/integration/account_test.rb +++ b/test/integration/account_test.rb @@ -8,26 +8,16 @@ class AccountTest < ActionDispatch::IntegrationTest assert_current_url new_user_session_path end - should "redirect to user's edit page for admin users" do - user = FactoryBot.create(:admin_user) - - visit new_user_session_path - signin_with user - - visit account_path - - assert_current_url edit_user_path(user) - end - - should "redirect to edit email or password page for normal users" do + should "link to Change email/password" do user = FactoryBot.create(:user) visit new_user_session_path signin_with user visit account_path + assert page.has_selector?("h1", text: "Settings") - assert_current_url account_email_password_path + assert page.has_link?("Change your email or password", href: account_email_password_path) end end end diff --git a/test/integration/user_link_test.rb b/test/integration/user_link_test.rb deleted file mode 100644 index be6448b2c..000000000 --- a/test/integration/user_link_test.rb +++ /dev/null @@ -1,16 +0,0 @@ -require_relative "../test_helper" - -class UserLinkTest < ActionDispatch::IntegrationTest - context "logged in as an admin" do - setup do - @admin = create(:admin_user, name: "Adam Adminson", email: "admin@example.com") - visit new_user_session_path - signin_with(@admin) - end - - should "link to the current user's edit page" do - click_on "Adam Adminson" - assert page.has_content?("Edit “Adam Adminson”") - end - end -end From 240e98e2afc76180420ebc4be16cbf2c4a9999ba Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 22 Sep 2023 17:35:31 +0100 Subject: [PATCH 15/35] Use breadcrumbs on Change email or password page This "Edit your account" title might need a rethink, but it's consistent with the pre-existing page title and heading --- app/views/account/email_passwords/show.html.erb | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/app/views/account/email_passwords/show.html.erb b/app/views/account/email_passwords/show.html.erb index b7da9b171..94e38d4e9 100644 --- a/app/views/account/email_passwords/show.html.erb +++ b/app/views/account/email_passwords/show.html.erb @@ -1,5 +1,20 @@ <% content_for :title, "Edit your account" %> -<% content_for :back_link, root_path %> + +<% content_for :breadcrumbs, + render("govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "Dashboard", + url: root_path, + }, + { + title: "Edit your account", + } + ] + }) +%> + <% if current_user.unconfirmed_email.present? %> <%= render "govuk_publishing_components/components/notice", { From 63d3fe33600111f4455b8fa31f607ca4c0371fef Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 22 Sep 2023 17:43:42 +0100 Subject: [PATCH 16/35] Add Account page to sub-page's breadcrumbs Again, I've taken the breadcrumb name from the page's title/heading. I'm no breadcrumb theorist... we also link to some of these pages directly from the dashboard. It's OK for breadcrumbs to not reflect the actual route that the user's taken, right? --- app/views/account/email_passwords/show.html.erb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/views/account/email_passwords/show.html.erb b/app/views/account/email_passwords/show.html.erb index 94e38d4e9..e69a220a3 100644 --- a/app/views/account/email_passwords/show.html.erb +++ b/app/views/account/email_passwords/show.html.erb @@ -8,6 +8,10 @@ title: "Dashboard", url: root_path, }, + { + title: "Settings", + url: account_path, + }, { title: "Edit your account", } From d38344bdb0693fb9534badea1950bb99ad39e281 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 29 Sep 2023 09:59:15 +0100 Subject: [PATCH 17/35] Point redirects to Account page, not to Dashboard Instead of redirecting to the Dashboard, Account page-related actions now redirect to the Account page on success. The page that these actions are performed from can be accessed from either the Dashboard or the Account page, but: 1. (I think) we can't tell which route the user took to get here 2. the Account page is now their canonical home I can't say I really know where these actions would be best redirecting back to in terms of the user experience. How they fit with a typical user's expectations might depend on the specific action they're performing, too. --- .../account/email_passwords_controller.rb | 8 ++++---- .../email_passwords_controller_test.rb | 20 +++++++++++++++---- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/app/controllers/account/email_passwords_controller.rb b/app/controllers/account/email_passwords_controller.rb index c77c027c8..cd585c792 100644 --- a/app/controllers/account/email_passwords_controller.rb +++ b/app/controllers/account/email_passwords_controller.rb @@ -15,7 +15,7 @@ def update_email elsif current_user.update(email: new_email) EventLog.record_email_change(current_user, current_email, new_email) UserMailer.email_changed_notification(current_user).deliver_later - redirect_to root_path, notice: email_change_notice + redirect_to account_path, notice: email_change_notice else render :show end @@ -26,7 +26,7 @@ def update_password EventLog.record_event(current_user, EventLog::SUCCESSFUL_PASSWORD_CHANGE, ip_address: user_ip_address) flash[:notice] = t(:updated, scope: "devise.passwords") bypass_sign_in(current_user) - redirect_to root_path + redirect_to account_path else EventLog.record_event(current_user, EventLog::UNSUCCESSFUL_PASSWORD_CHANGE, ip_address: user_ip_address) render :show @@ -36,7 +36,7 @@ def update_password def resend_email_change current_user.resend_confirmation_instructions if current_user.errors.empty? - redirect_to root_path, notice: email_change_notice + redirect_to account_path, notice: email_change_notice else redirect_to account_email_password_path, alert: "Failed to send email change email" end @@ -45,7 +45,7 @@ def resend_email_change def cancel_email_change current_user.cancel_email_change! - redirect_to root_path, notice: "You have cancelled your pending email address change." + redirect_to account_path, notice: "You have cancelled your pending email address change." end private diff --git a/test/controllers/account/email_passwords_controller_test.rb b/test/controllers/account/email_passwords_controller_test.rb index c3ce79c09..8fd2c9453 100644 --- a/test/controllers/account/email_passwords_controller_test.rb +++ b/test/controllers/account/email_passwords_controller_test.rb @@ -39,6 +39,11 @@ class Account::EmailPasswordsControllerTest < ActionController::TestCase put :update_email, params: { user: { email: "new@email.com" } } assert_equal 1, EventLog.where(event_id: EventLog::EMAIL_CHANGE_INITIATED.id, uid: @user.uid, initiator_id: @user.id).count end + + should "redirect to account page" do + put :update_email, params: { user: { email: "new@email.com" } } + assert_redirected_to account_path + end end end @@ -64,8 +69,7 @@ def change_user_password(user_factory, new_password) should "changing passwords to something strong should succeed" do user, orig_password = change_user_password(:user, "destabilizers842}orthophosphate") - assert_equal "302", response.code - assert_equal root_url, response.location + assert_redirected_to account_path user.reload assert_not_equal orig_password, user.encrypted_password @@ -127,6 +131,14 @@ def change_user_password(user_factory, new_password) assert_not_equal "old token", @user.reload.confirmation_token end end + + should "redirect to account page" do + @user = create(:user_with_pending_email_change) + sign_in @user + + put :resend_email_change + assert_redirected_to account_path + end end context "DELETE cancel_email_change" do @@ -143,9 +155,9 @@ def change_user_password(user_factory, new_password) assert_nil @user.confirmation_token end - should "redirect to the change email password page" do + should "redirect to account page" do delete :cancel_email_change - assert_redirected_to account_email_password_path + assert_redirected_to account_path end end end From 76dea66c1f87f6ad2781bb71bc1c06c6d3272ac5 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 29 Sep 2023 09:23:35 +0100 Subject: [PATCH 18/35] Build a temporary Manage permissions page This is going to be linked to from the new Account page (for users who are allowed to changing their own permissions). There's an entire new permissions management area being developed as we speak, so it doesn't make sense to put a lot of additional effort into this version of it. As a working placeholder, I've copied the permissions management section of `Users#edit` and I've tried to keep the changes to a minimum. Some things just didn't make sense in the new (current_user's account) context. For example, I've tried to remove confusing references to the user that's having their permissions changed -- in this case that's just the current_user, changing their own permissions. --- .../account/manage_permissions_controller.rb | 38 ++++ .../account/manage_permissions_policy.rb | 6 + .../account/manage_permissions/show.html.erb | 27 +++ config/routes.rb | 1 + .../manage_permissions_controller_test.rb | 189 ++++++++++++++++++ .../account/manage_permissions_policy_test.rb | 22 ++ 6 files changed, 283 insertions(+) create mode 100644 app/controllers/account/manage_permissions_controller.rb create mode 100644 app/policies/account/manage_permissions_policy.rb create mode 100644 app/views/account/manage_permissions/show.html.erb create mode 100644 test/controllers/account/manage_permissions_controller_test.rb create mode 100644 test/policies/account/manage_permissions_policy_test.rb diff --git a/app/controllers/account/manage_permissions_controller.rb b/app/controllers/account/manage_permissions_controller.rb new file mode 100644 index 000000000..657e5560f --- /dev/null +++ b/app/controllers/account/manage_permissions_controller.rb @@ -0,0 +1,38 @@ +class Account::ManagePermissionsController < ApplicationController + include UserPermissionsControllerMethods + helper_method :applications_and_permissions + + before_action :authenticate_user! + before_action :authorise_user + + def show + @application_permissions = all_applications_and_permissions_for(current_user) + end + + def update + updater = UserUpdate.new(current_user, user_params, current_user, user_ip_address) + if updater.call + redirect_to root_path, notice: "Your permissions have been updated." + else + @application_permissions = all_applications_and_permissions_for(current_user) + render :show + end + end + +private + + def authorise_user + authorize %i[account manage_permissions] + end + + def user_params + UserParameterSanitiser.new( + user_params: permitted_user_params, + current_user_role: current_user.role.to_sym, + ).sanitise + end + + def permitted_user_params + params.fetch(:user, {}).permit(supported_permission_ids: []).to_h + end +end diff --git a/app/policies/account/manage_permissions_policy.rb b/app/policies/account/manage_permissions_policy.rb new file mode 100644 index 000000000..42df00c3e --- /dev/null +++ b/app/policies/account/manage_permissions_policy.rb @@ -0,0 +1,6 @@ +class Account::ManagePermissionsPolicy < BasePolicy + def show? + !current_user.normal? + end + alias_method :update?, :show? +end diff --git a/app/views/account/manage_permissions/show.html.erb b/app/views/account/manage_permissions/show.html.erb new file mode 100644 index 000000000..b8492b3b3 --- /dev/null +++ b/app/views/account/manage_permissions/show.html.erb @@ -0,0 +1,27 @@ +<% content_for :title, "Manage permissions" %> + +

    Manage permissions

    + +<%= form_for current_user, url: account_manage_permissions_path, html: {class: "well add-top-margin"} do |f| %> + <% if current_user.errors.count > 0 %> + + <% end %> + +

    <%= current_user.publishing_manager? ? "Editable permissions" : "Permissions" %>

    + <%= render partial: "shared/user_permissions", locals: { user_object: current_user }%> + + <% if current_user.publishing_manager? %> +

    All permissions

    + <%= render partial: "users/app_permissions", locals: { user_object: current_user }%> + <% end %> + + <%= f.submit "Update permissions", class: "btn btn-primary" %> +<% end %> diff --git a/config/routes.rb b/config/routes.rb index 7849191b0..2bc137900 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -60,6 +60,7 @@ put :resend_email_change delete :cancel_email_change end + resource :manage_permissions, only: %i[show update] end resources :batch_invitations, only: %i[new create show] do diff --git a/test/controllers/account/manage_permissions_controller_test.rb b/test/controllers/account/manage_permissions_controller_test.rb new file mode 100644 index 000000000..ee3b08a2b --- /dev/null +++ b/test/controllers/account/manage_permissions_controller_test.rb @@ -0,0 +1,189 @@ +require "test_helper" + +class Account::ManagePermissionsControllerTest < ActionController::TestCase + include ActiveJob::TestHelper + + context "GET show" do + context "as Admin" do + should "can give permissions to all applications" do + user = create(:admin_user, email: "admin@gov.uk") + sign_in user + + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + delegatable_no_access_to_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + non_delegatable_no_access_to_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + + user.grant_application_signin_permission(delegatable_app) + user.grant_application_signin_permission(non_delegatable_app) + + get :show + + assert_select ".container" do + # can give permissions to a delegatable app + assert_select "td", count: 1, text: delegatable_app.name + # can give permissions to a non delegatable app + assert_select "td", count: 1, text: non_delegatable_app.name + # can give permissions to a delegatable app the admin doesn't have access to + assert_select "td", count: 1, text: delegatable_no_access_to_app.name + # can give permissions to a non-delegatable app the admin doesn't have access to + assert_select "td", count: 1, text: non_delegatable_no_access_to_app.name + end + end + end + + context "organisation admin" do + should "be able to give permissions only to applications they have access to and that also have delegatable signin permissions" do + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + + organisation_admin = create(:organisation_admin_user, with_signin_permissions_for: [delegatable_app, non_delegatable_app]) + + sign_in organisation_admin + + get :show + + assert_select "#editable-permissions" do + # can give access to a delegatable app they have access to + assert_select "td", count: 1, text: delegatable_app.name + # can not give access to a non-delegatable app they have access to + assert_select "td", count: 0, text: non_delegatable_app.name + end + end + + should "be able to see all permissions including those they can't change" do + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME, "Editor"]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "GDS Admin"]) + + organisation_admin = create( + :organisation_admin_user, + with_permissions: { + delegatable_app => %w[Editor], + non_delegatable_app => [SupportedPermission::SIGNIN_NAME, "GDS Admin"], + }, + ) + + sign_in organisation_admin + + get :show + + assert_select "h2", "All permissions" + assert_select "#all-permissions" do + # can see permissions for a delegatable app that they have access to + assert_select "td", count: 1, text: delegatable_app.name + # can see permissions to a non-delegatable app that they have access to + assert_select "td", count: 1, text: non_delegatable_app.name + # can see role + assert_select "td", count: 1, text: "Editor" + assert_select "td", count: 1, text: "GDS Admin" + end + end + end + + context "super organisation admin" do + should "be able to give permissions only to applications they have access to and that also have delegatable signin permissions" do + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME]) + + super_org_admin = create(:super_organisation_admin_user, with_signin_permissions_for: [delegatable_app, non_delegatable_app]) + + sign_in super_org_admin + + get :show + + assert_select "#editable-permissions" do + # can give access to a delegatable app they have access to + assert_select "td", count: 1, text: delegatable_app.name + # can not give access to a non-delegatable app they have access to + assert_select "td", count: 0, text: non_delegatable_app.name + end + end + + should "be able to see all permissions including those they can't change" do + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME, "Editor"]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "GDS Admin"]) + + super_org_admin = create( + :super_organisation_admin_user, + with_permissions: { + delegatable_app => %w[Editor], + non_delegatable_app => [SupportedPermission::SIGNIN_NAME, "GDS Admin"], + }, + ) + + sign_in super_org_admin + + get :show + + assert_select "h2", "All permissions" + assert_select "#all-permissions" do + # can see permissions for a delegatable app that they have access to + assert_select "td", count: 1, text: delegatable_app.name + # can see permissions to a non-delegatable app that they have access to + assert_select "td", count: 1, text: non_delegatable_app.name + # can see role + assert_select "td", count: 1, text: "Editor" + assert_select "td", count: 1, text: "GDS Admin" + end + end + end + + context "superadmin" do + should "not be able to see all permissions to applications that they can't change" do + delegatable_app = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME, "Editor"]) + non_delegatable_app = create(:application, with_supported_permissions: [SupportedPermission::SIGNIN_NAME, "GDS Admin"]) + + superadmin = create(:superadmin_user, with_signin_permissions_for: [delegatable_app, non_delegatable_app]) + + sign_in superadmin + + get :show + + assert_select "h2", count: 0, text: "All permissions" + assert_select "#all-permissions", count: 0 + end + end + end + + context "PUT update" do + context "update application access" do + setup do + @user = create(:admin_user) + sign_in @user + @application = create(:application) + end + + should "remove all applications access" do + @user.grant_application_signin_permission(@application) + + put :update, params: { user: {} } + + assert_empty @user.reload.application_permissions + end + + should "add application access" do + put( + :update, + params: { + user: { + supported_permission_ids: [ + @application.supported_permissions.first.id, + ], + }, + }, + ) + + assert_equal 1, @user.reload.application_permissions.count + end + + should "redirect to Dashboard on success" do + @user.grant_application_signin_permission(@application) + + put :update, params: { user: {} } + + assert_redirected_to root_path + assert_match(/Your permissions have been updated./, flash[:notice]) + end + end + end +end diff --git a/test/policies/account/manage_permissions_policy_test.rb b/test/policies/account/manage_permissions_policy_test.rb new file mode 100644 index 000000000..426da5985 --- /dev/null +++ b/test/policies/account/manage_permissions_policy_test.rb @@ -0,0 +1,22 @@ +require "test_helper" +require "support/policy_helpers" + +class Account::ManagePermissionsPolicyTest < ActiveSupport::TestCase + include PolicyHelpers + + context "#show?" do + %i[superadmin admin super_organisation_admin organisation_admin].each do |user_role| + should "permit access for #{user_role} users" do + current_user = FactoryBot.build(:"#{user_role}_user") + + assert permit?(current_user, nil, :show) + end + end + + should "forbid access for normal users" do + current_user = FactoryBot.build(:user) + + assert forbid?(current_user, nil, :show) + end + end +end From 5b4f7f51d9a69f1b7a495cf96a1cd419d5fb368f Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 22 Sep 2023 16:57:23 +0100 Subject: [PATCH 19/35] Link to Manage permissions from Account page --- app/views/accounts/show.html.erb | 9 +++++++++ test/integration/account_test.rb | 18 +++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/app/views/accounts/show.html.erb b/app/views/accounts/show.html.erb index 70429922b..9a23ef011 100644 --- a/app/views/accounts/show.html.erb +++ b/app/views/accounts/show.html.erb @@ -24,5 +24,14 @@ }, description: "Reset your password or change your email address.", }, + ( + { + link: { + text: "Manage permissions", + path: account_manage_permissions_path, + }, + description: "Make changes to your permissions across different apps.", + } if policy(%i[account manage_permissions]).show? + ), ].compact } %> diff --git a/test/integration/account_test.rb b/test/integration/account_test.rb index 9df0bd8b2..388e389f6 100644 --- a/test/integration/account_test.rb +++ b/test/integration/account_test.rb @@ -8,7 +8,21 @@ class AccountTest < ActionDispatch::IntegrationTest assert_current_url new_user_session_path end - should "link to Change email/password" do + should "link to Change email/password and Manage permissions for admin users" do + user = FactoryBot.create(:admin_user) + + visit new_user_session_path + signin_with user + + visit account_path + assert_current_url account_path + assert page.has_selector?("h1", text: "Settings") + + assert page.has_link?("Change your email or password", href: account_email_password_path) + assert page.has_link?("Manage permissions", href: account_manage_permissions_path) + end + + should "link to Change email/password for normal users" do user = FactoryBot.create(:user) visit new_user_session_path @@ -18,6 +32,8 @@ class AccountTest < ActionDispatch::IntegrationTest assert page.has_selector?("h1", text: "Settings") assert page.has_link?("Change your email or password", href: account_email_password_path) + + assert_not page.has_link?("Manage permissions") end end end From cc93b37e9379bd6b43fa5a0e12eddaf34484777d Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 29 Sep 2023 10:05:22 +0100 Subject: [PATCH 20/35] Redirect from Manage permissions to Account page As with the other Account sub-page's endpoints, I'm not 100% sure on the ideal place to redirect to but the Account page now makes more sense than the Dashboard --- app/controllers/account/manage_permissions_controller.rb | 2 +- .../controllers/account/manage_permissions_controller_test.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/account/manage_permissions_controller.rb b/app/controllers/account/manage_permissions_controller.rb index 657e5560f..b24690636 100644 --- a/app/controllers/account/manage_permissions_controller.rb +++ b/app/controllers/account/manage_permissions_controller.rb @@ -12,7 +12,7 @@ def show def update updater = UserUpdate.new(current_user, user_params, current_user, user_ip_address) if updater.call - redirect_to root_path, notice: "Your permissions have been updated." + redirect_to account_path, notice: "Your permissions have been updated." else @application_permissions = all_applications_and_permissions_for(current_user) render :show diff --git a/test/controllers/account/manage_permissions_controller_test.rb b/test/controllers/account/manage_permissions_controller_test.rb index ee3b08a2b..bc32182f5 100644 --- a/test/controllers/account/manage_permissions_controller_test.rb +++ b/test/controllers/account/manage_permissions_controller_test.rb @@ -176,12 +176,12 @@ class Account::ManagePermissionsControllerTest < ActionController::TestCase assert_equal 1, @user.reload.application_permissions.count end - should "redirect to Dashboard on success" do + should "redirect to Account page on success" do @user.grant_application_signin_permission(@application) put :update, params: { user: {} } - assert_redirected_to root_path + assert_redirected_to account_path assert_match(/Your permissions have been updated./, flash[:notice]) end end From 134877f4d650af9ea960dc6a911d1f0edd299e05 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 22 Sep 2023 14:37:34 +0100 Subject: [PATCH 21/35] Un-nest Devise routes I'm planning on moving the parent :two_step_verification resource and want to make small, incremental changes in case we need to pinpoint the introduction of anything unexpected! Because the `path` and `controller` are explicitly set on this resource declaration, I think the only side effect this change could have caused is to the naming of the Rails-generated URL helper methods (e.g. `two_step_verification_session_path`). I've prevented that from happening by prefixing the resource name ("two_step_verification_" + "session"). --- config/routes.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 2bc137900..31f06b487 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -24,12 +24,14 @@ devise_scope :user do post "/users/invitation/resend/:id" => "invitations#resend", :as => "resend_user_invitation" put "/users/confirmation" => "confirmations#update" + resource :two_step_verification_session, + only: %i[new create], + path: "/users/two_step_verification_session", + controller: "devise/two_step_verification_session" resource :two_step_verification, only: %i[show update], path: "/users/two_step_verification", controller: "devise/two_step_verification" do - resource :session, only: %i[new create], controller: "devise/two_step_verification_session" - member { get :prompt } end end From 67c279f3073af0b4a72bc447cc275c8493dbbcc2 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 22 Sep 2023 14:42:01 +0100 Subject: [PATCH 22/35] Move 2SV setup page to /account I'm planning to make this page another sub-page of the new Account page. For consistency with the Account page and the other Account sub-pages, I've changed this page's URL to live under /account instead of /users. Since this page is already only ever used by the current_user to configure their own account and since the controller already lives in the fairly neutral `Devise` namespace, it's only the URL that needs changing to make this an "account" page instead of a "user" one. And since the affected part of the URL isn't factored into its Rails-generated URL helper method names, no redirects or links need updating to complete this change, either. --- config/routes.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/routes.rb b/config/routes.rb index 31f06b487..aed7e6865 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -30,7 +30,7 @@ controller: "devise/two_step_verification_session" resource :two_step_verification, only: %i[show update], - path: "/users/two_step_verification", + path: "/account/two_step_verification", controller: "devise/two_step_verification" do member { get :prompt } end From 79c112aace35ca33e5c03db8487a70690b305761 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Thu, 28 Sep 2023 15:37:51 +0100 Subject: [PATCH 23/35] Extract conditional page title/name to helper I'm planning to make the 2SV setup page an Account sub-page. Without any better ideas, I'd like the wording of the link from the Account page to be consistent with the page's own existing title and heading. The wording of the link on the Dashboard and the (title and heading of the) 2SV page weren't exactly identical before this change, but the difference seemed arbitrary enough not to worry about. --- app/helpers/account_helper.rb | 9 +++++++++ .../two_step_verification/show.html.erb | 2 +- app/views/root/index.html.erb | 6 +----- test/helpers/account_helper_test.rb | 19 +++++++++++++++++++ 4 files changed, 30 insertions(+), 6 deletions(-) create mode 100644 app/helpers/account_helper.rb create mode 100644 test/helpers/account_helper_test.rb diff --git a/app/helpers/account_helper.rb b/app/helpers/account_helper.rb new file mode 100644 index 000000000..5d53536dd --- /dev/null +++ b/app/helpers/account_helper.rb @@ -0,0 +1,9 @@ +module AccountHelper + def two_step_verification_page_title + if current_user.has_2sv? + "Change your 2-step verification phone" + else + "Set up 2-step verification" + end + end +end diff --git a/app/views/devise/two_step_verification/show.html.erb b/app/views/devise/two_step_verification/show.html.erb index 5a14d9bf9..a6affbb3c 100644 --- a/app/views/devise/two_step_verification/show.html.erb +++ b/app/views/devise/two_step_verification/show.html.erb @@ -1,4 +1,4 @@ -<% content_for :title, current_user.has_2sv? ? "Use a new phone" : "Set up 2-step verification" %> +<% content_for :title, two_step_verification_page_title %> <% content_for :back_link, root_path %> <% if flash[:invalid_code].present? %> diff --git a/app/views/root/index.html.erb b/app/views/root/index.html.erb index 28497998a..5e4aa285f 100644 --- a/app/views/root/index.html.erb +++ b/app/views/root/index.html.erb @@ -66,11 +66,7 @@ <%= link_to "Change your email or password", account_email_password_path, class: "govuk-link" %> - <% if current_user.has_2sv? %> -
  • <%= link_to "Change your 2-step verification phone", two_step_verification_path, class: "govuk-link" %>
    • - <% else %> -
  • <%= link_to "Set up 2-step verification", two_step_verification_path, class: "govuk-link" %>
    • - <% end %> +
  • <%= link_to two_step_verification_page_title, two_step_verification_path, class: "govuk-link" %>
diff --git a/test/helpers/account_helper_test.rb b/test/helpers/account_helper_test.rb new file mode 100644 index 000000000..4daf171fc --- /dev/null +++ b/test/helpers/account_helper_test.rb @@ -0,0 +1,19 @@ +require "test_helper" + +class AccountHelperTest < ActionView::TestCase + context "#two_step_verification_page_title" do + should "say 'change' if current_user already has 2SV setup" do + user = build(:two_step_enabled_user) + stubs(:current_user).returns(user) + + assert_equal "Change your 2-step verification phone", two_step_verification_page_title + end + + should "say 'set up' if current_user doesn't have 2SV setup already" do + user = build(:user) + stubs(:current_user).returns(user) + + assert_equal "Set up 2-step verification", two_step_verification_page_title + end + end +end From 6c662c3bf859a77badb8621fa086e1ad1f7388b5 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 22 Sep 2023 16:57:23 +0100 Subject: [PATCH 24/35] Link to 2SV setup page from Account page Give the link a different name depending on whether the user has 2SV setup already or not --- app/views/accounts/show.html.erb | 6 ++++++ test/integration/account_test.rb | 22 ++++++++++++++++++---- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/app/views/accounts/show.html.erb b/app/views/accounts/show.html.erb index 9a23ef011..72ec64604 100644 --- a/app/views/accounts/show.html.erb +++ b/app/views/accounts/show.html.erb @@ -33,5 +33,11 @@ description: "Make changes to your permissions across different apps.", } if policy(%i[account manage_permissions]).show? ), + { + link: { + text: two_step_verification_page_title, + path: two_step_verification_path, + }, + }, ].compact } %> diff --git a/test/integration/account_test.rb b/test/integration/account_test.rb index 388e389f6..12a6c1438 100644 --- a/test/integration/account_test.rb +++ b/test/integration/account_test.rb @@ -8,8 +8,8 @@ class AccountTest < ActionDispatch::IntegrationTest assert_current_url new_user_session_path end - should "link to Change email/password and Manage permissions for admin users" do - user = FactoryBot.create(:admin_user) + should "link to Change email/password, Manage permissions and Change 2SV for admin users" do + user = FactoryBot.create(:admin_user, otp_secret_key: "2SVenabled") visit new_user_session_path signin_with user @@ -20,10 +20,11 @@ class AccountTest < ActionDispatch::IntegrationTest assert page.has_link?("Change your email or password", href: account_email_password_path) assert page.has_link?("Manage permissions", href: account_manage_permissions_path) + assert page.has_link?("Change your 2-step verification phone", href: two_step_verification_path) end - should "link to Change email/password for normal users" do - user = FactoryBot.create(:user) + should "link to Change email/password and Change 2SV for normal users" do + user = FactoryBot.create(:user, otp_secret_key: "2SVenabled") visit new_user_session_path signin_with user @@ -32,8 +33,21 @@ class AccountTest < ActionDispatch::IntegrationTest assert page.has_selector?("h1", text: "Settings") assert page.has_link?("Change your email or password", href: account_email_password_path) + assert page.has_link?("Change your 2-step verification phone", href: two_step_verification_path) assert_not page.has_link?("Manage permissions") end + + should "link to 2SV setup page for users who don't already have it" do + user = FactoryBot.create(:user) + + visit new_user_session_path + signin_with user + + visit account_path + assert page.has_selector?("h1", text: "Settings") + + assert page.has_link?("Set up 2-step verification", href: two_step_verification_path) + end end end From f57bbdd347182a034e2d8dde80b6f460a5ef05a0 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 22 Sep 2023 17:33:38 +0100 Subject: [PATCH 25/35] Use breadcrumbs on 2SV setup page --- .../devise/two_step_verification/show.html.erb | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/views/devise/two_step_verification/show.html.erb b/app/views/devise/two_step_verification/show.html.erb index a6affbb3c..be838ecc9 100644 --- a/app/views/devise/two_step_verification/show.html.erb +++ b/app/views/devise/two_step_verification/show.html.erb @@ -1,5 +1,19 @@ <% content_for :title, two_step_verification_page_title %> -<% content_for :back_link, root_path %> + +<% content_for :breadcrumbs, + render("govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "Dashboard", + url: root_path, + }, + { + title: two_step_verification_page_title, + } + ] + }) +%> <% if flash[:invalid_code].present? %> <% content_for :error_summary do %> From acc8ec6b2929518788f0f5e92251115a9d2ec50a Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Fri, 22 Sep 2023 17:43:42 +0100 Subject: [PATCH 26/35] Add Account page to 2SV page's breadcrumbs --- app/views/devise/two_step_verification/show.html.erb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/views/devise/two_step_verification/show.html.erb b/app/views/devise/two_step_verification/show.html.erb index be838ecc9..3ef8cb3a7 100644 --- a/app/views/devise/two_step_verification/show.html.erb +++ b/app/views/devise/two_step_verification/show.html.erb @@ -8,6 +8,10 @@ title: "Dashboard", url: root_path, }, + { + title: "Settings", + url: account_path, + }, { title: two_step_verification_page_title, } From e1d4c39f5bebf783754dc9ef56906613ab3a8d1e Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Tue, 26 Sep 2023 10:03:00 +0100 Subject: [PATCH 27/35] Introduce parameter to method --- app/controllers/application_controller.rb | 4 ++-- app/controllers/devise/two_step_verification_controller.rb | 2 +- .../devise/two_step_verification_session_controller.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 8bc7560f0..268de6845 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -66,7 +66,7 @@ def notify_bad_request(_exception) render plain: "Error: One or more recipients not in GOV.UK Notify team (code: 400)", status: :bad_request end - def redirect_to_prior_flow(args = {}) - redirect_to stored_location_for("2sv") || :root, args + def redirect_to_prior_flow_or_to(fallback_location, args = {}) + redirect_to stored_location_for("2sv") || fallback_location, args end end diff --git a/app/controllers/devise/two_step_verification_controller.rb b/app/controllers/devise/two_step_verification_controller.rb index 68f9f6b25..21e815ac7 100644 --- a/app/controllers/devise/two_step_verification_controller.rb +++ b/app/controllers/devise/two_step_verification_controller.rb @@ -16,7 +16,7 @@ def update if verify_code_and_update EventLog.record_event(current_user, success_event_for(mode), ip_address: user_ip_address) send_notification(current_user, mode) - redirect_to_prior_flow notice: I18n.t("devise.two_step_verification.messages.success.#{mode}") + redirect_to_prior_flow_or_to root_path, notice: I18n.t("devise.two_step_verification.messages.success.#{mode}") else EventLog.record_event(current_user, failure_event_for(mode), ip_address: user_ip_address) flash.now[:invalid_code] = "Sorry that code didn’t work. Please try again." diff --git a/app/controllers/devise/two_step_verification_session_controller.rb b/app/controllers/devise/two_step_verification_session_controller.rb index d9340f5d0..cb8ba99cf 100644 --- a/app/controllers/devise/two_step_verification_session_controller.rb +++ b/app/controllers/devise/two_step_verification_session_controller.rb @@ -26,7 +26,7 @@ def create warden.session(:user)["need_two_step_verification"] = false bypass_sign_in current_user set_flash_message :notice, :success - redirect_to_prior_flow + redirect_to_prior_flow_or_to root_path current_user.update!(second_factor_attempts_count: 0) else flash.now[:alert] = find_message(:attempt_failed) From 56b56ccbaaccbb7ad42f9ac489ce3d85fa95f240 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Mon, 25 Sep 2023 11:58:47 +0100 Subject: [PATCH 28/35] Redirect from 2SV setup page to Account page Consistent with the other Account sub-pages, instead of redirecting to the Dashboard, redirect to the Account page on success. --- .../devise/two_step_verification_controller.rb | 2 +- test/integration/two_step_verification_test.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/controllers/devise/two_step_verification_controller.rb b/app/controllers/devise/two_step_verification_controller.rb index 21e815ac7..839d98a35 100644 --- a/app/controllers/devise/two_step_verification_controller.rb +++ b/app/controllers/devise/two_step_verification_controller.rb @@ -16,7 +16,7 @@ def update if verify_code_and_update EventLog.record_event(current_user, success_event_for(mode), ip_address: user_ip_address) send_notification(current_user, mode) - redirect_to_prior_flow_or_to root_path, notice: I18n.t("devise.two_step_verification.messages.success.#{mode}") + redirect_to_prior_flow_or_to account_path, notice: I18n.t("devise.two_step_verification.messages.success.#{mode}") else EventLog.record_event(current_user, failure_event_for(mode), ip_address: user_ip_address) flash.now[:invalid_code] = "Sorry that code didn’t work. Please try again." diff --git a/test/integration/two_step_verification_test.rb b/test/integration/two_step_verification_test.rb index 10d723d82..e0a8fc420 100644 --- a/test/integration/two_step_verification_test.rb +++ b/test/integration/two_step_verification_test.rb @@ -46,6 +46,14 @@ class TwoStepVerificationTest < ActionDispatch::IntegrationTest end end + should "redirect to account page on success" do + enter_2sv_code(@new_secret) + click_button "Finish replacing your phone" + + assert_current_url account_path + assert_response_contains "2-step verification phone changed successfully" + end + should "require the code again on next login" do enter_2sv_code(@new_secret) click_button "Finish replacing your phone" From 0bf649683d459c08dbf4854fb430594aeb550a76 Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Tue, 26 Sep 2023 14:03:30 +0100 Subject: [PATCH 29/35] Introduce Role and organisation Account sub-page This provides some functionality that was lost in the switch from Users#edit to the new Account page. For Admin and Superadmin users it allows making changes, but for users with other roles it's read-only. Since this is another Account sub-page and another mixed bag of a page, for consistency I've mostly followed the approaches taken by the "Change your email or password" page. --- I've let one of the new helper methods be tested through the integration test only, because I didn't manage to figure out the correct test setup required for a helper method that's calling a Pundit method. --- .../account/role_organisations_controller.rb | 37 +++++++++ app/helpers/account_helper.rb | 13 ++++ .../account/role_organisations_policy.rb | 10 +++ .../account/role_organisations/show.html.erb | 76 +++++++++++++++++++ app/views/accounts/show.html.erb | 6 ++ config/routes.rb | 4 + test/helpers/account_helper_test.rb | 17 +++++ .../account_role_organisations_test.rb | 66 ++++++++++++++++ test/integration/account_test.rb | 6 +- .../account/role_organisations_policy_test.rb | 52 +++++++++++++ 10 files changed, 285 insertions(+), 2 deletions(-) create mode 100644 app/controllers/account/role_organisations_controller.rb create mode 100644 app/policies/account/role_organisations_policy.rb create mode 100644 app/views/account/role_organisations/show.html.erb create mode 100644 test/integration/account_role_organisations_test.rb create mode 100644 test/policies/account/role_organisations_policy_test.rb diff --git a/app/controllers/account/role_organisations_controller.rb b/app/controllers/account/role_organisations_controller.rb new file mode 100644 index 000000000..e186f3f8f --- /dev/null +++ b/app/controllers/account/role_organisations_controller.rb @@ -0,0 +1,37 @@ +class Account::RoleOrganisationsController < ApplicationController + layout "admin_layout" + + before_action :authenticate_user! + before_action :authorise_user + + def show; end + + def update_organisation + organisation_id = params[:user][:organisation_id] + organisation = Organisation.find(organisation_id) + + if UserUpdate.new(current_user, { organisation_id: }, current_user, user_ip_address).call + redirect_to account_path, notice: "Your organisation is now #{organisation.name}" + else + flash[:alert] = "There was a problem changing your organisation." + render :show + end + end + + def update_role + role = params[:user][:role] + + if UserUpdate.new(current_user, { role: }, current_user, user_ip_address).call + redirect_to account_path, notice: "Your role is now #{role.humanize}" + else + flash[:alert] = "There was a problem changing your role." + render :show + end + end + +private + + def authorise_user + authorize %i[account role_organisations] + end +end diff --git a/app/helpers/account_helper.rb b/app/helpers/account_helper.rb index 5d53536dd..391921cb3 100644 --- a/app/helpers/account_helper.rb +++ b/app/helpers/account_helper.rb @@ -6,4 +6,17 @@ def two_step_verification_page_title "Set up 2-step verification" end end + + def role_organisation_page_title + if policy(%i[account role_organisations]).update_role? && + policy(%i[account role_organisations]).update_organisation? + "Change your role or organisation" + else + "View your role and organisation" + end + end + + def current_user_organisation_name + current_user.organisation&.name_with_abbreviation || "No organisation" + end end diff --git a/app/policies/account/role_organisations_policy.rb b/app/policies/account/role_organisations_policy.rb new file mode 100644 index 000000000..5d0edf1af --- /dev/null +++ b/app/policies/account/role_organisations_policy.rb @@ -0,0 +1,10 @@ +class Account::RoleOrganisationsPolicy < BasePolicy + def show? + current_user.present? + end + + def update_organisation? + current_user.govuk_admin? + end + alias_method :update_role?, :update_organisation? +end diff --git a/app/views/account/role_organisations/show.html.erb b/app/views/account/role_organisations/show.html.erb new file mode 100644 index 000000000..1edede628 --- /dev/null +++ b/app/views/account/role_organisations/show.html.erb @@ -0,0 +1,76 @@ +<% content_for :title, "Role and organisation" %> + +<% content_for :breadcrumbs, + render("govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "Dashboard", + url: root_path, + }, + { + title: "Settings", + url: account_path, + }, + { + title: "Role and organisation", + } + ] + }) +%> + +
+
+
+ <%= render "govuk_publishing_components/components/heading", { + text: "Your role", + padding: true, + } %> + + <% if policy(%i[account role_organisations]).update_role? %> + <%= form_for current_user, url: update_role_account_role_organisation_path do |f| %> + <%= render "govuk_publishing_components/components/select", { + id: "user_role", + name: "user[role]", + label: "Role", + options: current_user.manageable_roles.map { |role| { text: role.humanize, value: role, selected: current_user.role == role } } + } %> + <%= render "govuk_publishing_components/components/button", { + text: "Change role" + } %> + <% end %> + <% else %> + <%= render "govuk_publishing_components/components/inset_text", { + text: current_user.role.humanize, + } %> + <% end %> +
+ +
+ +
+ <%= render "govuk_publishing_components/components/heading", { + text: "Your organisation", + padding: true, + } %> + + <% if policy(%i[account role_organisations]).update_organisation? %> + <%= form_for current_user, url: update_organisation_account_role_organisation_path do |f| %> + <%= render "govuk_publishing_components/components/select", { + id: "user_organisation_id", + name: "user[organisation_id]", + label: "Organisation", + options: policy_scope(Organisation).map { |organisation| { text: organisation.name_with_abbreviation, value: organisation.id, selected: current_user.organisation == organisation } } + } %> + <%= render "govuk_publishing_components/components/button", { + text: "Change organisation" + } %> + <% end %> + <% else %> + <%= render "govuk_publishing_components/components/inset_text", { + text: current_user_organisation_name, + } %> + <% end %> +
+
+
diff --git a/app/views/accounts/show.html.erb b/app/views/accounts/show.html.erb index 72ec64604..60d1164c0 100644 --- a/app/views/accounts/show.html.erb +++ b/app/views/accounts/show.html.erb @@ -39,5 +39,11 @@ path: two_step_verification_path, }, }, + { + link: { + text: role_organisation_page_title, + path: account_role_organisation_path, + }, + }, ].compact } %> diff --git a/config/routes.rb b/config/routes.rb index aed7e6865..b3edc18e6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -63,6 +63,10 @@ delete :cancel_email_change end resource :manage_permissions, only: %i[show update] + resource :role_organisation, only: [:show] do + patch :update_organisation + patch :update_role + end end resources :batch_invitations, only: %i[new create show] do diff --git a/test/helpers/account_helper_test.rb b/test/helpers/account_helper_test.rb index 4daf171fc..06293cb78 100644 --- a/test/helpers/account_helper_test.rb +++ b/test/helpers/account_helper_test.rb @@ -16,4 +16,21 @@ class AccountHelperTest < ActionView::TestCase assert_equal "Set up 2-step verification", two_step_verification_page_title end end + + context "#current_user_organisation_name" do + should "return current_user's organisation name with abbreviation" do + organisation = create(:organisation, name: "Handling Ministry", abbreviation: "HM") + current_user = build(:user, organisation:) + stubs(:current_user).returns(current_user) + + assert_equal "Handling Ministry – HM", current_user_organisation_name + end + + should "return helpful message if user has no organisation" do + current_user = build(:user) + stubs(:current_user).returns(current_user) + + assert_equal "No organisation", current_user_organisation_name + end + end end diff --git a/test/integration/account_role_organisations_test.rb b/test/integration/account_role_organisations_test.rb new file mode 100644 index 000000000..d9a8c830c --- /dev/null +++ b/test/integration/account_role_organisations_test.rb @@ -0,0 +1,66 @@ +require "test_helper" + +class AccountRoleOrganisationsTest < ActionDispatch::IntegrationTest + context "#show" do + should "display read-only values for users who aren't GOVUK Admins" do + organisation = create(:organisation, name: "Department for Viability") + non_govuk_admin_user = create(:super_organisation_admin_user, organisation:) + + visit new_user_session_path + signin_with non_govuk_admin_user + + visit account_role_organisation_path + + within "section", text: "Your role" do + assert has_text? "Super organisation admin" + end + + within "section", text: "Your organisation" do + assert has_text? "Department for Viability" + end + end + + should "allow Superadmin users to change their role" do + user = FactoryBot.create(:superadmin_user) + + visit new_user_session_path + signin_with user + + visit account_role_organisation_path + + select "Normal", from: "Role" + click_button "Change role" + + assert_current_url account_path + assert page.has_text? "Your role is now Normal" + + visit account_role_organisation_path + + within "section", text: "Your role" do + assert has_text? "Normal" + end + end + + should "allow GOVUK Admin users to change their organisation" do + current_organisation = create(:organisation, name: "Judiciary") + user = FactoryBot.create(:admin_user, organisation: current_organisation) + + create(:organisation, name: "Postage") + + visit new_user_session_path + signin_with user + + visit account_role_organisation_path + + select "Postage", from: "Organisation" + click_button "Change organisation" + + assert_current_url account_path + assert page.has_text? "Your organisation is now Postage" + + visit account_role_organisation_path + + assert page.has_select? "Organisation", selected: "Postage" + end + end +end diff --git a/test/integration/account_test.rb b/test/integration/account_test.rb index 12a6c1438..f1713655d 100644 --- a/test/integration/account_test.rb +++ b/test/integration/account_test.rb @@ -8,7 +8,7 @@ class AccountTest < ActionDispatch::IntegrationTest assert_current_url new_user_session_path end - should "link to Change email/password, Manage permissions and Change 2SV for admin users" do + should "link to Change email/password, Manage permissions, Change 2SV and Role/org for admin users" do user = FactoryBot.create(:admin_user, otp_secret_key: "2SVenabled") visit new_user_session_path @@ -21,9 +21,10 @@ class AccountTest < ActionDispatch::IntegrationTest assert page.has_link?("Change your email or password", href: account_email_password_path) assert page.has_link?("Manage permissions", href: account_manage_permissions_path) assert page.has_link?("Change your 2-step verification phone", href: two_step_verification_path) + assert page.has_link?("Change your role or organisation", href: account_role_organisation_path) end - should "link to Change email/password and Change 2SV for normal users" do + should "link to Change email/password, Change 2SV and Role/org for normal users" do user = FactoryBot.create(:user, otp_secret_key: "2SVenabled") visit new_user_session_path @@ -34,6 +35,7 @@ class AccountTest < ActionDispatch::IntegrationTest assert page.has_link?("Change your email or password", href: account_email_password_path) assert page.has_link?("Change your 2-step verification phone", href: two_step_verification_path) + assert page.has_link?("View your role and organisation", href: account_role_organisation_path) assert_not page.has_link?("Manage permissions") end diff --git a/test/policies/account/role_organisations_policy_test.rb b/test/policies/account/role_organisations_policy_test.rb new file mode 100644 index 000000000..0f4e0539d --- /dev/null +++ b/test/policies/account/role_organisations_policy_test.rb @@ -0,0 +1,52 @@ +require "test_helper" +require "support/policy_helpers" + +class Account::RoleOrganisationsPolicyTest < ActiveSupport::TestCase + include PolicyHelpers + + context "show?" do + should "allow logged in users to see show irrespective of their role" do + assert permit?(build(:user), nil, :show) + end + + should "not allow anonymous visitors to see show" do + assert forbid?(nil, nil, :show) + end + end + + context "update_organisation?" do + %i[superadmin admin].each do |user_role| + should "be permitted for #{user_role} users" do + user = FactoryBot.build(:"#{user_role}_user") + + assert permit?(user, nil, :update_organisation) + end + end + + %i[super_organisation_admin organisation_admin normal].each do |user_role| + should "be forbidden for #{user_role} users" do + user = FactoryBot.build(:"#{user_role}_user") + + assert forbid?(user, nil, :update_organisation) + end + end + end + + context "update_role?" do + %i[superadmin admin].each do |user_role| + should "be permitted for #{user_role} users" do + user = FactoryBot.build(:"#{user_role}_user") + + assert permit?(user, nil, :update_role) + end + end + + %i[super_organisation_admin organisation_admin normal].each do |user_role| + should "be forbidden for #{user_role} users" do + user = FactoryBot.build(:"#{user_role}_user") + + assert forbid?(user, nil, :update_role) + end + end + end +end From 549dd670bfe11c8612b033e88d3477fda1078fee Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Thu, 28 Sep 2023 12:52:03 +0100 Subject: [PATCH 30/35] Extract event logs table partial --- app/views/shared/_event_logs_table.html.erb | 20 ++++++++++++++++++++ app/views/users/event_logs.html.erb | 21 +-------------------- 2 files changed, 21 insertions(+), 20 deletions(-) create mode 100644 app/views/shared/_event_logs_table.html.erb diff --git a/app/views/shared/_event_logs_table.html.erb b/app/views/shared/_event_logs_table.html.erb new file mode 100644 index 000000000..dabc6dbb5 --- /dev/null +++ b/app/views/shared/_event_logs_table.html.erb @@ -0,0 +1,20 @@ +<% if logs.any? %> + <%= render "govuk_publishing_components/components/table", { + caption: pluralize(number_with_delimiter(logs.total_count), "event"), + caption_classes: "govuk-heading-m", + head: [ + { text: "Time" }, + { text: "Event" }, + ], + rows: logs.map do |log| + [ { text: formatted_date(log), format: "event-log-date" }, + { text: formatted_message(log) } ] + end + } %> + + <%= paginate(logs, theme: "gds") %> +<% else %> + <%= render "govuk_publishing_components/components/notice", { + title: "No activity logged" + } %> +<% end %> diff --git a/app/views/users/event_logs.html.erb b/app/views/users/event_logs.html.erb index 1dae3e8f6..b8308aee4 100644 --- a/app/views/users/event_logs.html.erb +++ b/app/views/users/event_logs.html.erb @@ -22,23 +22,4 @@ }) %> -<% if @logs.any? %> - <%= render "govuk_publishing_components/components/table", { - caption: pluralize(number_with_delimiter(@logs.total_count), 'event'), - caption_classes: "govuk-heading-m", - head: [ - { text: "Time" }, - { text: "Event" }, - ], - rows: @logs.map do |log| - [ { text: formatted_date(log), format: 'event-log-date' }, - { text: formatted_message(log) } ] - end - } %> - - <%= paginate(@logs, theme: 'gds') %> -<% else %> - <%= render "govuk_publishing_components/components/notice", { - title: "No activity logged" - } %> -<% end %> +<%= render "shared/event_logs_table", logs: @logs %> From 5c35d13c3e720f60af4c89ea01ce876fa824388d Mon Sep 17 00:00:00 2001 From: Mike Patrick Date: Thu, 28 Sep 2023 13:19:02 +0100 Subject: [PATCH 31/35] Introduce Your account access log Account sub-page This provides another bit of functionality that was lost in the switch from Users#edit to the new Account page. It's pretty much identical to the Users#event_logs page except for living under /account --- .../account/activities_controller.rb | 16 ++++++++++++++ app/policies/account/activities_policy.rb | 5 +++++ app/views/account/activities/show.html.erb | 22 +++++++++++++++++++ app/views/accounts/show.html.erb | 7 ++++++ config/routes.rb | 1 + test/integration/account_activities_test.rb | 16 ++++++++++++++ test/integration/account_test.rb | 2 ++ .../account/activities_policy_test.rb | 14 ++++++++++++ 8 files changed, 83 insertions(+) create mode 100644 app/controllers/account/activities_controller.rb create mode 100644 app/policies/account/activities_policy.rb create mode 100644 app/views/account/activities/show.html.erb create mode 100644 test/integration/account_activities_test.rb create mode 100644 test/policies/account/activities_policy_test.rb diff --git a/app/controllers/account/activities_controller.rb b/app/controllers/account/activities_controller.rb new file mode 100644 index 000000000..828d3b87b --- /dev/null +++ b/app/controllers/account/activities_controller.rb @@ -0,0 +1,16 @@ +class Account::ActivitiesController < ApplicationController + layout "admin_layout" + + before_action :authenticate_user! + before_action :authorise_user + + def show + @logs = current_user.event_logs.page(params[:page]).per(100) + end + +private + + def authorise_user + authorize %i[account activities] + end +end diff --git a/app/policies/account/activities_policy.rb b/app/policies/account/activities_policy.rb new file mode 100644 index 000000000..dc92f2d46 --- /dev/null +++ b/app/policies/account/activities_policy.rb @@ -0,0 +1,5 @@ +class Account::ActivitiesPolicy < BasePolicy + def show? + current_user.present? + end +end diff --git a/app/views/account/activities/show.html.erb b/app/views/account/activities/show.html.erb new file mode 100644 index 000000000..3e7e48484 --- /dev/null +++ b/app/views/account/activities/show.html.erb @@ -0,0 +1,22 @@ +<% content_for :title, "Account access log" %> + +<% content_for :breadcrumbs, + render("govuk_publishing_components/components/breadcrumbs", { + collapse_on_mobile: true, + breadcrumbs: [ + { + title: "Dashboard", + url: root_path, + }, + { + title: "Settings", + url: account_path, + }, + { + title: "Account access log", + }, + ] + }) +%> + +<%= render "shared/event_logs_table", logs: @logs %> diff --git a/app/views/accounts/show.html.erb b/app/views/accounts/show.html.erb index 60d1164c0..6fb12ae1b 100644 --- a/app/views/accounts/show.html.erb +++ b/app/views/accounts/show.html.erb @@ -45,5 +45,12 @@ path: account_role_organisation_path, }, }, + { + link: { + text: "Your account access log", + path: account_activity_path, + }, + description: "View your account activity.", + }, ].compact } %> diff --git a/config/routes.rb b/config/routes.rb index b3edc18e6..d7d64b38c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -50,6 +50,7 @@ resource :account, only: [:show] namespace :account do + resource :activity, only: [:show] resources :applications, only: %i[show index] do resources :permissions, only: [:index] resource :signin_permission, only: %i[create destroy] do diff --git a/test/integration/account_activities_test.rb b/test/integration/account_activities_test.rb new file mode 100644 index 000000000..893634e48 --- /dev/null +++ b/test/integration/account_activities_test.rb @@ -0,0 +1,16 @@ +require "test_helper" + +class AccountActivitiesTest < ActionDispatch::IntegrationTest + context "#show" do + should "list user's EventLogs in table" do + user = create(:user) + + visit new_user_session_path + signin_with user + + visit account_activity_path + + assert page.has_selector? "td", text: "Successful login" + end + end +end diff --git a/test/integration/account_test.rb b/test/integration/account_test.rb index f1713655d..efcced559 100644 --- a/test/integration/account_test.rb +++ b/test/integration/account_test.rb @@ -22,6 +22,7 @@ class AccountTest < ActionDispatch::IntegrationTest assert page.has_link?("Manage permissions", href: account_manage_permissions_path) assert page.has_link?("Change your 2-step verification phone", href: two_step_verification_path) assert page.has_link?("Change your role or organisation", href: account_role_organisation_path) + assert page.has_link?("Your account access log", href: account_activity_path) end should "link to Change email/password, Change 2SV and Role/org for normal users" do @@ -36,6 +37,7 @@ class AccountTest < ActionDispatch::IntegrationTest assert page.has_link?("Change your email or password", href: account_email_password_path) assert page.has_link?("Change your 2-step verification phone", href: two_step_verification_path) assert page.has_link?("View your role and organisation", href: account_role_organisation_path) + assert page.has_link?("Your account access log", href: account_activity_path) assert_not page.has_link?("Manage permissions") end diff --git a/test/policies/account/activities_policy_test.rb b/test/policies/account/activities_policy_test.rb new file mode 100644 index 000000000..4e31c9ad0 --- /dev/null +++ b/test/policies/account/activities_policy_test.rb @@ -0,0 +1,14 @@ +require "test_helper" +require "support/policy_helpers" + +class Account::ActivitiesPolicyTest < ActiveSupport::TestCase + include PolicyHelpers + + should "allow logged in users to see show irrespective of their role" do + assert permit?(build(:user), nil, :show) + end + + should "not allow anonymous visitors to see show" do + assert forbid?(nil, nil, :show) + end +end From 8b1c69952652a28d4dae23269a94a879db9d0878 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 3 Oct 2023 11:15:07 +0100 Subject: [PATCH 32/35] Only Super Admin users can update their role This matches the behaviour of the users edit page, where only Super Admin users can update their and other users' roles (implemented in `UserPolicy#assign_role?`[1]). [1]: https://github.com/alphagov/signon/blob/1faf579e720605006b693ad6ca56d6aa29fe2852/app/policies/user_policy.rb#L56 --- app/policies/account/role_organisations_policy.rb | 5 ++++- test/integration/account_test.rb | 2 +- test/policies/account/role_organisations_policy_test.rb | 4 ++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/app/policies/account/role_organisations_policy.rb b/app/policies/account/role_organisations_policy.rb index 5d0edf1af..8d51cb6ac 100644 --- a/app/policies/account/role_organisations_policy.rb +++ b/app/policies/account/role_organisations_policy.rb @@ -6,5 +6,8 @@ def show? def update_organisation? current_user.govuk_admin? end - alias_method :update_role?, :update_organisation? + + def update_role? + current_user.superadmin? + end end diff --git a/test/integration/account_test.rb b/test/integration/account_test.rb index efcced559..12ed2e15e 100644 --- a/test/integration/account_test.rb +++ b/test/integration/account_test.rb @@ -21,7 +21,7 @@ class AccountTest < ActionDispatch::IntegrationTest assert page.has_link?("Change your email or password", href: account_email_password_path) assert page.has_link?("Manage permissions", href: account_manage_permissions_path) assert page.has_link?("Change your 2-step verification phone", href: two_step_verification_path) - assert page.has_link?("Change your role or organisation", href: account_role_organisation_path) + assert page.has_link?("View your role and organisation", href: account_role_organisation_path) assert page.has_link?("Your account access log", href: account_activity_path) end diff --git a/test/policies/account/role_organisations_policy_test.rb b/test/policies/account/role_organisations_policy_test.rb index 0f4e0539d..b353212bd 100644 --- a/test/policies/account/role_organisations_policy_test.rb +++ b/test/policies/account/role_organisations_policy_test.rb @@ -33,7 +33,7 @@ class Account::RoleOrganisationsPolicyTest < ActiveSupport::TestCase end context "update_role?" do - %i[superadmin admin].each do |user_role| + %i[superadmin].each do |user_role| should "be permitted for #{user_role} users" do user = FactoryBot.build(:"#{user_role}_user") @@ -41,7 +41,7 @@ class Account::RoleOrganisationsPolicyTest < ActiveSupport::TestCase end end - %i[super_organisation_admin organisation_admin normal].each do |user_role| + %i[admin super_organisation_admin organisation_admin normal].each do |user_role| should "be forbidden for #{user_role} users" do user = FactoryBot.build(:"#{user_role}_user") From acad605426e87bd6c8cdd4f18da7f3da425ce57f Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 3 Oct 2023 12:05:10 +0100 Subject: [PATCH 33/35] Redirect users to account page for editing their own account So that users don't have two different ways of editing their own details (account page and users edit page). I've implemented this redirect in `#load_and_authorize_user` (instead of in the action itself) because in a future commit I want to update the `UserPolicy` to prevent users editing their own accounts through this controller. --- app/controllers/users_controller.rb | 2 ++ test/controllers/users_controller_test.rb | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 33e75a368..818638387 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -99,6 +99,8 @@ def require_2sv; end def load_and_authorize_user @user = current_user.normal? ? current_user : User.find(params[:id]) + redirect_to(account_path) and return if current_user == @user && action_name == "edit" + authorize @user end diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index ab54c23d9..cf76b94ca 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -310,6 +310,13 @@ class UsersControllerTest < ActionController::TestCase end context "GET edit" do + context "for the currently logged in user" do + should "redirect to the account page" do + get :edit, params: { id: @user.id } + assert_redirected_to account_path + end + end + should "show the form" do not_an_admin = create(:user) get :edit, params: { id: not_an_admin.id } From bcde44227c48a7a29cba0d925d6a740587adeb00 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 3 Oct 2023 12:28:17 +0100 Subject: [PATCH 34/35] Prevent users editing their details on users/edit pages Users should edit their own details using their account page functionality. This commit ensures that no one can continue using the /users pages to edit their own account. In the UserPolicy test for superadmins and admins, I have to skip the `create` action because these roles should continue to be able to create new users. This includes a fix for a bug in the _form_fields partial. The partial was previously checking whether the signed in user can mandate_2sv for themselves instead of checking whether the signed in user can mandate_2sv for the user they're editing. The change to UserPolicy in this commit highlighted the bug because users can no longer edit their details (including mandate_2sv) through the users controller. I've had to update two tests in users_controller_test so that we're mimicking a user editing another user, instead of the user editing themselves. --- app/policies/user_policy.rb | 2 ++ app/views/users/_form_fields.html.erb | 2 +- test/controllers/users_controller_test.rb | 22 +++++++++++++-------- test/policies/user_policy_test.rb | 24 +++++++++++++++++++++++ 4 files changed, 41 insertions(+), 9 deletions(-) diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 783d035b2..29cc68a11 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -15,6 +15,8 @@ def create? end def edit? + return false if current_user == record + case current_user.role when Roles::Superadmin.role_name true diff --git a/app/views/users/_form_fields.html.erb b/app/views/users/_form_fields.html.erb index 90fb69452..e6b89daca 100644 --- a/app/views/users/_form_fields.html.erb +++ b/app/views/users/_form_fields.html.erb @@ -40,7 +40,7 @@

This user's role is set to <%= @user.role %>. They are currently exempted from 2-step verification, meaning that their role cannot be changed as admins are required to have 2-step verification.

<% end %> -<% if policy(current_user).mandate_2sv? && @user.persisted? %> +<% if policy(@user).mandate_2sv? && @user.persisted? %>
Account security
diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index cf76b94ca..fc4033e0d 100644 --- a/test/controllers/users_controller_test.rb +++ b/test/controllers/users_controller_test.rb @@ -614,23 +614,29 @@ class UsersControllerTest < ActionController::TestCase end should "redisplay the form if save fails" do - admin = create(:organisation_admin_user) - sign_in admin + organisation = create(:organisation) + admin1 = create(:organisation_admin_user, organisation:) + admin2 = create(:organisation_admin_user, organisation:) + + sign_in admin1 - put :update, params: { id: admin.id, user: { name: "" } } + put :update, params: { id: admin2.id, user: { name: "" } } - assert_select "form#edit_user_#{admin.id}" + assert_select "form#edit_user_#{admin2.id}" end end context "super organisation admin" do should "redisplay the form if save fails" do - admin = create(:super_organisation_admin_user) - sign_in admin + organisation = create(:organisation) + admin1 = create(:super_organisation_admin_user, organisation:) + admin2 = create(:super_organisation_admin_user, organisation:) + + sign_in admin1 - put :update, params: { id: admin.id, user: { name: "" } } + put :update, params: { id: admin2.id, user: { name: "" } } - assert_select "form#edit_user_#{admin.id}" + assert_select "form#edit_user_#{admin2.id}" end end diff --git a/test/policies/user_policy_test.rb b/test/policies/user_policy_test.rb index 67d2ba35a..0e6354d42 100644 --- a/test/policies/user_policy_test.rb +++ b/test/policies/user_policy_test.rb @@ -41,6 +41,14 @@ class UserPolicyTest < ActiveSupport::TestCase assert permit?(user, build(:admin_user), permission) assert permit?(user, build(:superadmin_user), permission) end + + next if permission == :create + + should "not allow for #{permission} for the logged in user" do + user = create(:superadmin_user) + + assert forbid?(user, user, permission) + end end superadmin_actions.each do |permission| @@ -93,6 +101,14 @@ class UserPolicyTest < ActiveSupport::TestCase assert permit?(user, build(:admin_user), permission) assert forbid?(user, build(:superadmin_user), permission) end + + next if permission == :create + + should "not allow for #{permission} for the logged in user" do + user = create(:admin_user) + + assert forbid?(user, user, permission) + end end superadmin_actions.each do |permission| @@ -135,6 +151,10 @@ class UserPolicyTest < ActiveSupport::TestCase end super_org_admin_actions.each do |permission| + should "not allow for #{permission} for the logged in user" do + assert forbid?(@super_org_admin, @super_org_admin, permission) + end + should "allow for #{permission} and users of similar permissions or below from within their own organisation" do assert permit?(@super_org_admin, build(:user_in_organisation, organisation: @super_org_admin.organisation), permission) assert permit?(@super_org_admin, build(:organisation_admin_user, organisation: @super_org_admin.organisation), permission) @@ -192,6 +212,10 @@ class UserPolicyTest < ActiveSupport::TestCase end org_admin_actions.each do |permission| + should "not allow for #{permission} for the logged in user" do + assert forbid?(@organisation_admin, @organisation_admin, permission) + end + should "allow for #{permission} and users of similar permissions or below from within their own organisation" do assert permit?(@organisation_admin, build(:user_in_organisation, organisation: @organisation_admin.organisation), permission) assert permit?(@organisation_admin, build(:organisation_admin_user, organisation: @organisation_admin.organisation), permission) From 22c3626a676001be5a40b6a281861509a62898ce Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 3 Oct 2023 15:25:51 +0100 Subject: [PATCH 35/35] Avoid using Pundit.policy in _form_fields partial We're already using the `policy` helper method elsewhere so let's use that consistently. This also avoids us having to pass the `current_user` in. --- app/views/users/_form_fields.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/users/_form_fields.html.erb b/app/views/users/_form_fields.html.erb index e6b89daca..5fb4ce592 100644 --- a/app/views/users/_form_fields.html.erb +++ b/app/views/users/_form_fields.html.erb @@ -47,7 +47,7 @@ <% if @user.exempt_from_2sv? %>

The user has been made exempt from 2-step verification for the following reason: <%= @user.reason_for_2sv_exemption %> - <% if Pundit.policy(current_user, @user).exempt_from_two_step_verification? %> + <% if policy(@user).exempt_from_two_step_verification? %>
<%= link_to 'Edit reason or expiry date for 2-step verification exemption', edit_two_step_verification_exemption_path(@user) %> <% end %> @@ -77,7 +77,7 @@
User will be prompted to set up 2-step verification again the next time they sign in.

<% end %> - <% if @user.persisted? && Pundit.policy(current_user, @user).exempt_from_two_step_verification? && @user.reason_for_2sv_exemption.nil? %> + <% if @user.persisted? && policy(@user).exempt_from_two_step_verification? && @user.reason_for_2sv_exemption.nil? %>

<%= link_to 'Exempt user from 2-step verification', edit_two_step_verification_exemption_path(@user) %>