Confirmable (#580)

* Confirmable

* Remove obsolete rake task

* Skip confirmation for inactive users

* Send beta onboarding template if migrated from Softwire

* Default controller

* Use correct link

* Redirect confirmation to set password

* Confirm account within 3 days

* Only redirect to set password if not previously set

* Rubocop

* Confirm factory bot users

* Set password condition

* Changing email requires reconfirming

* No need to explicitly trigger email, devise does that for us now

* Remove flash banner

* Mock notify

* Mock in the right spec

* Test redirect and text

* User is confirmable

* Rubocop

* Redirect to url so we don't bypass authenticity token

* Update content

* Add test for resend invite flow

* Update link to resend confirmation email

* Rename password reset resend confirmation partial

* Expired link error page

* Remove resend confirmation link

* Update seed

* Expory contact

* Time zone

Co-authored-by: Paul Robert Lloyd <me+git@paulrobertlloyd.com>

app/controllers/auth/confirmations_controller.rb

@@ -0,0 +1,20 @@
+class Auth::ConfirmationsController < Devise::ConfirmationsController
+  # GET /resource/confirmation?confirmation_token=abcdef
+  def show
+    self.resource = resource_class.confirm_by_token(params[:confirmation_token])
+    yield resource if block_given?
+
+    if resource.errors.empty?
+      if resource.sign_in_count.zero?
+        token = resource.send(:set_reset_password_token)
+        redirect_to "#{edit_user_password_url}?reset_password_token=#{token}&confirmation=true"
+      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"
+    else
+      respond_with_navigational(resource.errors, status: :unprocessable_entity) { render :new }
+    end
+  end
+end

app/controllers/auth/passwords_controller.rb

@@ -11,7 +11,7 @@ class Auth::PasswordsController < Devise::PasswordsController
       resource.errors.add :email, I18n.t("validations.email.invalid")
       render "devise/passwords/new", status: :unprocessable_entity
     else
-      render "devise/confirmations/reset"
+      render "devise/passwords/reset_resend_confirmation"
     end
   end
 
@@ -24,6 +24,7 @@ class Auth::PasswordsController < Devise::PasswordsController
 
   def edit
     super
+    @confirmation = params["confirmation"]
     render "devise/passwords/reset_password"
   end
 

app/controllers/users_controller.rb

@@ -62,7 +62,6 @@ class UsersController < ApplicationController
     else
       user = User.create(user_params.merge(org_params).merge(password_params))
       if user.persisted?
-        user.send_reset_password_instructions
         redirect_to created_user_redirect_path
       else
         @resource.errors.add :email, I18n.t("validations.email.taken")

app/mailers/devise_notify_mailer.rb

@@ -13,21 +13,34 @@ class DeviseNotifyMailer < Devise::Mailer
     )
   end
 
-  def reset_password_instructions(record, token, _opts = {})
+  def personalisation(record, token, url)
-    url = public_send("edit_#{record.class.name.underscore}_password_url")
+    {
-    personalisation = {
       name: record.name || record.email,
       email: record.email,
       organisation: record.respond_to?(:organisation) ? record.organisation.name : "",
-      link: "#{url}?reset_password_token=#{token}",
+      link: "#{url}#{token}",
     }
-    send_email(record.email, record.reset_password_notify_template, personalisation)
   end
 
-  # def confirmation_instructions(record, token, _opts = {})
+  def reset_password_instructions(record, token, _opts = {})
-  #   super
+    base = public_send("edit_#{record.class.name.underscore}_password_url")
-  # end
+    url = "#{base}?reset_password_token="
-  #
+    send_email(
+      record.email,
+      record.reset_password_notify_template,
+      personalisation(record, token, url),
+    )
+  end
+
+  def confirmation_instructions(record, token, _opts = {})
+    url = "#{user_confirmation_url}?confirmation_token="
+    send_email(
+      record.email,
+      record.confirmable_template,
+      personalisation(record, token, url),
+    )
+  end
+
   # def unlock_instructions(record, token, opts = {})
   #   super
   # end

app/models/user.rb

@@ -1,8 +1,8 @@
 class User < ApplicationRecord
   # Include default devise modules. Others available are:
-  # :confirmable, :timeoutable and :omniauthable
+  # :omniauthable
   devise :database_authenticatable, :recoverable, :rememberable, :validatable,
-         :trackable, :lockable, :two_factor_authenticatable
+         :trackable, :lockable, :two_factor_authenticatable, :confirmable, :timeoutable
 
   belongs_to :organisation
   has_many :owned_case_logs, through: :organisation
@@ -66,10 +66,27 @@ class User < ApplicationRecord
 
   MFA_TEMPLATE_ID = "6bdf5ee1-8e01-4be1-b1f9-747061d8a24c".freeze
   RESET_PASSWORD_TEMPLATE_ID = "2c410c19-80a7-481c-a531-2bcb3264f8e6".freeze
-  SET_PASSWORD_TEMPLATE_ID   = "257460a6-6616-4640-a3f9-17c3d73d9e91".freeze
+  CONFIRMABLE_TEMPLATE_ID = "257460a6-6616-4640-a3f9-17c3d73d9e91".freeze
+  BETA_ONBOARDING_TEMPLATE_ID = "b48bc2cd-5887-4611-8296-d0ab3ed0e7fd".freeze
 
   def reset_password_notify_template
-    last_sign_in_at ? RESET_PASSWORD_TEMPLATE_ID : SET_PASSWORD_TEMPLATE_ID
+    RESET_PASSWORD_TEMPLATE_ID
+  end
+
+  def confirmable_template
+    if was_migrated_from_softwire?
+      BETA_ONBOARDING_TEMPLATE_ID
+    else
+      CONFIRMABLE_TEMPLATE_ID
+    end
+  end
+
+  def was_migrated_from_softwire?
+    old_user_id.present?
+  end
+
+  def skip_confirmation!
+    !active?
   end
 
   def need_two_factor_authentication?(_request)

app/views/devise/confirmations/expired.html.erb

@@ -0,0 +1,11 @@
+<% 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>

app/views/devise/confirmations/new.html.erb

@@ -1,7 +1,22 @@
-<h2>Resend confirmation instructions</h2>
+<% content_for :title, "Resend invitation link" %>
+
+<% content_for :before_content do %>
+  <%= govuk_back_link(
+    text: "Back",
+    href: :back,
+  ) %>
+<% end %>
 
 <%= form_for(resource, as: resource_name, url: confirmation_path(resource_name), html: { method: :post }) do |f| %>
-  <%= render "devise/shared/error_messages", resource: resource %>
+  <div class="govuk-grid-row">
+    <div class="govuk-grid-column-two-thirds">
+      <%= f.govuk_error_summary %>
+
+      <h1 class="govuk-heading-l">
+        <%= content_for(:title) %>
+      </h1>
+
+      <p class="govuk-body">Enter your email address to get a new invitation link.</p>
 
       <%= f.govuk_email_field :email,
         label: { text: "Email address" },
@@ -9,7 +24,9 @@
         spellcheck: "false",
         value: (resource.pending_reconfirmation? ? resource.unconfirmed_email : resource.email) %>
 
-  <%= f.govuk_submit "Resend confirmation instructions" %>
+      <%= f.govuk_submit "Send email" %>
+    </div>
+  </div>
 <% end %>
 
 <%= render "devise/shared/links" %>

app/views/devise/passwords/reset_password.html.erb

@@ -1,4 +1,4 @@
-<% content_for :title, "Reset your password" %>
+<% content_for :title, @confirmation ? I18n.t("user.create_password") : I18n.t("user.reset_password") %>
 
 <% content_for :before_content do %>
   <%= govuk_back_link(

app/views/devise/shared/_links.html.erb

@@ -11,7 +11,7 @@
 <% end %>
 
 <%- if devise_mapping.confirmable? && controller_name != 'confirmations' %>
-  <%= govuk_link_to "Didn’t receive confirmation instructions?", new_confirmation_path(resource_name) %><br>
+  <!-- <p class="govuk-body"><%#= govuk_link_to "Get a new invitation link", new_confirmation_path(resource_name) %> if it’s expired.</p>  -->
 <% end %>
 
 <%- if devise_mapping.lockable? && resource_class.unlock_strategy_enabled?(:email) && controller_name != 'unlocks' %>

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 = 3.days
+  config.confirm_within = 3.days
 
   # If true, requires any email changes to be confirmed (exactly the same way as
   # initial account confirmation) to be applied. Requires additional unconfirmed_email

config/locales/devise.en.yml

@@ -56,10 +56,10 @@ en:
       unlocked: "Your account has been successfully unlocked. Sign in to continue."
   errors:
     messages:
-      already_confirmed: "has already been confirmed. Sign in."
+      already_confirmed: "Email has already been confirmed. Sign in."
       blank: "can’t be blank"
-      confirmation: "doesn’t match new password"
+      confirmation: "Password confirmation doesn’t match new password"
-      confirmation_period_expired: "needs to be confirmed within %{period}. Request a new account."
+      confirmation_period_expired: "Email needs to be confirmed within %{period}. Request a new link below."
       expired: "Token has expired. Request a new token."
       not_found: "was not found"
       not_locked: "has not been locked"

config/locales/en.yml

@@ -34,6 +34,9 @@ en:
   feedback_form: "https://forms.office.com/Pages/ResponsePage.aspx?id=EGg0v32c3kOociSi7zmVqC4YDsCJ3llAvEZelBFBLUBURFVUTzFDTUJPQlM4M0laTE5DTlNFSjJBQi4u"
   organisation:
     updated: "Organisation details updated"
+  user:
+    create_password: "Create a password to finish setting up your account"
+    reset_password: "Reset your password"
 
   validations:
     other_field_missing: "If %{main_field_label} is other then %{other_field_label} must be provided"

config/routes.rb

@@ -26,6 +26,7 @@ Rails.application.routes.draw do
   devise_for :users, {
     path: :account,
     controllers: {
+      confirmations: "auth/confirmations",
       passwords: "auth/passwords",
       sessions: "auth/sessions",
       two_factor_authentication: "auth/two_factor_authentication",

db/migrate/20220517093906_add_confirmable_users.rb

@@ -0,0 +1,11 @@
+class AddConfirmableUsers < ActiveRecord::Migration[7.0]
+  def change
+    change_table :users, bulk: true do |t|
+      t.column :confirmation_token, :string
+      t.column :confirmed_at, :datetime
+      t.column :confirmation_sent_at, :datetime
+      t.string :unconfirmed_email
+    end
+    add_index :users, :confirmation_token, unique: true
+  end
+end

db/schema.rb

@@ -343,6 +343,11 @@ ActiveRecord::Schema[7.0].define(version: 2022_05_18_115438) do
     t.datetime "direct_otp_sent_at", precision: nil
     t.datetime "totp_timestamp", precision: nil
     t.boolean "active", default: true
+    t.string "confirmation_token"
+    t.datetime "confirmed_at", precision: nil
+    t.datetime "confirmation_sent_at", precision: nil
+    t.string "unconfirmed_email"
+    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
     t.index ["organisation_id"], name: "index_users_on_organisation_id"

db/seeds.rb

@@ -31,6 +31,7 @@ if Rails.env.development? && User.count.zero?
     password: "password",
     organisation: org,
     role: "data_provider",
+    confirmed_at: Time.zone.now,
   )
 
   User.create!(
@@ -38,6 +39,7 @@ if Rails.env.development? && User.count.zero?
     password: "password",
     organisation: org,
     role: "data_coordinator",
+    confirmed_at: Time.zone.now,
   )
 
   User.create!(
@@ -45,6 +47,7 @@ if Rails.env.development? && User.count.zero?
     password: "password",
     organisation: org,
     role: "support",
+    confirmed_at: Time.zone.now,
   )
 
   pp "Seeded 3 dummy users"

lib/tasks/onboarding_emails.rake

@@ -1,22 +0,0 @@
-namespace :onboarding_emails do
-  desc "Send onboarding emails to private beta users"
-  task :send, %i[organisation_id] => :environment do |_task, args|
-    organisation_id = args[:organisation_id]
-    host = ENV["APP_HOST"]
-    raise "Organisation id must be provided" unless organisation_id
-    raise "Host is not set" unless host
-
-    organisation = Organisation.find(organisation_id)
-    raise "Organisation #{organisation_id} does not exist" unless organisation
-
-    organisation.users.each do |user|
-      next unless URI::MailTo::EMAIL_REGEXP.match?(user.email)
-
-      onboarding_template_id = "b48bc2cd-5887-4611-8296-d0ab3ed0e7fd".freeze
-      token = user.send(:set_reset_password_token)
-      url = "#{host}/account/password/edit?reset_password_token=#{token}"
-      personalisation = { name: user.name || user.email, link: url }
-      DeviseNotifyMailer.new.send_email(user.email, onboarding_template_id, personalisation)
-    end
-  end
-end

spec/controllers/admin/users_controller_spec.rb

@@ -9,8 +9,13 @@ describe Admin::UsersController, type: :controller do
   let(:resource_title) { "Users" }
   let(:valid_session) { {} }
   let!(:admin_user) { FactoryBot.create(:admin_user) }
+  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)
     sign_in admin_user
   end
 

spec/factories/user.rb

@@ -15,6 +15,8 @@ FactoryBot.define do
     trait :support do
       role { "support" }
     end
+    sign_in_count { 5 }
+    confirmed_at { Time.zone.now }
     created_at { Time.zone.now }
     updated_at { Time.zone.now }
   end

spec/features/organisation_spec.rb

@@ -5,15 +5,15 @@ RSpec.describe "User Features" do
   include Helpers
   let(:organisation) { user.organisation }
   let(:org_id) { organisation.id }
-  let(:set_password_template_id) { User::SET_PASSWORD_TEMPLATE_ID }
+  let(:set_password_template_id) { User::CONFIRMABLE_TEMPLATE_ID }
   let(:notify_client) { instance_double(Notifications::Client) }
-  let(:reset_password_token) { "MCDH5y6Km-U7CFPgAMVS" }
+  let(:confirmation_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(Devise.token_generator).to receive(:generate).and_return(reset_password_token)
+    allow(Devise).to receive(:friendly_token).and_return(confirmation_token)
     allow(notify_client).to receive(:send_email).and_return(true)
     sign_in user
   end
@@ -55,7 +55,7 @@ RSpec.describe "User Features" do
               name: "New User",
               email: "new_user@example.com",
               organisation: organisation.name,
-              link: "http://localhost:3000/account/password/edit?reset_password_token=#{reset_password_token}",
+              link: "http://localhost:3000/account/confirmation?confirmation_token=#{confirmation_token}",
             },
           },
         )

