-
Notifications
You must be signed in to change notification settings - Fork 33
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
Make token credentials compatible with @azure/core-auth's TokenCredential #66
Closed
Closed
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
554e56f
Make token credentials compatible with core-auth's TokenCredential
daviwil a7f3433
Bump version to 3.0.0
daviwil 6ab4b17
Store intermediate prepareToken result in a const before returning
daviwil abfbfb0
Don't export TokenResponseLike
daviwil fab4f86
Explicitly check for undefined in prepareToken
daviwil 3f078c6
Verify MSI creds return AccessToken when used like TokenCredential
daviwil 10e63d5
Make prepareToken more tolerant of omitted expiresOn
daviwil 1652892
Add missing 'async' keyword to method overloads
daviwil d100d74
Add @azure/abort-controller as a devDependency
daviwil e9bafdb
Update 3.0.0 release date in Changelog.md
daviwil f6b5783
Merge remote-tracking branch 'origin/master' into pr/daviwil/66
ramya-rao-a e8320b8
Merge pull request #1 from ramya-rao-a/v3
daviwil File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
import { AccessToken } from "@azure/core-auth"; | ||
|
||
interface TokenResponseLike { | ||
accessToken: string; | ||
expiresOn?: Date | string; | ||
} | ||
|
||
/** | ||
* Prepares a TokenResponse to be returned as a TokenResponse or | ||
* a @azure/core-auth AccessToken depending on whether the 'scopes' | ||
* parameter is null or not (the key to determining which getToken | ||
* method has been called). | ||
*/ | ||
export function prepareToken<T extends TokenResponseLike>( | ||
token: T, | ||
scopes: string | string[] | undefined): T | AccessToken { | ||
// Scopes will contain _some_ value if a parameter was passed to getToken | ||
if (scopes !== undefined) { | ||
// Start with a default 'expiresOn' and then replace with | ||
// the actual 'expiresOn' if one is given | ||
let expiresOnTimestamp: number = Date.now() + 60 * 60 * 1000; | ||
if (token.expiresOn) { | ||
expiresOnTimestamp = | ||
typeof token.expiresOn === "string" | ||
? Date.parse(token.expiresOn) | ||
: token.expiresOn.getTime(); | ||
} | ||
|
||
return { | ||
token: token.accessToken, | ||
expiresOnTimestamp | ||
} as AccessToken; | ||
} else { | ||
return token; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,14 @@ | ||
// Copyright (c) Microsoft Corporation. All rights reserved. | ||
// Licensed under the MIT License. See License.txt in the project root for license information. | ||
|
||
import { prepareToken } from "./coreAuthHelpers"; | ||
import { TokenCredentialsBase } from "./tokenCredentialsBase"; | ||
import { TokenCredential, AccessToken, GetTokenOptions } from "@azure/core-auth"; | ||
import { Environment } from "@azure/ms-rest-azure-env"; | ||
import { AuthConstants, TokenAudience } from "../util/authConstants"; | ||
import { TokenResponse, TokenCache } from "adal-node"; | ||
|
||
export class DeviceTokenCredentials extends TokenCredentialsBase { | ||
export class DeviceTokenCredentials extends TokenCredentialsBase implements TokenCredential { | ||
|
||
readonly username: string; | ||
|
||
|
@@ -53,8 +55,11 @@ export class DeviceTokenCredentials extends TokenCredentialsBase { | |
this.username = username; | ||
} | ||
|
||
public getToken(): Promise<TokenResponse> { | ||
public async getToken(): Promise<TokenResponse>; | ||
public async getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken>; | ||
public async getToken(scopes?: string | string[]): Promise<TokenResponse | AccessToken> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing async keywords on the overloads |
||
// For device auth, this is just getTokenFromCache. | ||
return this.getTokenFromCache(this.username); | ||
const token = prepareToken(await this.getTokenFromCache(this.username), scopes); | ||
return token; | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 assumes that
expiresOn
exists on all the credential types in ms-rest-nodeauth.Are we sure about this assumption?
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.
Good point, I can't find
expiresOn
being assigned anywhere in a few of the credential implementations. @amarzavery, do you remember how users are supposed to deal with token expiration in this library? I only seeAzureCliCredentials
implementing any kind of token refresh logic.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.
adal-node has an in memory token cache. It gets a new token if it is about to get expired or gives the token from the token cache.
For AzureCliCredentials we are calling the cli to authenticate and give us the new token that will be valid for the next one hour. Cli knows how to refresh the token if it is using it internally. We can't be calling the Cli for every request, as that will terribly slow down the sdk (spawning a new process and asking the Cli for creds).
Thus we try to do the same thing that adal-python would do anyways (if we are within the token refresh window (last 5 mins before the token expiry) then ask the Cli for token, it will ask adal-python which will then get the new token that will be valid for the next one hour).
Thus we need to check the expiresOn property.
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've loosened the expectation of an
expiresOn
property on the token and put a default expiration timestamp of an hour if one is not provided. What do you think?