Skip to content

Commit

Permalink
Merge branch 'dev' into arc_file_detection
Browse files Browse the repository at this point in the history
  • Loading branch information
Robbie-Microsoft authored Aug 8, 2024
2 parents a9c56c8 + 9b44cb4 commit 09e00b5
Show file tree
Hide file tree
Showing 13 changed files with 782 additions and 8 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "minor",
"comment": "Retry in RedirectClient for invalid_grant errors #7231",
"packageName": "@azure/msal-browser",
"email": "[email protected]",
"dependentChangeType": "patch"
}
11 changes: 9 additions & 2 deletions lib/msal-browser/apiReview/msal-browser.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,8 @@ declare namespace BrowserAuthErrorCodes {
uninitializedPublicClientApplication,
nativePromptNotSupported,
invalidBase64String,
invalidPopTokenRequest
invalidPopTokenRequest,
failedToRetry
}
}
export { BrowserAuthErrorCodes }
Expand Down Expand Up @@ -448,6 +449,7 @@ export type BrowserAuthOptions = {
azureCloudOptions?: AzureCloudOptions;
skipAuthorityMetadataCache?: boolean;
supportsNestedAppAuth?: boolean;
onRedirectNavigate?: (url: string) => boolean | void;
};

// Warning: (ae-missing-release-tag) "BrowserCacheLocation" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
Expand Down Expand Up @@ -838,6 +840,11 @@ export { ExternalTokenResponse }
// @public (undocumented)
const failedToParseResponse = "failed_to_parse_response";

// Warning: (ae-missing-release-tag) "failedToRetry" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
const failedToRetry = "failed_to_retry";

// Warning: (ae-missing-release-tag) "getCurrentUri" is part of the package's API, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public
Expand Down Expand Up @@ -1699,7 +1706,7 @@ export type WrapperSKU = (typeof WrapperSKU)[keyof typeof WrapperSKU];
// src/app/PublicClientNext.ts:81:79 - (tsdoc-malformed-inline-tag) Expecting a TSDoc tag starting with "{@"
// src/app/PublicClientNext.ts:84:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/app/PublicClientNext.ts:85:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
// src/config/Configuration.ts:233:5 - (ae-forgotten-export) The symbol "InternalAuthOptions" needs to be exported by the entry point index.d.ts
// src/config/Configuration.ts:241:5 - (ae-forgotten-export) The symbol "InternalAuthOptions" needs to be exported by the entry point index.d.ts
// src/index.ts:8:12 - (tsdoc-characters-after-block-tag) The token "@azure" looks like a TSDoc tag but contains an invalid character "/"; if it is not a tag, use a backslash to escape the "@"
// src/index.ts:8:4 - (tsdoc-undefined-tag) The TSDoc tag "@module" is not defined in this configuration
// src/navigation/NavigationClient.ts:36:8 - (tsdoc-param-tag-missing-hyphen) The @param block should be followed by a parameter name and then a hyphen
Expand Down
1 change: 1 addition & 0 deletions lib/msal-browser/docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ const msalInstance = new PublicClientApplication(msalConfig);
| `protocolMode` | Enum representing the protocol mode to use. If `"AAD"`, will function on the OIDC-compliant AAD v2 endpoints; if `"OIDC"`, will function on other OIDC-compliant endpoints. | string | `"AAD"` |
| `azureCloudOptions` | A defined set of azure cloud options for developers to default to their specific cloud authorities, for specific clouds supported please refer to the [AzureCloudInstance](https://aka.ms/msaljs/azure_cloud_instance) | [AzureCloudOptions](https://azuread.github.io/microsoft-authentication-library-for-js/ref/modules/_azure_msal_common.html#azurecloudoptions) | [AzureCloudInstance.None](msaljs/azure_cloud_instance) |
| `skipAuthorityMetadataCache` | A flag to choose whether to use the local metadata cache during authority initialization. Metadata cache would be used if no authority metadata is provided and before a network call for metadata has been made (see [Authority](../../msal-common/docs/authority.md)) | boolean | `false` |
| `onRedirectNavigate` | A callback that will be passed the url that MSAL will navigate to in redirect flows. Returning false in the callback will stop navigation.

### Cache Config Options

Expand Down
98 changes: 98 additions & 0 deletions lib/msal-browser/src/cache/BrowserCacheManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1613,6 +1613,104 @@ export class BrowserCacheManager extends CacheManager {
this.setInteractionInProgress(false);
}

/**
* Create request retry key to cache retry status
* @param correlationId
*/
generateRequestRetriedKey(correlationId: string): string {
return `${Constants.CACHE_PREFIX}.${TemporaryCacheKeys.REQUEST_RETRY}.${correlationId}`;
}

/**
* Gets the request retry value from the cache
* @param correlationId
*/
getRequestRetried(correlationId: string): number | null {
const requestRetriedKey = this.generateRequestRetriedKey(correlationId);
const cachedRetryNumber = this.getTemporaryCache(requestRetriedKey);
if (!cachedRetryNumber) {
return null;
}
return parseInt(cachedRetryNumber);
}

/**
* Sets the request retry value to "retried" in the cache
* @param correlationId
*/
setRequestRetried(correlationId: string): void {
this.logger.trace("BrowserCacheManager.setRequestRetried called");
const requestRetriedKey = this.generateRequestRetriedKey(correlationId);
this.setTemporaryCache(requestRetriedKey, "1", false);
}

/**
* Removes all request retry values in the cache
*/
removeRequestRetried(): void {
this.temporaryCacheStorage.getKeys().forEach((key) => {
if (key.indexOf(TemporaryCacheKeys.REQUEST_RETRY) !== -1) {
this.removeTemporaryItem(key);
}
});
}

/**
* Caches the redirectRequest in the cache
* @param redirectRequest
*/
cacheRedirectRequest(redirectRequest: RedirectRequest): void {
this.logger.trace("BrowserCacheManager.cacheRedirectRequest called");
const { ...restParams } = redirectRequest;
delete restParams.onRedirectNavigate;
const encodedValue = JSON.stringify(restParams);

this.setTemporaryCache(
TemporaryCacheKeys.REDIRECT_REQUEST,
encodedValue,
true
);
}

/**
* Gets redirect request from the cache. Logs an error and returns undefined if nothing is found.
*/
getCachedRedirectRequest(): RedirectRequest | undefined {
this.logger.trace(
"BrowserCacheManager.getCachedRedirectRequest called"
);
const cachedRedirectRequest = this.getTemporaryCache(
TemporaryCacheKeys.REDIRECT_REQUEST,
true
);
if (!cachedRedirectRequest) {
this.logger.error(`No cached redirect request found.`);
} else {
this.removeTemporaryItem(
this.generateCacheKey(TemporaryCacheKeys.REDIRECT_REQUEST)
);
let parsedRequest: RedirectRequest;
try {
parsedRequest = JSON.parse(
cachedRedirectRequest
) as RedirectRequest;
} catch (e) {
this.logger.errorPii(
`Attempted to parse: ${cachedRedirectRequest}`
);
this.logger.error(
`Parsing cached redirect request threw with error: ${e}`
);
return;
}

if (parsedRequest) {
return parsedRequest;
}
}
return;
}

cacheCodeRequest(authCodeRequest: CommonAuthorizationCodeRequest): void {
this.logger.trace("BrowserCacheManager.cacheCodeRequest called");

Expand Down
10 changes: 9 additions & 1 deletion lib/msal-browser/src/config/Configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,19 @@ export type BrowserAuthOptions = {
* @deprecated This flag is deprecated and will be removed in the next major version. createNestablePublicClientApplication should be used instead.
*/
supportsNestedAppAuth?: boolean;
/**
* Callback that will be passed the url that MSAL will navigate to in redirect flows. Returning false in the callback will stop navigation.
*/
onRedirectNavigate?: (url: string) => boolean | void;
};

/** @internal */
export type InternalAuthOptions = Required<BrowserAuthOptions> & {
export type InternalAuthOptions = Omit<
Required<BrowserAuthOptions>,
"onRedirectNavigate"
> & {
OIDCOptions: Required<OIDCOptions>;
onRedirectNavigate?: (url: string) => boolean | void;
};

/**
Expand Down
2 changes: 2 additions & 0 deletions lib/msal-browser/src/error/BrowserAuthError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ export const BrowserAuthErrorMessages = {
"Invalid base64 encoded string.",
[BrowserAuthErrorCodes.invalidPopTokenRequest]:
"Invalid PoP token request. The request should not have both a popKid value and signPopToken set to true.",
[BrowserAuthErrorCodes.failedToRetry]:
"Unable to retry failed auth code redemption due to usage of the onRedirectNavigate request parameter. Please set onRedirectNavigate on the PublicClientApplication configuration instead or call loginRedirect again.",
};

/**
Expand Down
1 change: 1 addition & 0 deletions lib/msal-browser/src/error/BrowserAuthErrorCodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,4 @@ export const uninitializedPublicClientApplication =
export const nativePromptNotSupported = "native_prompt_not_supported";
export const invalidBase64String = "invalid_base64_string";
export const invalidPopTokenRequest = "invalid_pop_token_request";
export const failedToRetry = "failed_to_retry";
66 changes: 64 additions & 2 deletions lib/msal-browser/src/interaction_client/RedirectClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ import {
ServerResponseType,
UrlUtils,
InProgressPerformanceEvent,
ServerError,
} from "@azure/msal-common";
import { StandardInteractionClient } from "./StandardInteractionClient";
import {
ApiId,
BrowserConstants,
InteractionType,
TemporaryCacheKeys,
} from "../utils/BrowserConstants";
Expand Down Expand Up @@ -83,6 +85,14 @@ export class RedirectClient extends StandardInteractionClient {
* @param request
*/
async acquireToken(request: RedirectRequest): Promise<void> {
if (request.onRedirectNavigate) {
this.logger.warning(
"Unable to cache redirect request, onRedirectNavigate request option has been deprecated. Please set onRedirectNavigate on PublicClientApplication config instead."
);
} else {
this.browserStorage.cacheRedirectRequest(request);
}

const validRequest = await invokeAsync(
this.initializeAuthorizationRequest.bind(this),
PerformanceEvents.StandardInteractionClientInitializeAuthorizationRequest,
Expand Down Expand Up @@ -174,7 +184,9 @@ export class RedirectClient extends StandardInteractionClient {
navigationClient: this.navigationClient,
redirectTimeout: this.config.system.redirectNavigationTimeout,
redirectStartPage: redirectStartPage,
onRedirectNavigate: request.onRedirectNavigate,
onRedirectNavigate:
request.onRedirectNavigate ||
this.config.auth.onRedirectNavigate,
});
} catch (e) {
if (e instanceof AuthError) {
Expand All @@ -201,6 +213,7 @@ export class RedirectClient extends StandardInteractionClient {
const serverTelemetryManager = this.initializeServerTelemetryManager(
ApiId.handleRedirectPromise
);

try {
if (!this.browserStorage.isInteractionInProgress(true)) {
this.logger.info(
Expand Down Expand Up @@ -331,10 +344,59 @@ export class RedirectClient extends StandardInteractionClient {
(e as AuthError).setCorrelationId(this.correlationId);
serverTelemetryManager.cacheFailedRequest(e);
}

if (
e instanceof ServerError &&
e.errorCode === BrowserConstants.INVALID_GRANT_ERROR
) {
this.performanceClient.addFields(
{
retryError: e.errorCode,
},
this.correlationId
);

const requestRetried = this.browserStorage.getRequestRetried(
this.correlationId
);

if (requestRetried) {
this.logger.error(
"Retried request already detected. Throwing error."
);
this.browserStorage.removeRequestRetried();
throw e;
}

const redirectRequest =
this.browserStorage.getCachedRedirectRequest();
if (!redirectRequest) {
this.logger.error(
`Unable to retry. Please retry with redirect request and correlationId: ${this.correlationId}`
);
this.browserStorage.setRequestRetried(this.correlationId);
throw createBrowserAuthError(
BrowserAuthErrorCodes.failedToRetry
);
}

this.browserStorage.setRequestRetried(this.correlationId);

await this.acquireToken(redirectRequest);
return null;
}

this.browserStorage.removeTemporaryItem(
this.browserStorage.generateCacheKey(
TemporaryCacheKeys.REDIRECT_REQUEST
)
);
this.browserStorage.removeRequestRetried();
throw e;
} finally {
this.browserStorage.cleanRequestByInteractionType(
InteractionType.Redirect
);
throw e;
}
}

Expand Down
6 changes: 6 additions & 0 deletions lib/msal-browser/src/interaction_handler/RedirectHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,12 @@ export class RedirectHandler {
)) as AuthenticationResult;

this.browserStorage.cleanRequestByState(state);
this.browserStorage.removeRequestRetried();
this.browserStorage.removeTemporaryItem(
this.browserStorage.generateCacheKey(
TemporaryCacheKeys.REDIRECT_REQUEST
)
);
return tokenResponse;
}

Expand Down
5 changes: 5 additions & 0 deletions lib/msal-browser/src/request/RedirectRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ export type RedirectRequest = Partial<
> & {
scopes: Array<string>;
redirectStartPage?: string;
/**
* @deprecated
* onRedirectNavigate is deprecated and will be removed in the next major version.
* Set onRedirectNavigate in Configuration instead.
*/
onRedirectNavigate?: (url: string) => boolean | void;
tokenBodyParameters?: StringDict;
};
2 changes: 2 additions & 0 deletions lib/msal-browser/src/utils/BrowserConstants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ export const TemporaryCacheKeys = {
CORRELATION_ID: "request.correlationId",
NATIVE_REQUEST: "request.native",
REDIRECT_CONTEXT: "request.redirect.context",
REDIRECT_REQUEST: "request.redirect",
REQUEST_RETRY: "request.retry",
} as const;
export type TemporaryCacheKeys =
(typeof TemporaryCacheKeys)[keyof typeof TemporaryCacheKeys];
Expand Down
Loading

0 comments on commit 09e00b5

Please sign in to comment.