Make signinPopup work when calling window is iframe #1744
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pamapa
reviewed
Dec 10, 2024
| @@ -152,7 +152,7 @@ export class OidcClient { | |||
| return signinRequest; | |||
| } | |||
|
|
|||
| public async readSigninResponseState(url: string, removeState = false): Promise<{ state: SigninState; response: SigninResponse }> { | |||
| public async readSigninResponseState(url: string, removeState = false): Promise<{ state: SigninState|undefined; response: SigninResponse }> { | |||
There was a problem hiding this comment.
please add spaces in-between: SigninState | undefined
pamapa
reviewed
Dec 10, 2024
| @@ -183,6 +179,11 @@ export class OidcClient { | |||
| extraHeaders = { ...extraHeaders, "DPoP": dpopProof }; | |||
| } | |||
|
|
|||
| if (!state) { | |||
| logger.throw(new Error("No state was found in storage or response")); | |||
| throw null; // | |||
There was a problem hiding this comment.
please re-add comment from above // https://github.com/microsoft/TypeScript/issues/46972
pamapa
reviewed
Dec 10, 2024
| @@ -394,6 +394,13 @@ export class UserManager { | |||
| */ | |||
| public async signinCallback(url = window.location.href): Promise<User | undefined> { | |||
| const { state } = await this._client.readSigninResponseState(url); | |||
|
|
|||
| // if no state from storage, assume signin popup | |||
| if (state === undefined) { | |||
There was a problem hiding this comment.
How are we sure it was a "signin popup request" here? What about "No matching state found in storage" in the non "signin popup" request case?
|
Thanks for taking care of this feature! |
|
I guess using |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes/fixes #issue
Checklist
When using signinPopup and your app is within an iframe, the popup cannot access the local storage of the calling window due to fairly recent changes with partitioning local storage. As such the popup window cant access the stored state in storage from the main calling window.
What my changes do, is if the popup window cant find the SigninState in storage , then it returns undefined for State.
so readSigninResponseState in OidcClient.ts now returns Promise<{ state: SigninState|undefined; response: SigninResponse }
This prevents the Popup window from throwing an error that it couldnt find the state. And it returns the URL which includes the state and the calling window can verify the state instead of doing it in the popup window.
Then in UserManager.ts signinCallback function:
if state returned from await this._client.readSigninResponseState(url); is undefined, we assume we should call signinPopupCallback which sends the url back to the calling window, which itself then verifies state from the url.
I have confirmed that this now works when the calling window is in an iframe, when it previously failed at stage readSigninResponseState. I have also confirmed that auth fails if the response provides a bad state.
To summarize, the popup window was trying to verify the state in the response from the state in storage, and then when the calling window receives the URL it also verifies the state from storage. Now, if the popup cant get the state from storage it passes the url back and the main window only verifies the url and state from storage.
This allows the library to work when the popup window doesnt have access to the same partitioned local storage as the main calling window when the main calling window is in an iframe.