Spike One login implementation #10107

CatalinVoineag · 2024-11-26T16:48:21Z

Context

This is a POC, this PR won't be merged in main but a lot of the code here will be split out in different PRs once we get the green light from product.

This adds one login integration for the candidate path. The magic link implementation is not visible to the candidate.

The candidate will be redirected to one login test env where they can create an account and we will log them in apply. They can also recover their old account if their previous candidate email doesn't match the one login email.

Agreed technical flow(bottom diagram), this can change though
https://lucid.app/lucidspark/3647e5d9-5075-45d4-abd3-088ee4323fc9/edit?invitationId=inv_8027d1a6-8607-4cb9-a43e-7dc1a8975f38&page=0_0#

Changes proposed in this pull request

New columns and tables.
Candidate will only have 1 OmniAuthLogin and 1 AccountRecoveryRequest, because we use methods like candidate.create_one_login_auth which delets the old one and creates a new one
They are used to auth the user and to allow them to recover their account
The OmniAuthLogin holds the information coming back from one login
recovered and dismiss_recovery columns are there mostly for the Recover account banner

erDiagram
    Candidate ||--o{ OmniAuthLogin : has_one
    Candidate ||--o{ AccountRecoveryRequest : has_one
    Candidate {
      string email_address
      boolean recovered
      boolean dismiss_recovery
    }
    OmniAuthLogin {
      string token
      string email
      id candidate_id
    }
    AccountRecoveryRequest {
      int code
      string previous_account_email
      id candidate
    }

Screencast.2024-11-28.17.33.24.mp4

Guidance for review

Go on review app and try to login as a candidate.
Create a one login account and login.
Click on Recover account
Add your email gov/dfe address with a +1 before the @ so [email protected]
You should get an email with a unique code
Enter that code.
You should be logged in.

Try to break this, I have created 3 emails for our team members to recover, so +1,+2,+3
Try putting +100
Put the wrong or a bad email address
Create a new one login account and try to claim it after.
Go nuts

Things to check

CatalinVoineag · 2024-11-27T16:54:59Z

db/migrate/20241122143740_create_account_recovery_requests.rb

+class CreateAccountRecoveryRequests < ActiveRecord::Migration[8.0]
+  def change
+    create_table :account_recovery_requests do |t|
+      t.integer :code, null: false, index: { unique: true }


Need to encrypt this on the production PR

Also, the AccountRecoveryRequest table should have multiple codes attached to it as we want to allow the user to use multiple codes, all with a arbitrary expiration time

(Ha, I should have read the whole thing before I said you should encrypt the code...)

CatalinVoineag · 2024-11-29T09:29:37Z

Spike the one login technical flow

Previously you could not login if you already had a candidate account but no one_login_auth Now you can

And downcase and strip the account recovery email address

CatalinVoineag · 2024-11-29T15:12:00Z

app/models/candidate.rb

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


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

elceebee · 2024-11-29T16:11:48Z

app/forms/candidate_interface/account_recovery_form.rb

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


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?

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?

apply-for-teacher-training/app/models/account_recovery_request.rb

Lines 9 to 13 in cbdf703

def self.generate_code

code = SecureRandom.random_number(100_000..999_999)

AccountRecoveryRequest.generate_code while AccountRecoveryRequest.exists?(code:)

code

end

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.

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 👍🏻

app/forms/candidate_interface/account_recovery_form.rb

elceebee · 2024-11-29T16:22:44Z

app/models/account_recovery_request.rb

+  normalizes :previous_account_email, with: ->(email) { email.downcase.strip }
+
+  def self.generate_code
+    code = SecureRandom.random_number(100_000..999_999)


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)

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.

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

elceebee · 2024-11-29T16:31:10Z

config/analytics.yml

@@ -46,6 +48,13 @@ shared:
    - withdrawn_or_declined_for_candidate_by_provider
    - structured_withdrawal_reasons
    - school_placement_auto_selected
+  account_recovery_requests:
+    - id
+    - code


Should we put the code in the blocked list? I suppose in practice it will be hashed, but might as well not send it at all as an extra precaution.

Yes, we should

CatalinVoineag · 2024-12-02T13:29:16Z

app/forms/candidate_interface/account_recovery_request_form.rb

+          code: AccountRecoveryRequest.generate_code,
+        )
+
+        AccountRecoveryMailer.send_code(


Only send emails to candidates that exist in our system

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.

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

CatalinVoineag · 2024-12-04T12:04:04Z

We also need to allow our users to have access to their one login account, in the header of our pages https://www.sign-in.service.gov.uk/documentation/design-recommendations/let-users-navigate-sign-out

CatalinVoineag self-assigned this Nov 26, 2024

CatalinVoineag added DO NOT MERGE DO NOT REVIEW and removed DO NOT REVIEW labels Nov 26, 2024

CatalinVoineag commented Nov 27, 2024

View reviewed changes

CatalinVoineag added 5 commits November 28, 2024 10:41

Wip one login auth

4711aab

Account recovery basic implementation

8da9a97

Improved account recovery flow

459cdaa

Better validation and Generic message page

b356682

Working One login end to end

2181c82

CatalinVoineag force-pushed the cv/one-login-implementation branch from 8caf1e9 to 2181c82 Compare November 28, 2024 10:43

CatalinVoineag temporarily deployed to review_aks November 28, 2024 10:43 — with GitHub Actions Inactive

Clean up

e72a81a

CatalinVoineag temporarily deployed to review_aks November 28, 2024 11:22 — with GitHub Actions Inactive

CatalinVoineag added the deploy_v2 Deploy the review app to AKS label Nov 28, 2024

CatalinVoineag temporarily deployed to review_aks November 28, 2024 11:37 — with GitHub Actions Inactive

CatalinVoineag had a problem deploying to review_aks November 28, 2024 11:40 — with GitHub Actions Failure

github-actions bot temporarily deployed to review_aks-10107 November 28, 2024 11:40 Destroyed

Skip candidate smoke test

9eae2d3

CatalinVoineag temporarily deployed to review_aks November 28, 2024 11:50 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to review_aks November 28, 2024 11:56 — with GitHub Actions Inactive

github-actions bot temporarily deployed to review_aks-10107 November 28, 2024 11:56 Destroyed

New env variable names

e955978

CatalinVoineag temporarily deployed to review_aks November 28, 2024 14:10 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to review_aks November 28, 2024 14:15 — with GitHub Actions Inactive

github-actions bot temporarily deployed to review_aks-10107 November 28, 2024 14:15 Destroyed

CatalinVoineag temporarily deployed to review_aks November 28, 2024 14:27 — with GitHub Actions Inactive

github-actions bot temporarily deployed to review_aks-10107 November 28, 2024 14:27 Destroyed

github-actions bot temporarily deployed to review_aks-10107 November 28, 2024 14:57 Destroyed

CatalinVoineag temporarily deployed to review_aks November 28, 2024 14:57 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to review_aks November 28, 2024 16:03 — with GitHub Actions Inactive

github-actions bot temporarily deployed to review_aks-10107 November 28, 2024 16:10 Destroyed

CatalinVoineag temporarily deployed to review_aks November 28, 2024 16:10 — with GitHub Actions Inactive

Remove unnecessary link

9ce2ab3

CatalinVoineag temporarily deployed to review_aks November 28, 2024 16:26 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to review_aks November 28, 2024 16:31 — with GitHub Actions Inactive

github-actions bot temporarily deployed to review_aks-10107 November 28, 2024 16:31 Destroyed

CatalinVoineag requested a review from a team November 29, 2024 09:22

CatalinVoineag marked this pull request as ready for review November 29, 2024 09:23

Fix issue with existing_candidate login in.

19ecb26

Previously you could not login if you already had a candidate account but no one_login_auth Now you can

CatalinVoineag temporarily deployed to review_aks November 29, 2024 11:51 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to review_aks November 29, 2024 11:57 — with GitHub Actions Inactive

github-actions bot temporarily deployed to review_aks-10107 November 29, 2024 11:57 Destroyed

CatalinVoineag added 2 commits November 29, 2024 12:08

Removed the confirm account recovery step

ccc526b

And downcase and strip the account recovery email address

Content for the account recovery page

cbdf703

CatalinVoineag temporarily deployed to review_aks November 29, 2024 12:14 — with GitHub Actions Inactive

github-actions bot deployed to review_aks-10107 November 29, 2024 12:20 View deployment

CatalinVoineag temporarily deployed to review_aks November 29, 2024 12:20 — with GitHub Actions Inactive

CatalinVoineag commented Nov 29, 2024

View reviewed changes

elceebee reviewed Nov 29, 2024

View reviewed changes

app/forms/candidate_interface/account_recovery_form.rb Show resolved Hide resolved

elceebee reviewed Nov 29, 2024

View reviewed changes

CatalinVoineag changed the title ~~One login implementation~~ Spike One login implementation Nov 29, 2024

CatalinVoineag commented Dec 2, 2024

View reviewed changes

This was referenced Dec 3, 2024

One login integration migrations #10136

Closed

Add one login analytics columns #10141

Closed

CatalinVoineag mentioned this pull request Dec 4, 2024

One login integration migrations #10144

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spike One login implementation #10107

Spike One login implementation #10107

CatalinVoineag commented Nov 26, 2024 •

edited

Loading

CatalinVoineag Nov 27, 2024

CatalinVoineag Nov 29, 2024

elceebee Nov 29, 2024

CatalinVoineag commented Nov 29, 2024

CatalinVoineag Nov 29, 2024

elceebee Nov 29, 2024

CatalinVoineag Nov 29, 2024 •

edited

Loading

elceebee Nov 29, 2024

CatalinVoineag Nov 29, 2024 •

edited

Loading

elceebee Nov 29, 2024

elceebee Nov 29, 2024

CatalinVoineag Nov 29, 2024

elceebee Nov 29, 2024

CatalinVoineag Nov 29, 2024

CatalinVoineag Dec 2, 2024

elceebee Dec 3, 2024

CatalinVoineag Dec 3, 2024

CatalinVoineag commented Dec 4, 2024

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

Spike One login implementation #10107

Are you sure you want to change the base?

Spike One login implementation #10107

Conversation

CatalinVoineag commented Nov 26, 2024 • edited Loading

Context

Changes proposed in this pull request

Guidance for review

Things to check

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CatalinVoineag commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CatalinVoineag Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CatalinVoineag Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CatalinVoineag commented Dec 4, 2024

CatalinVoineag commented Nov 26, 2024 •

edited

Loading

CatalinVoineag Nov 29, 2024 •

edited

Loading

CatalinVoineag Nov 29, 2024 •

edited

Loading