spec/lib/tasks/onboarding_emails_spec.rb

@@ -1,41 +0,0 @@
-require "rails_helper"
-require "rake"
-
-describe "rake onboarding_emails:send", type: task do
-  subject(:task) { Rake::Task["onboarding_emails:send"] }
-
-  context "when onboarding a new organisation to private beta" do
-    let!(:user) { FactoryBot.create(:user) }
-    let(:notify_client) { instance_double(Notifications::Client) }
-    let(:devise_notify_mailer) { DeviseNotifyMailer.new }
-    let(:reset_password_token) { "MCDH5y6Km-U7CFPgAMVS" }
-    let(:host) { "http://localhost:3000" }
-
-    before do
-      Rake.application.rake_require("tasks/onboarding_emails")
-      Rake::Task.define_task(:environment)
-      task.reenable
-      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)
-      allow(ENV).to receive(:[])
-      allow(ENV).to receive(:[]).with("APP_HOST").and_return(host)
-    end
-
-    it "can send the onboarding emails" do
-      expect(notify_client).to receive(:send_email).with(
-        {
-          email_address: user.email,
-          template_id: "b48bc2cd-5887-4611-8296-d0ab3ed0e7fd",
-          personalisation: {
-            name: user.name,
-            link: "#{host}/account/password/edit?reset_password_token=#{reset_password_token}",
-          },
-        },
-      )
-
-      task.invoke(user.organisation.id)
-    end
-  end
-end

spec/models/user_spec.rb

@@ -73,6 +73,18 @@ RSpec.describe User, type: :model do
       expect(user.need_two_factor_authentication?(nil)).to be false
     end
 
+    it "is confirmable" do
+      allow(DeviseNotifyMailer).to receive(:confirmation_instructions).and_return(OpenStruct.new(deliver: true))
+      expect(DeviseNotifyMailer).to receive(:confirmation_instructions).once
+      described_class.create!(
+        name: "unconfirmed_user",
+        email: "unconfirmed_user@example.com",
+        password: "password123",
+        organisation: other_organisation,
+        role: "data_provider",
+      )
+    end
+
     context "when the user is a data provider" do
       it "cannot assign roles" do
         expect(user.assignable_roles).to eq({})

spec/requests/auth/confirmations_controller_spec.rb

@@ -0,0 +1,46 @@
+require "rails_helper"
+require_relative "../../support/devise"
+
+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) }
+
+  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
+
+  context "when a confirmation link is clicked by a new user" do
+    before do
+      user.send_confirmation_instructions
+      get "/account/confirmation?confirmation_token=#{user.confirmation_token}"
+    end
+
+    it "marks the user as confirmed" do
+      expect(user.reload.confirmed_at).to be_a(Time)
+    end
+
+    it "redirects to the set password page" do
+      follow_redirect!
+      expect(page).to have_content(I18n.t("user.create_password"))
+    end
+  end
+
+  context "when the token has expired" do
+    let(:period) { Devise::TimeInflector.time_ago_in_words(User.confirm_within.ago) }
+
+    before do
+      user.send_confirmation_instructions
+      allow(User).to receive(:find_first_by_auth_conditions).and_return(user)
+      allow(user).to receive(:confirmation_period_expired?).and_return(true)
+      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")
+    end
+  end
+end

