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: redirect on sso login #9369

Merged
merged 4 commits into from
May 28, 2024
Merged

fix: redirect on sso login #9369

merged 4 commits into from
May 28, 2024

Conversation

eecsliu
Copy link
Contributor

@eecsliu eecsliu commented May 15, 2024

Ticket

DET-10192

Description

SSO login was not redirecting to the page that users were desiring. This PR has the webui temporarily store the target URL when a login is attempted.
When SSO login occurs, the path is null; we can take advantage of this by only using the stored target URL when the path is null.

Test Plan

Following the steps first with normal username/password login, then with Okta login:

  • Login and verify that you are redirected to either the dashboard (RBAC not enabled) or workspaces (RBAC enabled).
  • log out
  • go to a page such as /det/tasks or /det/clusters and make sure you are redirected to the login page
  • Log in and verify that you are redirected to the page you are trying to reach (the two aforementioned are tasks or clusters, respectively).
  • log back out
  • log back in and verify that you are redirected to the dashboard.

Checklist

  • Changes have been manually QA'd
  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@eecsliu eecsliu requested a review from a team as a code owner May 15, 2024 00:18
@eecsliu eecsliu requested a review from johnkim-det May 15, 2024 00:18
@cla-bot cla-bot bot added the cla-signed label May 15, 2024
Copy link

netlify bot commented May 15, 2024

Deploy Preview for determined-ui ready!

Name Link
🔨 Latest commit 3c5c5c0
🔍 Latest deploy log https://app.netlify.com/sites/determined-ui/deploys/664fa0a55e5fe9000839cfc9
😎 Deploy Preview https://deploy-preview-9369--determined-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -37,6 +38,9 @@ const SignIn: React.FC = () => {
const location = useLocation();
const isAuthChecked = useObservable(authStore.isChecked);
const isAuthenticated = useObservable(authStore.isAuthenticated);
if (location.state != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move it in useEffect

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure storing the redirect path in localStorage is the best solution here because it might redirect to the unexpected path.
did you look for other solutions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this was the best idea I had. We have to save the url somewhere because SSO (after login) redirects to the default login

Copy link

codecov bot commented May 15, 2024

Codecov Report

Attention: Patch coverage is 44.00000% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 38.73%. Comparing base (3b1d0df) to head (9159aed).
Report is 35 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9369      +/-   ##
==========================================
- Coverage   45.28%   38.73%   -6.56%     
==========================================
  Files        1227      903     -324     
  Lines      154048   114206   -39842     
  Branches     2404     2404              
==========================================
- Hits        69767    44233   -25534     
+ Misses      84089    69781   -14308     
  Partials      192      192              
Flag Coverage Δ
harness ?
web 36.33% <44.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
webui/react/src/globalStorage.ts 76.27% <64.70%> (-4.69%) ⬇️
webui/react/src/pages/SignIn.tsx 0.00% <0.00%> (ø)

... and 325 files with indirect coverage changes

@@ -62,6 +63,10 @@ const SignIn: React.FC = () => {
* the user to the most recent requested page.
*/
useEffect(() => {
if (location.state != null) {
sessionStorage.landingRedirect = locationToPath(location.state) || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use ??

locationToPath(location.state) ||
(rbacEnabled ? rbacDefaultRoute.path : defaultRoute.path),
);
const path = sessionStorage.landingRedirect || null;
Copy link
Contributor

Choose a reason for hiding this comment

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

qq: how come || null?

@@ -20,6 +21,10 @@ class GlobalStorage {
return this.storage.get<string>(this.keys.serverAddress) || '';
}

get landingRedirect() {
return this.storage.get<string>(this.keys.landingRedirect) || '';
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use ??

@keita-determined
Copy link
Contributor

conversation summary:

  • basic login and SSO behavior should be the same
  • what happens if session key and/or value is changed with invalid value?
  • nice to validate value if possible

@eecsliu eecsliu merged commit a0f2e33 into main May 28, 2024
77 of 92 checks passed
@eecsliu eecsliu deleted the sso-redirect branch May 28, 2024 23:35
ShreyaLnuHpe pushed a commit that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants