Server SAML Auth. Declare configs, constants and metadata.#5415
Open
Server SAML Auth. Declare configs, constants and metadata.#5415
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Ktor Server SAML authentication plugin: OpenSAML dependency and repository, build wiring, implementation and public API for SAML (constants, config DSL, metadata parsing, initializer with secure parser pool, crypto helpers, replay cache, utils) and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant KtorAuth as Ktor SAML Auth
participant LibSaml as LibSaml/OpenSAML
participant Cache as SamlReplayCache
participant Keystore as KeyStore/Credentials
participant IdP as IdentityProvider
Client->>KtorAuth: Submit SAML Response / initiate SSO
KtorAuth->>LibSaml: ensureInitialized() / parse SAML XML
LibSaml-->>KtorAuth: XMLObject (Assertion)
KtorAuth->>Cache: tryRecordAssertion(assertionId, expiry)
alt not replayed
KtorAuth->>Keystore: loadCredential(...) (signature verification)
KtorAuth->>LibSaml: validate signature / verify assertion
KtorAuth->>IdP: (optional) parseSamlIdpMetadata(xml)
KtorAuth-->>Client: Authenticate principal / set session
else replayed
KtorAuth-->>Client: Reject (replay detected)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
![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: 7
🧹 Nitpick comments (1)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlConfigTest.kt (1)
12-12: Consider tightening test names to behavior-only phrasing.The tests are clear already; optionally drop the
testprefix to match the repository’s backtick naming pattern more closely.As per coding guidelines "Use descriptive test names in backticks following the pattern:
describe what is being tested".Also applies to: 22-22, 32-32, 47-47
🤖 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/SamlConfigTest.kt` at line 12, Rename the test functions to remove the redundant "test" prefix so they follow the backtick behavior-only naming convention; e.g., change the function named `test default SamlSpMetadata values` to ``default SamlSpMetadata values``, and do the same for the other functions referenced (the ones at the same positions as the declarations currently named with a `test` prefix). Update any direct references to these function names (if used elsewhere) and ensure the tests still compile and run.
🤖 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/SamlConfig.kt`:
- Around line 80-85: The KDoc example for SamlSpMetadata is stale: replace the
placeholder "keyStore { ... }" with the current SAML SP key configuration API
used by SamlSpMetadata (whatever property or builder function now exposes
keys/signing certificate, e.g., the current keystore/keyPair/signingKey
configuration); update the snippet under SamlSpMetadata so it compiles with the
present API and demonstrates setting the SP keys alongside spEntityId and
acsUrl, referring to the SamlSpMetadata builder and the actual key configuration
symbol (the current keystore/keyPair/signingKey API) used in the codebase.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt`:
- Around line 369-371: The code currently logs and continues when
signingCredentials.isEmpty() in the Saml metadata parsing path (the
signingCredentials check that calls LOGGER.warn("No signing certificates...")),
which allows invalid IdP metadata to silently pass; change this to fail fast by
throwing a descriptive exception (e.g., IllegalArgumentException or a
SamlMetadataParseException) when signingCredentials.isEmpty() so
parsing/initialization aborts immediately with a clear error instead of only
logging a warning.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlPrincipal.kt`:
- Around line 10-13: The KDoc in SamlPrincipal.kt incorrectly states that
SamlCredential "should never be exposed to application code" while the public
API (SamlConfig.validate) exposes it; update the documentation to accurately
reflect that SamlCredential may be provided to user code via SamlConfig.validate
(or clarify the exposure rules), mentioning the SamlCredential type and
SamlConfig.validate so readers understand when it is safe/validated and when it
is internal.
- Around line 142-150: The buildAttributesMap function in
Assertion.buildAttributesMap currently force-unwraps attribute.name (!!) and
overwrites previous entries for the same attribute name; change it to safely
handle null names (skip attributes with null name using attribute.name?.let {
name -> ... } or continue) and merge values when the same name appears across
attributeStatements by appending to an existing list instead of replacing it
(retrieve existing list with get(name) and addAll(values) or use compute(name)
to combine lists), preserving the non-null extracted texts from
attribute.attributeValues.mapNotNull { it.dom?.textContent } and only put into
the map when the resulting list is not empty.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlReplayCache.kt`:
- Around line 53-57: The public API is missing complete KDoc for parameters and
return values: add KDoc tags for the constructor parameter parentScope (describe
its purpose and lifecycle) and for the method tryRecordAssertion(assertionId:
String, expirationTime: Instant): Boolean (document assertionId, expirationTime,
returned Boolean meaning, and any notable exceptions or coroutine/timeout
behavior); update the KDoc on the SamlReplayCache class/constructor and the
tryRecordAssertion function to include `@param` for parentScope, assertionId,
expirationTime and an `@return` explaining true/false semantics and any `@throws` or
concurrency notes.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlSecurity.kt`:
- Around line 58-73: loadCredential currently can let IO/crypto exceptions and
NoSuchElementException escape instead of the declared KeyStoreException; modify
loadCredential to guard all calls that may throw (calls to loadKeyStore,
keyStore.getKey, keyStore.getCertificateChain and the .first() access) by
wrapping the body in a try/catch that catches Exception (or the specific checked
exceptions) and rethrows a KeyStoreException with the original exception as the
cause, and replace certificateChain.first() with certificateChain.firstOrNull()
and throw a KeyStoreException("Certificate chain for alias '$keyAlias' not found
or empty") when null so callers only receive KeyStoreException (preserving the
original exception as the cause).
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/InMemorySamlReplayCacheTest.kt`:
- Around line 56-75: The test `test concurrent access safety` uses launch
without a dispatcher so child coroutines run on the runTest test dispatcher
(sequential), which won't surface real data races in InMemorySamlReplayCache;
update the coroutine launches to run on a real parallel dispatcher (for example
use launch(Dispatchers.Default) or wrap the assertion loop in
withContext(Dispatchers.Default)) so the cache methods (recordAssertion,
isReplayed) are exercised concurrently across threads.
---
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlConfigTest.kt`:
- Line 12: Rename the test functions to remove the redundant "test" prefix so
they follow the backtick behavior-only naming convention; e.g., change the
function named `test default SamlSpMetadata values` to ``default SamlSpMetadata
values``, and do the same for the other functions referenced (the ones at the
same positions as the declarations currently named with a `test` prefix). Update
any direct references to these function names (if used elsewhere) and ensure the
tests still compile and run.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
build-settings-logic/src/main/kotlin/ktorsettings.dependency-resolution-management.settings.gradle.ktsgradle/libs.versions.tomlktor-server/ktor-server-plugins/ktor-server-auth-saml/api/ktor-server-auth-saml.apiktor-server/ktor-server-plugins/ktor-server-auth-saml/build.gradle.ktsktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/LibSaml.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlConfig.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlConstants.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlPrincipal.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlReplayCache.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlSecurity.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/InMemorySamlReplayCacheTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlConfigTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlInitializerTest.ktsettings.gradle.kts
...ver/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlConfig.kt
Show resolved
Hide resolved
...r/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt
Outdated
Show resolved
Hide resolved
.../ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlPrincipal.kt
Outdated
Show resolved
Hide resolved
.../ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlPrincipal.kt
Outdated
Show resolved
Hide resolved
...tor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlReplayCache.kt
Show resolved
Hide resolved
...r/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlSecurity.kt
Show resolved
Hide resolved
...ugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/InMemorySamlReplayCacheTest.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlSecurity.kt (1)
74-78:⚠️ Potential issue | 🟠 MajorAvoid catching
ThrowableinloadCredential, and add context to the wrapped error.Line 76 should catch
Exception(notThrowable) so fatal JVM errors are not swallowed. Also includekeyAlias/keystorePathin the wrapper message for diagnosability.🔧 Proposed fix
} catch (e: KeyStoreException) { throw e - } catch (e: Throwable) { - throw KeyStoreException("Failed to load credential", e) + } catch (e: Exception) { + throw KeyStoreException( + "Failed to load credential for alias '$keyAlias' from keystore '$keystorePath' (type='$keystoreType')", + e + ) }#!/bin/bash rg -n --type=kt 'catch\s*\(\s*\w+\s*:\s*Throwable\s*\)' \ ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/samlAs per coding guidelines, "Make error messages actionable by including the problematic value and context."
🤖 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/SamlSecurity.kt` around lines 74 - 78, The catch block in loadCredential should not catch Throwable; change the second catch to catch Exception instead of Throwable to avoid swallowing fatal JVM errors, and when wrapping the exception into a KeyStoreException include contextual details (e.g., keyAlias and keystorePath) in the error message so the thrown KeyStoreException("Failed to load credential", e) becomes something like KeyStoreException("Failed to load credential for keyAlias='...' keystorePath='...'", e); update the catch clause near the KeyStoreException throw in the loadCredential function accordingly.
🤖 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/SamlMetadata.kt`:
- Around line 323-330: Add missing KDoc tags to the public function
parseSamlIdpMetadata: keep the existing `@param` entries and add an `@return` that
describes the returned IdPMetadata, and add `@throws` tags describing the notable
failure modes callers must handle (e.g., XML parse errors, invalid/malformed
metadata, and certificate validation failures when validateCertificateExpiration
is true). Reference the function name parseSamlIdpMetadata and include the
behavior for expired certificates (exception) and near-expiry warnings so
callers know when exceptions are raised versus logged.
---
Duplicate comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlSecurity.kt`:
- Around line 74-78: The catch block in loadCredential should not catch
Throwable; change the second catch to catch Exception instead of Throwable to
avoid swallowing fatal JVM errors, and when wrapping the exception into a
KeyStoreException include contextual details (e.g., keyAlias and keystorePath)
in the error message so the thrown KeyStoreException("Failed to load
credential", e) becomes something like KeyStoreException("Failed to load
credential for keyAlias='...' keystorePath='...'", e); update the catch clause
near the KeyStoreException throw in the loadCredential function accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62d445d1-3b9b-4cf7-aa08-e070475ecfd4
📒 Files selected for processing (7)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlConfig.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlPrincipal.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlReplayCache.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlSecurity.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/InMemorySamlReplayCacheTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlConfigTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlConfigTest.kt
- ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlReplayCache.kt
- ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlPrincipal.kt
Comment on lines
+323
to
+330
| * Parses SAML IdP metadata from an XML string. | ||
| * | ||
| * @param xml The metadata XML content | ||
| * @param validateCertificateExpiration Whether to validate the certificate expiration (default: true). | ||
| * When enabled, expired certificates will cause an exception and certificates expiring | ||
| * within 30 days will trigger a warning log. | ||
| */ | ||
| public fun parseSamlIdpMetadata(xml: String, validateCertificateExpiration: Boolean = true): IdPMetadata { |
Contributor
There was a problem hiding this comment.
Complete the public KDoc contract for parseSamlIdpMetadata.
This public API currently documents parameters only; please add @return and notable @throws so callers can handle parse/validation failures intentionally.
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/SamlMetadata.kt`
around lines 323 - 330, Add missing KDoc tags to the public function
parseSamlIdpMetadata: keep the existing `@param` entries and add an `@return` that
describes the returned IdPMetadata, and add `@throws` tags describing the notable
failure modes callers must handle (e.g., XML parse errors, invalid/malformed
metadata, and certificate validation failures when validateCertificateExpiration
is true). Reference the function name parseSamlIdpMetadata and include the
behavior for expired certificates (exception) and near-expiry warnings so
callers know when exceptions are raised versus logged.
jk1
reviewed
Mar 9, 2026
| * @property ssoUrl The Single Sign-On service URL (default binding - HTTP-Redirect or HTTP-POST) | ||
| * @property sloUrl The Single Logout service URL (default binding - HTTP-Redirect or HTTP-POST), null if not available | ||
| */ | ||
| public class IdPMetadata internal constructor( |
Collaborator
There was a problem hiding this comment.
It seems that there's no way to supply IdpMetadata other than from an XML discovery document. While fairly common, a metadata endpoint is not always available or reachable. A backup option for constructing your own IdPMetadata would be handy.
jk1
requested changes
Mar 9, 2026
zibet27
force-pushed
the
zibet27/server-auth-saml-core
branch
from
e6cdbc7 to
c69399e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt (1)
396-409:⚠️ Potential issue | 🟡 MinorComplete the public KDoc contract for
parseSamlIdpMetadata.The KDoc is missing
@returnand@throwsdocumentation. Callers need to know what exceptions to handle for parse failures, invalid metadata structure, and certificate validation failures.📝 Suggested KDoc addition
/** * Parses SAML IdP metadata from an XML string. * * `@param` xml The metadata XML content * `@param` validateCertificateExpiration Whether to validate the certificate expiration (default: true). * When enabled, expired certificates will cause an exception and certificates expiring * within 30 days will trigger a warning log. + * `@return` The parsed [IdPMetadata] containing IdP configuration + * `@throws` IllegalStateException if the metadata is missing required elements (entityID, IDPSSODescriptor, SSO endpoint) + * `@throws` java.security.cert.CertificateExpiredException if [validateCertificateExpiration] is true and a certificate is expired + * `@throws` org.xml.sax.SAXException if the XML is malformed */🤖 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/SamlMetadata.kt` around lines 396 - 409, Update the KDoc for parseSamlIdpMetadata to include an `@return` explaining it returns an IdPMetadata parsed from the provided XML, and add `@throws` tags documenting the possible failure modes: XML parsing/IO errors (e.g., IOException / SAXException) when the input cannot be parsed, invalid metadata structure errors (e.g., IllegalArgumentException) when required elements are missing or unmarshalling fails, and certificate validation failures when validateCertificateExpiration is true (e.g., CertificateException or the library's certificate validation exception) — mention that certificate expiration within 30 days will only log a warning while expired certificates will throw when validation is enabled.
🧹 Nitpick comments (1)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt (1)
277-283: Add@KtorDslannotation for DSL receiver consistency.The
IdPMetadataclass is used as a DSL receiver in theIdPMetadata()factory function (line 390), but it's missing the@KtorDslannotation thatSamlSpMetadataandSamlContactPersonhave.✨ Suggested fix
/** * Represents parsed Identity Provider (IdP) metadata. * * This class extracts and holds the essential information from SAML metadata * needed for the SP to interact with the IdP. */ +@KtorDsl public class IdPMetadata internal constructor() {As per coding guidelines: "All types used as receivers in DSL should be annotated with
@KtorDsl".🤖 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/SamlMetadata.kt` around lines 277 - 283, The IdPMetadata class is used as a DSL receiver but lacks the `@KtorDsl` annotation; add the `@KtorDsl` annotation to the IdPMetadata class declaration (public class IdPMetadata ...) and ensure the KtorDsl symbol is imported so the annotation compiles; this keeps it consistent with SamlSpMetadata and SamlContactPerson and satisfies the guideline that all DSL receiver types be annotated.
🤖 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/SamlMetadata.kt`:
- Around line 444-454: extractIdPMetadata builds and returns an IdPMetadata
instance without calling its validate() method, allowing empty
signingCredentials to slip through; update extractIdPMetadata to call validate()
on the constructed IdPMetadata before returning (same behavior as the
IdPMetadata() factory), so locate the extractIdPMetadata function and add a call
to validate() on the IdPMetadata instance (or return IdPMetadata().apply { ...
}.also { it.validate() }) to enforce required fields (e.g., signingCredentials)
and fail fast.
---
Duplicate comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt`:
- Around line 396-409: Update the KDoc for parseSamlIdpMetadata to include an
`@return` explaining it returns an IdPMetadata parsed from the provided XML, and
add `@throws` tags documenting the possible failure modes: XML parsing/IO errors
(e.g., IOException / SAXException) when the input cannot be parsed, invalid
metadata structure errors (e.g., IllegalArgumentException) when required
elements are missing or unmarshalling fails, and certificate validation failures
when validateCertificateExpiration is true (e.g., CertificateException or the
library's certificate validation exception) — mention that certificate
expiration within 30 days will only log a warning while expired certificates
will throw when validation is enabled.
---
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt`:
- Around line 277-283: The IdPMetadata class is used as a DSL receiver but lacks
the `@KtorDsl` annotation; add the `@KtorDsl` annotation to the IdPMetadata class
declaration (public class IdPMetadata ...) and ensure the KtorDsl symbol is
imported so the annotation compiles; this keeps it consistent with
SamlSpMetadata and SamlContactPerson and satisfies the guideline that all DSL
receiver types be annotated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b7e4192-3d83-46da-a450-fbbce6ea841a
📒 Files selected for processing (1)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt
...r/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt
Outdated
Show resolved
Hide resolved
zibet27
force-pushed
the
zibet27/server-auth-saml-core
branch
from
c69399e to
13a32a2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt (1)
396-404:⚠️ Potential issue | 🟡 MinorFinish the public KDoc contract for
parseSamlIdpMetadata.This still documents parameters only. Please add
@returnand notable@throwsfor XML parsing failures, malformed metadata, and expired/not-yet-valid certificates so callers can handle failures intentionally.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/SamlMetadata.kt` around lines 396 - 404, Update the KDoc for the public function parseSamlIdpMetadata(xml: String, validateCertificateExpiration: Boolean = true): IdPMetadata to include an `@return` describing the returned IdPMetadata structure and to document notable `@throws` cases: add entries for XML parsing failures (e.g., when the input is not well-formed), malformed/invalid SAML metadata (missing required elements or unexpected structure), and certificate validity errors when validateCertificateExpiration is true (expired certificates and certificates not yet valid), so callers know which exceptions to catch.
🧹 Nitpick comments (2)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt (2)
277-283: MarkIdPMetadataas a DSL receiver.
IdPMetadatais the receiver inIdPMetadata { ... }, but unlikeSamlSpMetadataandSamlContactPersonit isn't annotated with@KtorDsl. That leaves nested builders without the usual receiver-scope protection.As per coding guidelines, "All types used as receivers in DSL should be annotated with
@KtorDsl."🤖 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/SamlMetadata.kt` around lines 277 - 283, IdPMetadata is missing the `@KtorDsl` annotation used for DSL receivers; add the annotation to the IdPMetadata declaration so it reads similarly to SamlSpMetadata and SamlContactPerson, i.e., annotate the class declaration of IdPMetadata with `@KtorDsl` (ensure the correct import for Ktor’s `@KtorDsl` is present) to provide proper DSL receiver scoping and match project guidelines.
480-483: Please verify wrapped certificate text is safe withBase64.decode.Kotlin's default Base64 decoder throws on characters outside the Base64 alphabet, while XML
base64Binarypermits whitespace in the lexical form. IfcertValueincludes line breaks or indentation, valid<X509Certificate>blocks can fail here; stripping XML whitespace before decoding would make this path robust without relaxing other validation. (kotlinlang.org)🤖 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/SamlMetadata.kt` around lines 480 - 483, The Base64.decode call in SamlMetadata.kt can fail if the XML-wrapped certValue contains whitespace/newlines; before decoding the certValue (used by Base64.decode, then ByteArrayInputStream and certificateFactory.generateCertificate), normalize it by stripping XML whitespace (e.g., remove all characters where Char.isWhitespace() or apply a "\\s+" regex replacement) while preserving the existing null-check, then pass the cleaned string into Base64.decode so valid base64Binary with line breaks/indentation decodes robustly.
🤖 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/SamlMetadata.kt`:
- Around line 334-339: The validate() function currently only checks entityId
and ssoUrl for null; update it to reject null or blank values by replacing the
requireNotNull checks with checks that ensure entityId and ssoUrl are not null
or blank (e.g., use require(!entityId.isNullOrBlank()) and
require(!ssoUrl.isNullOrBlank()) ) and keep the existing signingCredentials
non-empty check; update the error messages to say "Entity ID cannot be null or
blank" and "SSO URL cannot be null or blank" so SamlMetadata.validate() fails
fast for empty strings.
---
Duplicate comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt`:
- Around line 396-404: Update the KDoc for the public function
parseSamlIdpMetadata(xml: String, validateCertificateExpiration: Boolean =
true): IdPMetadata to include an `@return` describing the returned IdPMetadata
structure and to document notable `@throws` cases: add entries for XML parsing
failures (e.g., when the input is not well-formed), malformed/invalid SAML
metadata (missing required elements or unexpected structure), and certificate
validity errors when validateCertificateExpiration is true (expired certificates
and certificates not yet valid), so callers know which exceptions to catch.
---
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt`:
- Around line 277-283: IdPMetadata is missing the `@KtorDsl` annotation used for
DSL receivers; add the annotation to the IdPMetadata declaration so it reads
similarly to SamlSpMetadata and SamlContactPerson, i.e., annotate the class
declaration of IdPMetadata with `@KtorDsl` (ensure the correct import for Ktor’s
`@KtorDsl` is present) to provide proper DSL receiver scoping and match project
guidelines.
- Around line 480-483: The Base64.decode call in SamlMetadata.kt can fail if the
XML-wrapped certValue contains whitespace/newlines; before decoding the
certValue (used by Base64.decode, then ByteArrayInputStream and
certificateFactory.generateCertificate), normalize it by stripping XML
whitespace (e.g., remove all characters where Char.isWhitespace() or apply a
"\\s+" regex replacement) while preserving the existing null-check, then pass
the cleaned string into Base64.decode so valid base64Binary with line
breaks/indentation decodes robustly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ae861c9-9fe8-4589-8781-dcee4f3747fe
📒 Files selected for processing (1)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt
...r/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt (2)
334-339:⚠️ Potential issue | 🟠 MajorReject blank
entityIdandssoUrlinvalidate().
requireNotNullstill allows""/ blank values here, soIdPMetadata { entityId = ""; ssoUrl = ""; ... }passes validation and only fails later with unusable metadata. Please tighten both checks to reject blank input and make the messages identify the offending property. This will also need theIdPMetadata(...)KDoc throws contract updated accordingly.Proposed fix
internal fun validate() { - requireNotNull(entityId) { "Entity ID cannot be null" } - requireNotNull(ssoUrl) { "SSO URL cannot be null" } + require(!entityId.isNullOrBlank()) { "IdPMetadata.entityId must not be blank" } + require(!ssoUrl.isNullOrBlank()) { "IdPMetadata.ssoUrl must not be blank" } require(signingCredentials.isNotEmpty()) { "No signing certificates found in IdP metadata. Signature verification will fail." } }As per coding guidelines, "Use
require(...)for argument validation,check(...)for state validation, anderror(...)for unreachable states".🤖 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/SamlMetadata.kt` around lines 334 - 339, The validate() method currently uses requireNotNull for entityId and ssoUrl which permits blank strings; update validate() to use require(!entityId.isNullOrBlank()) and require(!ssoUrl.isNullOrBlank()) with clear messages like "entityId cannot be null or blank" and "ssoUrl cannot be null or blank", keep the signingCredentials check as-is, and update the IdPMetadata(...) KDoc throws clause to reflect that both entityId and ssoUrl will throw on null or blank inputs.
396-404:⚠️ Potential issue | 🟡 MinorComplete the public KDoc for
parseSamlIdpMetadata.This API still documents only its parameters. Please add
@returnand notable@throwsentries for malformed XML/metadata and certificate-validation failures whenvalidateCertificateExpirationis enabled.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/SamlMetadata.kt` around lines 396 - 404, The KDoc for parseSamlIdpMetadata is incomplete; add `@return` describing that it returns an IdPMetadata representing parsed IdP metadata and add `@throws` entries documenting that it throws an exception for malformed XML or invalid/missing SAML metadata and that it can throw a certificate-validation exception when validateCertificateExpiration is true (including behavior for expired certificates vs. certificates expiring within 30 days); update the KDoc above the parseSamlIdpMetadata function to include these `@return` and `@throws` tags and concise descriptions referencing parseSamlIdpMetadata and the validateCertificateExpiration parameter.
🧹 Nitpick comments (2)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/api/ktor-server-auth-saml.api (2)
1-20: The public JVM ABI is leaking mangled inline-class accessors.Names like
getSHA256-x-8ZtNE,getNameIdFormat-W8VwlJw, andsetRequestedAuthnContext-Br-yMbYare now part of the published surface. That is awkward for Java consumers and expensive to clean up later, so if JVM interop matters I’d add Java-friendly bridges or avoid exposing these inline-value types directly in the public API.Based on learnings: All public/protected API changes must be intentional and well-documented.
Also applies to: 51-71, 79-100, 115-153
🤖 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/api/ktor-server-auth-saml.api` around lines 1 - 20, The public API is leaking mangled inline-class accessors (e.g., DigestAlgorithm$Companion.getSHA256-x-8ZtNE and similar getNameIdFormat-*/setRequestedAuthnContext-* methods); fix this by adding Java-friendly bridge methods with stable names (e.g., getSHA256(), getSHA384(), getSHA512(), getNameIdFormat(), setRequestedAuthnContext(...)) and either mark the mangled accessor declarations as `@JvmSynthetic` or make the inline-value-returning members internal/private so they are not part of the public ABI; update the classes referenced (DigestAlgorithm, DigestAlgorithm$Companion and the other companion/inline-returning members called out) to provide these stable `@JvmName/`@JvmOverloads-friendly entry points for Java callers and hide the compiler-generated mangled accessors.
171-185: Consider whether wrapping OpenSAML types is necessary for this pre-release module.
SamlCredential,SamlPrincipal,SamlSpMetadata, andSamlCryptoexposeorg.opensaml.*types in their public signatures. This couples the module's contract to OpenSAML and makes future library changes or replacements breaking for consumers.However, all exposed types are explicitly documented with examples showing intended usage. SAML protocol operations inherently require these types, and wrapping them would add abstraction without significant benefit. Since this module is pre-release, you may retain the current design if the OpenSAML coupling is intentional and acceptable for the target use case. If broader flexibility is desired, consider introducing Ktor-owned wrapper types in a future release.
🤖 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/api/ktor-server-auth-saml.api` around lines 171 - 185, Public APIs SamlCredential, SamlPrincipal, SamlSpMetadata, and SamlCrypto currently expose org.opensaml types and thus couple the module to OpenSAML; either (A) explicitly document this design decision and intended stability in the module README and API KDoc (mentioning classes SamlCredential, SamlPrincipal, SamlSpMetadata, SamlCrypto and their methods like getAssertion/getResponse/loadCredential) so consumers know the coupling is intentional for this pre-release, or (B) introduce Ktor-owned wrapper types (e.g., SamlCredentialWrapper, SamlPrincipalWrapper, SamlSpMetadataWrapper and adapt SamlCrypto.loadCredential to return a BasicX509Credential wrapper) and update public signatures to return/accept those wrappers while keeping internal conversions to org.opensaml.* to avoid leaking OpenSAML in the public contract; pick one approach and apply it consistently across the referenced classes.
🤖 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/api/ktor-server-auth-saml.api`:
- Around line 206-209: The SamlReplayCache API currently exposes a racy
check-then-write pair (isReplayed and recordAssertion) alongside
tryRecordAssertion; change the contract so replay enforcement is atomic by
making tryRecordAssertion the sole authoritative enforcement method: update the
SamlReplayCache interface documentation and implementation notes to state that
isReplayed and recordAssertion are non-authoritative helpers only (must not be
used for enforcement), remove or deprecate any public usages that rely on
isReplayed+recordAssertion for enforcement, and update callers to use
tryRecordAssertion for replay checks; reference the SamlReplayCache interface
and its methods isReplayed, recordAssertion, and tryRecordAssertion when making
these changes.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt`:
- Around line 277-283: IdPMetadata is missing the `@KtorDsl` annotation which is
required for DSL receiver types; add the annotation above the class declaration
for IdPMetadata (the class with internal constructor IdPMetadata()) so it reads
as an `@KtorDsl-annotated` DSL receiver, matching other DSL types in this file and
ensuring proper nested scope resolution.
---
Duplicate comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt`:
- Around line 334-339: The validate() method currently uses requireNotNull for
entityId and ssoUrl which permits blank strings; update validate() to use
require(!entityId.isNullOrBlank()) and require(!ssoUrl.isNullOrBlank()) with
clear messages like "entityId cannot be null or blank" and "ssoUrl cannot be
null or blank", keep the signingCredentials check as-is, and update the
IdPMetadata(...) KDoc throws clause to reflect that both entityId and ssoUrl
will throw on null or blank inputs.
- Around line 396-404: The KDoc for parseSamlIdpMetadata is incomplete; add
`@return` describing that it returns an IdPMetadata representing parsed IdP
metadata and add `@throws` entries documenting that it throws an exception for
malformed XML or invalid/missing SAML metadata and that it can throw a
certificate-validation exception when validateCertificateExpiration is true
(including behavior for expired certificates vs. certificates expiring within 30
days); update the KDoc above the parseSamlIdpMetadata function to include these
`@return` and `@throws` tags and concise descriptions referencing
parseSamlIdpMetadata and the validateCertificateExpiration parameter.
---
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/api/ktor-server-auth-saml.api`:
- Around line 1-20: The public API is leaking mangled inline-class accessors
(e.g., DigestAlgorithm$Companion.getSHA256-x-8ZtNE and similar
getNameIdFormat-*/setRequestedAuthnContext-* methods); fix this by adding
Java-friendly bridge methods with stable names (e.g., getSHA256(), getSHA384(),
getSHA512(), getNameIdFormat(), setRequestedAuthnContext(...)) and either mark
the mangled accessor declarations as `@JvmSynthetic` or make the
inline-value-returning members internal/private so they are not part of the
public ABI; update the classes referenced (DigestAlgorithm,
DigestAlgorithm$Companion and the other companion/inline-returning members
called out) to provide these stable `@JvmName/`@JvmOverloads-friendly entry points
for Java callers and hide the compiler-generated mangled accessors.
- Around line 171-185: Public APIs SamlCredential, SamlPrincipal,
SamlSpMetadata, and SamlCrypto currently expose org.opensaml types and thus
couple the module to OpenSAML; either (A) explicitly document this design
decision and intended stability in the module README and API KDoc (mentioning
classes SamlCredential, SamlPrincipal, SamlSpMetadata, SamlCrypto and their
methods like getAssertion/getResponse/loadCredential) so consumers know the
coupling is intentional for this pre-release, or (B) introduce Ktor-owned
wrapper types (e.g., SamlCredentialWrapper, SamlPrincipalWrapper,
SamlSpMetadataWrapper and adapt SamlCrypto.loadCredential to return a
BasicX509Credential wrapper) and update public signatures to return/accept those
wrappers while keeping internal conversions to org.opensaml.* to avoid leaking
OpenSAML in the public contract; pick one approach and apply it consistently
across the referenced classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8c79591-ab1b-4824-841e-332ebb924032
📒 Files selected for processing (2)
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/SamlMetadata.kt
ktor-server/ktor-server-plugins/ktor-server-auth-saml/api/ktor-server-auth-saml.api
Show resolved
Hide resolved
...r/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadata.kt
Show resolved
Hide resolved
zibet27
force-pushed
the
zibet27/server-auth-saml-core
branch
from
dd58b5e to
e5fd9e5
Compare
Member
Code reviewFound 1 issue:
🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Collaborator
Author
It'll be used in the following PRs to serialise |
jk1
approved these changes
Mar 14, 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
This PR adds a new ktor-server-auth-saml module that provides SAML 2.0 SSO support for Ktor server applications. This is the foundational layer - it introduces configs, constants, and metadata parsing without the actual authentication flow yet.
What's included:
Configuration
Type-safe Constants
NameIdFormat - email, persistent, transient, etc.
SignatureAlgorithm - RSA/ECDSA with SHA-256/384/512
DigestAlgorithm - SHA-256/384/512
SamlBinding - HTTP-Redirect and HTTP-POST
SamlAuthnContext - Password, MFA, Kerberos, X509
Metadata Handling
SamlSpMetadata- SP configuration with signing/encryption credentials, contacts, organization infoIdPMetadata- parsed IdP metadata with SSO/SLO endpoints and signing certsparseSamlIdpMetadata()- parses IdP XML with cert expiration validationSecurity
SamlReplayCacheinterface +InMemorySamlReplayCache- replay attack protectionAlgorithm allowlisting to prevent downgrade attacks
Certificate expiration warnings
Principal Model
SamlCredential - unverified credential during auth flow
SamlPrincipal - verified principal with NameID, sessionIndex, attributes
What's next:
The actual authentication provider, AuthnRequest generation, response validation, and SLO handling will come in follow-up commits.
Summary by CodeRabbit
New Features
Tests
Chores