spec/requests/auth/passwords_controller_spec.rb

@@ -87,7 +87,7 @@ RSpec.describe Auth::PasswordsController, type: :request 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("h1", text: "Reset your password")
+        expect(page).to have_css("h1", text: I18n.t("user.reset_password"))
       end
 
       context "when passwords entered don't match" do

spec/requests/users_controller_spec.rb

@@ -255,7 +255,7 @@ RSpec.describe UsersController, type: :request do
 
           it "allows changing email but not dpo or key_contact" do
             user.reload
-            expect(user.email).to eq(new_email)
+            expect(user.unconfirmed_email).to eq(new_email)
             expect(user.is_data_protection_officer?).to be false
             expect(user.is_key_contact?).to be false
           end
@@ -539,7 +539,7 @@ RSpec.describe UsersController, type: :request do
 
           it "allows changing email and dpo" do
             user.reload
-            expect(user.email).to eq(new_email)
+            expect(user.unconfirmed_email).to eq(new_email)
             expect(user.is_data_protection_officer?).to be true
             expect(user.is_key_contact?).to be true
           end
@@ -586,7 +586,7 @@ RSpec.describe UsersController, type: :request do
             it "allows changing email, dpo, key_contact" do
               patch "/users/#{other_user.id}", headers: headers, params: params
               other_user.reload
-              expect(other_user.email).to eq(new_email)
+              expect(other_user.unconfirmed_email).to eq(new_email)
               expect(other_user.is_data_protection_officer?).to be true
               expect(other_user.is_key_contact?).to be true
             end
@@ -971,7 +971,7 @@ RSpec.describe UsersController, type: :request do
 
           it "allows changing email and dpo" do
             user.reload
-            expect(user.email).to eq(new_email)
+            expect(user.unconfirmed_email).to eq(new_email)
             expect(user.is_data_protection_officer?).to be true
             expect(user.is_key_contact?).to be true
           end
@@ -1018,7 +1018,7 @@ RSpec.describe UsersController, type: :request do
             it "allows changing email, dpo, key_contact" do
               patch "/users/#{other_user.id}", headers: headers, params: params
               other_user.reload
-              expect(other_user.email).to eq(new_email)
+              expect(other_user.unconfirmed_email).to eq(new_email)
               expect(other_user.is_data_protection_officer?).to be true
               expect(other_user.is_key_contact?).to be true
             end
@@ -1075,7 +1075,7 @@ RSpec.describe UsersController, type: :request do
               it "allows changing email, dpo, key_contact" do
                 patch "/users/#{other_user.id}", headers: headers, params: params
                 other_user.reload
-                expect(other_user.email).to eq(new_email)
+                expect(other_user.unconfirmed_email).to eq(new_email)
                 expect(other_user.is_data_protection_officer?).to be true
                 expect(other_user.is_key_contact?).to be true
               end

spec/services/imports/user_import_service_spec.rb

@@ -7,6 +7,14 @@ RSpec.describe Imports::UserImportService do
   let(:user_file) { File.open("#{fixture_directory}/#{old_user_id}.xml") }
   let(:storage_service) { instance_double(StorageService) }
   let(:logger) { instance_double(ActiveSupport::Logger) }
+  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
 
   context "when importing users" do
     subject(:import_service) { described_class.new(storage_service, logger) }