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.

Sign up for GitHub

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

Jump to bottom

Fix cases where request is undefined for user passed 'state' checks #698

Merged
merged 6 commits into from
May 20, 2019
Merged

Fix cases where request is undefined for user passed 'state' checks #698

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d4256e9
fix cases where request is undefined sending "state" to ServerRequest…
sameerag May 8, 2019
af8ae78
More meaningful comments
sameerag May 8, 2019
5f89f03
add a comment
sameerag May 9, 2019
b84908e
Uniform check across code for request.state
sameerag May 17, 2019
3fc888b
Update all checks
sameerag May 17, 2019
0003bfc
Updating request as optional param in cases it is not sent
sameerag May 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 17 additions & 31 deletions lib/msal-core/src/UserAgentApplication.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,11 +302,7 @@ export class UserAgentApplication {

// Creates navigate url; saves value in cache; redirect user to AAD
if (this.loginInProgress) {
let reqState;
if (request) {
reqState = request.state;
}
this.redirectErrorHandler(ClientAuthError.createLoginInProgressError(), buildResponseStateOnly(reqState));
this.redirectErrorHandler(ClientAuthError.createLoginInProgressError(), buildResponseStateOnly(request && request.state));
return;
}

Expand Down Expand Up @@ -367,7 +363,7 @@ export class UserAgentApplication {
* @param AuthenticationParameters
* @param scopes
*/
private loginRedirectHelper(account: Account, request: AuthenticationParameters, scopes?: Array<string>) {
private loginRedirectHelper(account: Account, request?: AuthenticationParameters, scopes?: Array<string>) {
// Track login in progress
this.loginInProgress = true;

Expand All @@ -379,7 +375,7 @@ export class UserAgentApplication {
this.clientId, scopes,
ResponseTypes.id_token,
this.getRedirectUri(),
request.state
request && request.state
);

// populate QueryParameters (sid/login_hint/domain_hint) and any other extraQueryParameters set by the developer
Expand All @@ -402,11 +398,7 @@ export class UserAgentApplication {
this.promptUser(urlNavigate);
}).catch((err) => {
this.logger.warning("could not resolve endpoints");
let reqState;
if (request) {
reqState = request.state;
}
this.redirectErrorHandler(ClientAuthError.createEndpointResolutionError(err.toString), buildResponseStateOnly(reqState));
this.redirectErrorHandler(ClientAuthError.createEndpointResolutionError(err.toString), buildResponseStateOnly(request && request.state));
});
}

Expand All @@ -430,11 +422,7 @@ export class UserAgentApplication {

// If already in progress, do not proceed
if (this.acquireTokenInProgress) {
let reqState;
if (request) {
reqState = request.state;
}
this.redirectErrorHandler(ClientAuthError.createAcquireTokenInProgressError(), buildResponseStateOnly(this.getAccountState(reqState)));
this.redirectErrorHandler(ClientAuthError.createAcquireTokenInProgressError(), buildResponseStateOnly(this.getAccountState(request.state)));
return;
}

Expand Down Expand Up @@ -477,12 +465,7 @@ export class UserAgentApplication {
}
}).catch((err) => {
this.logger.warning("could not resolve endpoints");

let reqState;
if (request) {
reqState = request.state;
}
this.redirectErrorHandler(ClientAuthError.createEndpointResolutionError(err.toString), buildResponseStateOnly(reqState));
this.redirectErrorHandler(ClientAuthError.createEndpointResolutionError(err.toString), buildResponseStateOnly(request.state));
});
}

