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

feat: before app submission refresh access token #1006

Merged
merged 4 commits into from
Dec 18, 2024

Conversation

YazeedLoonat
Copy link
Collaborator

This PR addresses #987

  • Addresses the issue in full

Description

There are some edge cases where a public user could have their session voided at the time of app submission, but the UI isn't aware and allowed the submission to go through

This pr makes it so that we attempt a refresh of the access token so if the session is voided the user is given a heads up that their session expired and they are bounced

How Can This Be Tested/Reviewed?

The most easily reproducible way to test this is:

  • create a public user (userA)
  • go through the app submission process but STOP just before you accept the terms and fully submit the record
  • in an incognito window log in as that same user (userA)
  • in the OG window hit submit
    you should be prompted with a dialog and whether you hit the "x" or "OK" you will be bounced to the sign in page and logged out

you should also test that going through the regular app submission process still works

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

@mcgarrye
Copy link
Collaborator

When I tried to complete an application in the incognito window, I was also bounced at the end of the process

@ludtkemorgan
Copy link
Collaborator

@mcgarrye with this implementation that is expected. It kicks off any person that is logged in with those credentials.

There is potentially a long term solution of refactoring how the tokens work so that you can be logged into multiple browsers at once, but for this bug we are just going to kick them out.

@emilyjablonski
Copy link
Collaborator

Could we get @mcgarrye to confirm as well as he requested changes?

@mcgarrye
Copy link
Collaborator

I am still being redirected to the home page, not the sign-in page

@ludtkemorgan
Copy link
Collaborator

@mcgarrye @emilyjablonski Ok, I think I finally got it. I was missing an await when redirecting. Can you retest it to see if it is working for you?

@mcgarrye
Copy link
Collaborator

Screenshot 2024-12-18 at 9 42 26 AM May be out of scope from this ticket but if a user is signed out when in the application dashboard and attempts to edit their information they will see this unhandled error

Copy link
Collaborator

@mcgarrye mcgarrye left a comment

Choose a reason for hiding this comment

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

LGTM

@ludtkemorgan ludtkemorgan merged commit 62dd0b1 into main Dec 18, 2024
16 checks passed
@ludtkemorgan ludtkemorgan deleted the 987/refresh-access-token-before-app-submission branch December 18, 2024 14:57
@ludtkemorgan
Copy link
Collaborator

@mcgarrye that's a good find. The user won't see that banner as we only show that when you have the app running in dev mode (yarn dev)

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

Successfully merging this pull request may close these issues.

4 participants