Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions sdk/identity/identity/review/identity.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ export class ClientSecretCredential implements TokenCredential {
getToken(scopes: string | string[], options?: GetTokenOptions): Promise<AccessToken | null>;
}

// @public
export class CredentialUnavailable extends Error {
}

// @public
export class DefaultAzureCredential extends ChainedTokenCredential {
constructor(tokenCredentialOptions?: TokenCredentialOptions);
Expand Down
7 changes: 7 additions & 0 deletions sdk/identity/identity/src/client/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,13 @@ function isErrorResponse(errorResponse: any): errorResponse is OAuthErrorRespons
);
}

/**
* This signifies that the credential that was tried in a chained credential
* was not available to be used as the credential. Rather than treating this as
* an error that should halt the chain, it's caught and the chain continues
*/
export class CredentialUnavailable extends Error {}

/**
* The Error.name value of an AuthenticationError
*/
Expand Down
8 changes: 4 additions & 4 deletions sdk/identity/identity/src/credentials/azureCliCredential.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-http";
import { createSpan } from "../util/tracing";
import { AuthenticationErrorName } from "../client/errors";
import { AuthenticationErrorName, CredentialUnavailable } from "../client/errors";
import { CanonicalCode } from "@opentelemetry/api";
import { logger } from "../util/logging";

Expand Down Expand Up @@ -86,15 +86,15 @@ export class AzureCliCredential implements TokenCredential {
obj.stderr.match("az:(.*)not found") ||
obj.stderr.startsWith("'az' is not recognized");
if (isNotInstallError) {
throw new Error(
throw new CredentialUnavailable(
"Azure CLI could not be found. Please visit https://aka.ms/azure-cli for installation instructions and then, once installed, authenticate to your Azure account using 'az login'."
);
} else if (isLoginError) {
throw new Error(
throw new CredentialUnavailable(
"Please run 'az login' from a command prompt to authenticate before using this credential."
);
}
throw new Error(obj.stderr);
throw new CredentialUnavailable(obj.stderr);
} else {
responseData = obj.stdout;
const response: { accessToken: string; expiresOn: string } = JSON.parse(responseData);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
// Licensed under the MIT License.

import { AccessToken, TokenCredential, GetTokenOptions } from "@azure/core-http";
import { AggregateAuthenticationError } from "../client/errors";
import { AggregateAuthenticationError, CredentialUnavailable } from "../client/errors";
import { createSpan } from "../util/tracing";
import { CanonicalCode } from "@opentelemetry/api";

Expand Down Expand Up @@ -52,7 +52,11 @@ export class ChainedTokenCredential implements TokenCredential {
try {
token = await this._sources[i].getToken(scopes, newOptions);
} catch (err) {
errors.push(err);
if (err instanceof CredentialUnavailable) {
errors.push(err);
} else {
throw err;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { AccessToken, TokenCredential, GetTokenOptions } from "@azure/core-http"
import { TokenCredentialOptions } from "../client/identityClient";
import { ClientSecretCredential } from "./clientSecretCredential";
import { createSpan } from "../util/tracing";
import { AuthenticationError, AuthenticationErrorName } from "../client/errors";
import { AuthenticationError, AuthenticationErrorName, CredentialUnavailable } from "../client/errors";
import { CanonicalCode } from "@opentelemetry/api";
import { logger } from "../util/logging";
import { ClientCertificateCredential } from "./clientCertificateCredential";
Expand Down Expand Up @@ -135,14 +135,11 @@ export class EnvironmentCredential implements TokenCredential {
// the user knows the credential was not configured appropriately
span.setStatus({ code: CanonicalCode.UNAUTHENTICATED });
span.end();
throw new AuthenticationError(400, {
error: "missing_environment_variables",
error_description: `EnvironmentCredential cannot return a token because one or more of the following environment variables is missing:
throw new CredentialUnavailable(`EnvironmentCredential cannot return a token because one or more of the following environment variables is missing:

${this._environmentVarsMissing.join("\n")}

To authenticate with a service principal AZURE_TENANT_ID, AZURE_CLIENT_ID, and either AZURE_CLIENT_SECRET or AZURE_CLIENT_CERTIFICATE_PATH must be set. To authenticate with a user account AZURE_TENANT_ID, AZURE_USERNAME, and AZURE_PASSWORD must be set.
`
});
`);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from "@azure/core-http";
import { IdentityClient, TokenCredentialOptions } from "../client/identityClient";
import { createSpan } from "../util/tracing";
import { AuthenticationErrorName } from "../client/errors";
import { AuthenticationErrorName, CredentialUnavailable } from "../client/errors";
import { CanonicalCode } from "@opentelemetry/api";
import { logger } from "../util/logging";

Expand Down Expand Up @@ -70,7 +70,7 @@ export class ManagedIdentityCredential implements TokenCredential {
let scope = "";
if (Array.isArray(scopes)) {
if (scopes.length !== 1) {
throw "To convert to a resource string the specified array must be exactly length 1";
throw new Error("To convert to a resource string the specified array must be exactly length 1");
}

scope = scopes[0];
Expand Down Expand Up @@ -334,6 +334,8 @@ export class ManagedIdentityCredential implements TokenCredential {
// endpoints are available. In this case, don't try them in future
// requests.
this.isEndpointUnavailable = result === null;
} else {
throw new CredentialUnavailable("The managed identity endpoint is not currently available");
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure if we want to throw an exception here. Currently, it looks like managed identity returns null rather than throwing to signal "failed because we couldn't reach the endpoint".

Adding the exception may change the existing API, and could be considered a breaking change.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would prefer to throw CredentialUnavailable in these cases, otherwise. I think the credential abstraction becomes unclear if sometimes when a credential is unavailable we throw and other times we return null. I fear this results in too many cases which need to handled everywhere credentials are used. However, since this would be a breaking change I am open to debate on this. @bterlson thoughts?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it is a breaking change. I also agree after talking with Scott that throwing an error makes sense. Would shipping a new major be out of the question for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure. @ramya-rao-a / @joshfree - how do we feel about a breaking change/major version bump for aligning how we do error reporting back to the user if a credential fails?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm in favor of making relatively minor changes like this to get JS inline with the other languages, especially while our product is still very young.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bterlson what's our plan around quirks mode for the JS SDKs?

@jonathandturner why don't we hold off on merging this PR until we have some offline discussions around versioning and compat?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@joshfree can you say more about your question re: quirks mode? Not sure I understand that term in this context.

I tend to agree it's very unlikely anyone would be broken by this, but it should also be fairly easy to cut a major if we need.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

had a conversation with @joshfree and given where we're at with this library, I'm ok taking this technically breaking change because the current behavior is unexpected and is in practice extremely unlikely to break anyone.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bterlson - I'm not sure if we can say it's unexpected if it's in the documentation that it works this way.

"If authentication cannot be performed at this time, this method may return null." from https://docs.microsoft.com/en-us/javascript/api/@azure/identity/managedidentitycredential?view=azure-node-latest

That being said, I agree that throwing an exception would be less surprising that returning null for some cases and throwing an exception in others. After chatting with @schaabs - seems like we can take this fix for the release after this one, which lets us decide what version we want to give it, too.

}

return result;
Expand Down
5 changes: 3 additions & 2 deletions sdk/identity/identity/src/credentials/vscodeCredential.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-http";
import { TokenCredentialOptions, IdentityClient } from '../client/identityClient';
import * as keytar from 'keytar';
import { CredentialUnavailable } from "../client/errors";

const CommonTenantId = 'common';
const AzureAccountClientId = 'aebc6443-996d-45c2-90f0-388ff96faa56'; // VSC: 'aebc6443-996d-45c2-90f0-388ff96faa56'
Expand Down Expand Up @@ -66,10 +67,10 @@ export class VSCodeCredential implements TokenCredential {
if (tokenResponse) {
return tokenResponse.accessToken;
} else {
return null;
throw new CredentialUnavailable("Could not retrieve the token associated with VSCode. Have you connected using the 'Azure Account' extension recently?");
}
} else {
return null;
throw new CredentialUnavailable("Could not retrieve the token associated with VSCode. Did you connect using the 'Azure Account' extension?");
}
}
}
3 changes: 2 additions & 1 deletion sdk/identity/identity/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ export {
ErrorResponse,
AggregateAuthenticationError,
AuthenticationErrorName,
AggregateAuthenticationErrorName
AggregateAuthenticationErrorName,
CredentialUnavailable
} from "./client/errors";

export { TokenCredential, GetTokenOptions, AccessToken } from "@azure/core-http";
Expand Down
10 changes: 5 additions & 5 deletions sdk/identity/identity/test/chainedTokenCredential.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import {
ChainedTokenCredential,
TokenCredential,
AccessToken,
AggregateAuthenticationError
AggregateAuthenticationError,
CredentialUnavailable
} from "../src";

function mockCredential(returnPromise: Promise<AccessToken | null>): TokenCredential {
Expand All @@ -19,7 +20,7 @@ function mockCredential(returnPromise: Promise<AccessToken | null>): TokenCreden
describe("ChainedTokenCredential", function() {
it("returns the first token received from a credential", async () => {
const chainedTokenCredential = new ChainedTokenCredential(
mockCredential(Promise.resolve(null)),
mockCredential(Promise.reject(new CredentialUnavailable("unavailable."))),
mockCredential(Promise.resolve({ token: "firstToken", expiresOnTimestamp: 0 })),
mockCredential(Promise.resolve({ token: "secondToken", expiresOnTimestamp: 0 }))
);
Expand All @@ -30,9 +31,8 @@ describe("ChainedTokenCredential", function() {

it("returns an AggregateAuthenticationError when no token is returned and one credential returned an error", async () => {
const chainedTokenCredential = new ChainedTokenCredential(
mockCredential(Promise.reject(new Error("Boom."))),
mockCredential(Promise.resolve(null)),
mockCredential(Promise.reject(new Error("Boom.")))
mockCredential(Promise.reject(new CredentialUnavailable("unavailable."))),
mockCredential(Promise.reject(new CredentialUnavailable("unavailable.")))
);

await assertRejects(
Expand Down
12 changes: 6 additions & 6 deletions sdk/identity/identity/test/node/environmentCredential.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import assert from "assert";
import path from "path";
import { EnvironmentCredential, AuthenticationError } from "../../src";
import { EnvironmentCredential, AuthenticationError, CredentialUnavailable } from "../../src";
import {
MockAuthHttpClient,
assertClientCredentials,
Expand Down Expand Up @@ -136,14 +136,14 @@ describe("EnvironmentCredential", function() {
assert.strictEqual(tracer.getActiveSpans().length, 0, "All spans should have had end called");
});

it("throws an AuthenticationError when getToken is called and no credential was configured", async () => {
it("throws an CredentialUnavailable when getToken is called and no credential was configured", async () => {
const mockHttpClient = new MockAuthHttpClient();

const credential = new EnvironmentCredential(mockHttpClient.tokenCredentialOptions);
await assertRejects(
credential.getToken("scope"),
(error: AuthenticationError) =>
error.errorResponse.errorDescription.indexOf(AllSupportedEnvironmentVariables.join("\n")) >
(error: CredentialUnavailable) =>
error.message.indexOf(AllSupportedEnvironmentVariables.join("\n")) >
-1
);

Expand All @@ -152,8 +152,8 @@ describe("EnvironmentCredential", function() {
const credentialDeux = new EnvironmentCredential(mockHttpClient.tokenCredentialOptions);
await assertRejects(
credentialDeux.getToken("scope"),
(error: AuthenticationError) =>
error.errorResponse.errorDescription.match(/^AZURE_TENANT_ID/gm) === null
(error: CredentialUnavailable) =>
error.message.match(/^AZURE_TENANT_ID/gm) === null
);

delete process.env.AZURE_TENANT_ID;
Expand Down
20 changes: 14 additions & 6 deletions sdk/identity/identity/test/node/managedIdentityCredential.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@

import qs from "qs";
import assert from "assert";
import { ManagedIdentityCredential } from "../../src";
import { ManagedIdentityCredential, CredentialUnavailable } from "../../src";
import {
ImdsEndpoint,
ImdsApiVersion,
AppServiceMsiApiVersion
} from "../../src/credentials/managedIdentityCredential";
import { MockAuthHttpClient, MockAuthHttpClientOptions } from "../authTestUtils";
import { MockAuthHttpClient, MockAuthHttpClientOptions, assertRejects } from "../authTestUtils";
import { WebResource, AccessToken } from "@azure/core-http";

interface AuthRequestDetails {
Expand Down Expand Up @@ -108,11 +108,19 @@ describe("ManagedIdentityCredential", function() {
// Run getToken twice and verify that an auth request is only
// attempted the first time. It should be skipped the second
// time after no IMDS endpoint was found.
const firstGetToken = await credential.getToken("scopes");
const secondGetToken = await credential.getToken("scopes");
await assertRejects(
credential.getToken("scope"),
(error: CredentialUnavailable) =>
error.message.indexOf("The managed identity endpoint is not currently available") == 0
);


await assertRejects(
credential.getToken("scope"),
(error: CredentialUnavailable) =>
error.message.indexOf("The managed identity endpoint is not currently available") == 0
);

assert.strictEqual(firstGetToken, null);
assert.strictEqual(secondGetToken, null);
assert.strictEqual(mockHttpClient.requests.length, 1);
});

Expand Down