[identity] DeviceCodeCredential uses MSALClient #29405
Conversation
|
API change check API changes are not detected in this pull request. |
|
/azp run js - identity - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
maorleger
force-pushed
the
migrate-device-code-credential
branch
from
4b12f21
to
3fba79b
Compare
maorleger
force-pushed
the
migrate-device-code-credential
branch
from
29c4764
to
c318ba0
Compare
maorleger
requested review from
pvaneck,
billwert,
ahsonkhan,
scottaddie and
xiangyan99
| } | ||
|
|
||
| /** | ||
| * Options for creating an instance of the MsalClient. | ||
| */ | ||
| export type MsalClientOptions = Partial<Omit<MsalNodeOptions, "clientId" | "tenantId">>; | ||
| export type MsalClientOptions = Partial< |
There was a problem hiding this comment.
Slowly working on defining this type. It's probably fine for now, but I'll clean this up soonish
| return this.msalFlow.getActiveAccount(); | ||
| await this.msalClient.getTokenByDeviceCode(arrayScopes, this.userPromptCallback, { | ||
| ...newOptions, | ||
| disableAutomaticAuthentication: false, // this method should always allow user interaction |
There was a problem hiding this comment.
I understand that we want to not attempt silent authentication, but the naming of this parameter confuses me since you'd think you'd want to disable automatic authentication rather than setting this to false which would enable it?
There was a problem hiding this comment.
Yeah, I agree that the naming is confusing. The reason I stuck with it is because it matches the same behavior and meaning as the public API's version of disableAutomaticAuthentication which can help when searching the codebase for this.
Personally I think a name like disableInteractiveAuthentication makes more sense, but I'm not sure if I want to stay consistent with the meaning of the public API field or do the name-mapping here
What this actually does, if I'm reading the source code correctly, is disable interactive auth - if an AuthenticationRequiredError is thrown from getTokenSilent and this option is true we will throw instead of continuing on to the interactive auth.
With that context in place - I don't have strong feelings and am happy to change it, but I have a slight preference to keeping the name and meaning consistent with the public API for better or worse. What do you think? @KarishmaGhiya would love your take as well!
There was a problem hiding this comment.
@maorleger Is this param on L132 actually set by the public API param (L105) ? Or is this used for disabling interactive auth in another scenario (other than when passed in as a parameter by the user) ?
There was a problem hiding this comment.
This is used for enabling interactive auth fallback when the user passes is disableAutomaticAuthentication: true - so in the getToken call we will simply pass whatever the user passed in the public API param. In authenticate we ignore what the user passed in because authenticate is meant to be called by the user when getToken fails to silent auth, and disableAutomaticAuthentication is true.
Here's a few scenarios:
| user sets disableAutomaticAuthentication | deviceCodeCredential#getToken | deviceCodeCredential#authenticate |
|---|---|---|
| undefined | false | false |
| false | false | false |
| true | true | false |
Happy to discuss more in our sync!
|
|
||
| /** | ||
| * The logger for all MsalClient instances. | ||
| */ | ||
| const msalLogger = credentialLogger("MsalClient"); | ||
|
|
||
| export interface GetTokenWithSilentAuthOptions extends GetTokenOptions { | ||
| /** | ||
| * Disables automatic authentication. If set to true, the method will throw an error if the user needs to authenticate. |
There was a problem hiding this comment.
should this be called throwOnUserAuthenticationNeeded or something similar?
There was a problem hiding this comment.
Heya @xirzec - Karishma and I chatted offline and we think that as backwards as this name is we will keep it throughout the codebase for consistency. At least it matches the meaning and behavior of the public API for better or worse. If you feel strongly, happy to make a follow-up PR with some renames! But merging for now
Packages impacted by this PR
@azure/identity
Issues associated with this PR
Resolves #29377
Describe the problem that is addressed by this PR
This PR makes the following changes to the Identity library:
The reason this is important and exciting is because DeviceCodeCredential is the first PublicClientApplication based
flow that is being migrated to MSALClient!
Checklists