Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spike One login implementation #10107

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
35 changes: 35 additions & 0 deletions app/controllers/candidate_interface/account_recovery_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
module CandidateInterface
class AccountRecoveryController < CandidateInterfaceController
before_action :check_if_user_recovered

def new
@account_recovery = CandidateInterface::AccountRecoveryForm.new(current_candidate:)
end

def create
@account_recovery = CandidateInterface::AccountRecoveryForm.new(
current_candidate:,
code: permitted_params[:code],
)

if @account_recovery.call
sign_in(@account_recovery.old_candidate, scope: :candidate)
redirect_to root_path
else
render :new
end
end

private

def permitted_params
strip_whitespace(
params.require(:candidate_interface_account_recovery_form).permit(:code),
)
end

def check_if_user_recovered
redirect_to candidate_interface_details_path if current_candidate.recovered?
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
module CandidateInterface
class AccountRecoveryRequestsController < CandidateInterfaceController
before_action :check_if_user_recovered

def new
@account_recovery_request = CandidateInterface::AccountRecoveryRequestForm
.build_from_candidate(current_candidate)
end

def create
@account_recovery_request = CandidateInterface::AccountRecoveryRequestForm.new(
current_candidate:,
previous_account_email: permitted_params[:previous_account_email],
)

if @account_recovery_request.save
if permitted_params[:resend_pressed]
flash[:success] = "A new code has been sent to #{permitted_params[:previous_account_email]}"
end

redirect_to candidate_interface_account_recovery_new_path
else
render :new
end
end

private

def permitted_params
strip_whitespace(
params.require(:candidate_interface_account_recovery_request_form).permit(
:previous_account_email,
:resend_pressed,
),
)
end

def check_if_user_recovered
redirect_to candidate_interface_details_path if current_candidate.recovered?
end
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class CandidateInterface::DismissAccountRecoveryController < ApplicationController
def create
current_candidate.update!(dismiss_recovery: true)
redirect_to candidate_interface_details_path
end
end
36 changes: 36 additions & 0 deletions app/controllers/one_login_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
class OneLoginController < ApplicationController
def callback
auth = request.env['omniauth.auth']
session[:onelogin_id_token] = auth.credentials.id_token
candidate = OneLoginUser.authentificate(auth)

sign_in(candidate, scope: :candidate)
candidate.update!(last_signed_in_at: Time.zone.now)

redirect_to candidate_interface_interstitial_path
rescue OneLoginUser::Error => e
Sentry.capture_exception(e)
flash[:warning] = 'We cannot log you in, please contact support'
redirect_to auth_onelogin_sign_out_path
end

def sign_out
id_token = session[:onelogin_id_token]
redirect_to logout_onelogin_path(id_token_hint: id_token)
end

def sign_out_complete
saved_flash_state = flash
reset_session

flash[:warning] = saved_flash_state[:warning] if saved_flash_state[:warning].present?
redirect_to candidate_interface_create_account_or_sign_in_path
end

def failure
Sentry.capture_message("One login failure with #{params[:message]} for onelogin_id_token: #{session[:onelogin_id_token]}")
flash[:warning] = 'We cannot log you in, please contact support'

redirect_to auth_onelogin_sign_out_path
end
end
44 changes: 44 additions & 0 deletions app/forms/candidate_interface/account_recovery_form.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
module CandidateInterface
class AccountRecoveryForm
include ActiveModel::Model

attr_accessor :code
attr_reader :valid_account_recovery_request, :current_candidate, :old_candidate

validates :code, presence: true
validate :account_recovery, unless: -> { valid_account_recovery_request && old_candidate }
validate :previous_account_has_no_one_login, if: -> { valid_account_recovery_request && old_candidate }

def initialize(current_candidate:, code: nil)
self.code = code
@current_candidate = current_candidate
end

def call
@valid_account_recovery_request = AccountRecoveryRequest.where(code:)
.where('created_at >= ?', 1.hour.ago).first
@old_candidate = Candidate.find_by(email_address: valid_account_recovery_request&.previous_account_email)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking of the unlikely, but possible situation where there could be the same code generated for different users in the past hours. Should we loop through them to find the candidate?

Copy link
Contributor Author

@CatalinVoineag CatalinVoineag Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's a good point, I was hoping that this would always generate a unique code. I think this covers us against this edge case?

