-
Notifications
You must be signed in to change notification settings - Fork 354
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
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
webui/react/src/pages/SignIn.tsx
Outdated
@@ -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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
webui/react/src/pages/SignIn.tsx
Outdated
@@ -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) || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use ??
webui/react/src/pages/SignIn.tsx
Outdated
locationToPath(location.state) || | ||
(rbacEnabled ? rbacDefaultRoute.path : defaultRoute.path), | ||
); | ||
const path = sessionStorage.landingRedirect || null; |
There was a problem hiding this comment.
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) || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use ??
conversation summary:
|
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:
/det/tasks
or/det/clusters
and make sure you are redirected to the login pageChecklist
docs/release-notes/
.See Release Note for details.