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

SAML SLO support #11182

Merged
merged 20 commits into from
Aug 22, 2024
Merged

SAML SLO support #11182

merged 20 commits into from
Aug 22, 2024

Conversation

aalves08
Copy link
Member

@aalves08 aalves08 commented Jun 5, 2024

Summary

Fixes #10941

Occurred changes and/or fixed issues

  • update SAML auth provider edit/create interface to account for setting up this new log out behaviour config
  • add dialog to Header component so that user can choose log out type based on the auth provider config (will only appear if logoutAllEnabled && ! logoutAllForced)
  • add PromptModal to the home template so that the user can get the logout modal while browsing pages that use home template
  • update logout action in auth store to account for a different logout action
  • various adjustments in order to have the logout flow running in all scenarios (normal logout and logout ALL)

Technical notes summary

To whoever is the reviewer of this PR, some notes on what each file's responsibility are:

nav/Header.vue -> where logout modal was added (if logoutAllEnabled && ! logoutAllForced, then by clicking on the logout button, we'll get a modal to select type of logout)
templates/home.vue -> we were missing the PromptModal, so we wouldn't get the logout modal on the home screen
SloDialog -> actual logout modal added
edit/auth/saml.vue -> where the new inputs were added to setup the logout behaviour
mixins/auth-config -> where we handle the parsing of the model for the radio option selected in the UI for the log out behaviour. Since the mixin is responsible for fetching the necessary data, the parsing needs to be done here and not in edit/saml.vue
auth/logout.vue -> responsible for handling logout IF we aren't in a logout modal scenario (if logoutAllForced then we perform the logout ALL and no modal was triggered. That dispatches the logout action with different params, hence the code adjustment)
auth/verify -> where we handle the redirect from logout in case of a logout ALL. Still haven't been able to test this part at this point in time. Might need some adjustments here to do final "plumbing" of the whole logout cycle
store/auth.js - > where actual logout "action" to the backend get's triggered. logout ALL has a different logout action when calling the /v3/tokens endpoint, hence the adjustments needed
utils/auth.js -> helper methods. Had to add IS_SLO as param in the returnTo method so that we can differenciate a logout from a login in auth/verify

Areas or cases that should be tested

  • Was only able to validate the whole logout cycle using Okta. Follow our docs in order to setup Okta (doc Okta SAML setup Creation)
  • in Users & Authentication -> Auth Provider -> select Okta and setup everything according to the previously mentioned document. Set Log out behaviour to Allow the user to choose in an extra step to get access to the logout modal and therefore test both scenarios
  • Should do a "normal logout" (Rancher logout only) with no problems
  • Should do a "logout from ALL" (logout from all Okta apps) with no problems

Areas which could experience regressions

  • Test both normal and auth provider logins, just to be on the safe side (changes to auth/verify)

Screenshot/Video

Screenshot 2024-06-11 at 11 25 46 Screenshot 2024-06-11 at 11 25 51 Screenshot 2024-06-10 at 14 53 15

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

@aalves08 aalves08 added this to the v2.9.0 milestone Jun 5, 2024
@aalves08 aalves08 force-pushed the 10941-saml-slo-support branch 2 times, most recently from 4599021 to 39d0d99 Compare June 7, 2024 08:45
@aalves08 aalves08 marked this pull request as ready for review June 11, 2024 11:24
@richard-cox richard-cox self-assigned this Jun 14, 2024
@richard-cox
Copy link
Member

Linking backend changes over at rancher/rancher#45379

@richard-cox
Copy link
Member

Summary of my changes

  • Ensure rancher logout via modal uses same process as normal logout (show logging out page ... then log in page)
  • Log out option modal
    • even if enabled, don't show if user is not logged in via auth provider (i.e. they're a local user)
    • Use async buttons
    • Add cancel button
    • Show modal when logging out on fail-whale page (i.e. shown for invalid resource type in url)
  • Add auth provider slo error handling
    • backend now returns error in standard format
    • note backend will still log out of rancher even on slo failure

@aalves08
Copy link
Member Author

aalves08 commented Jun 21, 2024

@richard-cox been taking a look at your commits and codewise they seem fine. Eager to test this from end-to-end with Okta.

One other thing, which is a nice to have so we might bump it to another issue, is that with the IS_SLO flag, we could fine-tune the copy in the login and logout screens, specifically for these scenarios, similar to what we've done when logging out with an auth provider

@aalves08 aalves08 closed this Jun 21, 2024
@aalves08 aalves08 reopened this Jun 21, 2024
@nwmac nwmac modified the milestones: v2.9.0, v2.10.0 Jun 28, 2024
@nwmac
Copy link
Member

nwmac commented Jun 28, 2024

Backend pushed out to 2.10.0 - pushed PR out as well - unless this only shows up if the backend supports?

@aalves08
Copy link
Member Author

@richard-cox I've given this a go with v2.10-befdf11fc6753ed4a2322b28c5f3ebe7f47e4d96-head and I can say that it's working. I've added a specific copy for when a SLO happens. I think it's ready to go:

Screen.Recording.2024-08-21.at.15.29.14.mov

FYI @nwmac @gaktive

Copy link
Member

@rak-phillip rak-phillip left a comment

Choose a reason for hiding this comment

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

If possible, I think that it would be good to replace the deprecated slot syntax before we merge 🙂

shell/dialog/SloDialog.vue Outdated Show resolved Hide resolved
shell/dialog/SloDialog.vue Outdated Show resolved Hide resolved
shell/edit/auth/saml.vue Outdated Show resolved Hide resolved
aalves08 and others added 3 commits August 21, 2024 17:42
Co-authored-by: Phillip Rak <rak.phillip@gmail.com>
Co-authored-by: Phillip Rak <rak.phillip@gmail.com>
@aalves08
Copy link
Member Author

@rak-phillip made the changes, tested, and all works fine! Thanks for the review! 🙏

- it uses the plain template which now contains it
- caused x2 modals to show
@aalves08 aalves08 merged commit 340d410 into rancher:master Aug 22, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support SAML Single Logout (SLO)
4 participants