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

One login auth #10160

Merged
merged 2 commits into from
Dec 18, 2024
Merged

One login auth #10160

merged 2 commits into from
Dec 18, 2024

Conversation

CatalinVoineag
Copy link
Contributor

@CatalinVoineag CatalinVoineag commented Dec 10, 2024

Context

We want to move our candidates to use one login when sigin up/in to our
service.

This PR adds the basic implementation for users to log in or create
a candidate account in apply using one login. It also has a bypass for localhost and review apps.

Next steps:

  • Block requests to magic link implementation if one login feature flag is active
  • Add one login link in govuk header for users to manage their one login account
  • Implement the account recovery system:w

Changes proposed in this pull request

get '/auth/onelogin/callback' -- one login sends the email and token
get '/auth/one-login-developer/callback' -- bypass callback for review and local env
get '/auth/onelogin/sign-out' -- our sign-out route, here we send a request to onelogin to log the user out
get '/auth/onelogin/sign-out-complete' - one login comes back to us to this route when signing the user out
get '/auth/failure' -- one login will send a request to this route if there's a 500 error on their end

Changes to the smoke test e4fbcba

Guidance to review

For bypass

Go on review app and log in as a candidate
Put anything in the uid column, a new candidate with one login will be created
Email would be [email protected]
Log out
Use the same uid, you should be logged in as the same candidate now.

For normal flow

Go on an https://qa.apply-for-teacher-training.service.gov.uk/candidate/account
Click continue
Input your one login account, you might already have one, if not create one.
You should be redirected back to qa

kazam_proo7lg1.movie.mp4

Things to check

  • If the code removes any existing feature flags, a data migration has also been added to delete the entry from the database
  • This code does not rely on migrations in the same Pull Request
  • If this code includes a migration adding or changing columns, it also backfills existing records for consistency
  • If this code adds a column to the DB, decide whether it needs to be in analytics yml file or analytics blocklist
  • If this code adds a column that may include PII, the sanitise.sql script and 0025-protecting-personal-data-in-production-dump.md ADR have been updated.
  • API release notes have been updated if necessary
  • If it adds a significant user-facing change, is it documented in the CHANGELOG?
  • Required environment variables have been updated added to the Azure KeyVault
  • Inform data insights team due to database changes
  • Make sure all information from the Trello card is in here
  • Rebased main
  • Cleaned commit history
  • Tested by running locally
  • Add PR link to Trello card

@CatalinVoineag CatalinVoineag self-assigned this Dec 10, 2024
@CatalinVoineag CatalinVoineag added the deploy_v2 Deploy the review app to AKS label Dec 11, 2024
@CatalinVoineag CatalinVoineag changed the title Cv/one login auth One login auth Dec 11, 2024
@github-actions github-actions bot temporarily deployed to review_aks-10160 December 11, 2024 11:08 Destroyed
@github-actions github-actions bot temporarily deployed to review_aks-10160 December 11, 2024 11:23 Destroyed
@github-actions github-actions bot temporarily deployed to review_aks-10160 December 18, 2024 11:27 Destroyed
@github-actions github-actions bot temporarily deployed to review_aks-10160 December 18, 2024 11:46 Destroyed
Copy link
Collaborator

@dcyoung-dev dcyoung-dev left a comment

Choose a reason for hiding this comment

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

Nice one - I've highlighted a few name changes but other than that this is good to go 👌

app/controllers/one_login_controller.rb Show resolved Hide resolved
lib/omniauth/one_login_setup.rb Outdated Show resolved Hide resolved
spec/requests/one_login_controller_spec.rb Outdated Show resolved Hide resolved
spec/support/test_helpers/one_login_helper.rb Outdated Show resolved Hide resolved
spec/support/test_helpers/one_login_helper.rb Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to review_aks-10160 December 18, 2024 13:28 Destroyed
@github-actions github-actions bot temporarily deployed to review_aks-10160 December 18, 2024 13:45 Destroyed
We want to move our candidates to use one login when sigin up/in to our
service.

This commit adds the basic implementation for users to login or create
a candidate account in apply using one login.

It also adds a bypass for review and local development
The smoke test needs to be smarter now that we will introduce one login.

We need to check if one login is enabled but we don't have control over
the host app, as we use a remote host, when we deploy to review we use
the review app host, same with deploying to other envs.

This commit tries to make the smoke test smarted and tests one login or
magic link depending on what is the sign up method on the host.
@github-actions github-actions bot temporarily deployed to review_aks-10160 December 18, 2024 14:04 Destroyed
@CatalinVoineag CatalinVoineag merged commit c3d741c into main Dec 18, 2024
24 of 25 checks passed
@CatalinVoineag CatalinVoineag deleted the cv/one-login-auth branch December 18, 2024 14:14
Copy link

sentry-io bot commented Dec 18, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ One login failure with ActionController::InvalidAuthenticityToken for one_login_id_token: OneLoginController#sign_out_complete View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deploy_v2 Deploy the review app to AKS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants