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

Allow adding custom loopback client in acquireTokenInteractive #5578

Merged
merged 15 commits into from
Feb 6, 2023

Conversation

derisen
Copy link
Contributor

@derisen derisen commented Jan 17, 2023

This PR adds a loopback client interface to InteractiveRequest for devs who would like to run their own server for listening the authZ code response in acquireTokenInteractive API.

@derisen derisen marked this pull request as draft January 17, 2023 03:20
@github-actions github-actions bot added the msal-node Related to msal-node package label Jan 17, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2023

Codecov Report

Merging #5578 (cbd368b) into dev (a7d7ecc) will not change coverage.
The diff coverage is 100.00%.

Flag Coverage Δ *Carryforward flag
msal-angular 96.50% <ø> (ø) Carriedforward from a7d7ecc
msal-browser 86.46% <ø> (ø) Carriedforward from a7d7ecc
msal-common 84.54% <ø> (ø) Carriedforward from a7d7ecc
msal-core 82.84% <ø> (ø) Carriedforward from a7d7ecc
msal-node 83.39% <100.00%> (ø)
msal-node-extensions 75.64% <ø> (ø) Carriedforward from a7d7ecc
msal-react 94.68% <ø> (ø) Carriedforward from a7d7ecc
node-token-validation 88.88% <ø> (ø) Carriedforward from a7d7ecc

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
lib/msal-node/src/index.ts 100.00% <ø> (ø)
...ib/msal-node/src/client/PublicClientApplication.ts 76.31% <100.00%> (ø)
lib/msal-node/src/network/LoopbackClient.ts 63.63% <100.00%> (ø)

@derisen derisen marked this pull request as ready for review January 18, 2023 16:54
@ghost
Copy link

ghost commented Jan 30, 2023

Reminder: The next release is scheduled for next week and this PR appears to be stale :(

If changes have been requested please address feedback.
If this PR is still a work in progress please mark as draft.

@ghost ghost added the Needs: Attention 👋 Awaiting response from the MSAL.js team label Jan 30, 2023
@ghost ghost removed the Needs: Attention 👋 Awaiting response from the MSAL.js team label Feb 6, 2023
import { AuthorizationUrlRequest } from "./AuthorizationUrlRequest";

export type InteractiveRequest = Pick<AuthorizationUrlRequest, "authority"|"correlationId"|"claims"|"azureCloudOptions"|"account"|"extraQueryParameters"|"tokenQueryParameters"|"extraScopesToConsent"|"loginHint"|"prompt"> & {
openBrowser: (url: string) => Promise<void>;
scopes?: Array<string>;
successTemplate?: string;
errorTemplate?: string;
customLoopbackClient?: ILoopbackClient
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just call this loopbackClient

Suggested change
customLoopbackClient?: ILoopbackClient
loopbackClient?: ILoopbackClient

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link
Collaborator

@tnorling tnorling 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, 2 nits and any docs or samples we can add to help illustrate usage?

Doğan Erişen and others added 4 commits February 6, 2023 11:41
Co-authored-by: Thomas Norling <thomas.l.norling@gmail.com>
Co-authored-by: Thomas Norling <thomas.l.norling@gmail.com>
Copy link
Collaborator

@tnorling tnorling 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!

*/
async acquireTokenInteractive(request: InteractiveRequest): Promise<AuthenticationResult> {
public async acquireTokenInteractive(request: InteractiveRequest): Promise<AuthenticationResult> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: not needed

Suggested change
public async acquireTokenInteractive(request: InteractiveRequest): Promise<AuthenticationResult> {
async acquireTokenInteractive(request: InteractiveRequest): Promise<AuthenticationResult> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, but given the rest of acquireToken* implementations has the modifier explicitly, shouldn't we keep this for consistency?

@github-actions github-actions bot added the samples Related to the samples apps for the library. label Feb 6, 2023
@derisen
Copy link
Contributor Author

derisen commented Feb 6, 2023

Looks good, 2 nits and any docs or samples we can add to help illustrate usage?

done and done!

Copy link
Member

@sameerag sameerag 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. Do we have a docs (and .md file) defining why someone would need a loop back server? If yes, please link here, if not, can we add it? Can be handled as a separate PR.

@derisen
Copy link
Contributor Author

derisen commented Feb 6, 2023

Looks good. Do we have a docs (and .md file) defining why someone would need a loop back server? If yes, please link here, if not, can we add it? Can be handled as a separate PR.

Thanks! Added comments for API documentation and also to an Electron sample for usage. Need to update/re-organize some of our .md docs for API usage -so probably best to tackle this separately in another PR

@derisen derisen merged commit a5027ce into dev Feb 6, 2023
@derisen derisen deleted the custom-loopback-client branch February 6, 2023 23:41
@ghost
Copy link

ghost commented Mar 7, 2023

🎉@azure/msal-node@v1.16.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-node Related to msal-node package samples Related to the samples apps for the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with ElectronSystemBrowserTestApp deep linking with acquireTokenInteractive()
5 participants