def self.generate_code
code = SecureRandom.random_number(100_000..999_999)
AccountRecoveryRequest.generate_code while AccountRecoveryRequest.exists?(code:)
code
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Thanks for pointing that out. I should read the whole thing before I comment!

That said, I'm not sure this will work, or work efficiently, if we are encrypting the codes. It's something to keep in mind.

Copy link
Contributor Author

@CatalinVoineag CatalinVoineag Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. I haven't thought about encrypting the code when I wrote this method, I'll do some testing when I create the production PR 👍🏻


return false unless valid?

ActiveRecord::Base.transaction do
old_candidate.update!(recovered: true)
current_candidate.one_login_auth.update!(candidate: old_candidate)
current_candidate.reload
current_candidate.destroy!
CatalinVoineag marked this conversation as resolved.
Show resolved Hide resolved
end
end

private

def account_recovery
errors.add(:code, :invalid)
end

def previous_account_has_no_one_login
if old_candidate.one_login_auth.present?
errors.add(:code, "The email address you're trying to recover already has a one login account")
end
end
end
end
49 changes: 49 additions & 0 deletions app/forms/candidate_interface/account_recovery_request_form.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
module CandidateInterface
class AccountRecoveryRequestForm
include ActiveModel::Model

attr_accessor :previous_account_email
attr_reader :current_candidate, :previous_candidate

validates :previous_account_email, presence: true, format: { with: URI::MailTo::EMAIL_REGEXP }
validate :email_different_from_current_candidate, if: -> { previous_candidate.present? }

def initialize(current_candidate:, previous_account_email: nil)
self.previous_account_email = previous_account_email&.downcase&.strip
@current_candidate = current_candidate
end

def self.build_from_candidate(candidate)
new(
current_candidate: candidate,
previous_account_email: candidate.account_recovery_request&.previous_account_email,
)
end

def save
@previous_candidate = Candidate.find_by(email_address: previous_account_email)

return false unless valid?

ActiveRecord::Base.transaction do
account_recovery_request = current_candidate.create_account_recovery_request(
previous_account_email:,
code: AccountRecoveryRequest.generate_code,
)

AccountRecoveryMailer.send_code(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only send emails to candidates that exist in our system

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here is the thing, I did the account recovery thing and tried using my personal email address. And it sent me a code. But there is no candidate in this application with my personal email address. I will try it again and take a video.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this just sends emails to any valid email address. I'll keep this conversation open to remind myself to only send emails to candidates that exist in our system

email: previous_account_email,
code: account_recovery_request.code,
).deliver_later
end
end

private

def email_different_from_current_candidate
if current_candidate.one_login_auth.email == previous_account_email
errors.add(:previous_account_email, "You can't recover the same account you are logged in to")
end
end
end
end
14 changes: 14 additions & 0 deletions app/mailers/account_recovery_mailer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
class AccountRecoveryMailer < ApplicationMailer
helper UtmLinkHelper

def send_code(email:, code:)
@code = code
@account_recovery_url = candidate_interface_account_recovery_new_url

mailer_options = {
to: email,
subject: 'Account recovery',
}
notify_email(mailer_options)
end
end
14 changes: 14 additions & 0 deletions app/models/account_recovery_request.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
class AccountRecoveryRequest < ApplicationRecord
belongs_to :candidate
belongs_to :previous_candidate, optional: true, class_name: 'Candidate'

validates :code, presence: true

normalizes :previous_account_email, with: ->(email) { email.downcase.strip }

def self.generate_code
code = SecureRandom.random_number(100_000..999_999)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means we wouldn't have any codes that start with 0. If we want 6 random digits, rather than a number between 100_000 and 999_999, we should do something like 6.times.collect { rand(0...9) }.join instead -- and it would need to be a string rather that an integer if we want to persist the leading zeros. (also, in general, things with digits aren't numbers unless we are going to do maths to them, eg phone numbers and credit card numbers are strings rather than integers)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you will have already thought of this, but we'll also want these to be stored hashed_code, using bcrypt or something like that. I haven't done this in awhile, so I can't quite remember, but I remember it being pretty straight forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. I'll change it to a true random 6 digit string as you suggested, thanks 👍🏻

AccountRecoveryRequest.generate_code while AccountRecoveryRequest.exists?(code:)
code
end
end
2 changes: 2 additions & 0 deletions app/models/candidate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ class Candidate < ApplicationRecord
has_many :degree_qualifications, through: :application_forms
has_many :application_choices, through: :application_forms
has_many :application_references, through: :application_forms
has_one :one_login_auth, dependent: :destroy
has_one :account_recovery_request, dependent: :destroy
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add specs for these to make sure we only have 1 of each at any time.

belongs_to :course_from_find, class_name: 'Course', optional: true
belongs_to :duplicate_match, foreign_key: 'fraud_match_id', optional: true

Expand Down
3 changes: 3 additions & 0 deletions app/models/one_login_auth.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class OneLoginAuth < ApplicationRecord
belongs_to :candidate
end
50 changes: 50 additions & 0 deletions app/services/one_login_user.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
class OneLoginUser
class Error < StandardError; end
attr_reader :email, :token

def initialize(auth)
@email = auth.info.email
@token = auth.uid
end

def self.authentificate(request)
new(request).authentificate
end

def authentificate
one_login_auth = OneLoginAuth.find_by(token:)
existing_candidate = Candidate.find_by(email_address: email)

return candidate_with_one_login(one_login_auth) if one_login_auth
return existing_candidate_without_one_login(existing_candidate) if existing_candidate

created_candidate
end

private

def candidate_with_one_login(one_login_auth)
one_login_auth.update!(email:)
one_login_auth.candidate
end

def existing_candidate_without_one_login(existing_candidate)
if existing_candidate.one_login_auth.present? && existing_candidate.one_login_auth.token != token
raise(
Error,
"Candidate #{existing_candidate.id} has a different one login " \
"token than the user trying to login. Token used to auth #{token}",
)
end

existing_candidate.create_one_login_auth!(token:, email:)
existing_candidate
end

def created_candidate
candidate = Candidate.create!(email_address: email)
candidate.create_one_login_auth!(token:, email:)

candidate
end
end
7 changes: 7 additions & 0 deletions app/views/account_recovery_mailer/send_code.text.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Hello

This is your unique code to recover your old account.

<%= @code %>

Please input this code [here](<%= @account_recovery_url %>) to recover your account.
41 changes: 41 additions & 0 deletions app/views/candidate_interface/account_recovery/new.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<% content_for :title, 'Claim your old account' %>
<% content_for :before_content do %>
<%= govuk_back_link(
text: 'Back',
href: new_candidate_interface_account_recovery_request_path,
) %>
<% end %>

<h1 class="govuk-heading-xl govuk-!-margin-bottom-2">
Claim your old account
</h1>

<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
<p class="govuk-body">
We have emailed a code to this address, this code will expire in 1 hour.
</p>
<p class="govuk-body">
If you haven't received it or it expired you can resend the code.
</p>

<%= form_with model: @account_recovery, url: candidate_interface_account_recovery_create_path do |f| %>
<%= f.govuk_error_summary %>
<%= f.govuk_text_field :code, label: { text: 'Code', size: 'm' }, width: 20 %>

<%= f.govuk_submit %>
<% end %>

<h2 class="govuk-heading-m govuk-!-margin-bottom-2">
The unique code expired?
</h2>
<%= govuk_button_to 'Resend code', candidate_interface_account_recovery_requests_path(
params: {
candidate_interface_account_recovery_request_form: {
previous_account_email: current_candidate.account_recovery_request.previous_account_email,
resend_pressed: true,
}
}
) %>
</div>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<% content_for :title, 'Recover your account' %>
<% content_for :before_content do %>
<%= govuk_back_link(
text: 'Back',
href: candidate_interface_details_path,
) %>
<% end %>

<h1 class="govuk-heading-xl govuk-!-margin-bottom-2">
Recover your account
</h1>

<div class="govuk-grid-row">
<div class="govuk-grid-column-two-thirds">
<%= form_with model: @account_recovery_request, url: candidate_interface_account_recovery_requests_path do |f| %>
<%= f.govuk_error_summary %>

<%= f.govuk_text_field :previous_account_email,
label: { text: 'Email address', size: 'm' },
width: 20 %>

<%= f.govuk_submit %>
<% end %>
</div>
</div>
Loading
Loading