Skip to content

Commit

Permalink
[Identity] Decoupling CredentialUnavailableError from AuthenticationR…
Browse files Browse the repository at this point in the history
…equiredError (Azure#14775)

`AuthenticationRequiredError` (introduced in 2.0.0-beta.1) now doesn't extend `CredentialUnavailableError`.
  - `AuthenticationRequiredError`, besides being useful to express when a manual authentication with `authenticate()` is required, it also shares a similarity with `CredentialUnavailableError` in which both were designed to tell `ChainedTokenCredential` to move ahead and try another credential.
  - Because of how prone to errors is `instanceof`, we intentionally want to focus on asserting errors through the `error.name` property of each error.
  - Since each error writes its own name, the proper way to check for both errors is to check for both error names, thus making inheritance unnecessary for that use case.

Besides the code change, this PR also extends a current test with a simple case that showcases the underlying functionality.

Fixes Azure#14751
  • Loading branch information
sadasant authored Apr 8, 2021
1 parent 93d1131 commit 1784739
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 5 deletions.
1 change: 1 addition & 0 deletions sdk/identity/identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

## 2.0.0-beta.3 (Unreleased)

- `AuthenticationRequiredError` (introduced in 2.0.0-beta.1) now has the same impact on `ChainedTokenCredential` as the `CredentialUnavailableError` which is to allow the next credential in the chain to be tried.

## 2.0.0-beta.2 (2021-04-06)

Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/identity/review/identity.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export interface AuthenticationRecord {
}

// @public
export class AuthenticationRequiredError extends CredentialUnavailableError {
export class AuthenticationRequiredError extends Error {
constructor(
scopes: string[],
getTokenOptions?: GetTokenOptions, message?: string);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,10 @@ export class ChainedTokenCredential implements TokenCredential {
try {
token = await this._sources[i].getToken(scopes, updatedOptions);
} catch (err) {
if (err.name === "CredentialUnavailableError") {
if (
err.name === "CredentialUnavailableError" ||
err.name === "AuthenticationRequiredError"
) {
errors.push(err);
} else {
logger.getToken.info(formatError(scopes, err));
Expand Down
3 changes: 1 addition & 2 deletions sdk/identity/identity/src/msal/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@
// Licensed under the MIT license.

import { GetTokenOptions } from "@azure/core-http";
import { CredentialUnavailableError } from "../client/errors";

/**
* Error used to enforce authentication after trying to retrieve a token silently.
*/
export class AuthenticationRequiredError extends CredentialUnavailableError {
export class AuthenticationRequiredError extends Error {
constructor(
/**
* The list of scopes for which the token will have access.
Expand Down
12 changes: 11 additions & 1 deletion sdk/identity/identity/test/public/chainedTokenCredential.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import {
TokenCredential,
AccessToken,
AggregateAuthenticationError,
CredentialUnavailableError
CredentialUnavailableError,
AuthenticationRequiredError
} from "../../src";

function mockCredential(returnPromise: Promise<AccessToken | null>): TokenCredential {
Expand All @@ -21,6 +22,15 @@ describe("ChainedTokenCredential", function() {
it("returns the first token received from a credential", async () => {
const chainedTokenCredential = new ChainedTokenCredential(
mockCredential(Promise.reject(new CredentialUnavailableError("unavailable."))),
mockCredential(
Promise.reject(
new AuthenticationRequiredError(
["https://vault.azure.net/.default"],
{},
"authentication-required."
)
)
),
mockCredential(Promise.resolve({ token: "firstToken", expiresOnTimestamp: 0 })),
mockCredential(Promise.resolve({ token: "secondToken", expiresOnTimestamp: 0 }))
);
Expand Down

0 comments on commit 1784739

Please sign in to comment.