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

OAuth migration | Force user to sign in if Access token auth_time older than 30 minutes #1300

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

raphaelkabo
Copy link
Contributor

@raphaelkabo raphaelkabo commented Jan 22, 2024

Changes

Context

After deploying #1294 and enabling Okta on PROD manage-frontend, we discovered an Okta behaviour which was undocumented in the sense that it was only documented in a modal in the UI, three layers down, after you clicked a specific radio button, and the actual documentation completely contradicted it:

Screenshot 2024-01-29 at 12 09 57

One is reminded of a quote from The Hitchhiker's Guide To The Galaxy:

“But the plans were on display…”
“On display? I eventually had to go down to the cellar to find them.”
“That’s the display department.”
“With a flashlight.”
“Ah, well, the lights had probably gone.”
“So had the stairs.”
“But look, you found the notice, didn’t you?”
“Yes,” said Arthur, “yes I did. It was on display in the bottom of a locked filing cabinet stuck in a disused lavatory with a sign on the door saying ‘Beware of the Leopard.”

What this means in practice is that, when passing the max_age parameter when requesting OAuth tokens:

  • Regular email/password users will be forced to sign in again after max_age
  • Users of the FEDERATION or SOCIAL types (i.e. social login users who have never set a password) will never be forced to sign in again as long as their global Okta session is not expired. This is probably most of our social users.

This behaviour means that we can't reliably enforce the MMA behaviour where a user must reauthenticate if they last authenticated more than 30 minutes ago using Okta settings alone.

For this reason, this PR adds the following check into identityMiddleware:

  • Does the user's ID token have an auth_time claim longer than 30 minutes in the past?
  • If the claim is older than 30 minutes, we:
    • Call the Okta OAuth API to revoke the associated Access token
    • Delete the ID and Access token cookies
    • Redirect the browser to the Gateway /reauthenticate route. This route always shows a signin form, irrespective of any session set in the browser. This essentially replicates the previous, IDAPI behaviour in MMA.
  • If the claim is newer than 30 minutes, we proceed with the rest of the middleware.
  • For routes which don't require sign in, if the ID token is older than 30 minutes, the user is designated signedInNotRecently but allowed to proceed to the route. This again replicates the old IDAPI behaviour.

The auth_time claim

According to the OpenID spec, and confirmed by Okta, the auth_time claim is "Time when the End-User authentication occurred" in UNIX seconds, which in Okta land means the last time the user actively authenticated, i.e. created their Okta session. The OpenID spec states that auth_time is returned whenever the max_age parameter is sent in the OAuth request, which in MMA is always. The Okta docs state that auth_time is a 'base claim', always returned. We've added logging in the check for auth_time which will give us data on missing auth_time claims, if any.

Further reading for nerds here:

Other changes

  • We add end-to-end testing for the auth_time behaviour, which sets the Okta max_age to a very short value and attempts to refresh the page, expecting to be redirected.
  • We add unit tests for the new utility functions.
  • There's a fair amount of refactoring to improve the middleware - rather than various utility functions fetching their own Okta config object async, the top-level middleware does this once and passes it down the chain. This also required a lot of refactoring in tests.
  • We now redirect identityMiddleware error states to a new frontend error page, /sign-in-error, which shows a more helpful message and a link to sign out, which is likely to resolve edge-case sign in issues:
Screenshot 2024-01-29 at 15 39 12

Testing

  • Tested on CODE (Okta disabled)
  • Tested on CODE (Okta enabled)
  • Tested on PROD (Okta disabled)
  • Tested on PROD (Okta enabled)

@raphaelkabo raphaelkabo changed the title OAuth migration | Force login if Access token auth_time older than 30 minutes OAuth migration | Force login if Access token auth_time older than 30 minutes Jan 22, 2024
@raphaelkabo raphaelkabo changed the title OAuth migration | Force login if Access token auth_time older than 30 minutes OAuth migration | Force user to sign in if Access token auth_time older than 30 minutes Jan 22, 2024
This is a workaround for an Okta behaviour where
some user types won't be asked to reauthenticate
even when max_age is set on the OAuth request. We
check the auth_time of the ID token ourselves
in the identity middleware, ensuring that it is
within max_age. If it isn't, we revoke the access
token, clear the token cookies, and redirect the
browser to the Gateway /reauthenticate endpoint,
which always shows a sign-in form.
@raphaelkabo raphaelkabo force-pushed the rk/okta-validate-auth-time branch from 6ef9a3d to d94524e Compare January 29, 2024 12:32
@raphaelkabo raphaelkabo marked this pull request as ready for review January 29, 2024 12:32
@raphaelkabo raphaelkabo merged commit c57f6c9 into main Jan 30, 2024
11 checks passed
@raphaelkabo raphaelkabo deleted the rk/okta-validate-auth-time branch January 30, 2024 09:38
@prout-bot
Copy link
Collaborator

Overdue on PROD (merged by @raphaelkabo 15 minutes and 4 seconds ago) What's gone wrong?

@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @raphaelkabo 46 minutes and 52 seconds ago) Please check your changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants