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

When following a link that prompts sign in through SAML or Multi-Auth the destination page is not lost #1557

Merged
merged 16 commits into from
Aug 24, 2023

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Aug 17, 2023

Description

Fixes a bug with nextUrl and SAML when multiauth is enabled. The SAML login button was not correctly carrying the nextUrl from the URL after logging in and instead routing the user to the home page.

Now the user will be routed to where they previously were before their session timed out or if they log off.

Category

Bug fix

Issues Resolved

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #1557 (1a994d4) into main (c10031f) will increase coverage by 0.11%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main    #1557      +/-   ##
==========================================
+ Coverage   66.06%   66.18%   +0.11%     
==========================================
  Files          93       93              
  Lines        2328     2339      +11     
  Branches      310      312       +2     
==========================================
+ Hits         1538     1548      +10     
  Misses        722      722              
- Partials       68       69       +1     
Files Changed Coverage Δ
public/apps/login/login-page.tsx 75.78% <91.66%> (+1.97%) ⬆️

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Good find - encoding issues are something else.

renderLoginButton(AuthType.SAML, SAML_AUTH_LOGIN_WITH_FRAGMENT, samlConfig)
);
const urlParams = new URLSearchParams(window.location.search);
let nextUrl = urlParams.get('nextUrl');
Copy link
Member

Choose a reason for hiding this comment

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

Confusing to see this variable assigned values multiple times.

Copy link
Member Author

Choose a reason for hiding this comment

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

The function should be simpler now. I reduced the amount of usages of nextUrl

formBodyOp.push(
renderLoginButton(AuthType.SAML, SAML_AUTH_LOGIN_WITH_FRAGMENT, samlConfig)
);
const urlParams = new URLSearchParams(window.location.search);
Copy link
Member

Choose a reason for hiding this comment

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

Could this new logic be broken out into a function, I think that could help clarify while this block is doing and keep its logic isolated

Copy link
Member Author

Choose a reason for hiding this comment

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

I abstracted this out into a function in login-utils

Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

Other than Peter's comment about extracting the code block into the function this looks good to me. Nice job with the tests.

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
common/index.ts Show resolved Hide resolved
public/utils/login-utils.tsx Outdated Show resolved Hide resolved
test/jest_integration/saml_multiauth.test.ts Show resolved Hide resolved
@DarshitChanpura
Copy link
Member

DarshitChanpura commented Aug 18, 2023

Unit tests CI fails due to linter errors. Once those are fixed via #1558, I'll add my approval.

@cwperks
Copy link
Member Author

cwperks commented Aug 23, 2023

@DarshitChanpura @peternied @scrawfor99 This is ready for review again after the PR to remove hardcoded colors was merged.

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @cwperks !

Copy link
Collaborator

@RyanL1997 RyanL1997 left a comment

Choose a reason for hiding this comment

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

Hi @cwperks, thanks for putting them together. I apologize for the late review. I just left some questions/comments.

test/jest_integration/saml_multiauth.test.ts Show resolved Hide resolved
test/jest_integration/saml_multiauth.test.ts Show resolved Hide resolved
.github/workflows/integration-test.yml Show resolved Hide resolved
@cwperks
Copy link
Member Author

cwperks commented Aug 24, 2023

@RyanL1997 We still have the saml_auth tests they are just skipped on windows because of the issue bringing up the node-based IdP on the Github Windows runner.

@RyanL1997
Copy link
Collaborator

@RyanL1997 We still have the saml_auth tests they are just skipped on windows because of the issue bringing up the node-based IdP on the Github Windows runner.

Yes, I know that. I was just confusing for a second when I see that line change haha. But I get it now. Thanks

Copy link
Collaborator

@RyanL1997 RyanL1997 left a comment

Choose a reason for hiding this comment

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

@cwperks Thanks for adding the unit test. This is looking good to me.

@cwperks cwperks merged commit f655ccf into opensearch-project:main Aug 24, 2023
13 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 24, 2023
#1557)

* Fix bug with nextUrl using SAML and multiauth enabled

Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit f655ccf)
cwperks added a commit that referenced this pull request Aug 24, 2023
#1557) (#1561)

* Fix bug with nextUrl using SAML and multiauth enabled

Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit f655ccf)

Co-authored-by: Craig Perkins <[email protected]>
@peternied peternied changed the title Use value from nextUrl when logging in with SAML and multiauth enabled When following a link that prompts sign in through SAML or Multi-Auth the destination page is not lost Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] SAML nextUrl is empty when clicking on the Button Log in with single sign-on
6 participants