From 24a369a2c42fc5a24897d3e212718da24fe2c17b Mon Sep 17 00:00:00 2001 From: baarkerlounger Date: Wed, 16 Feb 2022 18:07:26 +0000 Subject: [PATCH] Fix sending reset email for admins --- app/mailers/devise_notify_mailer.rb | 13 ++-- app/models/admin_user.rb | 9 ++- app/models/user.rb | 7 ++ app/views/admin_users/reset_password.html.erb | 33 ++++++++++ ...20216163601_add_trackable_to_admin_user.rb | 13 ++++ db/schema.rb | 6 ++ spec/features/admin_user_spec.rb | 55 ++++++++++++++++ spec/features/organisation_spec.rb | 5 +- spec/features/reset_password.html.erb | 33 ++++++++++ spec/features/user_spec.rb | 5 +- .../devise/passwords_controller_spec.rb | 65 +++++++++++++++++++ 11 files changed, 228 insertions(+), 16 deletions(-) create mode 100644 app/views/admin_users/reset_password.html.erb create mode 100644 db/migrate/20220216163601_add_trackable_to_admin_user.rb create mode 100644 spec/features/admin_user_spec.rb create mode 100644 spec/features/reset_password.html.erb create mode 100644 spec/requests/active_admin/devise/passwords_controller_spec.rb diff --git a/app/mailers/devise_notify_mailer.rb b/app/mailers/devise_notify_mailer.rb index 11cea379a..61d8f4b2d 100644 --- a/app/mailers/devise_notify_mailer.rb +++ b/app/mailers/devise_notify_mailer.rb @@ -1,9 +1,6 @@ class DeviseNotifyMailer < Devise::Mailer require "notifications/client" - RESET_PASSWORD_TEMPLATE_ID = "2c410c19-80a7-481c-a531-2bcb3264f8e6".freeze - SET_PASSWORD_TEMPLATE_ID = "257460a6-6616-4640-a3f9-17c3d73d9e91".freeze - def notify_client @notify_client ||= ::Notifications::Client.new(ENV["GOVUK_NOTIFY_API_KEY"]) end @@ -21,14 +18,14 @@ class DeviseNotifyMailer < Devise::Mailer end def reset_password_instructions(record, token, _opts = {}) - template_id = record.last_sign_in_at ? RESET_PASSWORD_TEMPLATE_ID : SET_PASSWORD_TEMPLATE_ID + url = public_send("edit_#{record.class.name.underscore}_password_url") personalisation = { - name: record.name, + name: record.name || record.email, email: record.email, - organisation: record.organisation.name, - link: "https://#{host}/users/password/edit?reset_password_token=#{token}", + organisation: record.respond_to?(:organisation) ? record.organisation.name : "", + link: "#{url}?reset_password_token=#{token}", } - send_email(record.email, template_id, personalisation) + send_email(record.email, record.reset_password_notify_template, personalisation) end # def confirmation_instructions(record, token, _opts = {}) diff --git a/app/models/admin_user.rb b/app/models/admin_user.rb index 351bf834d..05f368a16 100644 --- a/app/models/admin_user.rb +++ b/app/models/admin_user.rb @@ -1,8 +1,8 @@ class AdminUser < ApplicationRecord # Include default devise modules. Others available are: - # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable + # :confirmable, :lockable, :timeoutable, :omniauthable devise :two_factor_authenticatable, :database_authenticatable, :recoverable, - :rememberable, :validatable + :rememberable, :validatable, :trackable has_one_time_password(encrypted: true) @@ -11,10 +11,15 @@ class AdminUser < ApplicationRecord validates :phone, presence: true, numericality: true MFA_SMS_TEMPLATE_ID = "bf309d93-804e-4f95-b1f4-bd513c48ecb0".freeze + RESET_PASSWORD_TEMPLATE_ID = "fbb2d415-b9b1-4507-ba0a-6e542fa3504d".freeze def send_two_factor_authentication_code(code) template_id = MFA_SMS_TEMPLATE_ID personalisation = { otp: code } Sms.send(phone, template_id, personalisation) end + + def reset_password_notify_template + RESET_PASSWORD_TEMPLATE_ID + end end diff --git a/app/models/user.rb b/app/models/user.rb index 217b0a1ac..d3ecc6090 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -25,4 +25,11 @@ class User < ApplicationRecord def not_completed_case_logs case_logs.not_completed end + + RESET_PASSWORD_TEMPLATE_ID = "2c410c19-80a7-481c-a531-2bcb3264f8e6".freeze + SET_PASSWORD_TEMPLATE_ID = "257460a6-6616-4640-a3f9-17c3d73d9e91".freeze + + def reset_password_notify_template + last_sign_in_at ? RESET_PASSWORD_TEMPLATE_ID : SET_PASSWORD_TEMPLATE_ID + end end diff --git a/app/views/admin_users/reset_password.html.erb b/app/views/admin_users/reset_password.html.erb new file mode 100644 index 000000000..a1e5e7c0b --- /dev/null +++ b/app/views/admin_users/reset_password.html.erb @@ -0,0 +1,33 @@ +<% content_for :title, "Reset your password" %> + +<% content_for :before_content do %> + <%= govuk_back_link( + text: 'Back', + href: :back, + ) %> +<% end %> + +<%= form_for(@admin_user, as: :admin_user, url: password_path(AdminUser), html: { method: :put }) do |f| %> + <%= f.hidden_field :reset_password_token %> +
+
+ <%= f.govuk_error_summary %> + +

+ <%= content_for(:title) %> +

+ + <%= f.govuk_password_field :password, + label: { text: "New password" }, + hint: @minimum_password_length ? { text: "Your password must be at least #{@minimum_password_length} characters and hard to guess." } : nil, + autocomplete: "new-password" + %> + + <%= f.govuk_password_field :password_confirmation, + label: { text: "Confirm new password" } + %> + + <%= f.govuk_submit "Update" %> +
+
+<% end %> diff --git a/db/migrate/20220216163601_add_trackable_to_admin_user.rb b/db/migrate/20220216163601_add_trackable_to_admin_user.rb new file mode 100644 index 000000000..f1fe8277e --- /dev/null +++ b/db/migrate/20220216163601_add_trackable_to_admin_user.rb @@ -0,0 +1,13 @@ +class AddTrackableToAdminUser < ActiveRecord::Migration[7.0] + def change + change_table :admin_users, bulk: true do |t| + t.string :name + ## Trackable + t.integer :sign_in_count, default: 0, null: false + t.datetime :current_sign_in_at + t.datetime :last_sign_in_at + t.string :current_sign_in_ip + t.string :last_sign_in_ip + end + end +end diff --git a/db/schema.rb b/db/schema.rb index e0fee6a27..75b05b615 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -30,6 +30,12 @@ ActiveRecord::Schema[7.0].define(version: 202202071123100) do t.datetime "direct_otp_sent_at", precision: nil t.datetime "totp_timestamp", precision: nil t.string "phone" + t.string "name" + t.integer "sign_in_count", default: 0, null: false + t.datetime "current_sign_in_at", precision: nil + t.datetime "last_sign_in_at", precision: nil + t.string "current_sign_in_ip" + t.string "last_sign_in_ip" t.index ["encrypted_otp_secret_key"], name: "index_admin_users_on_encrypted_otp_secret_key", unique: true end diff --git a/spec/features/admin_user_spec.rb b/spec/features/admin_user_spec.rb new file mode 100644 index 000000000..4ca656643 --- /dev/null +++ b/spec/features/admin_user_spec.rb @@ -0,0 +1,55 @@ +require "rails_helper" + +RSpec.describe "Admin Features" do + let!(:admin_user) { FactoryBot.create(:admin_user, last_sign_in_at: Time.zone.now) } + let(:notify_client) { instance_double(Notifications::Client) } + let(:reset_password_token) { "MCDH5y6Km-U7CFPgAMVS" } + let(:devise_notify_mailer) { DeviseNotifyMailer.new } + + before do + allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer) + allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client) + allow(notify_client).to receive(:send_email).and_return(true) + allow(Devise.token_generator).to receive(:generate).and_return(reset_password_token) + end + + context "when the admin has forgotten their password" do + it " is redirected to the reset password page when they click the reset password link" do + visit("/admin") + click_link("Forgot your password?") + expect(page).to have_current_path("/admin/password/new") + end + + it " is shown an error message if they submit without entering an email address" do + visit("/admin/password/new") + click_button("Reset My Password") + expect(page).to have_selector("#error_explanation") + expect(page).to have_content("can't be blank") + end + + it " is redirected to admin login page after reset email is sent" do + visit("/admin/password/new") + fill_in("admin_user[email]", with: admin_user.email) + click_button("Reset My Password") + expect(page).to have_current_path("/admin/login") + end + + it " is sent a reset password email via Notify" do + expect(notify_client).to receive(:send_email).with( + { + email_address: admin_user.email, + template_id: admin_user.reset_password_notify_template, + personalisation: { + name: admin_user.email, + email: admin_user.email, + organisation: "", + link: "http://localhost:3000/admin/password/edit?reset_password_token=#{reset_password_token}", + }, + }, + ) + visit("/admin/password/new") + fill_in("admin_user[email]", with: admin_user.email) + click_button("Reset My Password") + end + end +end diff --git a/spec/features/organisation_spec.rb b/spec/features/organisation_spec.rb index 1eb906212..04c602c9a 100644 --- a/spec/features/organisation_spec.rb +++ b/spec/features/organisation_spec.rb @@ -5,7 +5,7 @@ RSpec.describe "User Features" do include Helpers let(:organisation) { user.organisation } let(:org_id) { organisation.id } - let(:set_password_template_id) { DeviseNotifyMailer::SET_PASSWORD_TEMPLATE_ID } + let(:set_password_template_id) { User::SET_PASSWORD_TEMPLATE_ID } let(:notify_client) { instance_double(Notifications::Client) } let(:reset_password_token) { "MCDH5y6Km-U7CFPgAMVS" } let(:devise_notify_mailer) { DeviseNotifyMailer.new } @@ -13,7 +13,6 @@ RSpec.describe "User Features" do before do allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer) allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client) - allow(devise_notify_mailer).to receive(:host).and_return("test.com") allow(Devise.token_generator).to receive(:generate).and_return(reset_password_token) allow(notify_client).to receive(:send_email).and_return(true) sign_in user @@ -56,7 +55,7 @@ RSpec.describe "User Features" do name: "New User", email: "new_user@example.com", organisation: organisation.name, - link: "https://test.com/users/password/edit?reset_password_token=#{reset_password_token}", + link: "http://localhost:3000/users/password/edit?reset_password_token=#{reset_password_token}", }, }, ) diff --git a/spec/features/reset_password.html.erb b/spec/features/reset_password.html.erb new file mode 100644 index 000000000..e12a0beed --- /dev/null +++ b/spec/features/reset_password.html.erb @@ -0,0 +1,33 @@ +<% content_for :title, "Reset your password" %> + +<% content_for :before_content do %> + <%= govuk_back_link( + text: 'Back', + href: :back, + ) %> +<% end %> + +<%= form_for(@user, as: :user, url: password_path(User), html: { method: :put }) do |f| %> + <%= f.hidden_field :reset_password_token %> +
+
+ <%= f.govuk_error_summary %> + +

+ <%= content_for(:title) %> +

+ + <%= f.govuk_password_field :password, + label: { text: "New password" }, + hint: @minimum_password_length ? { text: "Your password must be at least #{@minimum_password_length} characters and hard to guess." } : nil, + autocomplete: "new-password" + %> + + <%= f.govuk_password_field :password_confirmation, + label: { text: "Confirm new password" } + %> + + <%= f.govuk_submit "Update" %> +
+
+<% end %> diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index e6b0fafd2..70eeeb5fa 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -2,7 +2,7 @@ require "rails_helper" RSpec.describe "User Features" do let!(:user) { FactoryBot.create(:user, last_sign_in_at: Time.zone.now) } - let(:reset_password_template_id) { DeviseNotifyMailer::RESET_PASSWORD_TEMPLATE_ID } + let(:reset_password_template_id) { User::RESET_PASSWORD_TEMPLATE_ID } let(:notify_client) { instance_double(Notifications::Client) } let(:reset_password_token) { "MCDH5y6Km-U7CFPgAMVS" } let(:devise_notify_mailer) { DeviseNotifyMailer.new } @@ -10,7 +10,6 @@ RSpec.describe "User Features" do before do allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer) allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client) - allow(devise_notify_mailer).to receive(:host).and_return("test.com") allow(notify_client).to receive(:send_email).and_return(true) allow(Devise.token_generator).to receive(:generate).and_return(reset_password_token) end @@ -109,7 +108,7 @@ RSpec.describe "User Features" do name: user.name, email: user.email, organisation: user.organisation.name, - link: "https://test.com/users/password/edit?reset_password_token=#{reset_password_token}", + link: "http://localhost:3000/users/password/edit?reset_password_token=#{reset_password_token}", }, }, ) diff --git a/spec/requests/active_admin/devise/passwords_controller_spec.rb b/spec/requests/active_admin/devise/passwords_controller_spec.rb new file mode 100644 index 000000000..948b15309 --- /dev/null +++ b/spec/requests/active_admin/devise/passwords_controller_spec.rb @@ -0,0 +1,65 @@ +require "rails_helper" + +RSpec.describe ActiveAdmin::Devise::PasswordsController, type: :request do + let(:admin_user) { FactoryBot.create(:admin_user) } + let(:headers) { { "Accept" => "text/html" } } + let(:page) { Capybara::Node::Simple.new(response.body) } + let(:new_value) { "new-password" } + let(:notify_client) { instance_double(Notifications::Client) } + let(:devise_notify_mailer) { DeviseNotifyMailer.new } + + before do + allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer) + allow(devise_notify_mailer).to receive(:notify_client).and_return(notify_client) + allow(notify_client).to receive(:send_email).and_return(true) + end + + describe "reset password" do + it "renders the user edit password view" do + _raw, enc = Devise.token_generator.generate(AdminUser, :reset_password_token) + get "/admin/password/edit?reset_password_token=#{enc}" + expect(page).to have_css("h2", text: "DLUHC CORE Change your password") + end + + context "when passwords entered don't match" do + let(:raw) { admin_user.send_reset_password_instructions } + let(:params) do + { + id: admin_user.id, + admin_user: { + password: new_value, + password_confirmation: "something_else", + reset_password_token: raw, + }, + } + end + + it "shows an error" do + put "/admin/password", headers: headers, params: params + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_content("doesn't match Password") + end + end + + context "when passwords is reset" do + let(:raw) { admin_user.send_reset_password_instructions } + let(:params) do + { + id: admin_user.id, + admin_user: { + password: new_value, + password_confirmation: new_value, + reset_password_token: raw, + }, + } + end + + it "updates the password" do + expect { + put "/admin/password", headers: headers, params: params + admin_user.reload + }.to change(admin_user, :encrypted_password) + end + end + end +end