Skip to content

Commit

Permalink
Fix iframe fallback when RT is not found in cache (#6599)
Browse files Browse the repository at this point in the history 
It seems this fallback behavior was missed in the transition from v2 to
v3. When the RT cannot be found in the cache we should try to fallback
to the iframe flow. Also includes minor refactor to make the code more
readable and remove some unnecessary work.
  • Loading branch information
@tnorling
tnorling authored Oct 24, 2023
1 parent 289b937 commit a944241
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 71 deletions.
7 changes: 7 additions & 0 deletions change/@azure-msal-browser-6d9ef336-f49e-47ba-9191-05676f55c887.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Fix iframe fallback when RT is not found in cache",
"packageName": "@azure/msal-browser",
"email": "[email protected]",
"dependentChangeType": "patch"
}
6 changes: 0 additions & 6 deletions lib/msal-browser/src/controllers/IController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import {
Logger,
PerformanceCallbackFunction,
IPerformanceClient,
CommonSilentFlowRequest,
AccountFilter,
} from "@azure/msal-common";
import { RedirectRequest } from "../request/RedirectRequest";
Expand Down Expand Up @@ -48,11 +47,6 @@ export interface IController {
accountId?: string
): Promise<AuthenticationResult>;

acquireTokenByRefreshToken(
commonRequest: CommonSilentFlowRequest,
silentRequest: SilentRequest
): Promise<AuthenticationResult>;

addEventCallback(callback: EventCallbackFunction): string | null;

