Add SSO session reuse and RP-initiated logout integration tests - #4078
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a live-server SSO integration suite covering fixture lifecycle, authorization-code login, per-flow SSO reuse, and RP-initiated logout with redirect validation, cookie clearing, and session termination. ChangesSSO session lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RPTestClient
participant LogoutEndpoint
participant SignOutFlow
participant LogoutCallback
RPTestClient->>LogoutEndpoint: POST id_token_hint, redirect URI, and state
LogoutEndpoint-->>RPTestClient: 302 redirect with executionId and logoutId
RPTestClient->>SignOutFlow: Execute sign-out steps
SignOutFlow-->>RPTestClient: COMPLETE and cleared SSO cookies
RPTestClient->>LogoutCallback: POST logoutId
LogoutCallback-->>RPTestClient: Validated redirect_uri and state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/oauth/sso/rp_logout_test.go`:
- Around line 64-66: Strengthen the assertion in the reauthorization flow using
reAuthStep: verify the exact credential-prompt status rather than only asserting
it is not COMPLETE, and validate the expected prompt metadata (or complete a
second credential login) to prove logout prevented SSO skipping.
In `@tests/integration/oauth/sso/suite_test.go`:
- Around line 268-277: Update the teardown cleanup block to delete the user type
identified by ts.entityTypeID before deleting the organization unit identified
by testOUID. Preserve the existing error logging and cleanup conditions for both
deletions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 566b6a35-f103-4f72-ae60-4ec4478bec6c
📒 Files selected for processing (3)
tests/integration/oauth/sso/rp_logout_test.gotests/integration/oauth/sso/sso_reuse_test.gotests/integration/oauth/sso/suite_test.go
madurangasiriwardena
added
skip-changelog
6e3452e to
8321f3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/oauth/sso/suite_test.go`:
- Around line 265-278: Update the cleanup logic in the suite teardown to use
ts.T().Errorf instead of ts.T().Logf for failures from testutils.DeleteUser and
testutils.DeleteOrganizationUnit, so persistent fixture cleanup failures fail
the suite. Keep the existing error messages and DeleteUserType handling
unchanged.
- Around line 29-40: Update newSessionClient to configure a finite HTTP client
timeout, ensuring stalled SSO requests terminate before the outer test timeout
while preserving the existing client and cookie-jar setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a8d0568-bcbf-49bf-9679-631715c5dbec
📒 Files selected for processing (3)
tests/integration/oauth/sso/rp_logout_test.gotests/integration/oauth/sso/sso_reuse_test.gotests/integration/oauth/sso/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/oauth/sso/sso_reuse_test.go
- tests/integration/oauth/sso/rp_logout_test.go
| import ( | ||
| "bytes" | ||
| "crypto/tls" | ||
| "encoding/json" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "net/http/cookiejar" | ||
| "net/url" | ||
| "strings" | ||
| "testing" | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline tests/integration/oauth/sso/suite_test.go \
--match newSessionClient --view expanded
rg -n -C3 --glob '*.go' \
'\bnewSessionClient\s*\(|\bTimeout\s*:' tests/integration/oauth/ssoRepository: thunder-id/thunderid
Length of output: 2469
🏁 Script executed:
#!/bin/bash
sed -n '360,390p' tests/integration/oauth/sso/suite_test.goRepository: thunder-id/thunderid
Length of output: 1261
Add an HTTP client timeout to newSessionClient. A stalled request can block the SSO integration suite until the outer test timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/oauth/sso/suite_test.go` around lines 29 - 40, Update
newSessionClient to configure a finite HTTP client timeout, ensuring stalled SSO
requests terminate before the outer test timeout while preserving the existing
client and cookie-jar setup.
Source: Linters/SAST tools
| for _, userID := range ts.userIDs { | ||
| if err := testutils.DeleteUser(userID); err != nil { | ||
| ts.T().Logf("Failed to delete test user %s: %v", userID, err) | ||
| } | ||
| } | ||
| if ts.entityTypeID != "" { | ||
| if err := testutils.DeleteUserType(ts.entityTypeID); err != nil { | ||
| ts.T().Errorf("Failed to delete test user type: %v", err) | ||
| } | ||
| } | ||
| if testOUID != "" { | ||
| if err := testutils.DeleteOrganizationUnit(testOUID); err != nil { | ||
| ts.T().Logf("Failed to delete test organization unit %s: %v", testOUID, err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fail the suite when cleanup leaves persistent fixtures behind.
Using Logf for user or OU deletion failures lets the suite pass while leaking fixed-name resources that can break subsequent runs.
Proposed fix
for _, userID := range ts.userIDs {
if err := testutils.DeleteUser(userID); err != nil {
- ts.T().Logf("Failed to delete test user %s: %v", userID, err)
+ ts.T().Errorf("Failed to delete test user %s: %v", userID, err)
}
}
...
if testOUID != "" {
if err := testutils.DeleteOrganizationUnit(testOUID); err != nil {
- ts.T().Logf("Failed to delete test organization unit %s: %v", testOUID, err)
+ ts.T().Errorf("Failed to delete test organization unit %s: %v", testOUID, err)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, userID := range ts.userIDs { | |
| if err := testutils.DeleteUser(userID); err != nil { | |
| ts.T().Logf("Failed to delete test user %s: %v", userID, err) | |
| } | |
| } | |
| if ts.entityTypeID != "" { | |
| if err := testutils.DeleteUserType(ts.entityTypeID); err != nil { | |
| ts.T().Errorf("Failed to delete test user type: %v", err) | |
| } | |
| } | |
| if testOUID != "" { | |
| if err := testutils.DeleteOrganizationUnit(testOUID); err != nil { | |
| ts.T().Logf("Failed to delete test organization unit %s: %v", testOUID, err) | |
| } | |
| for _, userID := range ts.userIDs { | |
| if err := testutils.DeleteUser(userID); err != nil { | |
| ts.T().Errorf("Failed to delete test user %s: %v", userID, err) | |
| } | |
| } | |
| if ts.entityTypeID != "" { | |
| if err := testutils.DeleteUserType(ts.entityTypeID); err != nil { | |
| ts.T().Errorf("Failed to delete test user type: %v", err) | |
| } | |
| } | |
| if testOUID != "" { | |
| if err := testutils.DeleteOrganizationUnit(testOUID); err != nil { | |
| ts.T().Errorf("Failed to delete test organization unit %s: %v", testOUID, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/oauth/sso/suite_test.go` around lines 265 - 278, Update the
cleanup logic in the suite teardown to use ts.T().Errorf instead of ts.T().Logf
for failures from testutils.DeleteUser and testutils.DeleteOrganizationUnit, so
persistent fixture cleanup failures fail the suite. Keep the existing error
messages and DeleteUserType handling unchanged.
Cover two gaps in integration coverage for the flow-based SSO session lifecycle: reusing an established per-flow session on a subsequent authorize (login skip), and ending it via the OIDC RP-initiated logout end_session_endpoint (sign-out flow, cookie clear, post-logout redirect). Also add the missing postgres RUNTIME_STORE partition for the 'logout:req' namespace. The namespace was introduced with the sign-out feature but its LIST partition was never declared, so persisting a logout request fails on postgres (no partition of relation "RUNTIME_STORE" found for row). This surfaced only under the postgres backend, which the new logout test exercises.
8321f3b to
9b0d128
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Purpose
Adds end-to-end integration tests for the flow-based SSO session lifecycle, closing two gaps in integration coverage that existed after the session sign-out feature (#3973) merged:
Before this PR, the only integration-level assertion touching this feature was the discovery document advertising end_session_endpoint; the session establish/reuse and full logout flows were covered only by unit/component tests.
Approach
The tests live in a new package tests/integration/oauth/sso/ and run against the live test server, exercising the same observable HTTP behaviour a browser would. A cookiejar-backed HTTP client carries the per-flow SSO cookie (tid_sso_*) across requests so session reuse and cookie clearing are genuinely tested, not mocked.
Shared suite setup (suite_test.go) creates:
TestSSOSessionReuseSkipsAuthentication — after a first credential login establishes the session, a second authorize completes on its initial /flow/execute step (assertion issued) without prompting, proving the SSO skip.
TestRPInitiatedLogoutEndsSession — drives the full RP-initiated logout: POST /oauth2/logout (valid id_token_hint + registered post_logout_redirect_uri) → run the sign-out flow → /oauth2/logout/callback returns the post-logout redirect with state; then asserts the SSO cookie is cleared and a fresh authorize re-prompts for credentials.
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit