Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handleRedirectCallback Updates for MSAL 2.0 #1490

Merged
merged 12 commits into from
Apr 23, 2020
Merged
1 change: 0 additions & 1 deletion lib/msal-browser/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
"doc": "npm run doc:generate && npm run doc:deploy",
"doc:generate": "typedoc --mode modules --excludePrivate --excludeProtected --out ./ref ./src/ --gitRevision dev",
"doc:deploy": "gh-pages -d ref -a -e ref/msal-browser",
"pretest": "npm link @azure/msal-common",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed anymore since lerna bootstrap should take care of linking

"test": "mocha",
"test:coverage": "nyc --reporter=text mocha",
"test:coverage:only": "npm run clean:coverage && npm run test:coverage",
Expand Down
4 changes: 2 additions & 2 deletions lib/msal-browser/src/app/Configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ export type Configuration = {
const DEFAULT_AUTH_OPTIONS: BrowserAuthOptions = {
clientId: "",
authority: null,
redirectUri: () => BrowserUtils.getDefaultRedirectUri(),
postLogoutRedirectUri: () => BrowserUtils.getDefaultRedirectUri(),
redirectUri: () => BrowserUtils.getCurrentUri(),
postLogoutRedirectUri: () => BrowserUtils.getCurrentUri(),
navigateToLoginRequestUrl: true
};

Expand Down
127 changes: 67 additions & 60 deletions lib/msal-browser/src/app/PublicClientApplication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ export class PublicClientApplication {
// auth functions imported from @azure/msal-common module
private authModule: AuthorizationCodeModule;

// callback for error/token response
private authCallback: AuthCallback = null;

// Crypto interface implementation
private browserCrypto: CryptoOps;

Expand All @@ -39,6 +36,9 @@ export class PublicClientApplication {
// Network interface implementation
private networkClient: INetworkModule;

// Response promise
private tokenExchangePromise: Promise<TokenResponse>;

/**
* @constructor
* Constructor for the PublicClientApplication used to instantiate the PublicClientApplication object
Expand Down Expand Up @@ -88,13 +88,15 @@ export class PublicClientApplication {
networkInterface: this.networkClient,
storageInterface: this.browserStorage
});

// Check for hash and save response promise
this.tokenExchangePromise = this.handleRedirectResponse();
}

// #region Redirect Flow

/**
* Set the callback functions for the redirect flow to send back the success or error object, and process
* any redirect-related data.
* Process any redirect-related data and send back the success or error object.
* IMPORTANT: Please do not use this function when using the popup APIs, as it may break the response handling
* in the main window.
*
Expand All @@ -109,14 +111,14 @@ export class PublicClientApplication {
throw BrowserConfigurationAuthError.createInvalidCallbackObjectError(authCallback);
}

// Set the callback object.
this.authCallback = authCallback;

// Check if we need to navigate, otherwise handle hash
try {
await this.handleRedirectResponse();
const tokenResponse = await this.tokenExchangePromise;
if (tokenResponse) {
authCallback(null, tokenResponse);
}
} catch (err) {
this.authCallback(err);
authCallback(err);
}
}

Expand All @@ -125,7 +127,7 @@ export class PublicClientApplication {
* - if true, performs logic to cache and navigate
* - if false, handles hash string and parses response
*/
private async handleRedirectResponse(): Promise<void> {
private async handleRedirectResponse(): Promise<TokenResponse> {
// Get current location hash from window or cache.
const { location: { hash } } = window;
const cachedHash = this.browserStorage.getItem(TemporaryCacheKeys.URL_HASH);
Expand All @@ -134,14 +136,20 @@ export class PublicClientApplication {
// Returned from authority using redirect - need to perform navigation before processing response
this.browserStorage.setItem(TemporaryCacheKeys.URL_HASH, hash);
const loginRequestUrl = this.browserStorage.getItem(TemporaryCacheKeys.ORIGIN_URI);
const currentUrl = BrowserUtils.getCurrentUri();
if (StringUtils.isEmpty(loginRequestUrl) || loginRequestUrl === "null") {
// Redirect to home page if login request url is null (real null or the string null)
this.authModule.logger.warning("Unable to get valid login request url from cache, redirecting to home page");
BrowserUtils.navigateWindow("/", true);
} else {
} else if (currentUrl !== loginRequestUrl) {
// Navigate to target url
BrowserUtils.navigateWindow(loginRequestUrl, true);
tnorling marked this conversation as resolved.
Show resolved Hide resolved
} else {
// We don't need to navigate - check for hash and prepare to process
BrowserUtils.clearHash();
return this.handleHash(hash);
}
return;
return null;
}

if (!isResponseHash) {
Expand All @@ -154,23 +162,25 @@ export class PublicClientApplication {
BrowserUtils.clearHash();
return this.handleHash(hash);
}

return null;
}

/**
* Checks if hash exists and handles in window. Otherwise, cancel any current requests and continue.
* @param responseHash
* @param interactionHandler
*/
private async handleHash(responseHash: string): Promise<void> {
private async handleHash(responseHash: string): Promise<TokenResponse> {
const interactionHandler = new RedirectHandler(this.authModule, this.browserStorage);
if (!StringUtils.isEmpty(responseHash)) {
// Hash contains known properties - handle and return in callback
const tokenResponse = await interactionHandler.handleCodeResponse(responseHash);
this.authCallback(null, tokenResponse);
} else {
// There is no hash - assume we are in clean state and clear any current request data.
this.cleanRequest();
return interactionHandler.handleCodeResponse(responseHash);
}

// There is no hash - assume we are in clean state and clear any current request data.
this.cleanRequest();
return null;
}

/**
Expand All @@ -179,19 +189,8 @@ export class PublicClientApplication {
* @param {@link (AuthenticationParameters:type)}
*/
loginRedirect(request: AuthenticationParameters): void {
// block the reload if it occurred inside a hidden iframe
BrowserUtils.blockReloadInHiddenIframes();

// Check if callback has been set. If not, handleRedirectCallbacks wasn't called correctly.
if (!this.authCallback) {
throw BrowserConfigurationAuthError.createRedirectCallbacksNotSetError();
}

// Check if interaction is in progress. Throw error in callback and return if true.
if (this.interactionInProgress()) {
this.authCallback(BrowserAuthError.createInteractionInProgressError());
return;
}
// Preflight request
this.preflightRequest();

try {
// Create redirect interaction handler.
Expand All @@ -216,19 +215,8 @@ export class PublicClientApplication {
* To acquire only idToken, please pass clientId as the only scope in the Authentication Parameters
*/
acquireTokenRedirect(request: AuthenticationParameters): void {
// block the reload if it occurred inside a hidden iframe
BrowserUtils.blockReloadInHiddenIframes();

// Check if callback has been set. If not, handleRedirectCallbacks wasn't called correctly.
if (!this.authCallback) {
throw BrowserConfigurationAuthError.createRedirectCallbacksNotSetError();
}

// Check if interaction is in progress. Throw error in callback and return if true.
if (this.interactionInProgress()) {
this.authCallback(BrowserAuthError.createInteractionInProgressError());
return;
}
// Preflight request
this.preflightRequest();

try {
// Create redirect interaction handler.
Expand Down Expand Up @@ -257,13 +245,8 @@ export class PublicClientApplication {
* @returns {Promise.<TokenResponse>} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object
*/
async loginPopup(request: AuthenticationParameters): Promise<TokenResponse> {
// block the reload if it occurred inside a hidden iframe
BrowserUtils.blockReloadInHiddenIframes();

// Check if interaction is in progress. Throw error if true.
if (this.interactionInProgress()) {
throw BrowserAuthError.createInteractionInProgressError();
}
// Preflight request
this.preflightRequest();

// Create login url, which will by default append the client id scope to the call.
const navigateUrl = await this.authModule.createLoginUrl(request);
Expand All @@ -280,13 +263,8 @@ export class PublicClientApplication {
* @returns {Promise.<TokenResponse>} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object
*/
async acquireTokenPopup(request: AuthenticationParameters): Promise<TokenResponse> {
// block the reload if it occurred inside a hidden iframe
BrowserUtils.blockReloadInHiddenIframes();

// Check if interaction is in progress. Throw error if true.
if (this.interactionInProgress()) {
throw BrowserAuthError.createInteractionInProgressError();
}
// Preflight request
this.preflightRequest();

// Create acquire token url.
const navigateUrl = await this.authModule.createAcquireTokenUrl(request);
Expand Down Expand Up @@ -319,6 +297,22 @@ export class PublicClientApplication {

// #region Silent Flow

/**
* This function uses a hidden iframe to fetch an authorization code from the eSTS. There are cases where this may not work:
* - Any browser using a form of Intelligent Tracking Prevention
* - If there is not an established session with the service
*
* In these cases, the request must be done inside a popup or full frame redirect.
*
* For the cases where interaction is required, you cannot send a request with prompt=none.
*
* If your refresh token has expired, you can use this function to fetch a new set of tokens silently as long as
* you session on the server still exists.
* @param {@link AuthenticationParameters}
*
* To renew idToken, please pass clientId as the only scope in the Authentication Parameters.
* @returns {Promise.<TokenResponse>} - a promise that is fulfilled when this function has completed, or rejected if an error was raised. Returns the {@link AuthResponse} object
*/
async ssoSilent(request: AuthenticationParameters): Promise<TokenResponse> {
// block the reload if it occurred inside a hidden iframe
BrowserUtils.blockReloadInHiddenIframes();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add preFlightRequest() call here for uniformity? Even if interaction is false, it is in a conditional, so we should be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to add side effects if there were any. For now I will leave it as is, but I can update it if we find a reason for it.

Expand Down Expand Up @@ -420,13 +414,26 @@ export class PublicClientApplication {
// #region Helpers

/**
* Helper to check whether interaction is in progress
* Helper to check whether interaction is in progress.
*/
private interactionInProgress(): boolean {
// Check whether value in cache is present and equal to expected value
return this.browserStorage.getItem(BrowserConstants.INTERACTION_STATUS_KEY) === BrowserConstants.INTERACTION_IN_PROGRESS_VALUE;
}

/**
* Helper to validate app environment before making a request.
*/
private preflightRequest(): void {
// block the reload if it occurred inside a hidden iframe
BrowserUtils.blockReloadInHiddenIframes();

// Check if interaction is in progress. Throw error if true.
if (this.interactionInProgress()) {
throw BrowserAuthError.createInteractionInProgressError();
}
}

/**
* Helper to remove interaction status and remove tempoarary request data.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export class RedirectHandler extends InteractionHandler {
// Navigate if valid URL
if (!StringUtils.isEmpty(requestUrl)) {
// Set interaction status in the library.
this.browserStorage.setItem(TemporaryCacheKeys.ORIGIN_URI, window.location.href);
this.browserStorage.setItem(TemporaryCacheKeys.ORIGIN_URI, BrowserUtils.getCurrentUri());
this.browserStorage.setItem(BrowserConstants.INTERACTION_STATUS_KEY, BrowserConstants.INTERACTION_IN_PROGRESS_VALUE);
this.authModule.logger.infoPii("Navigate to:" + requestUrl);
const isIframedApp = BrowserUtils.isInIframe();
Expand Down
2 changes: 1 addition & 1 deletion lib/msal-browser/src/utils/BrowserUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export class BrowserUtils {
/**
* Returns current window URL as redirect uri
*/
static getDefaultRedirectUri(): string {
static getCurrentUri(): string {
return window.location.href.split("?")[0].split("#")[0];
}

Expand Down
24 changes: 4 additions & 20 deletions lib/msal-browser/test/app/PublicClientApplication.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,18 +276,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {

describe("loginRedirect", () => {

it("loginRedirect throws an error if authCallback is not set", () => {
expect(() => pca.loginRedirect({})).to.throw(BrowserConfigurationAuthErrorMessage.noRedirectCallbacksSet.desc);
expect(() => pca.loginRedirect({})).to.throw(BrowserConfigurationAuthError);
});

it("loginRedirect throws an error if interaction is currently in progress", async () => {
await pca.handleRedirectCallback((authErr: AuthError, response: AuthResponse) => {
expect(authErr instanceof BrowserAuthError).to.be.true;
expect(authErr.errorMessage).to.be.eq(BrowserAuthErrorMessage.interactionInProgress.desc);
});
window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${BrowserConstants.INTERACTION_STATUS_KEY}`, BrowserConstants.INTERACTION_IN_PROGRESS_VALUE);
pca.loginRedirect({});
expect(() => pca.loginRedirect({})).to.throw(BrowserAuthErrorMessage.interactionInProgress.desc);
expect(() => pca.loginRedirect({})).to.throw(BrowserAuthError);
});

it("loginRedirect navigates to created login url", async () => {
Expand Down Expand Up @@ -324,18 +316,10 @@ describe("PublicClientApplication.ts Class Unit Tests", () => {

describe("acquireTokenRedirect", () => {

it("acquireTokenRedirect throws an error if authCallback is not set", () => {
expect(() => pca.acquireTokenRedirect({})).to.throw(BrowserConfigurationAuthErrorMessage.noRedirectCallbacksSet.desc);
expect(() => pca.acquireTokenRedirect({})).to.throw(BrowserConfigurationAuthError);
});

it("acquireTokenRedirect throws an error if interaction is currently in progress", async () => {
await pca.handleRedirectCallback((authErr: AuthError, response: AuthResponse) => {
expect(authErr instanceof BrowserAuthError).to.be.true;
expect(authErr.errorMessage).to.be.eq(BrowserAuthErrorMessage.interactionInProgress.desc);
});
window.sessionStorage.setItem(`${Constants.CACHE_PREFIX}.${TEST_CONFIG.MSAL_CLIENT_ID}.${BrowserConstants.INTERACTION_STATUS_KEY}`, BrowserConstants.INTERACTION_IN_PROGRESS_VALUE);
pca.acquireTokenRedirect({});
expect(() => pca.acquireTokenRedirect({})).to.throw(BrowserAuthErrorMessage.interactionInProgress.desc);
expect(() => pca.acquireTokenRedirect({})).to.throw(BrowserAuthError);
});

it("acquireTokenRedirect navigates to created login url", async () => {
Expand Down