-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[Identity] ChainedTokenCredential logging fix #14847
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
Changes from 4 commits
c181ebc
4de802b
f3b4e9f
ba5d542
6687093
b706286
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,7 +94,7 @@ export function credentialLoggerInstance( | |
| const fullTitle = parent ? `${parent.fullTitle} ${title}` : title; | ||
|
|
||
| function info(message: string): void { | ||
| log.info(`${fullTitle} =>`, message); | ||
| log.info(`${fullTitle} => ${message}`); | ||
| } | ||
|
|
||
| return { | ||
|
|
@@ -109,6 +109,7 @@ export function credentialLoggerInstance( | |
| * It has all the properties of a CredentialLoggerInstance, plus other logger instances, one per method. | ||
| */ | ||
| export interface CredentialLogger extends CredentialLoggerInstance { | ||
| parent: AzureLogger; | ||
| getToken: CredentialLoggerInstance; | ||
| } | ||
|
|
||
|
|
@@ -126,6 +127,7 @@ export function credentialLogger(title: string, log: AzureLogger = logger): Cred | |
| const credLogger = credentialLoggerInstance(title, undefined, log); | ||
| return { | ||
| ...credLogger, | ||
| parent: log, | ||
|
Contributor
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. What does this change do? The PR description does not have anything regarding this.
Contributor
Author
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. This is an internal change that allows us to test what the parent is logging. |
||
| getToken: credentialLoggerInstance("=> getToken()", credLogger, log) | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT license. | ||
|
|
||
| import assert from "assert"; | ||
| import { ChainedTokenCredential, TokenCredential, AccessToken } from "../../../src"; | ||
| import Sinon from "sinon"; | ||
| import { logger as chainedTokenCredentialLogger } from "../../../src/credentials/chainedTokenCredential"; | ||
|
|
||
| class TestMockCredential implements TokenCredential { | ||
| constructor(public returnPromise: Promise<AccessToken | null>) {} | ||
|
|
||
| getToken(): Promise<AccessToken | null> { | ||
| return this.returnPromise; | ||
| } | ||
| } | ||
|
|
||
| describe("ChainedTokenCredential", function() { | ||
| it("Logs the expected successful message", async () => { | ||
| const chainedTokenCredential = new ChainedTokenCredential( | ||
| new TestMockCredential(Promise.resolve({ token: "firstToken", expiresOnTimestamp: 0 })) | ||
| ); | ||
|
|
||
| const infoSpy = Sinon.spy(chainedTokenCredentialLogger.parent, "info"); | ||
| const getTokenInfoSpy = Sinon.spy(chainedTokenCredentialLogger.getToken, "info"); | ||
|
|
||
| const accessToken = await chainedTokenCredential.getToken("<scope>"); | ||
| assert.notStrictEqual(accessToken, null); | ||
|
|
||
| assert.equal( | ||
| infoSpy.getCalls()[0].args[0], | ||
| "ChainedTokenCredential => getToken() => Result for TestMockCredential: SUCCESS. Scopes: <scope>." | ||
| ); | ||
| assert.equal( | ||
| getTokenInfoSpy.getCalls()[0].args[0], | ||
| "Result for TestMockCredential: SUCCESS. Scopes: <scope>." | ||
| ); | ||
|
|
||
| infoSpy.restore(); | ||
| getTokenInfoSpy.restore(); | ||
| }); | ||
| }); |
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.
Imagine I made up my own credential following the TokenCredential interface but without using a class.
Then,
this._sources[i].constructorwould be undefinedUh oh!
There was an error while loading. Please reload this page.
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 name will be the name of the parent class, which is probably "Object" for the case you describe.
I don't think our code is thought to work at all with credentials without a proper constructor.name. There are more places where this could be an issue.
I can log an issue, what do you think?
Uh oh!
There was an error while loading. Please reload this page.
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 made an issue to follow up later: #14848