Server SAML Auth. Metadata generation endpoint#5418
Open
zibet27 wants to merge 3 commits into
Open
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:
WalkthroughThis change adds SAML Service Provider (SP) metadata generation capability to the Ktor SAML authentication plugin. It introduces a new public extension function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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: 2
🧹 Nitpick comments (1)
ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadataGenerator.kt (1)
25-27: Validate metadata fields before initializing OpenSAML.
LibSaml.ensureInitialized()currently runs before argument checks. Reordering avoids unnecessary global init work and ensures invalid input fails fast with the documentedIllegalArgumentException.♻️ Proposed change
public fun SamlSpMetadata.toXml(): String { - LibSaml.ensureInitialized() require(!spEntityId.isNullOrBlank()) { "spEntityId is required for SP metadata" } require(acsUrl.isNotBlank()) { "acsUrl is required for SP metadata" } + LibSaml.ensureInitialized() val entityDescriptor = buildEntityDescriptor(this) return entityDescriptor.marshalToString() }🤖 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/SamlMetadataGenerator.kt` around lines 25 - 27, Move the input validation for spEntityId and acsUrl to run before calling LibSaml.ensureInitialized(): validate that spEntityId is not null/blank and acsUrl is not blank (the existing require checks referencing spEntityId and acsUrl) and only then call LibSaml.ensureInitialized(); this avoids initializing OpenSAML when the arguments are invalid and preserves the documented IllegalArgumentException behavior.
🤖 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/test/io/ktor/server/auth/saml/SamlMetadataGeneratorTest.kt`:
- Around line 125-136: The tests currently pass a failure message into
assertFailsWith which doesn't check the thrown exception text; change both
assertions to capture the thrown exception and assert on its message: replace
assertFailsWith<IllegalArgumentException>("spEntityId is required") {
SamlSpMetadata { acsUrl = "..." }.toXml() } with val ex1 =
assertFailsWith<IllegalArgumentException> { SamlSpMetadata { acsUrl = "..."
}.toXml() } followed by assertEquals("spEntityId is required for SP metadata",
ex1.message), and do the same for the acsUrl case using assertFailsWith to get
ex2 and assertEquals("acsUrl is required for SP metadata", ex2.message); target
the SamlSpMetadata builder and toXml invocation when making these changes.
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/TestUtil.kt`:
- Around line 7-8: Replace the two explicit imports for generateCertificate and
saveToFile with the repository's configured star-import for the io.ktor package:
change the explicit imports from
io.ktor.network.tls.certificates.generateCertificate and
io.ktor.network.tls.certificates.saveToFile to a single wildcard import
io.ktor.network.tls.certificates.* so the TestUtil.kt complies with the
io.ktor.* star-import rule.
---
Nitpick comments:
In
`@ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/io/ktor/server/auth/saml/SamlMetadataGenerator.kt`:
- Around line 25-27: Move the input validation for spEntityId and acsUrl to run
before calling LibSaml.ensureInitialized(): validate that spEntityId is not
null/blank and acsUrl is not blank (the existing require checks referencing
spEntityId and acsUrl) and only then call LibSaml.ensureInitialized(); this
avoids initializing OpenSAML when the arguments are invalid and preserves the
documented IllegalArgumentException behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
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/SamlMetadataGenerator.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlMetadataGeneratorTest.ktktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/TestUtil.kt
Comment on lines
+125
to
+136
| assertFailsWith<IllegalArgumentException>("spEntityId is required") { | ||
| SamlSpMetadata { | ||
| acsUrl = "https://sp.example.com/saml/acs" | ||
| }.toXml() | ||
| } | ||
|
|
||
| assertFailsWith<IllegalArgumentException>("acsUrl is required") { | ||
| SamlSpMetadata { | ||
| spEntityId = "https://sp.example.com" | ||
| acsUrl = "" | ||
| }.toXml() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
kotlin.test assertFailsWith message parameter exception text verification
💡 Result:
In kotlin.test.assertFailsWith, the message parameter is not the expected exception’s text. It’s the assertion failure message prefix used only if the assertion fails (e.g., wrong type or no exception thrown). (kotlinlang.org)
To verify the thrown exception’s message/text, capture the returned exception and assert on exception.message:
import kotlin.test.*
`@Test`
fun throws_with_expected_message() {
val ex = assertFailsWith<IllegalArgumentException> {
error("boom") // or throw IllegalArgumentException("boom")
}
assertEquals("boom", ex.message)
}assertFailsWith returns the caught exception specifically so you can inspect its properties (like message, cause, custom fields). (kotlinlang.org)
Citations:
- 1: https://kotlinlang.org/api/core/kotlin-test/kotlin.test/assert-fails-with.html?utm_source=openai
- 2: https://kotlinlang.org/api/core/kotlin-test/kotlin.test/assert-fails-with.html?utm_source=openai
🏁 Script executed:
# Check if there's a kotlin.test dependency or gradle config that reveals version info
fd -t f "build.gradle" "gradle.properties" "settings.gradle" | head -5 | xargs -I {} grep -l "kotlin" {} 2>/dev/null | head -3Repository: ktorio/ktor
Length of output: 221
🏁 Script executed:
# Find the test file
fd -t f "SamlMetadataGeneratorTest.kt"Repository: ktorio/ktor
Length of output: 171
🏁 Script executed:
# Check the implementation to see what exceptions are thrown
fd -t f "SamlSpMetadata" -e kt | head -10Repository: ktorio/ktor
Length of output: 37
🏁 Script executed:
# Read the test file lines 125-136
cat -n ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/test/io/ktor/server/auth/saml/SamlMetadataGeneratorTest.kt | sed -n '125,136p'Repository: ktorio/ktor
Length of output: 554
🏁 Script executed:
# Find the SamlSpMetadata implementation
fd -t f "\.kt$" ktor-server/ktor-server-plugins/ktor-server-auth-saml | grep -v test | head -10Repository: ktorio/ktor
Length of output: 1110
🏁 Script executed:
# Search for SamlSpMetadata class definition and where exceptions are thrown
rg -A 10 "class SamlSpMetadata" ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/Repository: ktorio/ktor
Length of output: 1536
🏁 Script executed:
# Search for the error messages in the source code
rg "spEntityId is required|acsUrl is required" ktor-server/ktor-server-plugins/ktor-server-auth-saml/jvm/src/Repository: ktorio/ktor
Length of output: 438
Test assertions don't verify exception messages.
The string argument in assertFailsWith<T>(message, block) is only an assertion failure message—it doesn't validate the thrown exception's text. The actual error messages thrown by the implementation differ from the test strings (e.g., implementation throws "spEntityId is required for SP metadata" but test uses "spEntityId is required").
Capture the exception and assert on its message to properly validate error text:
✅ Suggested fix
- assertFailsWith<IllegalArgumentException>("spEntityId is required") {
+ val missingEntityId = assertFailsWith<IllegalArgumentException> {
SamlSpMetadata {
acsUrl = "https://sp.example.com/saml/acs"
}.toXml()
}
+ assertTrue(missingEntityId.message?.contains("spEntityId is required") == true)
- assertFailsWith<IllegalArgumentException>("acsUrl is required") {
+ val missingAcs = assertFailsWith<IllegalArgumentException> {
SamlSpMetadata {
spEntityId = "https://sp.example.com"
acsUrl = ""
}.toXml()
}
+ assertTrue(missingAcs.message?.contains("acsUrl is required") == true)📝 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.
Suggested change
| assertFailsWith<IllegalArgumentException>("spEntityId is required") { | |
| SamlSpMetadata { | |
| acsUrl = "https://sp.example.com/saml/acs" | |
| }.toXml() | |
| } | |
| assertFailsWith<IllegalArgumentException>("acsUrl is required") { | |
| SamlSpMetadata { | |
| spEntityId = "https://sp.example.com" | |
| acsUrl = "" | |
| }.toXml() | |
| } | |
| val missingEntityId = assertFailsWith<IllegalArgumentException> { | |
| SamlSpMetadata { | |
| acsUrl = "https://sp.example.com/saml/acs" | |
| }.toXml() | |
| } | |
| assertTrue(missingEntityId.message?.contains("spEntityId is required") == true) | |
| val missingAcs = assertFailsWith<IllegalArgumentException> { | |
| SamlSpMetadata { | |
| spEntityId = "https://sp.example.com" | |
| acsUrl = "" | |
| }.toXml() | |
| } | |
| assertTrue(missingAcs.message?.contains("acsUrl is required") == true) |
🤖 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/SamlMetadataGeneratorTest.kt`
around lines 125 - 136, The tests currently pass a failure message into
assertFailsWith which doesn't check the thrown exception text; change both
assertions to capture the thrown exception and assert on its message: replace
assertFailsWith<IllegalArgumentException>("spEntityId is required") {
SamlSpMetadata { acsUrl = "..." }.toXml() } with val ex1 =
assertFailsWith<IllegalArgumentException> { SamlSpMetadata { acsUrl = "..."
}.toXml() } followed by assertEquals("spEntityId is required for SP metadata",
ex1.message), and do the same for the acsUrl case using assertFailsWith to get
ex2 and assertEquals("acsUrl is required for SP metadata", ex2.message); target
the SamlSpMetadata builder and toXml invocation when making these changes.
…th-saml-metadata-generator
jk1
approved these changes
Mar 14, 2026
bjhham
approved these changes
Apr 21, 2026
Contributor
bjhham
left a comment
bjhham
left a comment
There was a problem hiding this comment.
Added a couple cosmetic comments, but it looks good overall.
Comment on lines
+50
to
+65
| // Add SLO endpoint if configured | ||
| val sloUrl = metadata.sloUrl | ||
| if (sloUrl.isNotBlank()) { | ||
| val sloRedirect = builderFactory.build<SingleLogoutService>(SingleLogoutService.DEFAULT_ELEMENT_NAME) { | ||
| binding = SAMLConstants.SAML2_REDIRECT_BINDING_URI | ||
| location = sloUrl | ||
| } | ||
| singleLogoutServices.add(sloRedirect) | ||
|
|
||
| // Also add POST binding for SLO | ||
| val sloPost = builderFactory.build<SingleLogoutService>(SingleLogoutService.DEFAULT_ELEMENT_NAME) { | ||
| binding = SAMLConstants.SAML2_POST_BINDING_URI | ||
| location = sloUrl | ||
| } | ||
| singleLogoutServices.add(sloPost) | ||
| } |
Contributor
There was a problem hiding this comment.
Minor simplification here:
Suggested change
| // Add SLO endpoint if configured | |
| val sloUrl = metadata.sloUrl | |
| if (sloUrl.isNotBlank()) { | |
| val sloRedirect = builderFactory.build<SingleLogoutService>(SingleLogoutService.DEFAULT_ELEMENT_NAME) { | |
| binding = SAMLConstants.SAML2_REDIRECT_BINDING_URI | |
| location = sloUrl | |
| } | |
| singleLogoutServices.add(sloRedirect) | |
| // Also add POST binding for SLO | |
| val sloPost = builderFactory.build<SingleLogoutService>(SingleLogoutService.DEFAULT_ELEMENT_NAME) { | |
| binding = SAMLConstants.SAML2_POST_BINDING_URI | |
| location = sloUrl | |
| } | |
| singleLogoutServices.add(sloPost) | |
| } | |
| // Add SLO endpoint if configured | |
| if (metadata.sloUrl.isNotBlank()) { | |
| singleLogoutServices.addAll( | |
| listOf( | |
| SAMLConstants.SAML2_REDIRECT_BINDING_URI, | |
| // Also add POST binding for SLO | |
| SAMLConstants.SAML2_POST_BINDING_URI | |
| ).map { bindingUri -> | |
| builderFactory.build(SingleLogoutService.DEFAULT_ELEMENT_NAME) { | |
| binding = bindingUri | |
| location = metadata.sloUrl | |
| } | |
| }) | |
| } |
Comment on lines
+90
to
+103
| // Add Organization if configured | ||
| if (metadata.organizationUrl != null || | ||
| metadata.organizationName != null || | ||
| metadata.organizationDisplayName != null | ||
| ) { | ||
| organization = buildOrganization(metadata) | ||
| } | ||
|
|
||
| metadata.technicalContacts.forEach { contact: SamlContactPerson -> | ||
| contactPersons.add(contact.toOpenSaml(type = ContactPersonTypeEnumeration.TECHNICAL)) | ||
| } | ||
| metadata.supportContacts.forEach { contact: SamlContactPerson -> | ||
| contactPersons.add(contact.toOpenSaml(type = ContactPersonTypeEnumeration.SUPPORT)) | ||
| } |
Contributor
There was a problem hiding this comment.
Maybe we can simplify with scoping here:
Suggested change
| // Add Organization if configured | |
| if (metadata.organizationUrl != null || | |
| metadata.organizationName != null || | |
| metadata.organizationDisplayName != null | |
| ) { | |
| organization = buildOrganization(metadata) | |
| } | |
| metadata.technicalContacts.forEach { contact: SamlContactPerson -> | |
| contactPersons.add(contact.toOpenSaml(type = ContactPersonTypeEnumeration.TECHNICAL)) | |
| } | |
| metadata.supportContacts.forEach { contact: SamlContactPerson -> | |
| contactPersons.add(contact.toOpenSaml(type = ContactPersonTypeEnumeration.SUPPORT)) | |
| } | |
| with(metadata) { | |
| // Add Organization if configured | |
| if ((organizationUrl ?: organizationName ?: organizationDisplayName) != null) { | |
| organization = buildOrganization(metadata) | |
| } | |
| technicalContacts.forEach { contact: SamlContactPerson -> | |
| contactPersons.add(contact.toOpenSaml(type = ContactPersonTypeEnumeration.TECHNICAL)) | |
| } | |
| supportContacts.forEach { contact: SamlContactPerson -> | |
| contactPersons.add(contact.toOpenSaml(type = ContactPersonTypeEnumeration.SUPPORT)) | |
| } | |
| } |
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
The metadata generator (
SamlSpMetadata.toXml()) produces standard SAML 2.0 SP metadata XML that you can share with the IdP for registration.It includes: