[DUOS-2462][DUOS-2850] B2C OAuth Integration #2512

sjkobori · 2024-03-27T03:33:00Z

Addresses

https://broadworkbench.atlassian.net/browse/DUOS-2462
https://broadworkbench.atlassian.net/browse/DUOS-2850

Summary

B2C

Add OidcBroker.ts for wrapping calls to the oidc-client-ts library
Adds Auth.ts for managing DUOS UI specific session logic and communicating to the OidcBroker
Adds OAuth2.ts for making api calls to get our oauth config
Adds appLoader.ts, RedirectFromOAuth.ts to handle redirect after signing in through popup window
Removes GoogleIS and related code

Other

As per request, moved the SignIn button from the Home.jsx to DuosHeader.jsx
Sign out calls now delegate through Auth.signOut
Typescript conversion for files affected by these changes (except for large files requiring their own ticket to convert)

…essions

…gnInButton errors

package.json

dvoet · 2024-04-22T14:20:36Z

src/libs/auth/oidcBroker.ts

+  const metadata: Partial<OidcMetadata> = {
+    authorization_endpoint: `${await Config.getApiUrl()}/oauth2/authorize`,
+    token_endpoint: `${await Config.getApiUrl()}/oauth2/token`,
+  };


I don't know the right answer here, but I wonder if adding this metadata is required. Does oidc-client-ts know how to use the authority config value to lookup the .well-known/openid-configuration? If so, we probably don't need to use the custom authorization and token endpoints.

Note that the custom endpoints were ported by me from TDR. I didn't fully understand why it has many of the custom bits. I now realize that it was mostly to be able to support switching back and forth between B2C and Google. All of that is over with and we are B2C all the way. So the custom endpoints don't really provide any value. They are used by the swagger ui but I have figured out a better way that does not require them.

Bottom line: I think we should work towards removing these endpoints.

https://authts.github.io/oidc-client-ts/#md:required-settings

looks like our config url (in dev) does not have CORS headers set. We could look into setting those or add a new DUOS api to serve the config. It actually already gets and caches the config, there is jut no api.

I'm all for removing the oauth2 proxy endpoints if they are not needed anymore. I think another thing the custom endpoints do is inject the clientId as a scope and add ?prompt=login to the authorize request. See:

https://github.com/broadinstitute/terra-helmfile/blob/fa1dabdd92833cb4e736e7203d7ef6d0e315b3a3/charts/consent/templates/_consent.yaml.tpl#L69-L73

https://github.com/DataBiosphere/consent/blob/develop/src/main/java/org/broadinstitute/consent/http/service/OidcService.java#L33-L36

There is probably a way to set these in the oidc-client-ts frontend instead.

Extra parameters can be set with the extraQueryParams config setting.

prompt=login is good but I don't know if the client id is required. In my experimentation with the swagger UI's oidc integration it doesn't seem to be.

rtitle · 2024-04-22T17:15:49Z

src/libs/auth/oidcBroker.ts

+    silent_redirect_uri: `${window.origin}/redirect-from-oauth-silent`,
+    metadata,
+    prompt: 'consent login',
+    scope: 'openid email profile',


Re my above comment: I see prompt=login is already set here. We'd need to figure out how to set the clientId as a scope if not using the custom oauth2 endpoints. (Or validate that this is still a requirement in B2C?)

rtitle · 2024-04-22T17:29:08Z

src/libs/auth/oidcBroker.ts

+  },
+
+  signOut: async (): Promise<void> => {
+    await OidcBroker.getUserManager().removeUser();


Just wondering if we have to clear any storage upon signout, or does removeUser take care of it?

(Just remembering that terra-ui had to clear several things upon signOut, but that might have been Terra UI's custom state store.)

It is probably worth throwing in a clearStaleState (https://authts.github.io/oidc-client-ts/classes/UserManager.html#clearStaleState) here just to clean out anything else that may be in the store. The specific duos-ui state is cleared out in Auth.signOut. Good callout 👍

rtitle

Overall it looks good to me!

I do think it's an interesting exploration whether the backend oauth2 proxy endpoints are truly needed anymore..

Ghost-in-a-Jar · 2024-04-22T19:24:46Z

src/components/SignInButton.tsx

+  return <div>{!showError ? signInButton() : errorAlert(errorInfo)}</div>;
+};
+
+export default SignInButton;


Just for future pr's it is nice for these sorts of js -> ts conversions to have inline pr comments on the new file for reviewers of what you changed relative to the js file since there is no direct diff shown in github. I just opened up both side by side and did the comparison for this pr.

Yeah, I definitely agree it is hard to tell the difference easily. In hindsight, I should have done this TS conversion in a separate PR and then made the b2c changes in this one.

Ghost-in-a-Jar

This looks good overall, I would definitely smoke test this thoroughly and expect there to maybe be some unforeseen behavior/bugs given the changes. In particular make sure that the background token expiration/refresh works as expected etc. I think you set yourself up well with logging and metrics collection.

rushtong · 2024-04-22T20:36:21Z

src/Routes.jsx

-    <Route exact path="/backgroundsignin" render={
-      (routeProps) =>
-        checkEnv(envGroups.NON_STAGING)
-          ? <BackgroundSignIn {...routeProps} />
-          : <NotFound />
-    } />


This page is what we use for E2E tests so we didn't have to manually trigger an auth dance. Currently, they're not run on PRs or in GHA, but we can run it locally. We either need a B2C version of this, or bring it back so we can test authenticated pages.

rushtong · 2024-04-22T20:42:23Z

src/pages/TermsOfServiceAcceptance.jsx

+    // TODO: Determine if this is the correct user flow
+    // Can the user only accept ToS once the have signed in?
+    // What state is the user in after signIn, but before accepting ToS?


This is the correct flow. Yes, the user can only accept ToS after signing in - otherwise we don't know who they are and could not register their ToS acceptance in Sam. If they sign in, but have not yet accepted ToS, they are directed to this page where their only choice is to accept or reject.

That makes sense. I was trying to determine if the ToS accept button should set the setUserIsLogged to true, since by signing in, userIsLogged should already be set to true. With this context, I believe the ToS accept button should not change the userIsLogged state.

Correct - userIsLogged state is separate and distinct from ToS acceptance.

src/libs/auth/auth.ts

src/libs/ajax.js

src/components/SignInButton.tsx

.eslintrc

rushtong · 2024-04-22T21:10:19Z

cypress/component/SignIn/sign_in.spec.js

@@ -2,7 +2,7 @@

 import React from 'react';
 import { mount } from 'cypress/react';
-import SignIn from '../../../src/components/SignIn';
+import SignInButton from '../../../src/components/SignInButton';
 import { Config } from '../../../src/libs/config';

 const signInText = 'Sign-in';


This text is no longer accurate - the first test should pass with

Suggested change

const signInText = 'Sign-in';

const signInText = 'Sign In';

rushtong · 2024-04-22T21:11:48Z

cypress/component/SignIn/sign_in.spec.js

  it('Spinner loads when client id is empty', function () {
    cy.viewport(600, 300);
    cy.stub(Config, 'getGoogleClientId').returns('');
-    mount(<SignIn />);
+    mount(<SignInButton />);
    cy.contains(signInText).should('not.exist');
  });


I don't think we need this test any longer - there is longer a spinner that loads in place of the sign in text during the google client id loading time.

I'll update this test with the new SignInButton functionality.

nawatts · 2024-04-22T22:52:58Z

.eslintrc

+    // `any` is useful for incremental type improvements during the transition from JS to TS.
+    "@typescript-eslint/no-explicit-any": "off",


Depending on how often an explicit any is desired, consider disabling this for specific lines or files instead of in global ESLint config. That way, it increases the friction to adding any and makes it easier to ratchet up type safety while incrementally removing any later.

package.json

nawatts · 2024-04-22T23:06:57Z

src/components/DuosHeader.jsx

+          <div
+            style={{ display: 'flex', alignItems: 'center', flexDirection: orientation === 'vertical' ? 'column' : 'row' }}
+          >
+            <SignInButton customStyle={{ whiteSpace: 'nowrap',  }}></SignInButton>


Nit:

Suggested change

<SignInButton customStyle={{ whiteSpace: 'nowrap', }}></SignInButton>

<SignInButton customStyle={{ whiteSpace: 'nowrap', }} />

src/components/SignInButton.tsx

nawatts · 2024-04-22T23:11:53Z

src/components/SignInButton.tsx

+    const { tosAccepted } = userStatus;
+    if (!isEmpty(userStatus) && !tosAccepted) {


If userStatus was empty, wouldn't the destructuring in const { tosAccepted } = userStatus; throw an error?

When would we expect userStatus to be empty?

When would we expect userStatus to be empty?

I believe userStatus is one of the flavors of the Sam user info object. We collect that on every back end API call as part of the DUOS user to determine ToS acceptance.

nawatts · 2024-04-22T23:19:09Z

src/libs/ajax/OAuth2.ts

+export const OAuth2 = {
+  getConfig: async (): Promise<OAuthConfig> => getConfig(),
+};


For what it's worth, as an alternative to namespace objects, modules could export individual methods and consumers could:

import * as OAuth2 from 'path/to/OAuth2';

if they really want the namespace.

nawatts · 2024-04-22T23:20:40Z

src/libs/auth/RedirectFromOAuth.ts

+  const userManager: UserManager = new UserManager(
+    OidcBroker.getUserManagerSettings()
+  );
+  const url = window.location.href;
+  const isSilent = window.location.pathname.startsWith(
+    '/redirect-from-oauth-silent'
+  );
+
+  if (isSilent) {
+    userManager.signinSilentCallback(url);
+  } else {
+    userManager.signinPopupCallback(url);
+  }


This looks like it belongs in a useEffect hook instead of in render.

But does this need to be in a React component at all? It looks like oauth-redirect-loader could call the UserManager functions directly (and render a Spinner if desired).

I don't think this needs to be in a React component since this should only happen once each time the user signs in through the popup window, and then I believe that app instance ends once that window closes.

nawatts · 2024-04-22T23:20:50Z

src/libs/auth/RedirectFromOAuth.ts

+  } else {
+    userManager.signinPopupCallback(url);
+  }
+  return Spinner;


Suggested change

return Spinner;

return <Spinner />;

Seems like spinner was not a proper React component. Since it was only used in one other place, I converted it to one in order to make this change.

Oh. I assumed it was based on the name. But if it was an element instead, that change will likely break other places it was used.

nawatts · 2024-04-22T23:24:52Z

src/libs/auth/auth.ts

+  signIn: async (
+    popup: boolean,
+    onSuccess?: (response: OidcUser) => Promise<void> | void,
+    onFailure?: (response: any) => Promise<void> | void
+  ): Promise<void> => {


Since this is already returning a Promise, consider leaning into that pattern instead of mixing it with callbacks.

Consumers could handle success or failure with signIn(...).then(onSuccess, onFailure).

nawatts · 2024-04-22T23:27:10Z

src/libs/auth/oidcBroker.ts

+let config: OAuthConfig | null = null;
+let userManagerSettings: UserManagerSettings | null = null;
+let userManager: UserManager | null = null;


A possible alternative to global state could be to have an OidcBroker class and have this module export a singleton instance of it?

rushtong

See comments inline - we need at least one test fix and the background sign-in feature replaced. Outside of that, this works as expected. I've been able to test standard OAuth with google users and OAuth with an azure account.

… module, simplified RedirectFromOAuth

fboulnois

I was able to run the PR after correctly making certain dependencies explicit. After limited testing, I was able to log in and out of my Google and Azure accounts and verify that the sign in button was in the login bar.

# Conflicts: # package-lock.json # package.json

# Conflicts: # .eslintrc # src/components/Spinner.tsx # src/custom.d.ts # tsconfig.json

sjkobori · 2024-06-25T03:32:25Z

#2611 Will continue this work as this PR too large to easily track the changes.

sjkobori force-pushed the DUOS-2462_B2C_auth branch from fd2c720 to 1ca9f86 Compare April 3, 2024 14:18

sjkobori added 6 commits April 12, 2024 16:19

Added oidcBroker and populated configs

d589b8b

WIP appLoader

1645f0d

Successful sign in

39be391

Added linting scripts

9b8d342

Successful register/log in with b2c, next need to handle persistent s…

b9fd0d1

…essions

added oauth2 ajax module

407faf7

sjkobori force-pushed the DUOS-2462_B2C_auth branch from 85ba8f9 to 407faf7 Compare April 12, 2024 22:21

sjkobori added 2 commits April 17, 2024 08:35

added UserManager events, auth initializer, removed oidc context

7eca6ff

Initial PR passthrough fixes (cleaning up comments etc.)

d793eaf

sjkobori marked this pull request as ready for review April 17, 2024 13:04

sjkobori requested a review from a team as a code owner April 17, 2024 13:04

sjkobori changed the title ~~Added oidcBroker and populated configs~~ [DUOS-2462] B2C OAuth Integration Apr 17, 2024

sjkobori changed the title ~~[DUOS-2462] B2C OAuth Integration~~ [DUOS-2462][DUOS-2850] B2C OAuth Integration Apr 17, 2024

Made signOut/signIn calls consistent, added spinner and cleaned up si…

a76df4f

…gnInButton errors

rushtong reviewed Apr 19, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

sjkobori requested a review from nawatts April 22, 2024 03:37

dvoet reviewed Apr 22, 2024

View reviewed changes

sjkobori requested a review from rtitle April 22, 2024 15:55

rtitle reviewed Apr 22, 2024

View reviewed changes

rtitle approved these changes Apr 22, 2024

View reviewed changes

Ghost-in-a-Jar reviewed Apr 22, 2024

View reviewed changes

Ghost-in-a-Jar approved these changes Apr 22, 2024

View reviewed changes