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

Update browser authorization to oidc-client-ts #88

Merged
merged 38 commits into from
Jan 27, 2023

Conversation

evelynpreslar-bentley
Copy link
Collaborator

No description provided.

@evelynpreslar-bentley evelynpreslar-bentley linked an issue Oct 5, 2022 that may be closed by this pull request
@evelynpreslar-bentley
Copy link
Collaborator Author

evelynpreslar-bentley commented Oct 5, 2022

oidc-client-ts is the currently active fork of oidc-client-js.

While the API is mostly identical, oidc-client-ts does make some breaking changes in its client constructor.
The client_id and authority parameters are now required, and necessary in order to process callbacks (previously, only a redirectURL was required, and the other properties were inferred).

This has the direct effect of requiring more upfront configuration in order to call BrowserAuthorizationCallbackHandler.handleCallback, but also has the indirect effect of disallowing callbacks to be processed with a single callback handler configuration.

Because oidc-client-ts no longer permits authority and clientId to be loaded from the state during a callback, a callback handler must be created for each authority/clientId combination. Otherwise, the state will not be loaded due to mismatching configuration.

This is of concern to any non-IMS services or extensions which utilize BrowserAuthorizationClient (like the SafetiBase frontstage in Design Review), since they are reliant on the parent app to handle callbacks for them.

@calebmshafer calebmshafer marked this pull request as ready for review December 23, 2022 15:43
@mattbjordan
Copy link
Contributor

This PR still needs a codeowner review before it can be merged.

@aruniverse @ben-polinsky

@ben-polinsky
Copy link
Collaborator

@aruniverse trying to follow this. It's definitely a breaking change - do we need to test or update anything dependent on this package?

@aruniverse
Copy link
Member

@aruniverse trying to follow this. It's definitely a breaking change - do we need to test or update anything dependent on this package?

yea im worried what the fallout of this will look like.

@mattbjordan can you pull this into a cospace with a viewer and see what happens?

@mattbjordan
Copy link
Contributor

@mattbjordan can you pull this into a cospace with a viewer and see what happens?

Will do. Gotta figure out how to use cospace though as I haven't needed to before.

packages/browser/src/utils.ts Outdated Show resolved Hide resolved
packages/browser/src/utils.ts Show resolved Hide resolved
packages/browser/src/Logger.ts Show resolved Hide resolved
packages/browser/src/utils.ts Show resolved Hide resolved
packages/browser/src/Client.ts Outdated Show resolved Hide resolved
packages/browser/src/CallbackHandler.ts Show resolved Hide resolved
packages/browser/src/CallbackHandler.ts Outdated Show resolved Hide resolved
@mattbjordan
Copy link
Contributor

@mattbjordan can you pull this into a cospace with a viewer and see what happens?

So, I got the cospace working and was able to run the viewer and connect to an iModel just fine. The only thing I had to change was having to use a BrowserAuthorizationCallbackHandlerConfiguration in the viewer.

Before

await BrowserAuthorizationCallbackHandler.handleSigninCallback(
  oidcConfiguration.redirectUri
);

After

await BrowserAuthorizationCallbackHandler.handleSigninCallback({
  redirectUri: oidcConfiguration.redirectUri,
  clientId: oidcConfiguration.clientId,
  authority: oidcConfiguration.authority // optional
});

Is there anything specific that you'd want me to test while I have this environment set up?

@mattbjordan mattbjordan self-assigned this Jan 12, 2023
packages/browser/src/Client.ts Outdated Show resolved Hide resolved
packages/browser/src/utils.ts Show resolved Hide resolved
@aruniverse
Copy link
Member

This is of concern to any non-IMS services or extensions which utilize BrowserAuthorizationClient (like the SafetiBase frontstage in Design Review), since they are reliant on the parent app to handle callbacks for them.

@jason-crow @jjbeckman13 fyi

@aruniverse aruniverse merged commit e8d420e into main Jan 27, 2023
@aruniverse aruniverse deleted the evelyn.preslar/browser-auth-update-oidc-client-ts branch January 27, 2023 18:44
ben-polinsky pushed a commit that referenced this pull request Mar 2, 2023
ben-polinsky pushed a commit that referenced this pull request Mar 2, 2023
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.

Move to non-deprecated oidc-client package
5 participants