Server SAML Auth. Logout support.#5417
Open
zibet27 wants to merge 5 commits intozibet27/server-auth-saml-flowfrom
Open
Server SAML Auth. Logout support.#5417zibet27 wants to merge 5 commits intozibet27/server-auth-saml-flowfrom
zibet27 wants to merge 5 commits intozibet27/server-auth-saml-flowfrom
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds SAML Single Logout (SLO) support to the Ktor SAML authentication plugin through new public APIs, internal processors for handling logout requests and responses, and integration into the authentication provider. Includes SP-initiated and IdP-initiated logout flows with session management, signature verification, and replay protection. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable the changed files summary in the walkthrough.Disable the |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlLogoutTest.kt (1)
164-173: Consider consolidating repeatedSamlLogoutProcessorsetup in tests.There are multiple near-identical constructor blocks; reusing
createProcessor(...)(or a variant with flags) would reduce drift.Also applies to: 216-225, 305-314, 361-370, 409-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlLogoutTest.kt` around lines 164 - 173, Multiple tests repeatedly instantiate SamlLogoutProcessor with the same arguments; refactor by adding a helper factory (e.g., createProcessor or createProcessor(requireSignedLogoutRequest: Boolean = true, requireSignedLogoutResponse: Boolean = true, requireDestination: Boolean = true, other overrides...)) in SamlLogoutTest (or a companion/test util) that returns a configured SamlLogoutProcessor using idpMetadata, SamlSignatureVerifier(idpMetadata), clockSkew, and InMemorySamlReplayCache; replace the repeated constructor blocks (the one shown and the ones at the other ranges) with calls to this helper, passing only overrides when tests need different flags.ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/TestUtil.kt (1)
360-372: Deduplicate IdP metadata helper construction.
createTestIdPMetadataandcreateTestIdPMetadataWithSlobuild the same object shape and can be unified to avoid divergence.Also applies to: 479-491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/TestUtil.kt` around lines 360 - 372, Two helper functions createTestIdPMetadata and createTestIdPMetadataWithSlo duplicate constructing the same IdPMetadata; consolidate them into a single function. Replace both helpers with one createTestIdPMetadata(entityId: String = "https://idp.example.com", ssoUrl: String = "https://idp.example.com/sso", sloUrl: String? = "https://idp.example.com/slo") that calls generateTestCredentials() and returns IdPMetadata(entityId, ssoUrl, sloUrl, signingCredentials = listOf(credentials.credential)); then update all call sites that used createTestIdPMetadataWithSlo to call the unified createTestIdPMetadata (passing sloUrl as needed).ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlAuth.kt (1)
103-175: Document exception behavior and session prerequisite in publicsamlLogoutKDoc.These overloads can fail via
require(...)/checkNotNull(...), but notable failure cases are not documented in the public API docs.💡 Proposed KDoc additions
/** * Initiates SP-initiated SAML Single Logout. * * `@param` principal The authenticated SAML principal containing NameID and session info * `@param` spMetadata The Service Provider metadata containing the entity ID and signing credential * `@param` idpSloUrl The IdP's SLO URL * `@param` relayState Optional URL to redirect to after logout completes * `@param` signatureAlgorithm The signature algorithm to use for signing the LogoutRequest * `@return` [SamlRedirectResult] containing the request ID and redirect URL to the IdP + * `@throws` IllegalArgumentException if `nameId`, `idpSloUrl`, or `spEntityId` is blank + * `@throws` IllegalStateException if no [SamlSession] is available in [ApplicationCall.sessions] */As per coding guidelines "Public API must include KDoc documentation for parameters, return values, and notable exceptions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlAuth.kt` around lines 103 - 175, The public KDoc for both ApplicationCall.samlLogout overloads must document the failure cases: add notes that require(...) checks will throw IllegalArgumentException when spEntityId, nameId, or idpSloUrl are blank, and that the session lookup via checkNotNull(sessions.get<SamlSession>()) will throw IllegalStateException if no SamlSession is present; also document the prerequisite that sessions must be installed and an authenticated session (e.g., via authenticate()) must exist before calling samlLogout, referencing the functions ApplicationCall.samlLogout, the use of sessions.get<SamlSession>() and sessions.set(...), and the SamlSession.logoutRequestId usage so callers know to expect and handle these exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlAuthenticationProvider.kt`:
- Around line 294-301: The SLO handler currently silently prefers samlRequest
when both samlRequest and samlResponse are present; change the branching in
SamlAuthenticationProvider (the block that inspects samlRequest and samlResponse
and calls handleIdpLogoutRequest or handleLogoutResponse) to explicitly detect
the invalid case where both parameters are present, log an error
(logger.debug/error) and respond with HttpStatusCode.BadRequest (with a clear
message), otherwise keep the existing behavior: call handleIdpLogoutRequest when
only samlRequest is present and handleLogoutResponse when only samlResponse is
present.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlLogoutProcessor.kt`:
- Around line 159-170: processResponse currently decodes and parses the SAML
response without normalizing failures, so wrap the decode/parse/unmarshall
sequence inside the same withValidationException used by processRequest so
malformed Base64/XML throws SamlValidationException; specifically, enclose the
call to samlResponseBase64.decodeSamlMessage(...), the
LibSaml.parserPool.parse(responseXml.toByteArray().inputStream()), and
document.documentElement.unmarshall<LogoutResponse>() within
withValidationException to translate parsing errors into
SamlValidationException.
- Around line 171-173: The current check in SamlLogoutProcessor uses
samlAssert(expectedRequestId == null || inResponseTo == expectedRequestId) which
lets a null expectedRequestId bypass validation; change it to require strict
correlation by asserting inResponseTo == expectedRequestId and fail if
expectedRequestId is null (e.g., samlAssert(expectedRequestId != null &&
inResponseTo == expectedRequestId) with a clear message like "InResponseTo
mismatch or missing expectedRequestId"), ensuring logoutResponse.inResponseTo is
always validated for SP-initiated flows.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlUtils.kt`:
- Around line 95-103: In decodeSamlMessage, the custom Inflater created for
decompression must be explicitly ended and the InflaterInputStream closed to
avoid native buffer leaks; update the function (decodeSamlMessage) to open the
Inflater and InflaterInputStream in a try/finally or Kotlin use{}-style block,
ensure inflaterInputStream.close() and call inflater.end() in the finally (or
after use) so both the stream and the Inflater are properly released when
isDeflated is true.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlLogoutIntegrationTest.kt`:
- Around line 123-159: The test currently posts LogoutResponse without a prior
session-stored logoutRequestId so InResponseTo correlation isn't exercised;
update the test `test LogoutResponse processing with success and failure status`
to first create a session logout request (e.g., call the logout initiation
endpoint or otherwise populate the session with the logoutRequestId used by the
SLO flow) before posting the success and failure SAMLResponses (use the same ID
for the success case to exercise the correlation path and a different/missing ID
for the failure case), then assert the behavior when `InResponseTo` matches the
stored logoutRequestId versus when it does not (locations to change:
`configureSamlAuth`, the test function, and where you build
`successResponseXml`/`failureResponseXml` using
`SamlTestUtils.createLogoutResponse` and `SLO_PATH`).
---
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlAuth.kt`:
- Around line 103-175: The public KDoc for both ApplicationCall.samlLogout
overloads must document the failure cases: add notes that require(...) checks
will throw IllegalArgumentException when spEntityId, nameId, or idpSloUrl are
blank, and that the session lookup via checkNotNull(sessions.get<SamlSession>())
will throw IllegalStateException if no SamlSession is present; also document the
prerequisite that sessions must be installed and an authenticated session (e.g.,
via authenticate()) must exist before calling samlLogout, referencing the
functions ApplicationCall.samlLogout, the use of sessions.get<SamlSession>() and
sessions.set(...), and the SamlSession.logoutRequestId usage so callers know to
expect and handle these exceptions.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlLogoutTest.kt`:
- Around line 164-173: Multiple tests repeatedly instantiate SamlLogoutProcessor
with the same arguments; refactor by adding a helper factory (e.g.,
createProcessor or createProcessor(requireSignedLogoutRequest: Boolean = true,
requireSignedLogoutResponse: Boolean = true, requireDestination: Boolean = true,
other overrides...)) in SamlLogoutTest (or a companion/test util) that returns a
configured SamlLogoutProcessor using idpMetadata,
SamlSignatureVerifier(idpMetadata), clockSkew, and InMemorySamlReplayCache;
replace the repeated constructor blocks (the one shown and the ones at the other
ranges) with calls to this helper, passing only overrides when tests need
different flags.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/TestUtil.kt`:
- Around line 360-372: Two helper functions createTestIdPMetadata and
createTestIdPMetadataWithSlo duplicate constructing the same IdPMetadata;
consolidate them into a single function. Replace both helpers with one
createTestIdPMetadata(entityId: String = "https://idp.example.com", ssoUrl:
String = "https://idp.example.com/sso", sloUrl: String? =
"https://idp.example.com/slo") that calls generateTestCredentials() and returns
IdPMetadata(entityId, ssoUrl, sloUrl, signingCredentials =
listOf(credentials.credential)); then update all call sites that used
createTestIdPMetadataWithSlo to call the unified createTestIdPMetadata (passing
sloUrl as needed).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/api/ktor-server-auth-saml.apiktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlAuth.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlAuthenticationProvider.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlLogoutBuilder.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlLogoutProcessor.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlUtils.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlLogoutIntegrationTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlLogoutTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/TestUtil.kt
...plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlAuthenticationProvider.kt
Show resolved
Hide resolved
...server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlLogoutProcessor.kt
Show resolved
Hide resolved
...server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlLogoutProcessor.kt
Show resolved
Hide resolved
...rver/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlUtils.kt
Show resolved
Hide resolved
...plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlLogoutIntegrationTest.kt
Show resolved
Hide resolved
zibet27
force-pushed
the
zibet27/server-auth-saml-logout
branch
from
5b9b71e to
c57c368
Compare
jk1
reviewed
Mar 16, 2026
|
|
||
| when { | ||
| samlRequest != null && samlResponse != null -> { | ||
| logger.debug("SLO endpoint failed. Both `SAMLRequest` and `SAMLRequest` are present") |
Collaborator
There was a problem hiding this comment.
SAMLRequest and SAMLResponse?
jk1
reviewed
Mar 16, 2026
| idpSloUrl = idpSloUrl, | ||
| inResponseTo = logoutRequest.requestId, | ||
| statusCodeValue = StatusCode.SUCCESS, | ||
| relayState = parameters["RelayState"], |
Collaborator
There was a problem hiding this comment.
It's usually a good idea to validate every incoming relay state against an allowed list to avoid open redirect attacks. As a framework, Ktor obviously lacks context to implement transparent relay state validation under the hood. Yet it can support optional user-defined validators.
This is also applicable to other flows involving the relay state parameter, e.g. IdP-initiated sign-in.
Collaborator
Author
There was a problem hiding this comment.
Yeah, we already check the relay state in AuthenticationContext.handleChallenge, but it should be done everywhere.
Atm, we accept optional allowedRelayStateUrls, but I'll improve it and support custom validators.
jk1
requested changes
Mar 16, 2026
jk1
approved these changes
Mar 18, 2026
e5l
requested changes
Mar 31, 2026
| document.documentElement.unmarshall<LogoutResponse>() | ||
| } | ||
|
|
||
| val inResponseTo = logoutResponse.inResponseTo |
| } | ||
|
|
||
| val destination = logoutRequest.destination | ||
| samlAssert(!requireDestination || destination != null) { "LogoutRequest Destination is not present" } |
Member
There was a problem hiding this comment.
the validation (both for request and response) is complicated and it is easy to miss a field. Please consider extracting it to the utility function and avoid mixing validation and authentication logic
| signatureVerifier = signatureVerifier, | ||
| requireDestination = config.requireDestination, | ||
| requireSignedLogoutRequest = config.requireSignedLogoutRequest, | ||
| requireSignedLogoutResponse = config.requireSignedResponse, |
Member
There was a problem hiding this comment.
Could you explain how the flag requireSignedResponse will work?
| ) | ||
|
|
||
| // Store the logout request ID in the session for InResponseTo validation | ||
| val currentSession = checkNotNull(sessions.get<SamlSession>()) { |
Member
There was a problem hiding this comment.
Please add test with expires session. It looks like it will fail with wrong message and status code
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
3 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.
Subsystem
Server Auth
Motivation
KTOR-601 SAML Support
Chunk of #5392
Solution
This PR adds SAML 2.0 Single Logout support to the new server auth SAML plugin.
Logout features:
LogoutRequestsand sendsLogoutResponsesInResponseTovalidation for logout responses