Browse Source

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>
pull/591/head
baarkerlounger 3 years ago committed by GitHub
parent
commit
326644216e
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 20
      app/controllers/auth/confirmations_controller.rb
  2. 3
      app/controllers/auth/passwords_controller.rb
  3. 1
      app/controllers/users_controller.rb
  4. 31
      app/mailers/devise_notify_mailer.rb
  5. 25
      app/models/user.rb
  6. 11
      app/views/devise/confirmations/expired.html.erb
  7. 33
      app/views/devise/confirmations/new.html.erb
  8. 2
      app/views/devise/passwords/reset_password.html.erb
  9. 0
      app/views/devise/passwords/reset_resend_confirmation.html.erb
  10. 2
      app/views/devise/shared/_links.html.erb
  11. 2
      config/initializers/devise.rb
  12. 6
      config/locales/devise.en.yml
  13. 3
      config/locales/en.yml
  14. 1
      config/routes.rb
  15. 11
      db/migrate/20220517093906_add_confirmable_users.rb
  16. 5
      db/schema.rb
  17. 3
      db/seeds.rb
  18. 22
      lib/tasks/onboarding_emails.rake
  19. 5
      spec/controllers/admin/users_controller_spec.rb
  20. 2
      spec/factories/user.rb
  21. 8
      spec/features/organisation_spec.rb
  22. 41
      spec/lib/tasks/onboarding_emails_spec.rb
  23. 12
      spec/models/user_spec.rb
  24. 46
      spec/requests/auth/confirmations_controller_spec.rb
  25. 2
      spec/requests/auth/passwords_controller_spec.rb
  26. 12
      spec/requests/users_controller_spec.rb
  27. 8
      spec/services/imports/user_import_service_spec.rb

20
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

3
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

1
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")

31
app/mailers/devise_notify_mailer.rb

@ -13,21 +13,34 @@ class DeviseNotifyMailer < Devise::Mailer
)
end
def reset_password_instructions(record, token, _opts = {})
url = public_send("edit_#{record.class.name.underscore}_password_url")
personalisation = {
def personalisation(record, token, url)
{
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 = {})
# super
# end
#
def reset_password_instructions(record, token, _opts = {})
base = public_send("edit_#{record.class.name.underscore}_password_url")
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

25
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)

11
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>

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

@ -1,15 +1,32 @@
<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" },
autocomplete: "email",
spellcheck: "false",
value: (resource.pending_reconfirmation? ? resource.unconfirmed_email : resource.email) %>
<%= 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 "Resend confirmation instructions" %>
<%= f.govuk_submit "Send email" %>
</div>
</div>
<% end %>
<%= render "devise/shared/links" %>

2
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(

0
app/views/devise/confirmations/reset.html.erb → app/views/devise/passwords/reset_resend_confirmation.html.erb

2
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' %>

2
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

6
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_period_expired: "needs to be confirmed within %{period}. Request a new account."
confirmation: "Password confirmation doesn’t match new password"
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"

3
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"

1
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",

11
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

5
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"

3
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"

22
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

5
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

2
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

8
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}",
},
},
)

41
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

12
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({})

46
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

2
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

12
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

8
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) }

Loading…
Cancel
Save