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

Reset app auth state if Auth0 session has expired #1398

Merged
merged 1 commit into from
Nov 8, 2024

Conversation

schroerbrian
Copy link
Contributor

refs #1391

Copy link
Contributor

@katerina-kossler katerina-kossler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes here look right from what I can tell but I see that some tests in CI are failing and I don't see any test coverage for this (is that normal for the web project?)

Copy link
Member

@richardxia richardxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Though should we also actually clear that state when we detect this issue? I guess I'm thinking that if someone navigated away from the log-in page, then they'd still be in this inconsistent state where the app thinks they're logged in but their credentials have expired.

@richardxia
Copy link
Member

the changes here look right from what I can tell but I see that some tests in CI are failing and I don't see any test coverage for this (is that normal for the web project?)

Regrettably, it's normal for the web project to have flaky failures with the end-to-end browser tests. Normally I just restart it until it finally succeeds.

@schroerbrian
Copy link
Contributor Author

the changes here look right from what I can tell but I see that some tests in CI are failing and I don't see any test coverage for this (is that normal for the web project?)

Regrettably, it's normal for the web project to have flaky failures with the end-to-end browser tests. Normally I just restart it until it finally succeeds.

Unfortunately, this failure seems to do with a deprecated artifact in our build system. I just haven’t had time to look further into this. I’m away this weekend so i can’t work on it. Anyhow, i suppose this could be safely merged in if we want to, but i don’t mind waiting until we get to the bottom of this , either

Error: This request has been automatically failed because it uses a deprecated version of actions/upload-artifact: v2. Learn more: https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

@schroerbrian
Copy link
Contributor Author

LGTM! Though should we also actually clear that state when we detect this issue? I guess I'm thinking that if someone navigated away from the log-in page, then they'd still be in this inconsistent state where the app thinks they're logged in but their credentials have expired.

Is there another state I’m missing? The code sets the authState to null prior to the redirect

@schroerbrian
Copy link
Contributor Author

Thanks for the review @richardxia and @katerina-kossler ! Sorry for my late replies here

@richardxia
Copy link
Member

Is there another state I’m missing? The code sets the authState to null prior to the redirect

Wow, sorry, I don't know how I missed that. My brain managed to completely not see that line in between the if and the redirect. Yeah, sorry, this LGTM!

@schroerbrian
Copy link
Contributor Author

I am going to go ahead and merge this in as the test failure is unrelated. Thanks all!

@schroerbrian schroerbrian merged commit 9a210ec into master Nov 8, 2024
4 of 5 checks passed
@schroerbrian schroerbrian deleted the 1391-handle-expired-session branch November 8, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants