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

[msal-browser] Add platform-level state object to state string and read interaction type #2045

Merged
merged 18 commits into from Aug 4, 2020

Conversation

pkanher617
Copy link
Contributor

This PR addresses an issue where the handleRedirectPromise() code would be processed inside a popup before monitorWindowForHash can detect the hash value. This may happen if the opening window's execution is paused or suspended.

I have added the following:

  • A new object for the browser library to populate parameters sent in the state
  • A platform state string in the common library to allow platforms to set state values

@github-actions github-actions bot added msal-browser Related to msal-browser package msal-common Related to msal-common package labels Jul 28, 2020
@coveralls
Copy link

coveralls commented Jul 28, 2020

Coverage Status

Coverage increased (+0.01%) to 81.916% when pulling 0b147be on interaction-type-v2 into 3e27237 on dev.

@pkanher617 pkanher617 marked this pull request as ready for review July 28, 2020 21:15
@pkanher617 pkanher617 self-assigned this Jul 28, 2020
// Deserialize hash fragment response parameters.
const hashUrlString: UrlString = new UrlString(hash);
const serverParams: ServerAuthorizationCodeResponse = hashUrlString.getDeserializedHash<ServerAuthorizationCodeResponse>();
const requestStateObj: RequestStateObject = ProtocolUtils.parseRequestState(this.browserCrypto, serverParams.state);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to write a helper function that gets the interactionType from state? Is this something we might need to do in other places as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can extract this in the future if we need it, but for now this is only needed for redirect flows.

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.

A couple questions, but nothing blocking. LGTM

lib/msal-browser/src/app/PublicClientApplication.ts Outdated Show resolved Hide resolved
export enum InteractionType {
REDIRECT = "redirect",
POPUP = "popup",
SILENT = "silent"
Copy link
Contributor

Choose a reason for hiding this comment

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

Throttling I believe also has this concept. @jmckennon


const stateString = JSON.stringify(stateObj);

return browserCrypto.base64Encode(stateString);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs to be encoded here. What about returning the stateObj and then adding that as is to the library state object? That way we are only encoding/stringifying/decoding/parsing once.

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 common to specify the exact object structure, which is why I made it a string. I felt like adding any type for platformState was too generic. We can talk about this offline.

@tnorling tnorling mentioned this pull request Jul 31, 2020
@tnorling tnorling merged commit b03a96c into dev Aug 4, 2020
@pkanher617 pkanher617 deleted the interaction-type-v2 branch October 2, 2020 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package msal-common Related to msal-common package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

handleRedirectPromise throws a state mismatch error when it is invoked multiple times
4 participants