feat: auto-redirect to SSO provider when expired remote session detected (DET-10392) #9613

corban-beaird · 2024-07-05T19:56:10Z

Ticket

Description

Detect when an SSO user is attempting to leverage an expired session token, and rather than redirect them to the SignIn page, instead directly call the SSO provider & send them back to the requested page.

Docs

release note
update master config ref docs
add a description of handling sesh expiry to "managing remote users" (see docs: Update remote users auto redirect #9623 )

Test Plan

Setup:

Launch a devcluster that is configured to with at least on SSO provider
- saml example: .circleci/devcluster/saml.devcluster.yaml
- Add always_redirect: true in the OIDC/SAML config to test the alwaysRedirect behavior.

SSO User Token Expiration

Sign in using SSO
Navigate to a page other than the dashboard
Set the expiration date of the session token in the DB to a time to now (or before)
- Example Query: UPDATE user_sessions SET expiry = current_timestamp WHERE user_id = <your user id>;
Confirm that:
- User is kicked from current page for being unauthenticated
- SignIn page is NOT displayed
- User is redirected to SSO & then back original page

Non-SSO User Token Expiration

Sign in NOT using SSO
Navigate to a page other than the dashboard
Set the expiration date of the session token in the DB to a time to now (or before)
- Example Query: UPDATE user_sessions SET expiry = current_timestamp WHERE user_id = <your user id>;
Confirm that:
- User is kicked from current page for being unauthenticated
- SignIn page is displayed

Always Redirect Enabled

Sign into determined as an admin
Create a workspace & project
Sign out
Attempt to access the following pages (signing out between each)
- /det/tasks
- /det/workspaces/<workspace_id>/projects
- /det/projects/<project_id>/experiments
Confirm that you are either direct to the SSO provider or to the given page as the SSO user
If the user doesn't have access to the page, they should see a 404 error w/a button directing them back to home

Checklist

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

codecov · 2024-07-05T19:56:31Z

Codecov Report

Attention: Patch coverage is 33.03571% with 75 lines in your changes missing coverage. Please review.

Project coverage is 53.32%. Comparing base (086de84) to head (85232b9).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9613      +/-   ##
==========================================
- Coverage   53.34%   53.32%   -0.03%     
==========================================
  Files        1255     1255              
  Lines      152702   152789      +87     
  Branches     3250     3253       +3     
==========================================
+ Hits        81455    81469      +14     
- Misses      71095    71168      +73     
  Partials      152      152

Flag	Coverage Δ
backend	`44.74% <0.00%> (-0.03%)`	⬇️
harness	`72.83% <ø> (-0.01%)`	⬇️
web	`51.56% <37.37%> (-0.02%)`	⬇️

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

Files	Coverage Δ
master/internal/config/det_cloud_config.go	`50.00% <ø> (ø)`
master/internal/config/oidc_config.go	`50.00% <ø> (ø)`
master/internal/config/saml_config.go	`20.00% <ø> (ø)`
master/pkg/aproto/master_message.go	`0.00% <ø> (ø)`
webui/react/src/stores/determinedInfo.tsx	`86.17% <100.00%> (+0.14%)`	⬆️
webui/react/src/components/NavigationSideBar.tsx	`0.00% <0.00%> (ø)`
webui/react/src/components/NavigationTabbar.tsx	`0.00% <0.00%> (ø)`
webui/react/src/utils/service.ts	`86.27% <93.75%> (+0.76%)`	⬆️
webui/react/src/stores/userSettings.tsx	`89.18% <75.00%> (-0.69%)`	⬇️
master/internal/plugin/sso/sso.go	`34.06% <0.00%> (-1.17%)`	⬇️
... and 5 more

... and 6 files with indirect coverage changes

netlify · 2024-07-05T19:56:40Z

✅ Deploy Preview for determined-ui ready!

Name	Link
🔨 Latest commit	`85232b9`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66a01e2b857479000856a739
😎 Deploy Preview	https://deploy-preview-9613--determined-ui.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

BOsterbuhr · 2024-07-05T20:39:12Z

If the user doesn't have an expired session token will they see the login page still? If so, could a user create a fake session token to ensure they don't see the login page at all?

corban-beaird · 2024-07-05T20:47:29Z

If the user doesn't have an expired session token will they see the login page still? If so, could a user create a fake session token to ensure they don't see the login page at all?

@BOsterbuhr
If the they don't have an expired token, they'll be sent directly in to the app, since they're already authenticated. Also if a user provides a fake session token that isn't within our system, we won't report it as an expired token & they'll be directed to the login page to authenticate

BOsterbuhr · 2024-07-05T21:28:09Z

If the user doesn't have an expired session token will they see the login page still? If so, could a user create a fake session token to ensure they don't see the login page at all?

@BOsterbuhr

If the they don't have an expired token, they'll be sent directly in to the app, since they're already authenticated. Also if a user provides a fake session token that isn't within our system, we won't report it as an expired token & they'll be directed to the login page to authenticate

If they don't have a token at all (first time logging in) how can they avoid hitting the login page? We need a way to avoid the login page completely in all instances.

corban-beaird · 2024-07-05T21:55:08Z

If they don't have a token at all (first time logging in) how can they avoid hitting the login page? We need a way to avoid the login page completely in all instances.

@BOsterbuhr
Currently that's not what's being done in this PR. However, I'm happy to add in a new config to the master to allow for auto-redirect to the SSO provider by default for every user regardless of if they have a token.

keita-determined

\o/

webui/react/src/utils/service.ts

webui/react/src/utils/error.ts

webui/react/src/stores/userSettings.tsx

webui/react/src/hooks/useAuthCheck.ts

webui/react/src/stores/userSettings.tsx

webui/react/src/pages/SignIn.tsx

webui/react/src/pages/SignOut.tsx

BOsterbuhr · 2024-07-05T22:19:40Z

Currently that's not what's being done in this PR. However, I'm happy to add in a new config to the master to allow for auto-redirect to the SSO provider by default for every user regardless of if they have a token.

@corban-beaird
That would be perfect, thank you 🙏🏼

maxrussell

Some nits, but this LGTM.

master/internal/grpcutil/auth.go

master/internal/user/postgres_users.go

dougdet · 2024-07-08T14:01:29Z

@corban-beaird 👋🏻

Currently that's not what's being done in this PR. However, I'm happy to add in a new config to the master to allow for auto-redirect to the SSO provider by default for every user regardless of if they have a token.

Is this being worked into the PR? This is critical. Also, sorry if I missed the explanation: if the user has an Okta token that hasn't yet been "seen" by MLDE, will the user be re-directed to the requested page?

corban-beaird · 2024-07-08T20:17:42Z

Is this being worked into the PR? This is critical. Also, sorry if I missed the explanation: if the user has an Okta token that hasn't yet been "seen" by MLDE, will the user be re-directed to the requested page?

@dougdet Yep, I pushing up a PR to add in the ability to always redirect to the designated SSO provider; with the exception of cases where users perform a hard logout (i.e. clicking the logout button).

dougdet · 2024-07-08T20:27:37Z

@dougdet Yep, I pushing up a PR to add in the ability to always redirect to the designated SSO provider; with the exception of cases where users perform a hard logout (i.e. clicking the logout button).

Great!

"i.e. clicking the logout button". Is there no way around this? The user may logout that way and may still be auth'd in Okta (have its token). If no way around, we'll just have to make this very clear to the customer that if a user does this they'll bump into the Login page. I'll bet they'll ask to hide the Logout button if the master.yaml is set to "always redirect".

corban-beaird · 2024-07-08T20:33:51Z

"i.e. clicking the logout button". Is there no way around this? The user may logout that way and may still be auth'd in Okta (have its token). If no way around, we'll just have to make this very clear to the customer that if a user does this they'll bump into the Login page. I'll bet they'll ask to hide the Logout button if the master.yaml is set to "always redirect".

@dougdet If we aren't sending folks to the the determined login page & they still have an active session with Okta that we don't manage, they'll immediately get logged back into determined after hitting sign out. Is there ever a case where a user would ever want to log in under a different account?

dougdet · 2024-07-08T20:39:50Z

@dougdet If we aren't sending folks to the the determined login page & they still have an active session with Okta that we don't manage, they'll immediately get logged back into determined after hitting sign out. Is there ever a case where a user would ever want to log in under a different account?
@corban-beaird I think you have it covered. Not sure a "regular" user would ever be logging in under a different account.

BOsterbuhr

Helm values LGTM

…e attempted to be accessed without auth

docs/reference/deploy/master-config-reference.rst

Co-authored-by: Tara <[email protected]>

docs/reference/deploy/master-config-reference.rst

Co-authored-by: Tara <[email protected]>

docs/reference/deploy/master-config-reference.rst

Co-authored-by: Tara <[email protected]>

docs/reference/deploy/master-config-reference.rst

Co-authored-by: Tara <[email protected]>

…pected URL

kkunapuli · 2024-07-23T16:15:29Z

proto/src/determined/api/v1/master.proto

@@ -22,6 +22,9 @@ message SSOProvider {
  string sso_url = 2;
  // The type of SSO (such as SAML, OIDC).
  string type = 3;
+  // The flag to indicates if this provider should be always be redirected to,


super nit picky: I think there's a typo here The flag to indicates

All good this is a solid catch! Thanks!

… eventually be done in the future to ensure we can test both enviroment but is out of scope for this PR

tara-hpe

LGTM

JComins000

codeowners says i should only need to review changes in these directories, but i don't see any changes there. approved

/webui/react/src/e2e              @determined-ai/test
/webui/react/playwright.config.ts @determined-ai/test

corban-beaird requested a review from keita-determined July 5, 2024 19:56

corban-beaird requested review from a team as code owners July 5, 2024 19:56

corban-beaird requested a review from amandavialva01 July 5, 2024 19:56

cla-bot bot added the cla-signed label Jul 5, 2024

corban-beaird requested a review from salonig23 July 5, 2024 19:56

corban-beaird requested review from maxrussell and removed request for amandavialva01 and salonig23 July 5, 2024 19:57

corban-beaird requested a review from BOsterbuhr July 5, 2024 20:47

determined-ci added the documentation Improvements or additions to documentation label Jul 5, 2024

determined-ci requested a review from a team July 5, 2024 20:51

keita-determined approved these changes Jul 5, 2024

View reviewed changes

maxrussell approved these changes Jul 6, 2024

View reviewed changes

master/internal/grpcutil/auth.go Outdated Show resolved Hide resolved

master/internal/grpcutil/auth.go Outdated Show resolved Hide resolved

master/internal/user/postgres_users.go Show resolved Hide resolved

corban-beaird requested a review from a team as a code owner July 8, 2024 20:25

corban-beaird requested a review from kkunapuli July 8, 2024 20:25

BOsterbuhr approved these changes Jul 8, 2024

View reviewed changes

corban-beaird force-pushed the corban/redirect-expired-sso-users-to-sso-provider branch from d148f69 to 24bcd07 Compare July 8, 2024 22:56

corban-beaird added 4 commits July 22, 2024 08:10

feat: redirect users back to initial page upon re-authentication

83bfd1a

WIP

957ef15

WIP

f967eb6

chore: ensure that landingRedirect gets overwritten when new pages ar…

ac1e249

…e attempted to be accessed without auth

corban-beaird force-pushed the corban/redirect-expired-sso-users-to-sso-provider branch from 8dba3ec to ac1e249 Compare July 22, 2024 15:49

corban-beaird requested a review from a team as a code owner July 22, 2024 15:49

tara-hpe reviewed Jul 23, 2024

View reviewed changes

docs/reference/deploy/master-config-reference.rst Outdated Show resolved Hide resolved

Update docs/reference/deploy/master-config-reference.rst

0c49253

Co-authored-by: Tara <[email protected]>

determined-ci requested a review from a team July 23, 2024 15:48

tara-hpe reviewed Jul 23, 2024

View reviewed changes

docs/reference/deploy/master-config-reference.rst Outdated Show resolved Hide resolved

Update docs/reference/deploy/master-config-reference.rst

8c9dcdb

Co-authored-by: Tara <[email protected]>

determined-ci requested a review from a team July 23, 2024 15:50

tara-hpe reviewed Jul 23, 2024

View reviewed changes

docs/reference/deploy/master-config-reference.rst Outdated Show resolved Hide resolved

Update docs/reference/deploy/master-config-reference.rst

8827e21

Co-authored-by: Tara <[email protected]>

determined-ci requested a review from a team July 23, 2024 15:51

tara-hpe reviewed Jul 23, 2024

View reviewed changes

docs/reference/deploy/master-config-reference.rst Outdated Show resolved Hide resolved

Update docs/reference/deploy/master-config-reference.rst

310a082

Co-authored-by: Tara <[email protected]>

determined-ci requested a review from a team July 23, 2024 15:54

WIP debug print expectedURL, seeing if CI failures are a result of ex…

022821f

…pected URL

kkunapuli approved these changes Jul 23, 2024

View reviewed changes

corban-beaird added 2 commits July 23, 2024 13:33

chore: revert dynamic EE-based landing title & page work, this should…

1065267

… eventually be done in the future to ensure we can test both enviroment but is out of scope for this PR

refactor: clean up docs linter issue

e6203ac

tara-hpe approved these changes Jul 23, 2024

View reviewed changes

refactor: correct typo in master.proto comment

0d130fc

determined-ci requested a review from a team July 23, 2024 20:45

JComins000 approved these changes Jul 23, 2024

View reviewed changes

chore: rebuild proto after comment change

85232b9

corban-beaird force-pushed the corban/redirect-expired-sso-users-to-sso-provider branch from 47fef7e to 85232b9 Compare July 23, 2024 21:18

corban-beaird merged commit 1255d07 into main Jul 23, 2024
80 of 95 checks passed

corban-beaird deleted the corban/redirect-expired-sso-users-to-sso-provider branch July 23, 2024 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auto-redirect to SSO provider when expired remote session detected (DET-10392) #9613

feat: auto-redirect to SSO provider when expired remote session detected (DET-10392) #9613

corban-beaird commented Jul 5, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading

netlify bot commented Jul 5, 2024 •

edited

Loading

BOsterbuhr commented Jul 5, 2024

corban-beaird commented Jul 5, 2024 •

edited

Loading

BOsterbuhr commented Jul 5, 2024

corban-beaird commented Jul 5, 2024

keita-determined left a comment

BOsterbuhr commented Jul 5, 2024

maxrussell left a comment

dougdet commented Jul 8, 2024 •

edited

Loading

corban-beaird commented Jul 8, 2024

dougdet commented Jul 8, 2024

corban-beaird commented Jul 8, 2024

dougdet commented Jul 8, 2024

BOsterbuhr left a comment

kkunapuli Jul 23, 2024

corban-beaird Jul 23, 2024

tara-hpe left a comment

JComins000 left a comment

feat: auto-redirect to SSO provider when expired remote session detected (DET-10392) #9613

feat: auto-redirect to SSO provider when expired remote session detected (DET-10392) #9613

Conversation

corban-beaird commented Jul 5, 2024 • edited Loading

Ticket

Description

Docs

Test Plan

Checklist

codecov bot commented Jul 5, 2024 • edited Loading

Codecov Report

netlify bot commented Jul 5, 2024 • edited Loading

✅ Deploy Preview for determined-ui ready!

BOsterbuhr commented Jul 5, 2024

corban-beaird commented Jul 5, 2024 • edited Loading

BOsterbuhr commented Jul 5, 2024

corban-beaird commented Jul 5, 2024

keita-determined left a comment

Choose a reason for hiding this comment

BOsterbuhr commented Jul 5, 2024

maxrussell left a comment

Choose a reason for hiding this comment

dougdet commented Jul 8, 2024 • edited Loading

corban-beaird commented Jul 8, 2024

dougdet commented Jul 8, 2024

corban-beaird commented Jul 8, 2024

dougdet commented Jul 8, 2024

BOsterbuhr left a comment

Choose a reason for hiding this comment

kkunapuli Jul 23, 2024

Choose a reason for hiding this comment

corban-beaird Jul 23, 2024

Choose a reason for hiding this comment

tara-hpe left a comment

Choose a reason for hiding this comment

JComins000 left a comment

Choose a reason for hiding this comment

corban-beaird commented Jul 5, 2024 •

edited

Loading

codecov bot commented Jul 5, 2024 •

edited

Loading

netlify bot commented Jul 5, 2024 •

edited

Loading

corban-beaird commented Jul 5, 2024 •

edited

Loading

dougdet commented Jul 8, 2024 •

edited

Loading