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

fix: mutiple tabs login issue #473

Merged
merged 2 commits into from
Sep 17, 2020

Conversation

shuowu
Copy link
Contributor

@shuowu shuowu commented Sep 15, 2020

Use sessionStorage (if available in browser) as default storage for okta-oauth-redirect-params

Checked in okta repos and readme of auth-js, didn't find usage of okta-oauth-redirect-params cookie.

lib/token.ts Outdated
responseType,
state,
nonce,
scopes,
clientId,
urls: urls,
ignoreSignature
}), null, sdk.options.cookies);
});
if (browserStorage.browserHasSessionStorage()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this if/else block should be pulled out to a separate function.

lib/token.ts Outdated
if (!oauthParamsCookie) {
return Promise.reject(new AuthSdkError('Unable to retrieve OAuth redirect params cookie'));
let oauthParamsStr;
if (browserStorage.browserHasSessionStorage()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here

@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2020

Codecov Report

Merging #473 into master will decrease coverage by 0.18%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #473      +/-   ##
==========================================
- Coverage   90.31%   90.13%   -0.19%     
==========================================
  Files          31       31              
  Lines        1549     1561      +12     
  Branches      340      342       +2     
==========================================
+ Hits         1399     1407       +8     
- Misses        150      154       +4     
Impacted Files Coverage Δ
lib/token.ts 94.88% <78.94%> (-1.00%) ⬇️
lib/constants.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3208b7...94fa47c. Read the comment docs.

lib/token.ts Outdated
@@ -673,17 +673,22 @@ function getWithRedirect(sdk: OktaAuth, options: TokenParams): Promise<void> {
var urls = getOAuthUrls(sdk, options);
var requestUrl = urls.authorizeUrl + buildAuthorizeParams(tokenParams);

// Set session cookie to store the oauthParams
// Store the oauthParams in storage for re-visiting when redirect back
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this code would only be used in front end SDKs, not by the widget, so there's no concern with having duplicate and out-of-sync copies of auth-js, but I'd be happier if someone else explicitly agreed

@@ -2505,7 +2505,7 @@ describe('token.parseFromUrl', function() {
'&expires_in=3600' +
'&token_type=Bearer' +
'&state=' + oauthUtil.mockedState,
oauthCookie: JSON.stringify({
oauthParams: JSON.stringify({
responseType: 'token',
state: 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa',
Copy link
Contributor

Choose a reason for hiding this comment

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

"Do you hear that sound, Fezzik?"

Copy link
Contributor

@swiftone swiftone left a comment

Choose a reason for hiding this comment

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

PENDING section in the CHANGELOG?

@shuowu
Copy link
Contributor Author

shuowu commented Sep 15, 2020

PENDING section in the CHANGELOG?

Adding it, need a pr link first.

@shuowu-okta shuowu-okta merged commit ea2af79 into master Sep 17, 2020
shuowu-okta pushed a commit that referenced this pull request Sep 21, 2020
aarongranick-okta pushed a commit that referenced this pull request Sep 21, 2020
* fix: mutiple tabs login issue (#473)
shuowu-okta pushed a commit that referenced this pull request Sep 21, 2020
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.

4 participants