DCJ-654: Sign-in via OIDC #2667

rushtong · 2024-09-09T15:41:43Z

Addresses

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

Summary

This PR completes the efforts started in #2611 and #2657. This replaces Google Sign-in with Oidc sign-in.

Use oidc-client-ts library for sign-in
Expand unit testing around auth, oidcBroker, and sign-in button components
Minor refactoring around new user registration
See also: https://github.com/broadinstitute/terra-helmfile/pull/5871

Full Screen

Minimized Screen

Our (current) B2C Tenant Screen

This will be changed to a DUOS version in https://broadworkbench.atlassian.net/browse/DCJ-165

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

# Conflicts: # src/libs/ajax/Metrics.js

# Conflicts: # src/components/SignInButton.tsx

fboulnois

overall looks good, questions relating to tests:

fboulnois · 2024-09-18T13:25:40Z

cypress/component/Auth/auth.spec.ts

+    cy.on('fail', (err) => {
+      return err.message !== Auth.signInError();
+    });
+    Auth.signIn();


This function still seems to be async, does it need to be awaited?

Good call, but await-ing breaks the test, I think I need to use then

fboulnois · 2024-09-18T13:25:45Z

cypress/component/Auth/auth.spec.ts

      'clientId': 'clientId'
    });
-    await Auth.initialize();
+    Auth.initialize();


This function still seems to be async, does it need to be awaited?

I was initially confused by this. Cypress gives you a warning if you use async in beforeEach:

Cypress Warning: Cypress detected that you returned a promise in a test, but also invoked one or more cy commands inside of that promise. The test title was: > Auth Success "before each" hook While this works in practice, it's often indicative of an anti-pattern. You almost never need to return both a promise and also invoke cy commands. Cy commands themselves are already promise like, and you can likely avoid the use of the separate Promise. https://on.cypress.io/returning-promise-and-commands-in-test

rjohanek · 2024-09-18T13:42:21Z

config/alpha.json

@@ -5,7 +5,6 @@
  "hash": "alpha",
  "apiUrl": "https://consent.dsde-alpha.broadinstitute.org",
  "ontologyApiUrl": "https://consent-ontology.dsde-alpha.broadinstitute.org/",
-  "clientId": "1020846292598-hd801vsmmbhh97vaf6aar17lu0q2evfj.apps.googleusercontent.com",


I thought alpha didn't exist anymore? Why do we need this config?

This config file name is not important and is, unfortunately, an artifact of our build image process. Originally, it did reference real environment variables. I'll change this in a separate PR/ticket.

I created https://broadworkbench.atlassian.net/browse/DCJ-678 to cover this bit of tech debt.

rjohanek · 2024-09-18T17:38:27Z

src/components/SignInButton.tsx

