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

Conversation

pkanher617
Copy link
Contributor

This PR ports the changes made in msal-core to the new version of the library in #1358.

  • Moved processing of the hash into constructor
  • handleRedirectCallback is no longer required. If called it will just await the promise saved during handleRedirectResponse step in constructor

navigateToLoginRequestUrl changes are already part of this codebase.

@pkanher617 pkanher617 changed the base branch from dev to iframe-handler-2.0 April 10, 2020 18:26
@pkanher617 pkanher617 self-assigned this Apr 10, 2020
@@ -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

@tnorling
Copy link
Collaborator

Are we also planning to port the state changes Jason made in core? As those affect these pieces of code as well.

Copy link
Contributor

@DarylThayil DarylThayil left a comment

Choose a reason for hiding this comment

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

lgtm,

Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

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

Looks good!

*
* 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.

Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

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

lgtm, left a few comments.

@pkanher617 pkanher617 changed the base branch from iframe-handler-2.0 to dev April 23, 2020 20:14
@coveralls
Copy link

coveralls commented Apr 23, 2020

Coverage Status

Coverage decreased (-0.2%) to 79.49% when pulling 7ae1b05 on handleRedirectCallback-update-2.0 into 4ef6263 on dev.

@pkanher617 pkanher617 merged commit 9274fac into dev Apr 23, 2020
@tnorling tnorling mentioned this pull request May 5, 2020
5 tasks
@pkanher617 pkanher617 deleted the handleRedirectCallback-update-2.0 branch June 8, 2020 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants