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

DCJ-649: Use OidcBroker for sign-out functionality #2657

Merged
merged 12 commits into from
Sep 6, 2024

Conversation

rushtong
Copy link
Contributor

@rushtong rushtong commented Aug 29, 2024

Addresses

https://broadworkbench.atlassian.net/browse/DCJ-649

Summary

  • This PR tries to carry forward the effort started in [DCJ-89] B2C Auth base #2611
  • This implements a single feature - Sign Out via OIDC.
  • auth.ts is a minimized version of auth.ts in the above PR
  • oidcBroker.ts is a minimized version of oidcBroker.ts in the above PR
  • Due to testing, I updated the component test action to store failure screenshots for review.

Follow-on work is defined in a variety of tickets under the DUOS-2534 epic.

Testing Notes

Testing should include reviewing the developer panel in a browser when logging into a local instance.

  • Signing in is unchanged so there should be no regressions
  • Sign-out mimics the original implementation but additionally uses OidcBroker to invoke OIDC library functionality. This will be expanded in future work that layers in additional sign-in functionality
  • Inspecting local session storage should show user info when logged in, and cleared out when signed out.

Have you read Terra's Contributing Guide lately? If not, do that first.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@rushtong rushtong marked this pull request as ready for review August 29, 2024 21:28
@rushtong rushtong requested a review from a team as a code owner August 29, 2024 21:28
@rushtong rushtong requested review from fboulnois and okotsopoulos and removed request for a team August 29, 2024 21:28
Copy link
Contributor

@fboulnois fboulnois left a comment

Choose a reason for hiding this comment

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

looks good, thanks for adding some tests 👍

@rushtong rushtong changed the title DCJ-89: Use OidcBroker for sign-out functionality DCJ-3085: Use OidcBroker for sign-out functionality Sep 3, 2024
@rushtong rushtong changed the title DCJ-3085: Use OidcBroker for sign-out functionality DCJ-649: Use OidcBroker for sign-out functionality Sep 3, 2024
# Conflicts:
#	src/App.jsx
#	src/components/DuosHeader.jsx
Copy link
Contributor

@s-rubenstein s-rubenstein left a comment

Choose a reason for hiding this comment

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

This looks reasonable - I am curious how it works with sign in using previous auth, but sign out using oidc? Presumably its backwards compatible because Google Auth is included within OIDC?

path: cypress/screenshots
if-no-files-found: ignore # 'warn' or 'error' are also available, defaults to `warn`
- uses: actions/upload-artifact@v4
with:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also have if: failure()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, nice catch!

});
await Auth.initialize();
});
it('Sign Out Clears the session when called', async function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you store anything in localStorage on DUOS? That might also be good to check if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do - I'll boost the uses of Storage.setXYZ() and check those too, thank you!


const text = 'TOS Text';
const mocks = {
history: { push() {} }
history: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Is this a lint change? I personally find it more readable the other way.

Copy link
Contributor Author

@rushtong rushtong Sep 5, 2024

Choose a reason for hiding this comment

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

Yes, that's it exactly - I manually unformatted it, then ran our eslint checks and it made the same change.

await Storage.clearStorage();
await setIsLoggedIn(false);
};

const signIn = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will move to OIDC with https://broadworkbench.atlassian.net/browse/DCJ-654, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I'm trying to de-tangle the sign-in code here so it's less of a boil-the-ocean change.

@rushtong
Copy link
Contributor Author

rushtong commented Sep 5, 2024

This looks reasonable - I am curious how it works with sign in using previous auth, but sign out using oidc? Presumably its backwards compatible because Google Auth is included within OIDC?

Both sign-in and sign-out are fully functional. Sign-in is the bigger lift so I'm working on that now, but separately. The sign-out exercises both the old way of clearing local storage as well as triggering the OIDC library sign-out functionality.

@rushtong rushtong merged commit a9b8b1f into develop Sep 6, 2024
9 checks passed
@rushtong rushtong deleted the gr-DCJ-89-sign-out branch September 6, 2024 11:11
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