[PM-29885] Implement SSO cookie vending authentication flow#6522
Open
SaintPatrck wants to merge 3 commits intomainfrom
Open
[PM-29885] Implement SSO cookie vending authentication flow#6522SaintPatrck wants to merge 3 commits intomainfrom
SaintPatrck wants to merge 3 commits intomainfrom
Conversation
github-actions
bot
added
app:password-manager
Contributor
|
New Issues (4)Checkmarx found the following issues in this Pull Request
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...tlin/com/x8bit/bitwarden/data/platform/manager/util/CookieConfigurationDataExtensionsTest.kt
Dismissed
Show dismissed
Hide dismissed
...tlin/com/x8bit/bitwarden/data/platform/manager/util/CookieConfigurationDataExtensionsTest.kt
Dismissed
Show dismissed
Hide dismissed
...tlin/com/x8bit/bitwarden/data/platform/manager/util/CookieConfigurationDataExtensionsTest.kt
Dismissed
Show dismissed
Hide dismissed
...tlin/com/x8bit/bitwarden/data/platform/manager/util/CookieConfigurationDataExtensionsTest.kt
Dismissed
Show dismissed
Hide dismissed
Contributor
|
Claude finished @SaintPatrck's task in 5m 56s —— View job Code Review: PM-29885 SSO Cookie Vending Authentication Flow
Review complete. Inline comments have been posted on the PR for specific findings. Overall the implementation follows Bitwarden's architectural patterns well — proper interface/impl separation, clean UDF integration via |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6522 +/- ##
==========================================
- Coverage 86.38% 86.31% -0.07%
==========================================
Files 777 797 +20
Lines 56175 56629 +454
Branches 8170 8192 +22
==========================================
+ Hits 48527 48882 +355
- Misses 4804 4895 +91
- Partials 2844 2852 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add cookie interceptor that manages ELB session cookies for enterprise SSO configurations. The interceptor attaches stored cookies to API requests, preempts requests when cookie bootstrap is needed, and detects HTTP 302 redirects to trigger cookie re-acquisition via an identity provider. Key components: - CookieInterceptor for OkHttp request/response cookie handling - NetworkCookieManager for bootstrap detection and cookie storage - CookieRedirectException to signal cookie acquisition flow - CookieProvider interface bridging network and app layers Co-Authored-By: Claude <noreply@anthropic.com>
SaintPatrck
force-pushed
the
cookie-vending/p9-t9_cookie-interceptor
branch
from
5087f55 to
0dc2bc5
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This commit updates the `RetrofitsTest` to ensure that the `CookieInterceptor` is consistently called across all Retrofit client configurations. Previously, the tests for `api`, `identity`, `events`, and `notifications` clients did not verify the presence of the cookie interceptor. This change adds an assertion (`assertTrue(isCookieInterceptorCalled)`) to each relevant test case to confirm that the interceptor is included in the chain.
...rc/main/kotlin/com/x8bit/bitwarden/data/platform/manager/network/NetworkCookieManagerImpl.kt
Outdated
Show resolved
Hide resolved
| // Return the response if it is not a redirect or does not contain | ||
| // a Location header. | ||
| val location = response.header(HEADER_LOCATION) | ||
| if (response.code != HTTP_REDIRECT || location == null) { |
Collaborator
There was a problem hiding this comment.
Should this be an &&?
Contributor
Author
There was a problem hiding this comment.
Hm. Now that I think about it, we shouldn't need this location check. We should technically already have the IdP login URL saved. Could have swore I saw that in the spec somewhere. 🤔
Let me double check with the other teams and make sure we actually need to check and capture this.
Thanks for calling attention to this.
| baseUrlsProvider = bitwardenServiceClientConfig.baseUrlsProvider, | ||
| ), | ||
| cookieInterceptor = CookieInterceptor( | ||
| cookieProvider = bitwardenServiceClientConfig.cookieProvider, |
Collaborator
There was a problem hiding this comment.
Minor but can you just reference the cookieProvider property from above
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29885
📔 Objective
Add cookie interceptor support for enterprise SSO configurations that require ELB session cookies. The interceptor manages the full cookie lifecycle for API requests behind a load balancer:
CookieheaderKey components:
CookieInterceptor— OkHttp interceptor for request/response cookie handlingNetworkCookieManager/NetworkCookieManagerImpl— Bootstrap detection and cookie storage bridgeCookieRedirectException— Signal exception to trigger cookie acquisition flowCookieProvider— Interface bridging the network and app layers