Managed Identity - Reformatted ManagedIdentityTokenResponse + adjuste…

…d unit tests (#7167) Updated msal-common ResponseHandler's validateTokenResponse function to account for undefined portions of a network error. It now accounts for [different error formats](https://microsoft.sharepoint.com/teams/ADAL/_layouts/15/Doc.aspx?sourcedoc={0339bdd6-b5e5-4d94-b4c9-ea47127f5023}&action=edit&wd=target%28ID4S%2FMSALs%2FTechnical%2FMSI.one%7Ca2a57687-9a17-4c9e-86d1-60aa32ab5005%2FMSI%20Errors%20%20Exceptions%7C41fe3612-4fac-475d-9667-4a114aebeeda%2F%29&wdorigin=NavigationUrl) from all Managed Identity sources. Created new unit tests + overhauled existing Managed Identity unit tests. Manual testing was completed for all Managed Identity sources except Service Fabric.
AzureAD · Jun 25, 2024 · 348301b · 348301b
1 parent 362442e
commit 348301b
Show file tree

Hide file tree

Showing 16 changed files with 617 additions and 249 deletions.
diff --git a/change/@azure-msal-common-3040b4da-993d-4047-91f8-0405382492ce.json b/change/@azure-msal-common-3040b4da-993d-4047-91f8-0405382492ce.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Reformatted ManagedIdentityTokenResponse + adjusted unit tests #7167",
+  "packageName": "@azure/msal-common",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/change/@azure-msal-node-4b35e25c-7acf-468c-8323-9830342ad06c.json b/change/@azure-msal-node-4b35e25c-7acf-468c-8323-9830342ad06c.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Reformatted ManagedIdentityTokenResponse + adjusted unit tests #7167",
+  "packageName": "@azure/msal-node",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/lib/msal-common/apiReview/msal-common.api.md b/lib/msal-common/apiReview/msal-common.api.md
@@ -1650,6 +1650,7 @@ export const Constants: {
     NOT_DEFINED: string;
     EMPTY_STRING: string;
     NOT_APPLICABLE: string;
+    NOT_AVAILABLE: string;
     FORWARD_SLASH: string;
     IMDS_ENDPOINT: string;
     IMDS_VERSION: string;
@@ -3618,8 +3619,9 @@ export type ServerDeviceCodeResponse = {
 //
 // @public
 export class ServerError extends AuthError {
-    constructor(errorCode?: string, errorMessage?: string, subError?: string, errorNo?: string);
+    constructor(errorCode?: string, errorMessage?: string, subError?: string, errorNo?: string, status?: number);
     readonly errorNo?: string;
+    readonly status?: number;
 }
 
 // Warning: (ae-missing-release-tag) "ServerResponseType" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
@@ -4261,9 +4263,9 @@ const X_MS_LIB_CAPABILITY = "x-ms-lib-capability";
 // src/request/AuthenticationHeaderParser.ts:74:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
 // src/request/ScopeSet.ts:72:15 - (tsdoc-param-tag-with-invalid-type) The @param block should not include a JSDoc-style '{type}'
 // src/request/ScopeSet.ts:73:15 - (tsdoc-param-tag-with-invalid-type) The @param block should not include a JSDoc-style '{type}'
-// src/response/ResponseHandler.ts:419:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
-// src/response/ResponseHandler.ts:420:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
-// src/response/ResponseHandler.ts:421:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
+// src/response/ResponseHandler.ts:430:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
+// src/response/ResponseHandler.ts:431:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
+// src/response/ResponseHandler.ts:432:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
 // src/telemetry/performance/PerformanceClient.ts:886:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
 // src/telemetry/performance/PerformanceClient.ts:886:15 - (tsdoc-param-tag-with-invalid-type) The @param block should not include a JSDoc-style '{type}'
 // src/telemetry/performance/PerformanceClient.ts:898:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen

diff --git a/lib/msal-common/src/error/ServerError.ts b/lib/msal-common/src/error/ServerError.ts
@@ -14,15 +14,22 @@ export class ServerError extends AuthError {
      */
     readonly errorNo?: string;
 
+    /**
+     * Http status number;
+     */
+    readonly status?: number;
+
     constructor(
         errorCode?: string,
         errorMessage?: string,
         subError?: string,
-        errorNo?: string
+        errorNo?: string,
+        status?: number
     ) {
         super(errorCode, errorMessage, subError);
         this.name = "ServerError";
         this.errorNo = errorNo;
+        this.status = status;
 
         Object.setPrototypeOf(this, ServerError.prototype);
     }

diff --git a/lib/msal-common/src/response/ResponseHandler.ts b/lib/msal-common/src/response/ResponseHandler.ts
@@ -197,15 +197,26 @@ export class ResponseHandler {
             serverResponse.error_description ||
             serverResponse.suberror
         ) {
-            const errString = `${serverResponse.error_codes} - [${serverResponse.timestamp}]: ${serverResponse.error_description} - Correlation ID: ${serverResponse.correlation_id} - Trace ID: ${serverResponse.trace_id}`;
+            const errString = `Error(s): ${
+                serverResponse.error_codes || Constants.NOT_AVAILABLE
+            } - Timestamp: ${
+                serverResponse.timestamp || Constants.NOT_AVAILABLE
+            } - Description: ${
+                serverResponse.error_description || Constants.NOT_AVAILABLE
+            } - Correlation ID: ${
+                serverResponse.correlation_id || Constants.NOT_AVAILABLE
+            } - Trace ID: ${
+                serverResponse.trace_id || Constants.NOT_AVAILABLE
+            }`;
             const serverErrorNo = serverResponse.error_codes?.length
                 ? serverResponse.error_codes[0]
                 : undefined;
             const serverError = new ServerError(
                 serverResponse.error,
                 errString,
                 serverResponse.suberror,
-                serverErrorNo
+                serverErrorNo,
+                serverResponse.status
             );
 
             // check if 500 error

diff --git a/lib/msal-common/src/utils/Constants.ts b/lib/msal-common/src/utils/Constants.ts
@@ -45,6 +45,7 @@ export const Constants = {
     NOT_DEFINED: "not_defined",
     EMPTY_STRING: "",
     NOT_APPLICABLE: "N/A",
+    NOT_AVAILABLE: "Not Available",
     FORWARD_SLASH: "/",
     IMDS_ENDPOINT: "http://169.254.169.254/metadata/instance/compute/location",
     IMDS_VERSION: "2020-06-01",

diff --git a/lib/msal-common/test/error/ServerError.spec.ts b/lib/msal-common/test/error/ServerError.spec.ts
@@ -1,13 +1,18 @@
 import { ServerError } from "../../src/error/ServerError";
 import { AuthError } from "../../src/error/AuthError";
+import { Constants, HttpStatus } from "../../src";
 
 describe("ServerError.ts Class Unit Tests", () => {
     it("ServerError object can be created", () => {
         const TEST_ERROR_CODE: string = "test";
         const TEST_ERROR_MSG: string = "This is a test error";
+        const TEST_ERROR_STATUS: number = HttpStatus.BAD_REQUEST;
         const err: ServerError = new ServerError(
             TEST_ERROR_CODE,
-            TEST_ERROR_MSG
+            TEST_ERROR_MSG,
+            undefined,
+            undefined,
+            TEST_ERROR_STATUS
         );
 
         expect(err instanceof ServerError).toBe(true);
@@ -18,5 +23,21 @@ describe("ServerError.ts Class Unit Tests", () => {
         expect(err.message).toBe(`${TEST_ERROR_CODE}: ${TEST_ERROR_MSG}`);
         expect(err.name).toBe("ServerError");
         expect(err.stack?.includes("ServerError.spec.ts")).toBe(true);
+        expect(err.status).toBe(TEST_ERROR_STATUS);
+    });
+
+    it("Values are set as expected when no info was provided to ServerError", () => {
+        const err: ServerError = new ServerError();
+
+        expect(err instanceof ServerError).toBe(true);
+        expect(err instanceof AuthError).toBe(true);
+        expect(err instanceof Error).toBe(true);
+        expect(err.errorCode).toBe(Constants.EMPTY_STRING);
+        expect(err.errorMessage).toBe(Constants.EMPTY_STRING);
+        expect(err.errorNo).toBeUndefined();
+        expect(err.message).toBe(Constants.EMPTY_STRING);
+        expect(err.name).toBe("ServerError");
+        expect(err.stack?.includes("ServerError.spec.ts")).toBe(true);
+        expect(err.status).toBeUndefined();
     });
 });
diff --git a/lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts b/lib/msal-node/src/client/ManagedIdentitySources/BaseManagedIdentitySource.ts
@@ -102,8 +102,20 @@ export abstract class BaseManagedIdentitySource {
             refresh_in: refreshIn,
 
             // error
-            error: response.body.message,
-            correlation_id: response.body.correlationId,
+            correlation_id:
+                response.body.correlation_id || response.body.correlationId,
+            error:
+                typeof response.body.error === "string"
+                    ? response.body.error
+                    : response.body.error?.code,
+            error_description:
+                response.body.message ||
+                (typeof response.body.error === "string"
+                    ? response.body.error_description
+                    : response.body.error?.message),
+            error_codes: response.body.error_codes,
+            timestamp: response.body.timestamp,
+            trace_id: response.body.trace_id,
         };
 
         return serverTokenResponse;

diff --git a/lib/msal-node/src/response/ManagedIdentityTokenResponse.ts b/lib/msal-node/src/response/ManagedIdentityTokenResponse.ts
@@ -28,6 +28,28 @@ export type ManagedIdentityTokenResponse = {
     token_type?: AuthenticationScheme;
 
     // error
-    message?: string; // will be converted to error
-    correlationId?: string; // will be converted to correlation_id
+
+    /*
+     * (Web/Function) App Service
+     * 500 errors can return this from all MI sources as well
+     */
+    message?: string;
+    correlationId?: string;
+
+    // IMDS, Azure Arc, Service Fabric (unconfirmed)
+    error?: string | ErrorObject;
+    error_description?: string;
+    error_codes?: Array<string>;
+    correlation_id?: string;
+    timestamp?: string;
+    trace_id?: string;
+};
+
+/*
+ * This is the only error property that exists for Cloud Shell
+ * It can also be the only thing App Service will return
+ */
+export type ErrorObject = {
+    code: string;
+    message: string;
 };
diff --git a/lib/msal-node/test/client/ManagedIdentitySources/AppService.spec.ts b/lib/msal-node/test/client/ManagedIdentitySources/AppService.spec.ts
@@ -7,15 +7,22 @@ import { ManagedIdentityApplication } from "../../../src/client/ManagedIdentityA
 import {
     DEFAULT_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT,
     DEFAULT_USER_SYSTEM_ASSIGNED_MANAGED_IDENTITY_AUTHENTICATION_RESULT,
+    MANAGED_IDENTITY_APP_SERVICE_NETWORK_REQUEST_400_ERROR,
     MANAGED_IDENTITY_RESOURCE,
 } from "../../test_kit/StringConstants";
 
 import {
     userAssignedClientIdConfig,
     managedIdentityRequestParams,
     systemAssignedConfig,
+    networkClient,
+    ManagedIdentityNetworkErrorClient,
 } from "../../test_kit/ManagedIdentityTestUtils";
-import { AuthenticationResult } from "@azure/msal-common";
+import {
+    AuthenticationResult,
+    HttpStatus,
+    ServerError,
+} from "@azure/msal-common";
 import { ManagedIdentityClient } from "../../../src/client/ManagedIdentityClient";
 import {
     ManagedIdentityEnvironmentVariableNames,
@@ -106,4 +113,49 @@ describe("Acquires a token successfully via an App Service Managed Identity", ()
             );
         });
     });
+
+    describe("Errors", () => {
+        test("ensures that the error format is correct", async () => {
+            const managedIdentityNetworkErrorClient400 =
+                new ManagedIdentityNetworkErrorClient(
+                    MANAGED_IDENTITY_APP_SERVICE_NETWORK_REQUEST_400_ERROR,
+                    undefined,
+                    HttpStatus.BAD_REQUEST
+                );
+
+            jest.spyOn(networkClient, <any>"sendGetRequestAsync")
+                // permanently override the networkClient's sendGetRequestAsync method to return a 400
+                .mockReturnValue(
+                    managedIdentityNetworkErrorClient400.sendGetRequestAsync()
+                );
+
+            const managedIdentityApplication: ManagedIdentityApplication =
+                new ManagedIdentityApplication(systemAssignedConfig);
+            expect(managedIdentityApplication.getManagedIdentitySource()).toBe(
+                ManagedIdentitySourceNames.APP_SERVICE
+            );
+
+            let serverError: ServerError = new ServerError();
+            try {
+                await managedIdentityApplication.acquireToken(
+                    managedIdentityRequestParams
+                );
+            } catch (e) {
+                serverError = e as ServerError;
+            }
+
+            expect(
+                serverError.errorMessage.includes(
+                    MANAGED_IDENTITY_APP_SERVICE_NETWORK_REQUEST_400_ERROR.message as string
+                )
+            ).toBe(true);
+            expect(
+                serverError.errorMessage.includes(
+                    MANAGED_IDENTITY_APP_SERVICE_NETWORK_REQUEST_400_ERROR.correlation_id as string
+                )
+            ).toBe(true);
+
+            jest.restoreAllMocks();
+        });
+    });
 });