MNTOR-3353: add refresh token logic to session management #4777

mansaj · 2024-07-08T18:50:16Z

References:

Jira: MNTOR-3353

Description

Adding refresh token fetching logic to next-auth
The expiry / tokens are stored in the db

Test

Test for refresh token

run db:migration and make sure the fxa_session_expiry col exists
If the column is empty, login and see that the session expiry should be an hour in the future
Change the time to be some time in the past OR set it to null
Go to dashboard, watch the session refresh, come back to the table and check on the session field again, it should have refreshed
The access token should have also been refreshed

Test for refresh fail

empty out the columns fxa_session_expiry, fxa_access_token, and fxa_refresh_token columns in the subscribers table
navigate to either dashboard or settings
it should automatically log the user out and prompt a relogin

Checklist (Definition of Done)

Commits in this PR are minimal and have descriptive commit messages.
I've added or updated the relevant sections in readme and/or code comments
All acceptance criteria are met.
Jira ticket has been updated (if needed) to match changes made during the development process.
Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

mansaj · 2024-07-08T18:54:51Z

One thing I'm trying to figure out is where to add the client side of the session error to force re-sign in.
I'm thinking maybe in the dashboard (and settings), have a useSession and try to detect that error. If I see the error, then redirect?

github-actions · 2024-07-08T19:05:59Z

Preview URL 🚀 : https://blurts-server-pr-4777-mgjlpikfea-uk.a.run.app

src/app/api/utils/auth.ts

src/db/tables/subscribers.js

src/utils/fxa.js

Vinnl

Could you add a "How to test" section? I'm unsure how to tell now whether this works as intended.

The most important thing that I think needs changing is doing this check when constructing the JWT, rather than when validating the session, since the latter happens pretty often.

src/app/api/utils/auth.ts

src/db/tables/subscribers.js

Vinnl · 2024-07-09T12:15:21Z

src/db/tables/subscribers.js

+ */
+// Not covered by tests; mostly side-effects. See test-coverage.md#mock-heavy
+/* c8 ignore start */
+async function updateFxATokens (subscriber, fxaAccessToken, fxaRefreshToken, sessionExpiredAt) {


I'm a bit confused by the significant overlap between this function and updateFxAData - is that one used in similar situations as this one? Should it be? It's also calling destroyOAuthToken, should this function do that too?

Could you add some comments to the code to prevent such confusion from future readers?

src/db/tables/emailAddresses.js

src/app/api/utils/auth.ts

Vinnl · 2024-07-09T13:02:24Z

src/app/api/utils/auth.ts

+        const dbFxATokens = await getFxATokens(token.subscriber.id);
+        if (
+          !dbFxATokens.fxa_session_expiry ||
+          dbFxATokens.fxa_session_expiry.getTime() < Date.now()


Maybe we should try rotating it if it expires soon, rather than when it has already expired? Since sessions have a lifetime as well in which the token can expire.

Suggested change

dbFxATokens.fxa_session_expiry.getTime() < Date.now()

dbFxATokens.fxa_session_expiry.getTime() < (Date.now() - 5 * 60 * 1000)

I don't know if we should artificially set a time (not really a best practice)
I think the point is to let sessions expire and potentially force a refresh or relogin

I guess the question is: how do we do that? Are we going to refresh the session every time an API request to FxA fails with the relevant status code? Since this callback won't necessarily automatically get called when the session expires.

I guess the question is: how do we do that? Are we going to refresh the session every time an API request to FxA fails with the relevant status code? Since this callback won't necessarily automatically get called when the session expires.

Yes I am wondering that too... I'm not sure what is going to trigger this session callback if e.g. a fetch to some API (like subscriptions details) fails.

Vinnl · 2024-07-09T13:08:12Z

src/app/api/utils/auth.ts

@@ -209,7 +213,7 @@ export const authOptions: AuthOptions = {
      }
      return token;
    },
-    session({ session, token }) {
+    async session({ session, token }) {


Shouldn't refreshing the access token happen in the jwt callback? As I understand it, the session callback gets called pretty often, so hitting the database there can cause pretty heavy load. And the JWT would still contain the expired access token.

So there are two approaches to this. The database route is a bit heavy, but it's also a bit safer. It's outlined in the next-auth doc.

We've traditionally opted for storing the sessions etc the database (prior to moving to next-auth). We were also using redis for session management. I think it should be an either-or situation

Oh yeah, I'm not saying not going the database route, but doing it less often? I think the session gets refreshed whenever the user switches tabs, or about every minute when the user has a tab open, at least when an app is using the JWT session strategy - which Monitor does.

We could consider switching to the database session strategy, in which case I think the session callback will be called less often and doing the refresh there makes sense, but if we don't do that, I'd be afraid we'll be hammering our DB too much. But I'm not a back-end engineer, so I'll defer to your judgment :)

Co-authored-by: Vincent <Vinnl@users.noreply.github.com>

rhelmer

Sounds like from talking this morning that you're going to investigate switching over to the database strategy - I think that will make things a lot clearer, since we're sort-of using both strategies right now.

rhelmer · 2024-07-10T23:13:08Z

src/app/api/utils/auth.ts

+        const dbFxATokens = await getFxATokens(token.subscriber.id);
+        if (
+          !dbFxATokens.fxa_session_expiry ||
+          dbFxATokens.fxa_session_expiry.getTime() < Date.now()


I guess the question is: how do we do that? Are we going to refresh the session every time an API request to FxA fails with the relevant status code? Since this callback won't necessarily automatically get called when the session expires.

Yes I am wondering that too... I'm not sure what is going to trigger this session callback if e.g. a fetch to some API (like subscriptions details) fails.

…into MNTOR-3353

src/app/functions/server/applyCoupon.ts

src/app/functions/server/checkSession.ts

src/utils/fxa.js

mansaj · 2024-07-16T16:09:47Z

@rhelmer one thing we'd want to do in stage is to load test this for database connection (we will be checking db pretty frequently with the session changes). I'm optimistic that most of it should just be get and not update but it'd be nice to see

github-actions · 2024-07-18T22:40:16Z

Cleanup completed - database 'blurts-server-pr-4777' destroyed, cloud run service 'blurts-server-pr-4777' destroyed

mansaj added 2 commits July 3, 2024 14:49

feat: add refresh token logic to session management

d6678f1

Merge branch 'main' into MNTOR-3353

e647e4e

mansaj requested review from rhelmer and Vinnl July 8, 2024 18:53

pdehaan reviewed Jul 8, 2024

View reviewed changes

Vinnl reviewed Jul 9, 2024

View reviewed changes

mansaj and others added 7 commits July 9, 2024 09:02

Merge branch 'main' into MNTOR-3353

500656e

fix: expires instead of expired

4d6ce09

fix: spelling of OAuth

5d08738

Apply suggestions from code review

5894d5e

Co-authored-by: Vincent <Vinnl@users.noreply.github.com>

fix: first instead of select

cf043d2

fix: review comment

112cacd

fix: potential null case

04a001d

rhelmer requested changes Jul 10, 2024

View reviewed changes

mansaj added 13 commits July 10, 2024 17:13

Merge branch 'main' into MNTOR-3353

6f59570

fix: cannot use first() on update()

10716ea

Merge branch 'main' into MNTOR-3353

5a9a692

Merge branch 'MNTOR-3353' of https://github.com/mozilla/blurts-server …

e78761b

…into MNTOR-3353

Merge branch 'main' into MNTOR-3353

60e579e

Merge branch 'MNTOR-3353' of https://github.com/mozilla/blurts-server …

dc3663c

…into MNTOR-3353

feat: log out for dashboard and settings when session expires

f0adc63

Merge branch 'main' into MNTOR-3353

5e3912d

fix: smoke test

ad857b9

Merge branch 'main' into MNTOR-3353

6e7f237

fix: update secret

ab17972

fix: overwrite account uri

d8f7bb3

fix: revert

517cbad

mansaj requested a review from rhelmer July 15, 2024 22:30

revert: fix

bfdd3d5

rhelmer requested changes Jul 16, 2024

View reviewed changes

src/app/functions/server/applyCoupon.ts Outdated Show resolved Hide resolved

src/app/functions/server/checkSession.ts Outdated Show resolved Hide resolved

src/utils/fxa.js Show resolved Hide resolved

mansaj added 2 commits July 16, 2024 08:55

fix: review comments

14614a8

Merge branch 'main' into MNTOR-3353

99f5c50

mansaj requested a review from rhelmer July 16, 2024 16:36

mansaj added 3 commits July 17, 2024 07:57

Merge branch 'main' into MNTOR-3353

0a6769a

Merge branch 'main' into MNTOR-3353

f8f5042

Merge branch 'main' into MNTOR-3353

69777db

rhelmer approved these changes Jul 18, 2024

View reviewed changes

mansaj added 2 commits July 18, 2024 08:14

Merge branch 'main' into MNTOR-3353

8f57726

Merge branch 'main' into MNTOR-3353

fd1d902

mansaj merged commit fe5d69e into main Jul 18, 2024
15 checks passed

mansaj deleted the MNTOR-3353 branch July 18, 2024 22:38

rhelmer mentioned this pull request Jul 19, 2024

Revert "MNTOR-3353: add refresh token logic to session management" #4834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNTOR-3353: add refresh token logic to session management #4777

MNTOR-3353: add refresh token logic to session management #4777

mansaj commented Jul 8, 2024 •

edited

Loading

mansaj commented Jul 8, 2024

github-actions bot commented Jul 8, 2024

Vinnl left a comment

Vinnl Jul 9, 2024

Vinnl Jul 9, 2024

mansaj Jul 9, 2024

Vinnl Jul 10, 2024

rhelmer Jul 10, 2024

Vinnl Jul 9, 2024

mansaj Jul 9, 2024

Vinnl Jul 10, 2024 •

edited

Loading

rhelmer left a comment

rhelmer Jul 10, 2024

mansaj commented Jul 16, 2024

github-actions bot commented Jul 18, 2024

	dbFxATokens.fxa_session_expiry.getTime() < Date.now()
	dbFxATokens.fxa_session_expiry.getTime() < (Date.now() - 5 * 60 * 1000)

MNTOR-3353: add refresh token logic to session management #4777

MNTOR-3353: add refresh token logic to session management #4777

Conversation

mansaj commented Jul 8, 2024 • edited Loading

References:

Description

Test

Test for refresh token

Test for refresh fail

Checklist (Definition of Done)

mansaj commented Jul 8, 2024

github-actions bot commented Jul 8, 2024

Vinnl left a 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

Vinnl Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

rhelmer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansaj commented Jul 16, 2024

github-actions bot commented Jul 18, 2024

mansaj commented Jul 8, 2024 •

edited

Loading

Vinnl Jul 10, 2024 •

edited

Loading