From 144e9dc593f49590f9c0d025035bb348c3c66bd6 Mon Sep 17 00:00:00 2001
From: natdeanlewissoftwire
 <94526761+natdeanlewissoftwire@users.noreply.github.com>
Date: Thu, 9 Feb 2023 17:25:49 +0000
Subject: [PATCH] CLDC-1900 improve expired join links (#1265)

* feat: update expired join link page

* feat: update controller

* feat: show invalid page if token invalid

* feat: add password hint_text

* feat: use new templates

* refactor: min password length simplification

* feat: add dynamic template selection

* feat: revert devise behaviour

* feat: fix initial confirmation sent behaviour

* feat: show invalid page if link invalid

* test: update

* test: add tests for path

* db: update

* refactor: linting

* feat: move initial_confirmation_sent update

* feat: add further reconfirmation to tests

* refactor: simplify by removing before_action

* revert password hint text update (added to cldc-1963 instead)

* feat: reset initial_confirmation_sent if deactivated

* feat: reset initial_confirmation_sent if deactivated

* feat: add flash notice for already confirmed

* feat: fix reset passwords bugs - stop leaking between reset/change routes

* test: add tests for change vs reset password routes

* feat: add reset password persistence
---
 .../auth/confirmations_controller.rb          |  5 +-
 app/controllers/auth/passwords_controller.rb  |  7 ++-
 app/controllers/users_controller.rb           | 10 +--
 app/models/user.rb                            |  6 +-
 .../devise/confirmations/expired.html.erb     | 11 ----
 .../devise/confirmations/invalid.html.erb     | 12 ++++
 app/views/devise/confirmations/new.html.erb   | 36 ++++-------
 app/views/devise/passwords/edit.html.erb      |  2 +-
 .../devise/passwords/reset_password.html.erb  | 10 ++-
 config/initializers/devise.rb                 |  2 +-
 ..._add_initial_confirmation_sent_to_users.rb |  5 ++
 db/schema.rb                                  |  3 +-
 spec/mailers/devise_notify_mailer_spec.rb     | 21 +++++++
 .../auth/confirmations_controller_spec.rb     | 28 ++++++++-
 .../auth/passwords_controller_spec.rb         |  5 +-
 spec/requests/users_controller_spec.rb        | 63 +++++--------------
 16 files changed, 121 insertions(+), 105 deletions(-)
 delete mode 100644 app/views/devise/confirmations/expired.html.erb
 create mode 100644 app/views/devise/confirmations/invalid.html.erb
 create mode 100644 db/migrate/20230203174815_add_initial_confirmation_sent_to_users.rb

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" %>
-
-<div class="govuk-grid-row">
-  <div class="govuk-grid-column-two-thirds">
-    <h1 class="govuk-heading-l">
-      <%= content_for(:title) %>
-    </h1>
-
-    <p class="govuk-body">Contact the helpdesk to request a new one.</p>
-  </div>
-</div>
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" %>
+
+<div class="govuk-grid-row">
+  <div class="govuk-grid-column-two-thirds">
+    <h1 class="govuk-heading-l">
+      <%= content_for(:title) %>
+    </h1>
+
+    <p class="govuk-body">It looks like you have requested a newer join link than this one. Check your emails and follow the most recent link instead.</p>
+
+  </div>
+</div>
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 %>
+<div class="govuk-grid-row">
+  <div class="govuk-grid-column-two-thirds">
+    <h1 class="govuk-heading-l">
+      <%= content_for(:title) %>
+    </h1>
+    <%= 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| %>
-  <div class="govuk-grid-row">
-    <div class="govuk-grid-column-two-thirds">
-      <%= f.govuk_error_summary %>
+      <p class="govuk-body">For security reasons, your join link expired - get another one using the button below (valid for 3 hours).</p>
 
-      <h1 class="govuk-heading-l">
-        <%= content_for(:title) %>
-      </h1>
+      <%= f.hidden_field :email, value: (resource.pending_reconfirmation? ? resource.unconfirmed_email : resource.email) %>
+      <%= f.govuk_submit "Get a new join link" %>
+    <% end %>
 
-      <p class="govuk-body">Enter your email address to get a new invitation link.</p>
-
-      <%= 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" %>
-    </div>
   </div>
-<% end %>
-
-<%= render "devise/shared/links" %>
+</div>
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" %>
     </div>
   </div>
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