MCR-3849 Fix session timeout #2719

haworku · 2024-09-03T20:44:47Z

Summary

Address issues with session timeout and re-implement with IdleTimer .

Issues addressed include:

logout and continue work across tabs
if one tab has session timeout modal open then user opens another tab and then lets computer go to sleep, both tabs will be logged out of MC-Review after time elapses with no action
if user tries to add a document with file upload but it fails due to IDM having been logged out in the background for whatever reason, MC-Review will also force logout at this point and display session timeout message so the user knows what happened

Still in progress

write up acceptance jams steps
finish adding AuthenticatedRouteWrapper tests for idle timer behaviors

Related issues

https://jiraent.cms.gov/browse/MCR-3849

Screenshots

Test cases covered

Session Timeout acceptance jam document forthcoming

QA guidance

Please re-test the steps from this comment in Jira.

And also pick a couple of these acceptance jam cases to retest. Keep in mind the timeout for toph in review apps is set to 2min to make things go faster so you won't have to wait as long to verify things.

- inherit from base Modal - create SessionTimeoutModal as own component - simplify styles - removing V2 naming from UnlockResubmitModal

- add to package.json - remove session timeout custom timekeeping, props from AuthContext - add IdleTimer to AuthenticatedRouteWrapper - start to add tests

…incompatible values

pearl-truss · 2024-09-05T16:10:07Z

@haworku for "logout and continue work across tabs", is this the desired behavior? When I logged out of one tab I was still able to do work in another tab

haworku · 2024-09-05T21:59:19Z

@pearl-truss No, that's the buggy behavior that we want to test for and hopefully we don't see it.

Can you send me the replication steps you used and which user in the review app you were using (toph currently has shorter session duration)

pearl-truss · 2024-09-06T15:41:13Z

@pearl-truss No, that's the buggy behavior that we want to test for and hopefully we don't see it.

Can you send me the replication steps you used and which user in the review app you were using (toph currently has shorter session duration)

I was using aang.

I signed into 2 tabs
I signed out of one tab (it wasn't a session time out, I explicitly signed out)
I used the remaining open tab to view a draft submission

haworku · 2024-09-06T18:20:22Z

@pearl-truss TY for finding this! I added to acceptance testing script under a new section called basic login and logout.

FYI from discussion in standup today, looks like this one is an existing bug not a new regression, and not in scope for this ticket. The bug shouldn't block this PR going through. However, I will still look into solving it also on this branch in case I can fix now, will ping you if I do so.

If you see any other issues, please raise them – this manual testing is very helpful.

macrael

Love getting to replace custom code with a solid library. I have some questions about us throwing errors we get from Amplify but otherwise this looks great

macrael · 2024-09-11T17:00:00Z

services/app-web/src/contexts/S3Context.tsx

+            // s3 file upload failing could be due to IDM session timeout
+            // double check the user still has their session, if not, logout to update the React state with their login status
+            try {
+                await checkAuth()


why does checkAuth throw instead of return an error?

macrael · 2024-09-11T17:00:24Z

services/app-web/src/components/Modal/SessionTimeoutModal.tsx

+    const [countdownSeconds, setCountdownSeconds]= useState(idleTimer.getRemainingTime() / 1000)
+
+    const handleLogoutSession = async () => {
+        idleTimer.message({action:'LOGOUT_SESSION'}, true)


could these actions be constants?

macrael · 2024-09-11T17:03:12Z

services/app-web/src/pages/Auth/cognitoAuth.ts

        }
    }
+    throw Error('Cognito SignIn error')


It looks like we used to return errors when it was an error we understood and only threw when it was something truly unexpected. I don't love turning this into something that always throws. Honestly I think since writing this we've moved towards just returning un-understood errors as well

Yes agreed I can do that

@macrael Actually looking more at this code - all of cognitoAuth throws for errors. The oddity was returning both Errors and throwing and thats why I moved it to throw to unify it and unify that interface on the frontend. What do you want to do here? Shall I make a ticket to refactor cognitoAuth?

Shall I just change this one still?

If it's not going to change having to do try/catch in our pages then I don't care if you fix this one or not, but I would like to have a ticket tracking moving this to returning errors instead. My goal is that inside any of our pages/components we never have to call try/catch because anything that can throw has been wrapped up as a dependency

Ended up rewriting cognitoAuth anyway - but there are other files that have the same issue that I left alone. Looks like we already have a ticket to address this issue - I updated it to also include adding more eslint rules.

…ous cases

pearl-truss · 2024-09-16T15:34:45Z

@haworku on the review app as soon as I signed in I got a session timeout modal.

ETA But I was using toph...let me try with someone else

pearl-truss · 2024-09-16T15:45:23Z

@haworku nevermind my other comment! Everything looks good if this is okay: I put my computer to sleep and when I came back to MC-Review the session timeout modal had counted down to 0:00 but stayed on the page without automatically signing me out.

…to machine sleep

haworku · 2024-09-16T17:06:35Z

@pearl-truss Thanks for catching this. It really should be logging out, I see handling in the library for this too. However, I also see open issues about what happens when computer sleep when promptBeforeIdle is in use that could be at play.

For now I added handling to modal itself to disable continue and only allow logout in cases where the countdown finishes but for some reason it does not close and logout.
Screenshot 2024-09-16 at 11 59 36 AM

What do you think?

haworku changed the title ~~Mcr 3849 sessiontimeout~~ MCR-3849 Fix session timeout Sep 3, 2024

github-actions bot requested a deployment to review-apps September 3, 2024 21:37 In progress

haworku added 8 commits September 4, 2024 15:18

Bring back changes from PR #2159

e4fb419

Cleanup modals

68e208e

- inherit from base Modal - create SessionTimeoutModal as own component - simplify styles - removing V2 naming from UnlockResubmitModal

Add IdleTimer

d2b2cc1

- add to package.json - remove session timeout custom timekeeping, props from AuthContext - add IdleTimer to AuthenticatedRouteWrapper - start to add tests

Get existing tests passing

853c403

Add more mocked out tests

e7dabb3

Handle cases where timeout countdown and session duration are set to …

f81dc2a

…incompatible values

Fix countdown

618c367

Add cross-tab support

57b658c

haworku force-pushed the MCR-3849-sessiontimeout branch from 372144f to 57b658c Compare September 4, 2024 20:20

github-actions bot requested a deployment to review-apps September 4, 2024 20:23 In progress

Get s3Context to manually check auth if it fails and user logged out

18af39d

github-actions bot requested a deployment to review-apps September 4, 2024 21:11 In progress

haworku marked this pull request as ready for review September 4, 2024 21:14

Fixup AuthneticatedRouteWrapper tests

b0dbf52

github-actions bot deployed to review-apps September 4, 2024 21:25 View deployment

haworku requested review from JasonLin0991, macrael, mojotalantikite and pearl-truss September 4, 2024 21:36

Merge remote-tracking branch 'origin/main' into MCR-3849-sessiontimeout

793a5a1

Write more unit tests, mock out more tests as well to write

8b3460b

github-actions bot requested a deployment to review-apps September 6, 2024 18:01 In progress

haworku added 2 commits September 10, 2024 20:17

Get only one modal at a time behavior working

fc707b5

Get only one modal at a time behavior working

6ed777b

github-actions bot requested a deployment to review-apps September 11, 2024 01:18 In progress

Add more async handling - try to address CI only unit test flakes

7170c15

github-actions bot requested a deployment to review-apps September 11, 2024 12:48 In progress

Lint cleanup

33e059a

github-actions bot deployed to review-apps September 11, 2024 15:58 View deployment

macrael approved these changes Sep 11, 2024

View reviewed changes

haworku added 2 commits September 12, 2024 11:16

Get doubleclick bug addressed

0dd47e3

Merge remote-tracking branch 'origin/main' into MCR-3849-sessiontimeout

b511302

github-actions bot deployed to review-apps September 12, 2024 16:19 View deployment

haworku added 2 commits September 13, 2024 09:49

Code review comments and cleanup

7700079

Refactor cognitoAuth to return Errors, unify logout handling for vari…

ad846e8

…ous cases

github-actions bot deployed to review-apps September 16, 2024 01:02 View deployment

merge main

2a37d3b

github-actions bot deployed to review-apps September 16, 2024 01:29 View deployment

make modal close more snappy

b806f8a

github-actions bot deployed to review-apps September 16, 2024 01:48 View deployment

cleanup

a312146

github-actions bot deployed to review-apps September 16, 2024 14:51 View deployment

Don't allow continue session if timer elapsed (but logout failed due …

3037cc8

…to machine sleep

github-actions bot deployed to review-apps September 16, 2024 16:52 View deployment

Also update copy when session expired but modal still open

cece00b

github-actions bot deployed to review-apps September 16, 2024 17:08 View deployment

haworku merged commit 31bba04 into main Sep 16, 2024
28 checks passed

haworku deleted the MCR-3849-sessiontimeout branch September 16, 2024 18:08

haworku mentioned this pull request Sep 26, 2024

Session timeout adjustments - try to address sleep behavior #2786

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MCR-3849 Fix session timeout #2719

MCR-3849 Fix session timeout #2719

haworku commented Sep 3, 2024 •

edited

Loading

pearl-truss commented Sep 5, 2024

haworku commented Sep 5, 2024 •

edited

Loading

pearl-truss commented Sep 6, 2024

haworku commented Sep 6, 2024 •

edited

Loading

macrael left a comment

macrael Sep 11, 2024

macrael Sep 11, 2024

macrael Sep 11, 2024

haworku Sep 12, 2024

haworku Sep 12, 2024

macrael Sep 12, 2024

haworku Sep 16, 2024

pearl-truss commented Sep 16, 2024 •

edited

Loading

pearl-truss commented Sep 16, 2024

haworku commented Sep 16, 2024 •

edited

Loading

MCR-3849 Fix session timeout #2719

MCR-3849 Fix session timeout #2719

Conversation

haworku commented Sep 3, 2024 • edited Loading

Summary

Related issues

Screenshots

Test cases covered

QA guidance

pearl-truss commented Sep 5, 2024

haworku commented Sep 5, 2024 • edited Loading

pearl-truss commented Sep 6, 2024

haworku commented Sep 6, 2024 • edited Loading

macrael left a comment

Choose a reason for hiding this comment

macrael Sep 11, 2024

Choose a reason for hiding this comment

macrael Sep 11, 2024

Choose a reason for hiding this comment

macrael Sep 11, 2024

Choose a reason for hiding this comment

haworku Sep 12, 2024

Choose a reason for hiding this comment

haworku Sep 12, 2024

Choose a reason for hiding this comment

macrael Sep 12, 2024

Choose a reason for hiding this comment

haworku Sep 16, 2024

Choose a reason for hiding this comment

pearl-truss commented Sep 16, 2024 • edited Loading

pearl-truss commented Sep 16, 2024

haworku commented Sep 16, 2024 • edited Loading

haworku commented Sep 3, 2024 •

edited

Loading

haworku commented Sep 5, 2024 •

edited

Loading

haworku commented Sep 6, 2024 •

edited

Loading

pearl-truss commented Sep 16, 2024 •

edited

Loading

haworku commented Sep 16, 2024 •

edited

Loading