From 664bd8271bf3669e438b5f78bf5a1c3136e219ee Mon Sep 17 00:00:00 2001 From: shylasummers Date: Tue, 18 Jul 2023 15:42:00 -0700 Subject: [PATCH] Addressed comments --- .../src/interaction_client/PopupClient.ts | 27 +++++++++++++------ .../StandardInteractionClient.ts | 4 +-- .../test/app/PublicClientApplication.spec.ts | 16 ++++++----- .../interaction_client/PopupClient.spec.ts | 4 +-- .../test/utils/StringConstants.ts | 1 + .../src/client/AuthorizationCodeClient.ts | 18 ++++--------- lib/msal-common/src/url/UrlString.ts | 20 +++++++++++++- 7 files changed, 56 insertions(+), 34 deletions(-) diff --git a/lib/msal-browser/src/interaction_client/PopupClient.ts b/lib/msal-browser/src/interaction_client/PopupClient.ts index 04ca860827..633bbb7a3a 100644 --- 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 index 580ba3a66c..7509e3cf89 100644 --- 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 index b48ca36b2b..e1a7c0670f 100644 --- 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 => { diff --git a/lib/msal-browser/test/interaction_client/PopupClient.spec.ts b/lib/msal-browser/test/interaction_client/PopupClient.spec.ts index 7b6c23e029..f7e250a777 100644 --- 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 index 951cc3e84e..12d20cd9e7 100644 --- 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 index f93d607f21..65ebf91413 100644 --- 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 index caf5e1b359..bce2a61c11 100644 --- 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