@@ -139,11 +102,15 @@ export const SignInButton = (props: SignInButtonProps) => {
  const registerAndRedirectNewUser = async (redirectTo: string, shouldRedirect: boolean) => {
    const registeredUser: DuosUser = await User.registerUser();
    setUserRoleStatuses(registeredUser, Storage);
-    await onSignIn();
+    await syncSignInOrRegistrationEvent(eventList.userRegister);
+    history.push(`/tos_acceptance${shouldRedirect ? `?redirectTo=${redirectTo}` : ''}`);


why move this line from syncSignInOrRegistrationEvent to here?

My goal with syncSignInOrRegistrationEvent was to isolate the Metrics calls. I wanted that to remain short and focused. Since the history modification is not related to that functionality, I pulled it out so it would be more clear as to where it is being used. What I found in going through this code is that there was a lot of dual-function helper methods that were very confusing to debug, especially the new user registration piece. By isolating functionality, I was better able to log and reason about what was happening in the different cases.

rjohanek · 2024-09-18T17:41:54Z

src/index.tsx

 import {unregister} from './registerServiceWorker';
 import {BrowserRouter} from 'react-router-dom';

 const load = async () => {
  unregister();
  await Auth.initialize();
+  if (window.location.pathname.startsWith('/redirect-from-oauth')) {
+    await OidcBroker.getUserManager().signinPopupCallback(window.location.href);


what does this do?

Great question. In the original version of this work, there was a whole component dedicated to servicing this single path. This is my attempt to simplify it. In short, it does this:

The OIDC client calls a popup window with a redirect url here: https://github.com/DataBiosphere/duos-ui/pull/2667/files#diff-c22f0b724cae791bb887d11ce9ada280a1581625e133634e5c2356a5637d6980L42

The user goes through the auth dance and is redirected to that url, but the popup doesn't go away.

On reload, we detect the path

Call the signinPopupCallback which does two things

It closes the open popup

Redirects the user to the original href, which is either the home page, or a sub-page inside the app to which we redirect.

I'm going to add some comments around this for future devs.

rjohanek

overall, looks good! Just a couple questions for my own understanding

rushtong added 29 commits September 9, 2024 11:40

feat: sign-in via OIDC

db33dbc

feat: re-add register metrics

386d0f2

feat: rm unused

456cb7f

feat: fix typo

9bf40f3

feat: configs

d79a57e

feat: update test

924b8ee

feat: styling

becd2c8

feat: auth test cleanup

1115338

Merge branch 'refs/heads/develop' into gr-DCJ-654-sign-in

907746f

# Conflicts: # src/libs/ajax/Metrics.js

feat: sign-in test fix

a3eaeba

Merge branch 'refs/heads/develop' into gr-DCJ-654-sign-in

2e4e93b

feat: handle errors, simplify login options

7deecc4

feat: cleanup

df09f44

feat: auth tests

dfa721a

feat: oidc client tests

59cd7be

feat: sign-in tests and cleanup

20dad74

Merge branch 'refs/heads/develop' into gr-DCJ-654-sign-in

25d21fd

# Conflicts: # src/components/SignInButton.tsx

feat: merge fix

f2bb354

feat: docs and cleanup

28b4231

feat: test cleanup

c5c67a0

feat: revert comment

1c1288b

feat: test cleanup

e100a0f

feat: tos path test

14176ab

Merge branch 'refs/heads/develop' into gr-DCJ-654-sign-in

a45492f

feat: fix new user registration path

b035073

feat: rm duplicated metrics calls

a0b9fba

feat: registration test

18e7b13

feat: minor rename

8944f48

feat: revert comment change

9e5f497

rushtong marked this pull request as ready for review September 17, 2024 15:57

rushtong requested a review from a team as a code owner September 17, 2024 15:57

rushtong requested review from fboulnois and removed request for a team September 17, 2024 15:57

Merge branch 'refs/heads/develop' into gr-DCJ-654-sign-in

3c59ed3

fboulnois approved these changes Sep 18, 2024

View reviewed changes

rjohanek reviewed Sep 18, 2024

View reviewed changes

feat: PR feedback

ad2d8ed

rushtong requested a review from rjohanek September 18, 2024 16:06

rjohanek reviewed Sep 18, 2024

View reviewed changes

rjohanek approved these changes Sep 18, 2024

View reviewed changes

feat: additional documentation

54f025b

rushtong merged commit 1652274 into develop Sep 19, 2024
9 checks passed

rushtong deleted the gr-DCJ-654-sign-in branch September 19, 2024 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DCJ-654: Sign-in via OIDC #2667

DCJ-654: Sign-in via OIDC #2667

rushtong commented Sep 9, 2024 •

edited

Loading

fboulnois left a comment

fboulnois Sep 18, 2024

rushtong Sep 18, 2024

fboulnois Sep 18, 2024

rushtong Sep 18, 2024

rjohanek Sep 18, 2024

rushtong Sep 18, 2024

rushtong Sep 18, 2024

rjohanek Sep 18, 2024

rushtong Sep 18, 2024

rjohanek Sep 18, 2024

rushtong Sep 18, 2024

rushtong Sep 19, 2024

rjohanek left a comment

DCJ-654: Sign-in via OIDC #2667

DCJ-654: Sign-in via OIDC #2667

Conversation

rushtong commented Sep 9, 2024 • edited Loading

Addresses

Summary

Full Screen

Minimized Screen

Our (current) B2C Tenant Screen

fboulnois left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjohanek left a comment

Choose a reason for hiding this comment

rushtong commented Sep 9, 2024 •

edited

Loading