Addressed comments

AzureAD · Jul 18, 2023 · 664bd82 · 664bd82
1 parent 66b495c
commit 664bd82
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 34 deletions.
diff --git a/lib/msal-browser/src/interaction_client/PopupClient.ts b/lib/msal-browser/src/interaction_client/PopupClient.ts
@@ -579,14 +579,7 @@ export class PopupClient extends StandardInteractionClient {
                 }
 
                 const href = popupWindow.location.href;
-                let serverResponseString;
-                if(this.config.auth.protocolMode === ProtocolMode.OIDC && 
-                    this.config.auth.OIDCOptions?.serverResponseType === ServerResponseType.QUERY) {
-                        serverResponseString = UrlString.parseQueryServerResponse(href);
-                }
-                else {
-                    serverResponseString = popupWindow.location.hash;
-                }
+                const serverResponseString = this.extractServerResponseStringFromPopup(popupWindow, href);
                 try {
                     /*
                      * Will throw if cross origin,
@@ -878,4 +871,22 @@ export class PopupClient extends StandardInteractionClient {
         const homeAccountId = request.account && request.account.homeAccountId;
         return `${BrowserConstants.POPUP_NAME_PREFIX}.${this.config.auth.clientId}.${homeAccountId}.${this.correlationId}`;
     }
+
+    /** 
+     * Extracts the server response from the popup window
+     */
+    extractServerResponseStringFromPopup(
+        popupWindow: Window,
+        href: string
+    ): string {
+        let serverResponseString;
+        if(this.config.auth.protocolMode === ProtocolMode.OIDC && 
+            this.config.auth.OIDCOptions?.serverResponseType === ServerResponseType.QUERY) {
+                serverResponseString = UrlString.parseQueryServerResponse(href);
+        }
+        else {
+            serverResponseString = popupWindow.location.hash;
+        }
+        return serverResponseString;
+    }
 }
diff --git a/lib/msal-browser/src/interaction_client/StandardInteractionClient.ts b/lib/msal-browser/src/interaction_client/StandardInteractionClient.ts
@@ -439,14 +439,12 @@ export abstract class StandardInteractionClient extends BaseInteractionClient {
             this.correlationId
         );
 
-        const configResponseMode: ResponseMode = this.config.auth.OIDCOptions.serverResponseType;
-
         const validatedRequest: AuthorizationUrlRequest = {
             ...(await this.initializeBaseRequest(request)),
             redirectUri: redirectUri,
             state: state,
             nonce: request.nonce || this.browserCrypto.createNewGuid(),
-            responseMode: configResponseMode,
+            responseMode: this.config.auth.OIDCOptions.serverResponseType as ResponseMode,
         };
 
         const account =

diff --git a/lib/msal-browser/test/app/PublicClientApplication.spec.ts b/lib/msal-browser/test/app/PublicClientApplication.spec.ts
@@ -48,7 +48,6 @@ import {
     Authority,
     AuthError,
     ProtocolMode,
-    ClientConfigurationError,
     ServerResponseType
 } from "@azure/msal-common";
 import {
@@ -829,11 +828,9 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
             expect(tokenResponse1).toEqual(tokenResponse2);
             expect(tokenResponse4).toEqual(tokenResponse1);
         });
-        it("Looks for hash in query param if OIDCOptions.serverResponseType is set to query", async () => {
-            /**
-             * The testing environment does not accept query params, so instead we see that it ignores a hash fragment.
-             * That means handleRedirectPromise is looking for a hash in a query param instead of a hash fragment.
-             */
+    });
+    describe("OIDC Protocol Mode tests", () => {
+        beforeEach(() => {
             pca = new PublicClientApplication({
                 auth: {
                     clientId: TEST_CONFIG.MSAL_CLIENT_ID,
@@ -850,7 +847,12 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
                     allowNativeBroker: false,
                 },
             });
-
+        });
+        it("Looks for server code response in query param if OIDCOptions.serverResponseType is set to query", async () => {
+            /**
+             * The testing environment does not accept query params, so instead we see that it ignores a hash fragment.
+             * That means handleRedirectPromise is looking for a hash in a query param instead of a hash fragment.
+             */
             sinon
                 .stub(RedirectClient.prototype, "handleRedirectPromise")
                 .callsFake(async (hash): Promise<AuthenticationResult | null> => {

diff --git a/lib/msal-browser/test/interaction_client/PopupClient.spec.ts b/lib/msal-browser/test/interaction_client/PopupClient.spec.ts
@@ -1586,7 +1586,7 @@ describe("PopupClient", () => {
             });
         });
 
-        it("returns hash in query form when serverResponseType in OIDCOptions is query", (done) => {
+        it("returns server code response in query form when serverResponseType in OIDCOptions is query", (done) => {
             pca = new PublicClientApplication({
                 auth: {
                     clientId: TEST_CONFIG.MSAL_CLIENT_ID,
@@ -1621,7 +1621,7 @@ describe("PopupClient", () => {
 
             const popup = {
                 location: {
-                    href: "http://localhost/?code=hello",
+                    href: TEST_URIS.TEST_QUERY_CODE_RESPONSE,
                 },
                 history: {
                     replaceState: () => {

diff --git a/lib/msal-browser/test/utils/StringConstants.ts b/lib/msal-browser/test/utils/StringConstants.ts
@@ -34,6 +34,7 @@ export const TEST_URIS = {
     TEST_AZURE_CHINA_INSTANCE: "https://login.chinacloudapi.cn/",
     TEST_AZURE_GERMANY_INSTANCE: "https://login.microsoftonline.de/",
     TEST_AZURE_USGOV_INSTANCE: "https://login.microsoftonline.us/",
+    TEST_QUERY_CODE_RESPONSE: "http://localhost/?code=hello",
 };
 
 // Test MSAL config params

diff --git a/lib/msal-common/src/client/AuthorizationCodeClient.ts b/lib/msal-common/src/client/AuthorizationCodeClient.ts
@@ -15,7 +15,6 @@ import {
     Separators,
     AADServerParamKeys,
     HeaderNames,
-    ServerResponseType,
 } from "../utils/Constants";
 import { ClientConfiguration } from "../config/ClientConfiguration";
 import { ServerAuthorizationTokenResponse } from "../response/ServerAuthorizationTokenResponse";
@@ -48,12 +47,14 @@ import { PerformanceEvents } from "../telemetry/performance/PerformanceEvent";
 export class AuthorizationCodeClient extends BaseClient {
     // Flag to indicate if client is for hybrid spa auth code redemption
     protected includeRedirectUri: boolean = true;
+    private oidcDefaultScopes;
 
     constructor(
         configuration: ClientConfiguration,
         performanceClient?: IPerformanceClient
     ) {
         super(configuration, performanceClient);
+        this.oidcDefaultScopes = this.config.authOptions.authority.options.OIDCOptions?.defaultScopes;
     }
 
     /**
@@ -200,16 +201,7 @@ export class AuthorizationCodeClient extends BaseClient {
             null
         );
 
-        // Deserialize hash fragment response parameters.
-        const hashUrlString = new UrlString(hashFragment);
-        // Deserialize hash fragment response parameters.
-        let serverParams : ServerAuthorizationCodeResponse;
-        if(this.config.authOptions.authority.options.OIDCOptions?.serverResponseType === ServerResponseType.QUERY) {
-            serverParams = UrlString.getDeserializedQueryString(hashFragment);
-        }
-        else{
-            serverParams = UrlString.getDeserializedHash(hashUrlString.getHash());
-        }
+        const serverParams : ServerAuthorizationCodeResponse = UrlString.getDeserializedCodeResponse(this.config.authOptions.authority.options.OIDCOptions?.serverResponseType, hashFragment);
 
         // Get code response
         responseHandler.validateServerAuthorizationCodeResponse(
@@ -344,7 +336,7 @@ export class AuthorizationCodeClient extends BaseClient {
         }
 
         // Add scope array, parameter builder will add default scopes and dedupe
-        parameterBuilder.addScopes(request.scopes, undefined, this.config.authOptions.authority.options.OIDCOptions?.defaultScopes);
+        parameterBuilder.addScopes(request.scopes, undefined, this.oidcDefaultScopes);
 
         // add code: user set, not validated
         parameterBuilder.addAuthorizationCode(request.code);
@@ -504,7 +496,7 @@ export class AuthorizationCodeClient extends BaseClient {
             ...(request.scopes || []),
             ...(request.extraScopesToConsent || []),
         ];
-        parameterBuilder.addScopes(requestScopes, undefined, this.config.authOptions.authority.options.OIDCOptions?.defaultScopes);
+        parameterBuilder.addScopes(requestScopes, undefined, this.oidcDefaultScopes);
 
         // validate the redirectUri (to be a non null value)
         parameterBuilder.addRedirectUri(request.redirectUri);

diff --git a/lib/msal-common/src/url/UrlString.ts b/lib/msal-common/src/url/UrlString.ts
@@ -8,7 +8,7 @@ import { ClientConfigurationError } from "../error/ClientConfigurationError";
 import { ClientAuthError } from "../error/ClientAuthError";
 import { StringUtils } from "../utils/StringUtils";
 import { IUri } from "./IUri";
-import { AADAuthorityConstants, Constants } from "../utils/Constants";
+import { AADAuthorityConstants, Constants, ServerResponseType } from "../utils/Constants";
 
 /**
  * Url object class which can perform various transformations on url strings.
@@ -320,6 +320,24 @@ export class UrlString {
         }
         return deserializedQueryString;
     }
+    /**
+     * Returns either deserialized query string or deserialized hash, depending on the serverResponseType
+     * as a server auth code response object.
+     */
+    static getDeserializedCodeResponse(
+        serverResponseType: ServerResponseType | undefined,
+        hashFragment: string
+    ): ServerAuthorizationCodeResponse {
+        const hashUrlString = new UrlString(hashFragment);
+        let serverParams : ServerAuthorizationCodeResponse;
+        if(serverResponseType === ServerResponseType.QUERY) {
+            serverParams = UrlString.getDeserializedQueryString(hashFragment);
+        }
+        else{
+            serverParams = UrlString.getDeserializedHash(hashUrlString.getHash());
+        }
+        return serverParams;
+    }
 
     /**
      * Check if the hash of the URL string contains known properties