Make logout function more robust to prevent session issues #11361
Conversation
Improve cookie cleanup during logout by removing cookies across multiple paths and domains to ensure complete session termination. Fixes apache#11078
DaanHoogland
requested review from
Copilot and
DaanHoogland
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR improves the logout function's cookie cleanup mechanism to ensure complete session termination by removing cookies across multiple paths and domains. The change addresses session issues that could occur when cookies are not properly cleaned up during logout.
- Replaces simple cookie removal with comprehensive cleanup across multiple paths and domains
- Adds logic to handle cookies for root path, client path, and various domain configurations
- Uses nested loops to systematically remove all cookies with different path/domain combinations
| @@ -472,9 +472,17 @@ const user = { | |||
| }).catch(() => { | |||
| resolve() | |||
| }).finally(() => { | |||
| const paths = ['/', '/client'] | |||
There was a problem hiding this comment.
[nitpick] Consider extracting the hardcoded paths array to a configuration constant or module-level variable to improve maintainability and make it easier to modify these paths in the future.
| const paths = ['/', '/client'] | |
| const paths = COOKIE_PATHS |
Copilot uses AI. Check for mistakes.
| @@ -472,9 +472,17 @@ const user = { | |||
| }).catch(() => { | |||
| resolve() | |||
| }).finally(() => { | |||
| const paths = ['/', '/client'] | |||
| const hostname = window.location.hostname | |||
| const domains = [undefined, hostname, `.${hostname}`] | |||
There was a problem hiding this comment.
[nitpick] The domains array construction could be simplified and made more readable by extracting the dot-prefixed hostname to a separate variable, e.g., const dotHostname = '.${hostname}' and then using [undefined, hostname, dotHostname].
| const domains = [undefined, hostname, `.${hostname}`] | |
| const dotHostname = `.${hostname}` | |
| const domains = [undefined, hostname, dotHostname] |
Copilot uses AI. Check for mistakes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11361 +/- ##
=========================================
Coverage 16.15% 16.16%
- Complexity 13277 13281 +4
=========================================
Files 5656 5656
Lines 497912 497948 +36
Branches 60383 60388 +5
=========================================
+ Hits 80462 80490 +28
- Misses 408494 408496 +2
- Partials 8956 8962 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@kiranchavala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 14549 |
There was a problem hiding this comment.
LGTM - tested and verified without and with the fix (tested regular and SAML auth)
Test Results Overview:
- Regular Auth Logout: Fixed from 9+ error requests to 4 clean requests
- SAML Auth Logout: Fixed from API flooding (thousands of requests) to 4 clean requests
- Cookie Cleanup - Regular: Fixed from partial/incomplete removal to complete cleanup
- Cookie Cleanup - SAML: Fixed from mixed paths remaining to all paths cleaned
- Authorization Errors: Fixed from 401 Unauthorized errors to no errors
- User Experience: Fixed from error notifications to clean logout flow
SAML Testing Setup Process
Since the original issue was SAML-specific, MockSAML.com was used - a free online SAML 2.0 Identity Provider designed for testing SAML SSO integrations.
-
Accessed MockSAML.com
-
Configured CloudStack Global Settings:
saml2.enabled = true
saml2.sp.id = org.apache.cloudstack
saml2.idp.metadata.url = https://mocksaml.com/api/saml/metadata
saml2.redirect.url = http://10.0.35.20:8080/client/
saml2.default.idpid = https://saml.example.com/entityid
saml2.user.attribute = email
saml2.user.sessionkey.path = /
- Created SAML Test User via CloudMonkey:
bashcreate user [email protected] [email protected]
firstname=Jackson lastname=Test password=temppass123 account=admin
authorizeSamlSso enable=true entityid=https://saml.example.com/entityid
userid=b14581e4-dcbf-47a8-9b61-de94dc7d53f9
- Configured MockSAML Integration:
ACS URL: http://10.0.35.20:8080/client/api?command=samlSso
Test credentials: [email protected]
Detailed Test Results
Test Scenario 1: Regular Authentication (Control Test)
WITHOUT Fix
- Network Requests: 9+ requests with multiple 401 errors
- Cookie Cleanup: Incomplete - partial session state remains
WITH Fix
- Network Requests: 4 clean requests, no errors
- Cookie Cleanup: Complete removal of all session cookies
- User Experience: Clean logout to login page
Test Scenario 2: SAML Authentication (Primary Test)
SAML Login State Analysis
Critical Cookie Configuration Observed:
isSAML = true (confirms SAML authentication)
Mixed Path Cookies:
sessionkey at path /
sessionkey at path /client...
JSESSIONID at path /client
Multiple Domain Variations: cookies across different domain formats
This exact state previously caused the API flooding bug
SAML Logout Test Results - WITH Fix
- Network Requests: 4 clean requests (identical to regular auth)
- Request Breakdown:
logout&sessionkey=... (200 OK)
listIdps (200 OK)
listOauthProvider (200 OK)
forgotPassword (401 - expected)
Test Results:
- All SAML session cookies removed
- Both / and /client path cookies cleaned
- All domain variations cleared
- Zero cookies remaining
Description
Fixes #11078
This PR extends the logout function to improve cookie cleanup by removing cookies for multiple paths and domains to ensure complete session termination.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?