removeEventCallback(callbackId: string): void;
Expand Down
88 changes: 42 additions & 46 deletions lib/msal-browser/src/controllers/StandardController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
PromptValue,
InProgressPerformanceEvent,
RequestThumbprint,
ServerError,
AccountEntity,
ServerResponseType,
UrlString,
Expand All @@ -30,6 +29,7 @@ import {
ClientAuthErrorCodes,
AccountFilter,
buildStaticAuthorityOptions,
InteractionRequiredAuthErrorCodes,
} from "@azure/msal-common";
import {
BrowserCacheManager,
Expand Down Expand Up @@ -1047,13 +1047,13 @@ export class StandardController implements IController {
protected async acquireTokenFromCache(
silentCacheClient: SilentCacheClient,
commonRequest: CommonSilentFlowRequest,
silentRequest: SilentRequest
cacheLookupPolicy: CacheLookupPolicy
): Promise<AuthenticationResult> {
this.performanceClient.addQueueMeasurement(
PerformanceEvents.AcquireTokenFromCache,
commonRequest.correlationId
);
switch (silentRequest.cacheLookupPolicy) {
switch (cacheLookupPolicy) {
case CacheLookupPolicy.Default:
case CacheLookupPolicy.AccessToken:
case CacheLookupPolicy.AccessTokenAndRefreshToken:
Expand All @@ -1074,18 +1074,18 @@ export class StandardController implements IController {
/**
* Attempt to acquire an access token via a refresh token
* @param commonRequest CommonSilentFlowRequest
* @param silentRequest SilentRequest
* @param cacheLookupPolicy CacheLookupPolicy
* @returns A promise that, when resolved, returns the access token
*/
public async acquireTokenByRefreshToken(
commonRequest: CommonSilentFlowRequest,
silentRequest: SilentRequest
cacheLookupPolicy: CacheLookupPolicy
): Promise<AuthenticationResult> {
this.performanceClient.addQueueMeasurement(
PerformanceEvents.AcquireTokenByRefreshToken,
commonRequest.correlationId
);
switch (silentRequest.cacheLookupPolicy) {
switch (cacheLookupPolicy) {
case CacheLookupPolicy.Default:
case CacheLookupPolicy.AccessTokenAndRefreshToken:
case CacheLookupPolicy.RefreshToken:
Expand Down Expand Up @@ -2055,23 +2055,19 @@ export class StandardController implements IController {
request.correlationId
)(request, account);

const requestWithCLP = {
...request,
// set the request's CacheLookupPolicy to Default if it was not optionally passed in
cacheLookupPolicy:
request.cacheLookupPolicy || CacheLookupPolicy.Default,
};
const cacheLookupPolicy =
request.cacheLookupPolicy || CacheLookupPolicy.Default;

result = invokeAsync(
this.acquireTokenFromCache.bind(this),
PerformanceEvents.AcquireTokenFromCache,
this.logger,
this.performanceClient,
silentRequest.correlationId
)(silentCacheClient, silentRequest, requestWithCLP).catch(
)(silentCacheClient, silentRequest, cacheLookupPolicy).catch(
(cacheError: AuthError) => {
if (
requestWithCLP.cacheLookupPolicy ===
request.cacheLookupPolicy ===
CacheLookupPolicy.AccessToken
) {
throw cacheError;
Expand All @@ -2091,42 +2087,42 @@ export class StandardController implements IController {
this.logger,
this.performanceClient,
silentRequest.correlationId
)(silentRequest, requestWithCLP).catch(
)(silentRequest, cacheLookupPolicy).catch(
(refreshTokenError: AuthError) => {
const isServerError =
refreshTokenError instanceof ServerError;
const isInteractionRequiredError =
refreshTokenError instanceof
InteractionRequiredAuthError;
const isInvalidGrantError =
const isSilentlyResolvable =
(!(
refreshTokenError instanceof
InteractionRequiredAuthError
) &&
(refreshTokenError.errorCode ===
BrowserConstants.INVALID_GRANT_ERROR ||
refreshTokenError.errorCode ===
ClientAuthErrorCodes.tokenRefreshRequired)) ||
refreshTokenError.errorCode ===
BrowserConstants.INVALID_GRANT_ERROR;

if (
(!isServerError ||
!isInvalidGrantError ||
isInteractionRequiredError ||
requestWithCLP.cacheLookupPolicy ===
CacheLookupPolicy.AccessTokenAndRefreshToken ||
requestWithCLP.cacheLookupPolicy ===
CacheLookupPolicy.RefreshToken) &&
requestWithCLP.cacheLookupPolicy !==
CacheLookupPolicy.Skip
) {
InteractionRequiredAuthErrorCodes.noTokensFound;

const tryIframeRenewal =
cacheLookupPolicy ===
CacheLookupPolicy.Default ||
cacheLookupPolicy === CacheLookupPolicy.Skip ||
cacheLookupPolicy ===
CacheLookupPolicy.RefreshTokenAndNetwork;

if (isSilentlyResolvable && tryIframeRenewal) {
this.logger.verbose(
"Refresh token expired/invalid or CacheLookupPolicy is set to Skip, attempting acquire token by iframe.",
request.correlationId
);
return invokeAsync(
this.acquireTokenBySilentIframe.bind(this),
PerformanceEvents.AcquireTokenBySilentIframe,
this.logger,
this.performanceClient,
silentRequest.correlationId
)(silentRequest);
} else {
throw refreshTokenError;
}

this.logger.verbose(
"Refresh token expired/invalid or CacheLookupPolicy is set to Skip, attempting acquire token by iframe.",
request.correlationId
);
return invokeAsync(
this.acquireTokenBySilentIframe.bind(this),
PerformanceEvents.AcquireTokenBySilentIframe,
this.logger,
this.performanceClient,
silentRequest.correlationId
)(silentRequest);
}
);
}
Expand Down
24 changes: 5 additions & 19 deletions lib/msal-browser/src/interaction_client/SilentRefreshClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ export class SilentRefreshClient extends StandardInteractionClient {
silentRequest.authority,
silentRequest.azureCloudOptions
);
this.logger.verbose("Refresh token client created");
// Send request to renew token. Auth module will throw errors if token cannot be renewed.
return invokeAsync(
refreshTokenClient.acquireTokenByRefreshToken.bind(
Expand All @@ -63,24 +62,11 @@ export class SilentRefreshClient extends StandardInteractionClient {
this.logger,
this.performanceClient,
request.correlationId
)(silentRequest)
.then((result) => result as AuthenticationResult)
.then((result: AuthenticationResult) => {
this.performanceClient.addFields(
{
fromCache: result.fromCache,
requestId: result.requestId,
},
request.correlationId
);

return result;
})
.catch((e: AuthError) => {
(e as AuthError).setCorrelationId(this.correlationId);
serverTelemetryManager.cacheFailedRequest(e);
throw e;
});
)(silentRequest).catch((e: AuthError) => {
(e as AuthError).setCorrelationId(this.correlationId);
serverTelemetryManager.cacheFailedRequest(e);
throw e;
}) as Promise<AuthenticationResult>;
}

/**
Expand Down
46 changes: 46 additions & 0 deletions lib/msal-browser/test/app/PublicClientApplication.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3461,6 +3461,52 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {
expect(silentIframeSpy.calledOnce).toBe(true);
});

it("Calls SilentIframeClient.acquireToken and returns its response if no RT is cached", async () => {
const testAccount: AccountInfo = {
homeAccountId: TEST_DATA_CLIENT_INFO.TEST_HOME_ACCOUNT_ID,
localAccountId: TEST_DATA_CLIENT_INFO.TEST_UID,
environment: "login.windows.net",
tenantId: "3338040d-6c67-4c5b-b112-36a304b66dad",
username: "[email protected]",
};
const testTokenResponse: AuthenticationResult = {
authority: TEST_CONFIG.validAuthority,
uniqueId: testAccount.localAccountId,
tenantId: testAccount.tenantId,
scopes: TEST_CONFIG.DEFAULT_SCOPES,
idToken: "test-idToken",
idTokenClaims: {},
accessToken: "test-accessToken",
fromCache: false,
correlationId: RANDOM_TEST_GUID,
expiresOn: new Date(Date.now() + 3600000),
account: testAccount,
tokenType: AuthenticationScheme.BEARER,
};
const silentCacheSpy = sinon
.stub(SilentCacheClient.prototype, "acquireToken")
.rejects("Expired");
const silentRefreshSpy = sinon
.stub(SilentRefreshClient.prototype, "acquireToken")
.rejects(
createInteractionRequiredAuthError(
InteractionRequiredAuthErrorCodes.noTokensFound
)
);
const silentIframeSpy = sinon
.stub(SilentIframeClient.prototype, "acquireToken")
.resolves(testTokenResponse);

const response = await pca.acquireTokenSilent({
scopes: ["openid"],
account: testAccount,
});
expect(response).toEqual(testTokenResponse);
expect(silentCacheSpy.calledOnce).toBe(true);
expect(silentRefreshSpy.calledOnce).toBe(true);
expect(silentIframeSpy.calledOnce).toBe(true);
});

it("makes one network request with multiple parallel silent requests with same request", async () => {
const testServerTokenResponse = {
token_type: TEST_CONFIG.TOKEN_TYPE_BEARER,
Expand Down

0 comments on commit a944241

Please sign in to comment.