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

corban-beaird commented 1 week ago

Ticket

DET-10392

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

[x] release note
[x] update master config ref docs
[x] add a description of handling sesh expiry to "managing remote users" (see #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
1. Sign in using SSO
2. Navigate to a page other than the dashboard
3. 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>;
4. 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[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 33.33333% with 74 lines in your changes missing coverage. Please review.

Project coverage is 52.89%. Comparing base (547a4c4) to head (8dba3ec). Report is 11 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #9613 +/- ## ======================================= Coverage 52.88% 52.89% ======================================= Files 1255 1255 Lines 153087 153177 +90 Branches 3230 3234 +4 ======================================= + Hits 80961 81021 +60 - Misses 71975 72005 +30 Partials 151 151 ``` | [Flag](https://app.codecov.io/gh/determined-ai/determined/pull/9613/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | Coverage Δ | | |---|---|---| | [backend](https://app.codecov.io/gh/determined-ai/determined/pull/9613/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | `44.04% <0.00%> (+0.04%)` | :arrow_up: | | [harness](https://app.codecov.io/gh/determined-ai/determined/pull/9613/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | `72.77% <ø> (+0.01%)` | :arrow_up: | | [web](https://app.codecov.io/gh/determined-ai/determined/pull/9613/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | `51.28% <37.75%> (-0.02%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/determined-ai/determined/pull/9613?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | Coverage Δ | | |---|---|---| | [master/internal/config/det\_cloud\_config.go](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree&filepath=master%2Finternal%2Fconfig%2Fdet_cloud_config.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-bWFzdGVyL2ludGVybmFsL2NvbmZpZy9kZXRfY2xvdWRfY29uZmlnLmdv) | `50.00% <ø> (ø)` | | | [master/internal/config/oidc\_config.go](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree&filepath=master%2Finternal%2Fconfig%2Foidc_config.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-bWFzdGVyL2ludGVybmFsL2NvbmZpZy9vaWRjX2NvbmZpZy5nbw==) | `50.00% <ø> (ø)` | | | [master/internal/config/saml\_config.go](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree&filepath=master%2Finternal%2Fconfig%2Fsaml_config.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-bWFzdGVyL2ludGVybmFsL2NvbmZpZy9zYW1sX2NvbmZpZy5nbw==) | `20.00% <ø> (ø)` | | | [master/pkg/aproto/master\_message.go](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree&filepath=master%2Fpkg%2Faproto%2Fmaster_message.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-bWFzdGVyL3BrZy9hcHJvdG8vbWFzdGVyX21lc3NhZ2UuZ28=) | `0.00% <ø> (ø)` | | | [webui/react/src/stores/determinedInfo.tsx](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree&filepath=webui%2Freact%2Fsrc%2Fstores%2FdeterminedInfo.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-d2VidWkvcmVhY3Qvc3JjL3N0b3Jlcy9kZXRlcm1pbmVkSW5mby50c3g=) | `86.17% <100.00%> (+0.14%)` | :arrow_up: | | [webui/react/src/components/NavigationSideBar.tsx](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree&filepath=webui%2Freact%2Fsrc%2Fcomponents%2FNavigationSideBar.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-d2VidWkvcmVhY3Qvc3JjL2NvbXBvbmVudHMvTmF2aWdhdGlvblNpZGVCYXIudHN4) | `0.00% <0.00%> (ø)` | | | [webui/react/src/components/NavigationTabbar.tsx](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree&filepath=webui%2Freact%2Fsrc%2Fcomponents%2FNavigationTabbar.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-d2VidWkvcmVhY3Qvc3JjL2NvbXBvbmVudHMvTmF2aWdhdGlvblRhYmJhci50c3g=) | `0.00% <0.00%> (ø)` | | | [webui/react/src/utils/service.ts](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree&filepath=webui%2Freact%2Fsrc%2Futils%2Fservice.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-d2VidWkvcmVhY3Qvc3JjL3V0aWxzL3NlcnZpY2UudHM=) | `86.27% <93.75%> (+0.76%)` | :arrow_up: | | [webui/react/src/stores/userSettings.tsx](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree&filepath=webui%2Freact%2Fsrc%2Fstores%2FuserSettings.tsx&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-d2VidWkvcmVhY3Qvc3JjL3N0b3Jlcy91c2VyU2V0dGluZ3MudHN4) | `89.18% <75.00%> (-0.69%)` | :arrow_down: | | [webui/react/src/utils/error.ts](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree&filepath=webui%2Freact%2Fsrc%2Futils%2Ferror.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai#diff-d2VidWkvcmVhY3Qvc3JjL3V0aWxzL2Vycm9yLnRz) | `89.86% <40.00%> (-0.76%)` | :arrow_down: | | ... and [5 more](https://app.codecov.io/gh/determined-ai/determined/pull/9613?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai) | | ... and [7 files with indirect coverage changes](https://app.codecov.io/gh/determined-ai/determined/pull/9613/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=determined-ai)

netlify[bot] commented 1 week ago

Deploy Preview for determined-ui ready!

Name	Link
Latest commit	8dba3ec8d0832e6b912dcf683d8694c0ee9f5b3a
Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/66904c78e6a408000804fe62
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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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.

BOsterbuhr commented 1 week ago

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 🙏🏼

dougdet commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

"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 commented 1 week ago

@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.

keita-determined commented 6 days ago

with always_redirect: true, it always go to OIDC/SAML page instead of det login page, is it expected? otherwise lgtm

https://github.com/user-attachments/assets/03d8e27e-5679-4d66-a58f-5865c695e2ab

corban-beaird commented 6 days ago

with always_redirect: true, it always go to OIDC/SAML page instead of det login page, is it expected? otherwise lgtm

Screen.Recording.2024-07-12.at.2.50.03.PM.mov

Yep that's the expected behavior!

determined-ai / determined