diff --git a/app/controllers/auth/confirmations_controller.rb b/app/controllers/auth/confirmations_controller.rb index c3be85f51..d3f617822 100644 --- a/app/controllers/auth/confirmations_controller.rb +++ b/app/controllers/auth/confirmations_controller.rb @@ -11,9 +11,10 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController else respond_with_navigational(resource) { redirect_to after_confirmation_path_for(resource_name, resource) } end - elsif resource.errors.map(&:type).include?(:confirmation_period_expired) - render "devise/confirmations/expired" + elsif %i[blank invalid].any? { |error| resource.errors.map(&:type).include?(error) } + render "devise/confirmations/invalid" elsif resource.errors.map(&:type).include?(:already_confirmed) + flash[:notice] = I18n.t("errors.messages.already_confirmed") redirect_to user_session_path else respond_with_navigational(resource.errors, status: :unprocessable_entity) { render :new } diff --git a/app/controllers/auth/passwords_controller.rb b/app/controllers/auth/passwords_controller.rb index bd4b119cf..c76ebaf6a 100644 --- a/app/controllers/auth/passwords_controller.rb +++ b/app/controllers/auth/passwords_controller.rb @@ -19,11 +19,13 @@ class Auth::PasswordsController < Devise::PasswordsController self.resource = resource_class.send_reset_password_instructions(resource_params) yield resource if block_given? + @minimum_password_length = Devise.password_length.min respond_with({}, location: after_sending_reset_password_instructions_path_for(resource_name)) end def edit super + @minimum_password_length = Devise.password_length.min @confirmation = params["confirmation"] render "devise/passwords/reset_password" end @@ -44,8 +46,9 @@ class Auth::PasswordsController < Devise::PasswordsController end respond_with resource, location: after_resetting_password_path_for(resource) else - set_minimum_password_length - respond_with resource, status: :unprocessable_entity + @minimum_password_length = Devise.password_length.min + @confirmation = resource_params["confirmation"] + render "devise/passwords/reset_password", status: :unprocessable_entity end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 4ba32a442..0c343b8c3 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -49,7 +49,7 @@ class UsersController < ApplicationController user_name = @user.name&.possessive || @user.email.possessive case user_params[:active] when "false" - @user.update!(confirmed_at: nil, sign_in_count: 0) + @user.update!(confirmed_at: nil, sign_in_count: 0, initial_confirmation_sent: false) flash[:notice] = I18n.t("devise.activation.deactivated", user_name:) when "true" @user.send_confirmation_instructions @@ -145,14 +145,14 @@ private def user_params if @user == current_user if current_user.data_coordinator? || current_user.support? - params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact) + params.require(:user).permit(:email, :name, :password, :password_confirmation, :role, :is_dpo, :is_key_contact, :initial_confirmation_sent) else - params.require(:user).permit(:email, :name, :password, :password_confirmation) + params.require(:user).permit(:email, :name, :password, :password_confirmation, :initial_confirmation_sent) end elsif current_user.data_coordinator? - params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact, :active) + params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact, :active, :initial_confirmation_sent) elsif current_user.support? - params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact, :organisation_id, :active) + params.require(:user).permit(:email, :name, :role, :is_dpo, :is_key_contact, :organisation_id, :active, :initial_confirmation_sent) end end diff --git a/app/models/user.rb b/app/models/user.rb index 4d2aeeca1..6354086b5 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -95,7 +95,8 @@ class User < ApplicationRecord MFA_TEMPLATE_ID = "6bdf5ee1-8e01-4be1-b1f9-747061d8a24c".freeze RESET_PASSWORD_TEMPLATE_ID = "2c410c19-80a7-481c-a531-2bcb3264f8e6".freeze - CONFIRMABLE_TEMPLATE_ID = "257460a6-6616-4640-a3f9-17c3d73d9e91".freeze + CONFIRMABLE_TEMPLATE_ID = "3fc2e3a7-0835-4b84-ab7a-ce51629eb614".freeze + RECONFIRMABLE_TEMPLATE_ID = "bcdec787-f0a7-46e9-8d63-b3e0a06ee455".freeze BETA_ONBOARDING_TEMPLATE_ID = "b48bc2cd-5887-4611-8296-d0ab3ed0e7fd".freeze USER_REACTIVATED_TEMPLATE_ID = "ac45a899-490e-4f59-ae8d-1256fc0001f9".freeze @@ -108,6 +109,8 @@ class User < ApplicationRecord USER_REACTIVATED_TEMPLATE_ID elsif was_migrated_from_softwire? && last_sign_in_at.blank? BETA_ONBOARDING_TEMPLATE_ID + elsif initial_confirmation_sent + RECONFIRMABLE_TEMPLATE_ID else CONFIRMABLE_TEMPLATE_ID end @@ -121,6 +124,7 @@ class User < ApplicationRecord return unless active? super + update!(initial_confirmation_sent: true) end def need_two_factor_authentication?(_request) diff --git a/app/views/devise/confirmations/expired.html.erb b/app/views/devise/confirmations/expired.html.erb deleted file mode 100644 index 5a27eee4d..000000000 --- a/app/views/devise/confirmations/expired.html.erb +++ /dev/null @@ -1,11 +0,0 @@ -<% content_for :title, "Your invitation link has expired" %> - -
-
-

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

- -

Contact the helpdesk to request a new one.

-
-
diff --git a/app/views/devise/confirmations/invalid.html.erb b/app/views/devise/confirmations/invalid.html.erb new file mode 100644 index 000000000..8d3ad96df --- /dev/null +++ b/app/views/devise/confirmations/invalid.html.erb @@ -0,0 +1,12 @@ +<% content_for :title, "Expired link" %> + +
+
+

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

+ +

It looks like you have requested a newer join link than this one. Check your emails and follow the most recent link instead.

+ +
+
diff --git a/app/views/devise/confirmations/new.html.erb b/app/views/devise/confirmations/new.html.erb index 0f44c394f..5f36dc388 100644 --- a/app/views/devise/confirmations/new.html.erb +++ b/app/views/devise/confirmations/new.html.erb @@ -1,29 +1,17 @@ -<% content_for :title, "Resend invitation link" %> +<% content_for :title, "Expired link" %> -<% content_for :before_content do %> - <%= govuk_back_link(href: :back) %> -<% end %> +
+
+

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

+ <%= form_for(resource, as: resource_name, url: confirmation_path(resource_name), html: { method: :post }) do |f| %> -<%= form_for(resource, as: resource_name, url: confirmation_path(resource_name), html: { method: :post }) do |f| %> -
-
- <%= f.govuk_error_summary %> +

For security reasons, your join link expired - get another one using the button below (valid for 3 hours).

-

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

+ <%= f.hidden_field :email, value: (resource.pending_reconfirmation? ? resource.unconfirmed_email : resource.email) %> + <%= f.govuk_submit "Get a new join link" %> + <% end %> -

Enter your email address to get a new invitation link.

- - <%= f.govuk_email_field :email, - label: { text: "Email address" }, - autocomplete: "email", - spellcheck: "false", - value: (resource.pending_reconfirmation? ? resource.unconfirmed_email : resource.email) %> - - <%= f.govuk_submit "Send email" %> -
-<% end %> - -<%= render "devise/shared/links" %> +
diff --git a/app/views/devise/passwords/edit.html.erb b/app/views/devise/passwords/edit.html.erb index 94ef6f44c..34137a0d6 100644 --- a/app/views/devise/passwords/edit.html.erb +++ b/app/views/devise/passwords/edit.html.erb @@ -15,7 +15,7 @@ <%= 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, + hint: { text: "Your password must be at least #{@minimum_password_length} characters and hard to guess." }, autocomplete: "new-password" %> <%= f.govuk_password_field :password_confirmation, diff --git a/app/views/devise/passwords/reset_password.html.erb b/app/views/devise/passwords/reset_password.html.erb index 26ee6f590..2e0da0e66 100644 --- a/app/views/devise/passwords/reset_password.html.erb +++ b/app/views/devise/passwords/reset_password.html.erb @@ -1,8 +1,4 @@ -<% content_for :title, @confirmation ? I18n.t("user.create_password") : I18n.t("user.reset_password") %> - -<% content_for :before_content do %> - <%= govuk_back_link(href: :back) %> -<% end %> +<% content_for :title, @confirmation.present? ? I18n.t("user.create_password") : I18n.t("user.reset_password") %> <%= form_for(resource, as: resource_name, url: password_path(resource_name), html: { method: :put }) do |f| %> <%= f.hidden_field :reset_password_token %> @@ -16,12 +12,14 @@ <%= 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, + hint: { text: "Your password must be at least #{@minimum_password_length} characters and hard to guess." }, autocomplete: "new-password" %> <%= f.govuk_password_field :password_confirmation, label: { text: "Confirm new password" } %> + <%= f.hidden_field :confirmation, value: @confirmation %> + <%= f.govuk_submit "Update" %>
diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index cd1a30ee8..63389432d 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -152,7 +152,7 @@ Devise.setup do |config| # their account can't be confirmed with the token any more. # Default is nil, meaning there is no restriction on how long a user can take # before confirming their account. - config.confirm_within = 5.days + config.confirm_within = 3.hours # If true, requires any email changes to be confirmed (exactly the same way as # initial account confirmation) to be applied. Requires additional unconfirmed_email diff --git a/db/migrate/20230203174815_add_initial_confirmation_sent_to_users.rb b/db/migrate/20230203174815_add_initial_confirmation_sent_to_users.rb new file mode 100644 index 000000000..8f7fcaf66 --- /dev/null +++ b/db/migrate/20230203174815_add_initial_confirmation_sent_to_users.rb @@ -0,0 +1,5 @@ +class AddInitialConfirmationSentToUsers < ActiveRecord::Migration[7.0] + def change + add_column :users, :initial_confirmation_sent, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 0d7173129..b9022d026 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_02_03_104238) do +ActiveRecord::Schema[7.0].define(version: 2023_02_03_174815) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -596,6 +596,7 @@ ActiveRecord::Schema[7.0].define(version: 2023_02_03_104238) do t.datetime "confirmed_at", precision: nil t.datetime "confirmation_sent_at", precision: nil t.string "unconfirmed_email" + t.boolean "initial_confirmation_sent" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true t.index ["email"], name: "index_users_on_email", unique: true t.index ["encrypted_otp_secret_key"], name: "index_users_on_encrypted_otp_secret_key", unique: true diff --git a/spec/mailers/devise_notify_mailer_spec.rb b/spec/mailers/devise_notify_mailer_spec.rb index a086eddaf..a0fe9171f 100644 --- a/spec/mailers/devise_notify_mailer_spec.rb +++ b/spec/mailers/devise_notify_mailer_spec.rb @@ -51,5 +51,26 @@ RSpec.describe DeviseNotifyMailer do end end end + + context "when a user is invited for the first time" do + let(:email) { "test@example.com" } + + it "sends initial confirmation template" do + expect(notify_client).to receive(:send_email).with(hash_including(template_id: User::CONFIRMABLE_TEMPLATE_ID)) + User.create!(name:, organisation:, email:, password:, role:) + end + end + + context "when a user requests further confirmation links" do + let(:email) { "test@example.com" } + + it "sends re-confirmation template" do + user = User.create!(name:, organisation:, email:, password:, role:) + expect(notify_client).to receive(:send_email).with(hash_including(template_id: User::RECONFIRMABLE_TEMPLATE_ID)) + user.send_confirmation_instructions + expect(notify_client).to receive(:send_email).with(hash_including(template_id: User::RECONFIRMABLE_TEMPLATE_ID)) + user.send_confirmation_instructions + end + end end end diff --git a/spec/requests/auth/confirmations_controller_spec.rb b/spec/requests/auth/confirmations_controller_spec.rb index fe27925b1..aa7f93d65 100644 --- a/spec/requests/auth/confirmations_controller_spec.rb +++ b/spec/requests/auth/confirmations_controller_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Auth::ConfirmationsController, type: :request do let(:page) { Capybara::Node::Simple.new(response.body) } let(:notify_client) { instance_double(Notifications::Client) } let(:devise_notify_mailer) { DeviseNotifyMailer.new } - let(:user) { FactoryBot.create(:user, :data_provider, sign_in_count: 0, confirmed_at: nil) } + let(:user) { FactoryBot.create(:user, :data_provider, sign_in_count: 0, confirmed_at: nil, initial_confirmation_sent: nil) } before do allow(DeviseNotifyMailer).to receive(:new).and_return(devise_notify_mailer) @@ -39,8 +39,30 @@ RSpec.describe Auth::ConfirmationsController, type: :request do get "/account/confirmation?confirmation_token=#{user.confirmation_token}" end - it "shows the error page" do - expect(page).to have_content("Your invitation link has expired") + it "shows the expired page" do + expect(page).to have_content("For security reasons, your join link expired - get another one using the button below (valid for 3 hours).") + end + end + + context "when the token is blank" do + before do + user.send_confirmation_instructions + get "/account/confirmation" + end + + it "shows the invalid page" do + expect(page).to have_content("It looks like you have requested a newer join link than this one. Check your emails and follow the most recent link instead.") + end + end + + context "when the token is invalid" do + before do + user.send_confirmation_instructions + get "/account/confirmation?confirmation_token=SOMETHING_INVALID" + end + + it "shows the invalid page" do + expect(page).to have_content("It looks like you have requested a newer join link than this one. Check your emails and follow the most recent link instead.") end end diff --git a/spec/requests/auth/passwords_controller_spec.rb b/spec/requests/auth/passwords_controller_spec.rb index 830a06713..70357062e 100644 --- a/spec/requests/auth/passwords_controller_spec.rb +++ b/spec/requests/auth/passwords_controller_spec.rb @@ -86,7 +86,7 @@ RSpec.describe Auth::PasswordsController, type: :request do it "renders the user edit password view" do _raw, enc = Devise.token_generator.generate(User, :reset_password_token) - get "/account/password/edit?reset_password_token=#{enc}" + get "/account/password/edit?reset_password_token=#{enc}?confirmation=true" expect(page).to have_css("h1", text: "Reset your password") end @@ -103,9 +103,10 @@ RSpec.describe Auth::PasswordsController, type: :request do } end - it "shows an error" do + it "shows an error on the same page" do put "/account/password", headers: headers, params: params expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_css("h1", text: "Reset your password") expect(page).to have_content("doesn’t match new password") end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 048799985..3328fdaf6 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -46,57 +46,28 @@ RSpec.describe UsersController, type: :request do end end - describe "reset password" do - it "renders the user edit password view" do - _raw, enc = Devise.token_generator.generate(User, :reset_password_token) - get "/account/password/edit?reset_password_token=#{enc}" - expect(page).to have_css("h1", class: "govuk-heading-l", text: "Reset your password") - end - + describe "change password" do context "when updating a user password" do - context "when the reset token is valid" do - let(:params) do - { - id: user.id, user: { password: new_name, password_confirmation: "something_else" } - } - end - - before do - sign_in user - put "/account", headers:, params: - end - - it "shows an error if passwords don't match" do - expect(response).to have_http_status(:unprocessable_entity) - expect(page).to have_selector("#error-summary-title") - expect(page).to have_content("Password confirmation doesn’t match new password") - end + let(:params) do + { + id: user.id, user: { password: new_name, password_confirmation: "something_else" } + } end - context "when a reset token is more than 3 hours old" do - let(:raw) { user.send_reset_password_instructions } - let(:params) do - { - id: user.id, - user: { - password: new_name, - password_confirmation: new_name, - reset_password_token: raw, - }, - } - end + before do + sign_in user + put "/account", headers:, params: + end - before do - allow(User).to receive(:find_or_initialize_with_error_by).and_return(user) - allow(user).to receive(:reset_password_sent_at).and_return(4.hours.ago) - put "/account/password", headers:, params: - end + it "renders the user change password view" do + expect(page).to have_css("h1", class: "govuk-heading-l", text: "Change your password") + end - it "shows an error" do - expect(response).to have_http_status(:unprocessable_entity) - expect(page).to have_selector("#error-summary-title") - expect(page).to have_content(I18n.t("errors.messages.expired")) - end + it "shows an error on the same page if passwords don't match" do + expect(response).to have_http_status(:unprocessable_entity) + expect(page).to have_css("h1", class: "govuk-heading-l", text: "Change your password") + expect(page).to have_selector("#error-summary-title") + expect(page).to have_content("Password confirmation doesn’t match new password") end end end