OAuth migration | Core OAuth authentication middleware#1223
Merged
Conversation
raphaelkabo
force-pushed
the
rk/oauth-middleware
branch
2 times, most recently
from
October 20, 2023 13:23
e042e6d to
f231bda
Compare
raphaelkabo
force-pushed
the
rk/oauth-middleware
branch
from
October 20, 2023 13:25
f231bda to
1df44cc
Compare
raphaelkabo
marked this pull request as ready for review
October 20, 2023 13:27
coldlink
previously requested changes
Oct 23, 2023
coldlink
left a comment
Member
There was a problem hiding this comment.
looking good so far from my side, a few changes required though i think
pvighi
reviewed
Oct 24, 2023
pvighi
reviewed
Oct 25, 2023
We do not need to generate IDAPI cookies by making a separate call to IDAPI as part of the OAuth flow because Gateway already generates these for us. However, in DEV, we have been pointing MMA to CODE Gateway during the OAuth flow, which means that the cookies have been inaccessible from DEV MMA. Changes: - Remove the code to generate IDAPI cookies - Point MMA DEV to Gateway DEV for OAuth - Create a dev OpenID client which correctly sets and checks the access/ID tokens, which are set to validate against CODE Okta, despite running in DEV.
raphaelkabo
force-pushed
the
rk/oauth-middleware
branch
from
October 25, 2023 13:05
ad0be7b to
90d7231
Compare
|
Overdue on PROD (merged by @raphaelkabo 15 minutes and 6 seconds ago) What's gone wrong? |
|
Seen on PROD (merged by @raphaelkabo 1 hour, 16 minutes and 17 seconds ago) Please check your changes! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR migrates the authentication middleware used by the MMA Express server from 'classic' (calling IDAPI's
/auth/redirectendpoint on every request) to Okta-powered OAuth2.Warning
The new OAuth mechanism this PR introduces is disabled by setting
useOAuth: falsein theokta-STAGE.jsonconfig file stored in S3. It should remain disabled until the upcoming 'server-side Okta auth' PR is merged in, because without that PR, if OAuth were enabled, we would be introducing a security vulnerability in accessing certain APIs. We want to merge this PR into main though, to prevent needing to keep it updated with main and to spot any hidden issues early in testing. While theuseOAuthflag is set tofalse, the authentication behaviour will be identical to how it has always worked.Changes
@okta/jwt-verifier(to verify OAuth tokens),openid-client(to set up an OIDC client in MMA), andms(because counting is hard and that's why we have computers).withIdentityauthentication middleware now calls one of two functions:authenticateWithIdapi, which is the old middleware, orauthenticateWithOAuth, which is the new middleware. This is controlled by auseOktaboolean in the Okta config file stored in S3 and retrieved byoktaConfig.ts.useOAuth. (This is okay because MMA sessions are only valid for 30 minutes anyway).useOAuthis true, we're still dual-running this migration with the classic authorization behaviour for API endpoints handled by MMA. WhenuseOAuthis true, users will authenticate via OAuth, but all API calls will still be authorized with the classic IDAPI cookies./oauth/callback, the callback route to which the browser returns after the user gets a valid session.Huge thanks to @coldlink (I borrowed a lot of the OAuth code from his work in Gateway) and @pvighi, who both helped a lot with bringing this PR together.
SC_GU_U,SC_GU_LA,GU_UGU_ACCESS_TOKEN,GU_ID_TOKEN,GU_SO,SC_GU_U,SC_GU_LA,GU_UGU_ACCESS_TOKEN,GU_ID_TOKEN,GU_SO,GU_UwithOAuth: false: SC_GU_LA generated in the last 30 minuteswithOAuth: true: OAuth tokens which have expiry of 30 minuteswithOAuth: false: Calls IDAPI to validate cookieswithOAuth: true: After local validation, calls Okta to validate tokens for API-token-based API routeswithOAuth: true: Locally validates OAuth tokensA quick recap of the new authentication flow:
GU_SOcookie, we need to log the user out. We delete the access and ID token cookies and redirect the user to Gateway for authorization./oauth/callback. Here we exchange the token we receive for access and ID tokens from Okta, and call IDAPI to give us a fresh set of classic IDAPI cookies.Reviews and testing
It might be easiest to go through this PR commit-by-commit, which I've done my best to make easy to do. There are a couple of support commits, one large commit introduces the two parts of the OAuth middleware flow, and then a commit with a suite of Jest tests.
We'll test this by deploying it on CODE and running some manual tests, as noted in this testing plan. Then we'll do the same on PROD.
Remaining work
maxAgeparameter to force reauth (Handle maxAge parameter gateway#2444)clientIdwhen validating access and ID tokensRelated PRs