Server SAML Auth. SSO authentication flow.#5416
Open
zibet27 wants to merge 4 commits intozibet27/server-auth-saml-corefrom
Open
Server SAML Auth. SSO authentication flow.#5416zibet27 wants to merge 4 commits intozibet27/server-auth-saml-corefrom
zibet27 wants to merge 4 commits intozibet27/server-auth-saml-corefrom
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:
WalkthroughIntroduces comprehensive SAML 2.0 authentication plugin for Ktor Server with public APIs for authentication configuration, request/response processing, and session handling. Includes internal components for signature verification, replay protection, and relay state validation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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 |
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: 4
🧹 Nitpick comments (4)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlUtils.kt (1)
87-94: Guard builder lookup/casts with validation errors instead of raw cast failures.On Line 87-94 and Line 162-168, direct casts from
getBuilder(key)can throw opaque cast/null errors; returningSamlValidationExceptionhere gives clearer diagnostics.💡 Suggested refactor
`@Suppress`("UNCHECKED_CAST") internal inline fun <O : SAMLObject> XMLObjectBuilderFactory.build( key: QName, crossinline configure: O.() -> Unit ): O { - val builder = getBuilder(key) as SAMLObjectBuilder<O> + val builder = samlRequire(getBuilder(key) as? SAMLObjectBuilder<O>) { + "No SAMLObjectBuilder found for element: $key" + } return builder.buildObject().apply(configure) } @@ `@Suppress`("UNCHECKED_CAST") internal inline fun <O : XMLObject> XMLObjectBuilderFactory.buildXmlObject( key: QName, crossinline configure: O.() -> Unit ): O { - val builder = getBuilder(key) as org.opensaml.core.xml.XMLObjectBuilder<O> + val builder = samlRequire(getBuilder(key) as? org.opensaml.core.xml.XMLObjectBuilder<O>) { + "No XMLObjectBuilder found for element: $key" + } return builder.buildObject(key).apply(configure) }Also applies to: 162-168
🤖 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/SamlUtils.kt` around lines 87 - 94, The XMLObjectBuilderFactory.build extension currently does an unchecked cast of getBuilder(key) to SAMLObjectBuilder<O> which can throw ClassCastException or return null; replace the raw cast with a safe lookup and explicit validation that throws a SamlValidationException with a helpful message if the builder is missing or of the wrong type: call getBuilder(key), check for null, verify it is an instance of SAMLObjectBuilder<*>, then cast safely to SAMLObjectBuilder<O> and proceed to buildObject().apply(configure); apply the same pattern to the other occurrence (the similar unchecked cast at the second build site) to ensure failures produce SamlValidationException with context (including the QName key and expected type).ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlResponseProcessor.kt (1)
240-243: Add KDoc for public exception class parameters.
SamlValidationExceptionis part of the public API but lacks documentation for its parameters.📝 Proposed KDoc addition
/** * Exception thrown when SAML validation fails. + * + * `@param` message A description of the validation failure + * `@param` cause The underlying exception that caused the validation failure, if any */ public class SamlValidationException(message: String, cause: Throwable? = null) : Exception(message, cause)🤖 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/SamlResponseProcessor.kt` around lines 240 - 243, SamlValidationException is a public API but its constructor parameters are undocumented; add KDoc for the class and explicitly document the constructor parameters `message: String` and `cause: Throwable?` (describe what message represents and that cause is the optional underlying exception) directly above the class declaration `SamlValidationException(message: String, cause: Throwable? = null)` so the public exception class has proper KDoc for both parameters.ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlAuthenticationProvider.kt (2)
335-339: Consider makingSamlSessiona data class.
SamlSessionis a simple value holder. Declaring it as adata classwould provideequals(),hashCode(),copy(), andcomponentN()functions, which can be useful for testing and session comparison.📝 Proposed change
`@Serializable` -public class SamlSession( +public data class SamlSession( public val requestId: String, public val logoutRequestId: String? = null )🤖 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/SamlAuthenticationProvider.kt` around lines 335 - 339, The SamlSession class is acting as a simple value holder and should be converted to a data class to get generated equals/hashCode/copy/componentN methods; change the declaration of SamlSession to a data class (i.e., replace "class SamlSession" with "data class SamlSession") while keeping the `@Serializable` annotation and the same primary constructor properties (requestId and logoutRequestId) so existing usages of SamlSession continue to work.
274-276: Path prefix matching may reject valid sub-paths.The condition
normalized == prefix || prefix.endsWith("/")means a prefix like/apiwill reject/api/usersbecause/apidoesn't end with/. Users must configure/api/(with trailing slash) for sub-path matching to work.Consider documenting this behavior or adjusting the logic to allow path-segment boundaries.
📝 Option A: Document the behavior
Add a note in the configuration DSL documentation that path prefixes must end with
/to match sub-paths.📝 Option B: Allow segment-boundary matching
pathPrefixes.any { prefix -> - normalized.startsWith(prefix) && (normalized == prefix || prefix.endsWith("/")) + normalized.startsWith(prefix) && (normalized == prefix || prefix.endsWith("/") || normalized.getOrNull(prefix.length) == '/') }🤖 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/SamlAuthenticationProvider.kt` around lines 274 - 276, The current path prefix check inside pathPrefixes.any (using variables normalized and prefix) rejects sub-paths unless the configured prefix ends with '/', so update the condition to allow segment-boundary matching: accept when normalized == prefix or normalized.startsWith(prefix + "/") or when prefix already endsWith("/"); modify the lambda in pathPrefixes.any accordingly (referencing the pathPrefixes.any { prefix -> ... } block) and add/adjust tests or docs for SamlAuthenticationProvider to cover both trailing-slash and segment-boundary matches.
🤖 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/SamlSignatureValidator.kt`:
- Around line 111-116: The current logic in SamlSignatureValidator uses
queryString.indexOf("&Signature=") which fails when Signature is the first
parameter; update the removal to handle Signature at any position by locating
"Signature=" (not just "&Signature=") and removing the entire "Signature=...{&
or end}" segment, or better parse the query string into key=value pairs, filter
out the "Signature" key, then rejoin and convert to UTF_8 for
queryStringWithoutSignature; update references to signatureIdx and
queryStringWithoutSignature accordingly so verification works regardless of
parameter order.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlUtils.kt`:
- Around line 210-215: The helper withValidationException currently catches all
Exceptions and always throws a new SamlValidationException("SAML validation
failed", e), which swallows existing SamlValidationException details; change it
so that if the caught exception is already a SamlValidationException it is
rethrown unchanged (preserving its message and context), and only wrap
non-SamlValidationException instances in a new SamlValidationException
(including the original exception as the cause) so existing context is not lost.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SAMLResponseProcessorTest.kt`:
- Around line 265-270: The test constructs an assertion with
SamlTestUtils.createTestAssertion where notBefore is set to the future
(Clock.System.now() + 10.minutes) to trigger the "not yet valid" condition, but
notOnOrAfter is incorrectly set to the past (Clock.System.now() - 20.minutes)
which could make the assertion expire instead; update the assertion parameters
so notOnOrAfter is after notBefore (e.g., set notOnOrAfter to Clock.System.now()
+ 30.minutes or any time after notBefore) to isolate the not-before check and
ensure the test fails only for "not yet valid" as intended.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/TestUtil.kt`:
- Around line 104-106: The signAssertion function uses OpenSAML builders but
does not call LibSaml.ensureInitialized(); to fix, add a call to
LibSaml.ensureInitialized() at the start of signAssertion (mirroring other
helpers) so the XMLObjectProviderRegistrySupport builder factory is initialized
before using it; update the signAssertion function to invoke
LibSaml.ensureInitialized() prior to obtaining the builderFactory and proceeding
with signature setup.
---
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlAuthenticationProvider.kt`:
- Around line 335-339: The SamlSession class is acting as a simple value holder
and should be converted to a data class to get generated
equals/hashCode/copy/componentN methods; change the declaration of SamlSession
to a data class (i.e., replace "class SamlSession" with "data class
SamlSession") while keeping the `@Serializable` annotation and the same primary
constructor properties (requestId and logoutRequestId) so existing usages of
SamlSession continue to work.
- Around line 274-276: The current path prefix check inside pathPrefixes.any
(using variables normalized and prefix) rejects sub-paths unless the configured
prefix ends with '/', so update the condition to allow segment-boundary
matching: accept when normalized == prefix or normalized.startsWith(prefix +
"/") or when prefix already endsWith("/"); modify the lambda in pathPrefixes.any
accordingly (referencing the pathPrefixes.any { prefix -> ... } block) and
add/adjust tests or docs for SamlAuthenticationProvider to cover both
trailing-slash and segment-boundary matches.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlResponseProcessor.kt`:
- Around line 240-243: SamlValidationException is a public API but its
constructor parameters are undocumented; add KDoc for the class and explicitly
document the constructor parameters `message: String` and `cause: Throwable?`
(describe what message represents and that cause is the optional underlying
exception) directly above the class declaration
`SamlValidationException(message: String, cause: Throwable? = null)` so the
public exception class has proper KDoc for both parameters.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlUtils.kt`:
- Around line 87-94: The XMLObjectBuilderFactory.build extension currently does
an unchecked cast of getBuilder(key) to SAMLObjectBuilder<O> which can throw
ClassCastException or return null; replace the raw cast with a safe lookup and
explicit validation that throws a SamlValidationException with a helpful message
if the builder is missing or of the wrong type: call getBuilder(key), check for
null, verify it is an instance of SAMLObjectBuilder<*>, then cast safely to
SAMLObjectBuilder<O> and proceed to buildObject().apply(configure); apply the
same pattern to the other occurrence (the similar unchecked cast at the second
build site) to ensure failures produce SamlValidationException with context
(including the QName key and expected type).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
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/SamlRequestBuilder.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlResponseProcessor.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlSignatureValidator.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/RelayStateValidationTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SAMLPrincipalTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SAMLRequestBuilderTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SAMLResponseProcessorTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlAuthTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/TestUtil.kt
...ver-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlSignatureValidator.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/SAMLResponseProcessorTest.kt
Show resolved
Hide resolved
...rver/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/TestUtil.kt
Show resolved
Hide resolved
zibet27
force-pushed
the
zibet27/server-auth-saml-core
branch
from
c69399e to
13a32a2
Compare
jk1
approved these changes
Mar 23, 2026
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
Security hardening:
Configuration DSL: