One login db session #10203

CatalinVoineag · 2024-12-20T16:34:02Z

Context

In testing the one login integration, we discovered that having session backed
login is not ideal. There is a risk that the session gets too big, over 5000
bytes. To fix this, we are switching to Database backed sessions.

This is inspired by the rails 8 new db backed sessions.
Most of the logic is in this concern Authentication using this and before
filters in controllers we control the one login mechanism.

We will use DB backed session for the normal candidate flow. Impersonation will
still use cookie session.

This means that deleting the session from the DB logs out the user.

Switching between one login and magic link auth system will be seamless as we
always check current_candidate.

We will still save some data in the session but it will be very little,
just ids rather than big tokens.

The session size decreased from roughly 3224 to 600

Screencast.2025-01-02.14.23.36.mp4

Changes proposed in this pull request

Migrations, models and controllers

Guidance to review

Go on QA and test the integration. It should work like before.

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 included inform data insights team of the changes
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?
Attach the PR to the Trello card

CatalinVoineag · 2024-12-20T16:34:40Z

https://trello.com/c/CVQruu2c/534-switch-to-db-backed-sessions-for-one-login?filter=member:catalinvoineag1

CatalinVoineag · 2025-01-02T10:30:27Z

app/controllers/concerns/authentication.rb

+  end
+
+  def require_authentication
+    current_candidate || resume_session || request_authentication


current_candidate is here to make the one login feature flag activation seamless. So if a candidate is loged in with magic link, they will still be logged in after switching the one login on.
The reverse is not true, if we switch off one login feature flag, we will log out the current candidates
It's also here for impersonation. Current_candidate will be used for impersonation.
So db backed session is used for candidates but support users impersonation will still be using cookies/session

CatalinVoineag · 2025-01-02T10:31:44Z

app/controllers/one_login_controller.rb

  before_action :redirect_to_candidate_sign_in_unless_one_login_enabled
+  allow_unauthenticated_access# only: %i[callback bypass_callback]


We need to allow requests to these endpoints without requiering authentication to be able to login the candidate and also log out the candidate by just calling these endpoints.
There are cases where the user is logged in in one admin but not in our system because of an error in our system, so we need to be able to call sign-out endpoint to log them out of one login

CatalinVoineag · 2025-01-02T10:32:11Z

app/controllers/one_login_controller.rb

-    redirect_to auth_one_login_sign_out_path
+  rescue StandardError => e
+    # We can't attach an error message to the session because in some cases we don't create a session if there's an error
+    session_error = SessionError.create!(


We can't attach an error message to the session because in some cases we don't create a session if there's an error

CatalinVoineag · 2025-01-02T13:44:31Z

https://trello.com/c/CVQruu2c/534-switch-to-db-backed-sessions-for-one-login?filter=member:catalinvoineag1

CatalinVoineag · 2025-01-02T14:28:10Z

config/brakeman.ignore

@@ -115,6 +115,29 @@
      ],
      "note": ""
    },
