-
Notifications
You must be signed in to change notification settings - Fork 32
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
Open up token audience for authentication #8
Conversation
// We dont need to get the subscriptionList if the tokenAudience is graph as graph clients are tenant based. | ||
if (!(options.tokenAudience && options.tokenAudience === TokenAudience.graph)) { | ||
// We only need to get the subscriptionList if the tokenAudience is for a management client. | ||
if (options.tokenAudience && options.tokenAudience === options.environment.activeDirectoryResourceId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check also needs to happen for interactive login. I think we missed adding it over there while doing the initial port from ms-rest-azure.
After building the tenantList, we get subscriptions from different tenants. This exercise populates the token cache with tokens across all the tenants. However, this exercise should only be done for managementPlane token audience (whether logging in via username/password, service/principal or interactive).
So the check happening over here in the old runtime needs to happen in this project over here as well. Basically pass an empty list of tenants to getSubscriptionFromTenants if the tokenAudience is not a managementplane token audience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore this comment. I just saw that you have taken care of this below.
@@ -455,7 +455,7 @@ export async function withInteractiveWithAuthResponse(options?: InteractiveLogin | |||
if (error) { | |||
return reject(error); | |||
} | |||
interactiveOptions.username = tokenResponse.userId; | |||
interactiveOptions.userName = tokenResponse.userId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amarzavery @RikkiGibson FYI, I think this fixes interactive login. ADAL couldn't match the token in the cache because it used [email protected] as cache key, while the token had been acquired correctly with the user's email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Thanks for fixing this. This is absolutely correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most JavaScript bug ever.
Thanks a ton for taking time and sending a PR 👍 . |
THANK YOU @alexeldeib! |
@@ -31,7 +30,7 @@ export declare abstract class TokenCredentialsBase { | |||
tokenCache: any; | |||
protected readonly isGraphContext: boolean; | |||
protected readonly authContext: any; | |||
constructor(clientId: string, domain: string, tokenAudience?: TokenAudience | undefined, environment?: { | |||
constructor(clientId: string, domain: string, tokenAudience?: string | undefined, environment?: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should still be TokenAudience | undefined. Since over here we have the TokenAudience type defined.
Moreover, we should keep the code backwards compatible. ms-rest-azure supported "graph" | "batch" | undefined
. Can you please add support for batch as well?
I think this change should also happen in ms-rest-azure-env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compiler is being smart and simplifying the union type of string with string automatically to remove the enum. I fiddled around with ways to trick the compiler (union type of string with enum of string, etc) but didn't get much.
I can manually fix the typings to use TokenAudience, it will cause drift in the future for generation. Just a thought for reference.
I'll add batch too.
@@ -31,7 +31,7 @@ export abstract class TokenCredentialsBase { | |||
throw new Error("domain must be a non empty string."); | |||
} | |||
|
|||
if (this.tokenAudience === TokenAudience.graph) { | |||
if (this.tokenAudience === "graph") { | |||
this.isGraphContext = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we only supported graph, this property isGraphContext
made sense. I believe don't need this anymore.
? this.environment.activeDirectoryGraphResourceId | ||
: this.environment.activeDirectoryResourceId; | ||
|
||
if (this.tokenAudience) { | ||
resource = this.tokenAudience; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be much simpler now
protected getActiveDirectoryResourceId(): string {
let resource = this.environment.activeDirectoryResourceId;
if (this.tokenAudience) {
resource = this.tokenAudience;
if (this.tokenAudience === "graph") {
resource = this.environment.activeDirectoryGraphResourceId;
} else if (this.tokenAudience === "graph") {
resource = this.environment.batchResourceId;
}
}
return resource;
}
I see. Then just having graph | batch | string | undefined would work |
Actually, I think something weirder is happening. The compiler uses TokenAudience on the non-abstract implementations of TokenCredentialsBase, but it doesn't want to use accept TokenAudience type on the abstract base class? Might be something weird on my machine/locally; happy to leave it whichever way you prefer, since it doesn't seem like there's an inherent issue. |
@@ -16,8 +16,8 @@ export class ApplicationTokenCredentials extends TokenCredentialsBase { | |||
* @param {string} clientId The active directory application client id. | |||
* @param {string} domain The domain or tenant id containing this application. | |||
* @param {string} secret The authentication secret for the application. | |||
* @param {string} [tokenAudience] The audience for which the token is requested. Valid value is "graph". If tokenAudience is provided | |||
* then domain should also be provided its value should not be the default "common" tenant. It must be a string (preferrably in a guid format). | |||
* @param {string} [tokenAudience] The audience for which the token is requested. Valid values are 'graph' or any other resource like 'https://vault.azure.com/'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
graph or batch or ...
* @param {string} [tokenAudience] The audience for which the token is requested. Valid value is "graph". If tokenAudience is provided | ||
* then domain should also be provided and its value should not be the default "common" tenant. It must be a string (preferrably in a guid format). | ||
* @param {string} [tokenAudience] The audience for which the token is requested. Valid values are 'graph' or any other resource like 'https://vault.azure.com/'. | ||
* If tokenAudience is 'graph' then domain should also be provided and its value should not be the default 'common' tenant. It must be a string (preferrably in a guid format). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above.
resource = this.tokenAudience; | ||
if (this.tokenAudience === "graph") { | ||
resource = this.environment.activeDirectoryGraphResourceId; | ||
} else if (this.tokenAudience === "graph") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=== "batch"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I messed it up while providing the code snippet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it, and then re copied it wrong 😄
const getUserCode = new Promise<any>((resolve, reject) => { | ||
return authContext.acquireUserCode(interactiveOptions.environment.activeDirectoryResourceId, interactiveOptions.clientId, interactiveOptions.language, (err: Error, userCodeRes: any) => { | ||
|
||
function tryAcquireToken(interactiveOptions: InteractiveLoginOptions, resolve: any, reject: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amarzavery This is about as close as I could get to the other implementation, but I haven't hit the mentioned issue or have a repro, so PSA that this is untested from me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. No worries. Looking at the changes it looks good to me.
Referencing this PR Azure/ms-rest-azure-env#2 as it is related to this change. |
Just published ms-rest-azure-env: "0.1.1" with support for batchResourceId. |
@alexeldeib - BIG BIG thank you for helping us improve the quality of ms-rest-nodeauth. Merging this PR. |
@amarzavery thanks in return for the quick turnaround! Glad I could contribute 😄 |
No description provided.