Expand Down Expand Up @@ -534,7 +517,7 @@ export class UserAgentApplication {
// add the prompt parameter to the 'extraQueryParameters' if passed
if (Utils.isSSOParam(request)) {
// if account is not provided, we pass null
this.loginPopupHelper(account, request, resolve, reject, scopes);
this.loginPopupHelper(account, resolve, reject, request, scopes);
}
// else handle the library data
else {
Expand All @@ -556,12 +539,12 @@ export class UserAgentApplication {
}, (error) => {
this.silentLogin = false;
this.logger.error("Error occurred during unified cache ATS");
this.loginPopupHelper(null, request, resolve, reject, scopes);
this.loginPopupHelper(null, resolve, reject, request, scopes);
});
}
// else proceed with login
else {
this.loginPopupHelper(null, request, resolve, reject, scopes );
this.loginPopupHelper(null, resolve, reject, request, scopes);
}
}
});
Expand All @@ -577,7 +560,7 @@ export class UserAgentApplication {
* @param reject
* @param scopes
*/
private loginPopupHelper(account: Account, request: AuthenticationParameters, resolve: any, reject: any, scopes?: Array<string>) {
private loginPopupHelper(account: Account, resolve: any, reject: any, request?: AuthenticationParameters, scopes?: Array<string>) {
if (!scopes) {
scopes = [this.clientId];
}
Expand All @@ -595,7 +578,7 @@ export class UserAgentApplication {

// Resolve endpoint
this.authorityInstance.resolveEndpointsAsync().then(() => {
let serverAuthenticationRequest = new ServerRequestParameters(this.authorityInstance, this.clientId, scopes, ResponseTypes.id_token, this.getRedirectUri(), request.state);
let serverAuthenticationRequest = new ServerRequestParameters(this.authorityInstance, this.clientId, scopes, ResponseTypes.id_token, this.getRedirectUri(), request && request.state);

// populate QueryParameters (sid/login_hint/domain_hint) and any other extraQueryParameters set by the developer;
serverAuthenticationRequest = this.populateQueryParams(account, request, serverAuthenticationRequest);
Expand Down Expand Up @@ -634,7 +617,7 @@ export class UserAgentApplication {
this.cacheStorage.setItem(Constants.msalError, ClientAuthErrorMessage.endpointResolutionError.code);
this.cacheStorage.setItem(Constants.msalErrorDescription, ClientAuthErrorMessage.endpointResolutionError.desc);

// What is this? Is this the reject that is passed in?? -- REDO this in the subsequent refactor, passing reject is confusing
// reject that is passed in - REDO this in the subsequent refactor, passing reject is confusing
if (reject) {
reject(ClientAuthError.createEndpointResolutionError());
}
Expand All @@ -643,6 +626,7 @@ export class UserAgentApplication {
if (popUpWindow) {
popUpWindow.close();
}
// this is an all catch for any failure for the above code except the specific 'reject' call
}).catch((err) => {
this.logger.warning("could not resolve endpoints");
reject(ClientAuthError.createEndpointResolutionError(err.toString));
Expand Down Expand Up @@ -720,17 +704,19 @@ export class UserAgentApplication {
}

}, () => {
// On rejection
// Endpoint resolution failure error
this.logger.info(ClientAuthErrorMessage.endpointResolutionError.code + ":" + ClientAuthErrorMessage.endpointResolutionError.desc);
this.cacheStorage.setItem(Constants.msalError, ClientAuthErrorMessage.endpointResolutionError.code);
this.cacheStorage.setItem(Constants.msalErrorDescription, ClientAuthErrorMessage.endpointResolutionError.desc);

// reject that is passed in - REDO this in the subsequent refactor, passing reject is confusing
if (reject) {
reject(ClientAuthError.createEndpointResolutionError());
}
if (popUpWindow) {
popUpWindow.close();
}
// this is an all catch for any failure for the above code except the specific 'reject' call
}).catch((err) => {
this.logger.warning("could not resolve endpoints");
reject(ClientAuthError.createEndpointResolutionError(err.toString()));
Expand Down Expand Up @@ -908,7 +894,7 @@ export class UserAgentApplication {
request.scopes,
responseType,
this.getRedirectUri(),
request.state
request && request.state
);

// populate QueryParameters (sid/login_hint/domain_hint) and any other extraQueryParameters set by the developer
Expand Down