+    {


Brakeman raises these warning but I don't think there's really a risk here since we have total control over what's happening

CatalinVoineag · 2025-01-02T16:13:43Z

We need to think about session expiry, we will have old sessions in our DB without implementing an expiration feature

In testing the one login integration, we discovered that having session backed login is not ideal. There is a risk that the session gets too big, over 5000 bytes. To fix this, we are switching to Database backed sessions. This is inspired by the rails 8 new db backed sessions. Most of the logic is in this concern `Authentication` using this and before filters in controllers we control the one login mechanism. We will use DB backed session for the normal candidate flow. Impersonation will still use cookie session. This means that deleting the session from the DB logs out the user. Switching between one login and magic link auth system will be seamless as we always check `current_candidate`. We will still save some data in the session but it will be very little, just ids rather than big tokens. The session size decreased from roughly 3224 to 600

elceebee · 2025-01-03T14:58:55Z

Nice one! I'm really glad we'll have db backed sessions from now.

As mentioned elsewhere, we are missing a session expiry strategy. But I'm happy for this to go in as it is, behind a feature flag, in the meanwhile.

dcyoung-dev

Nice work 👌 I love that this matches the Rails authentication 😍

I echo @elceebee concerns about expiring session but this can be added later.

It will be worth monitoring this in environments where OneLogin is in use before going to production

CatalinVoineag self-assigned this Dec 20, 2024

CatalinVoineag temporarily deployed to review_aks December 20, 2024 16:34 — with GitHub Actions Inactive

CatalinVoineag force-pushed the cv/one-login-db-session branch from c5f1f1a to 46f139e Compare January 2, 2025 10:11

CatalinVoineag temporarily deployed to review_aks January 2, 2025 10:11 — with GitHub Actions Inactive

CatalinVoineag commented Jan 2, 2025

View reviewed changes

CatalinVoineag temporarily deployed to review_aks January 2, 2025 12:13 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to qa_aks January 2, 2025 12:52 — with GitHub Actions Inactive

CatalinVoineag force-pushed the cv/one-login-db-session branch from 9fcd501 to 199728f Compare January 2, 2025 13:48

CatalinVoineag temporarily deployed to review_aks January 2, 2025 13:48 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to qa_aks January 2, 2025 14:09 — with GitHub Actions Inactive

CatalinVoineag requested a review from a team January 2, 2025 14:24

CatalinVoineag marked this pull request as ready for review January 2, 2025 14:24

CatalinVoineag commented Jan 2, 2025

View reviewed changes

CatalinVoineag temporarily deployed to review_aks January 2, 2025 16:56 — with GitHub Actions Inactive

CatalinVoineag force-pushed the cv/one-login-db-session branch from 5e0adfc to 97776e2 Compare January 3, 2025 10:39

CatalinVoineag temporarily deployed to review_aks January 3, 2025 10:39 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to review_aks January 3, 2025 10:47 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to qa_aks January 3, 2025 10:59 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to qa_aks January 3, 2025 11:02 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to review_aks January 3, 2025 11:04 — with GitHub Actions Inactive

DFE-Digital deleted a comment from elceebee Jan 3, 2025

CatalinVoineag force-pushed the cv/one-login-db-session branch from 80ca8c1 to 0fd57eb Compare January 3, 2025 14:07

CatalinVoineag temporarily deployed to review_aks January 3, 2025 14:07 — with GitHub Actions Inactive

CatalinVoineag force-pushed the cv/one-login-db-session branch from 0fd57eb to 1adb434 Compare January 3, 2025 14:09

CatalinVoineag temporarily deployed to qa_aks January 3, 2025 14:12 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to review_aks January 3, 2025 14:20 — with GitHub Actions Inactive

CatalinVoineag temporarily deployed to qa_aks January 3, 2025 14:31 — with GitHub Actions Inactive

elceebee approved these changes Jan 3, 2025

View reviewed changes

dcyoung-dev approved these changes Jan 6, 2025

View reviewed changes

CatalinVoineag merged commit a796746 into main Jan 6, 2025
25 checks passed

CatalinVoineag deleted the cv/one-login-db-session branch January 6, 2025 11:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

One login db session #10203

One login db session #10203

CatalinVoineag commented Dec 20, 2024 •

edited

Loading

CatalinVoineag commented Dec 20, 2024

CatalinVoineag Jan 2, 2025

CatalinVoineag Jan 2, 2025

CatalinVoineag Jan 2, 2025

CatalinVoineag commented Jan 2, 2025

CatalinVoineag Jan 2, 2025

CatalinVoineag commented Jan 2, 2025 •

edited

Loading

elceebee commented Jan 3, 2025

dcyoung-dev left a comment

		before_action :redirect_to_candidate_sign_in_unless_one_login_enabled
		allow_unauthenticated_access# only: %i[callback bypass_callback]

One login db session #10203

One login db session #10203

Conversation

CatalinVoineag commented Dec 20, 2024 • edited Loading

Context

Changes proposed in this pull request

Guidance to review

Things to check

CatalinVoineag commented Dec 20, 2024

CatalinVoineag Jan 2, 2025

Choose a reason for hiding this comment

CatalinVoineag Jan 2, 2025

Choose a reason for hiding this comment

CatalinVoineag Jan 2, 2025

Choose a reason for hiding this comment

CatalinVoineag commented Jan 2, 2025

CatalinVoineag Jan 2, 2025

Choose a reason for hiding this comment

CatalinVoineag commented Jan 2, 2025 • edited Loading

elceebee commented Jan 3, 2025

dcyoung-dev left a comment

Choose a reason for hiding this comment

CatalinVoineag commented Dec 20, 2024 •

edited

Loading

CatalinVoineag commented Jan 2, 2025 •

edited

Loading