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

PIMS-2117 Auth Header Bug #2700

Merged
merged 11 commits into from
Oct 7, 2024
Merged

PIMS-2117 Auth Header Bug #2700

merged 11 commits into from
Oct 7, 2024

Conversation

dbarkowsky
Copy link
Collaborator

@dbarkowsky dbarkowsky commented Sep 24, 2024

🎯 Summary

PIMS-2117

Changes

  • Make the config context into a static import. Didn't need to be a context.
  • Fix issue where access request provider was potentially uncontrolled.
  • Update sso-express package to version with BCSC support.
  • Update sso-react package to 1.0.0-alpha7.
  • Separate sso information from AuthContext and rename context to UserContext. It only provides the PIMS user now. Use the useSSO() hook from the sso-react package for keycloak information.

Issues

  • Refreshing the page with sso-react 1.0.1 and beyond causes the auth header to be Bearer undefined. Not sure why this is happening. I've commented out a bit of code that retries requests on a 401 in order to make this easier to test. Restore before merge. It seems like the state in the sso-react provider is not set when getAuthorizationHeaderValue is called. Maybe we could delay our own calls until it is, but this should probably be fixed in the sso-react package itself.

🔰 Checklist

  • I have read and agree with the following checklist and am following the guidelines in our Code of Conduct document.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation where required.
  • I have tested my changes to the best of my ability.
  • My changes generate no new warnings.

@dbarkowsky dbarkowsky self-assigned this Sep 24, 2024
Copy link

codeclimate bot commented Sep 24, 2024

Code Climate has analyzed commit baa1475 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 93.3%.

View more on Code Climate.

Copy link

🚀 Deployment Information

The Express API Image has been built with the tag: 2700. Please make sure to utilize this specific tag when promoting these changes to the TEST and PROD environments during the API deployment. For more updates please monitor Image Tags Page on Wiki.

Copy link

🚀 Deployment Information

The React APP Image has been built with the tag: 2700. Please make sure to utilize this specific tag when promoting these changes to the TEST and PROD environments during the APP deployment. For more updates please monitor Image Tags Page on Wiki.

@dbarkowsky
Copy link
Collaborator Author

dbarkowsky commented Oct 3, 2024

I ended up trying a lot of things to make the citz-imb-sso-react@1.1.0-beta to work. I cannot get around the issue where the auth header is undefined after a refresh. It does not seem to come back unless you log out and log in again. Things I tried to make this work:

  • Setting a delay/promise in our useFetch hook to try and wait for the header to be populated
  • Altering the get self and lookup all calls to refresh if the sso state changes. It never does.
  • Making getAuthorizationHeaderValue from the sso-react package asynchronous, so it would wait until there's a value to get rather than returning Bearer undefined. I still think there's a race condition between it refreshing your token on a refresh of the page and constructing the access token state. None of that seemed to matter, as it never gets the defined access token again.
  • A bunch of other hacky things I can't remember that didn't work.

At this point, I'm recommending that we stay on version 1.0.0-alpha7. It's the last-known working version, and as long as the backend is updated, it seems like we still save all the user information properly.

Copy link
Collaborator

@TaylorFries TaylorFries left a comment

Choose a reason for hiding this comment

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

changes work as expected. Rollback to alpha7 sso-react package resolves the refresh issue. N?o issues detected.
Should we open another branch once this is closed to link to bcgov/citz-imb-sso-react#169?

Copy link
Collaborator

@LawrenceLau2020 LawrenceLau2020 left a comment

Choose a reason for hiding this comment

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

Page refreshing seems to be working with these updated packages. Tested with IDIR as well as a BCSC user, refreshed page and no issues detected.

@dbarkowsky
Copy link
Collaborator Author

changes work as expected. Rollback to alpha7 sso-react package resolves the refresh issue. N?o issues detected. Should we open another branch once this is closed to link to bcgov/citz-imb-sso-react#169?

I'll make another ticket for the backlog as a "lookout for this"-kind of task. When the framework agnostic version eventually comes out, we may want to move to that.

@dbarkowsky dbarkowsky merged commit d1ce7ed into main Oct 7, 2024
16 checks passed
@dbarkowsky dbarkowsky deleted the PIMS-2117-Auth-Header-Bug branch October 7, 2024 19:53
dbarkowsky added a commit that referenced this pull request Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants