Conversation
📝 WalkthroughWalkthroughThis pull request implements external role extraction from OIDC ID tokens for the WAL-670 feature. It introduces configuration options, extraction logic that processes role claims from ID token payloads, updated session data structures to store extracted roles, comprehensive tests, and documentation describing the new capability. Changes
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
waltid-libraries/auth/waltid-ktor-authnz/src/test/kotlin/id/walt/OidcExternalRoleExtractorTest.kt (1)
109-128: Add one edge-case test for non-objectresource_access/ client entries.Consider extending malformed-claims coverage to include cases where
resource_access(or a client entry under it) is not a JSON object, to prevent regressions around claim-shape hardening.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@waltid-libraries/auth/waltid-ktor-authnz/src/test/kotlin/id/walt/OidcExternalRoleExtractorTest.kt` around lines 109 - 128, Add a second unit test alongside `gracefully handles missing or malformed role claims` that constructs a payload where `resource_access` is a non-object (e.g., a string) and another variant where the client entry under `resource_access` (e.g., `"waltid_ktor_authnz"`) is a non-object, then call OidcExternalRoleExtractor.extract with that payload (using the same `baseConfig` with `externalRoleExtraction.enabled = true`) and assert the extractor returns non-null result with empty realmRoles and empty clientRoles; this ensures the extractor handles non-object `resource_access` and client entries without throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OidcExternalRoleExtractor.kt`:
- Around line 29-35: The code assumes JsonElement.jsonObject will always
succeed; guard against non-object top-level and nested claim values by using
safe casts and type checks: replace direct use of resolvePath(...)? .jsonObject
with a safe cast like (resolvePath(idTokenPayload,
extractionConfig.clientRolesClaimPath) as? JsonObject) and in the allClientRoles
mapping ensure each mapped value is treated as a JsonObject (e.g., cast value
as? JsonObject) before accessing ["roles"], then call .asStringSet().orEmpty()
only when the roles element is present and of the expected type; update
references to clientRolesRoot, resolvePath,
extractionConfig.clientRolesClaimPath, and allClientRoles accordingly and add a
unit test where the top-level claim (e.g., resource_access) is a string to
verify graceful degradation.
---
Nitpick comments:
In
`@waltid-libraries/auth/waltid-ktor-authnz/src/test/kotlin/id/walt/OidcExternalRoleExtractorTest.kt`:
- Around line 109-128: Add a second unit test alongside `gracefully handles
missing or malformed role claims` that constructs a payload where
`resource_access` is a non-object (e.g., a string) and another variant where the
client entry under `resource_access` (e.g., `"waltid_ktor_authnz"`) is a
non-object, then call OidcExternalRoleExtractor.extract with that payload (using
the same `baseConfig` with `externalRoleExtraction.enabled = true`) and assert
the extractor returns non-null result with empty realmRoles and empty
clientRoles; this ensures the extractor handles non-object `resource_access` and
client entries without throwing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
waltid-libraries/auth/waltid-ktor-authnz/docs/WAL-670-IMPLEMENTATION.mdwaltid-libraries/auth/waltid-ktor-authnz/docs/oidc.mdwaltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OIDC.ktwaltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OidcExternalRoleExtractor.ktwaltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/config/OidcAuthConfiguration.ktwaltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/sessiondata/OidcSessionData.ktwaltid-libraries/auth/waltid-ktor-authnz/src/test/kotlin/id/walt/OidcExternalRoleExtractorTest.kt
| val clientRolesRoot = resolvePath(idTokenPayload, extractionConfig.clientRolesClaimPath) | ||
| ?.jsonObject | ||
|
|
||
| val allClientRoles = clientRolesRoot | ||
| ?.mapValues { (_, value) -> | ||
| value.jsonObject["roles"]?.asStringSet().orEmpty() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Examining OidcExternalRoleExtractor.kt ==="
cat -n waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OidcExternalRoleExtractor.kt | head -60
echo
echo "=== Checking for jsonObject usage in the file ==="
rg -n 'jsonObject' waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OidcExternalRoleExtractor.kt -C3
echo
echo "=== Checking test file for malformed claim tests ==="
if [ -f waltid-libraries/auth/waltid-ktor-authnz/src/test/kotlin/id/walt/OidcExternalRoleExtractorTest.kt ]; then
cat -n waltid-libraries/auth/waltid-ktor-authnz/src/test/kotlin/id/walt/OidcExternalRoleExtractorTest.kt
else
echo "Test file not found, searching for test files..."
find . -name "*OidcExternalRoleExtractor*Test*" -type f 2>/dev/null | head -10
fiRepository: walt-id/waltid-identity
Length of output: 8964
🌐 Web query:
JsonElement jsonObject property kotlinx.serialization throws
💡 Result:
In kotlinx.serialization.json, jsonObject is a convenience cast:
val obj: JsonObject = element.jsonObjectIt throws IllegalArgumentException when the JsonElement is not actually a JsonObject (e.g., it’s a JsonPrimitive, JsonArray, or JsonNull). [1]
Safer patterns:
val objOrNull = element as? JsonObject
// or
val objOrNull = element.takeIf { it is JsonObject }?.jsonObjectIf you share the exact exception text + the JSON snippet (or the JsonElement you’re accessing), I can point to the specific non-object value that’s triggering it.
Sources
[1] Kotlinx Serialization API docs: JsonElement.jsonObject (throws if not a JsonObject) https://kotlinlang.org/api/kotlinx.serialization/kotlinx-serialization-json/kotlinx.serialization.json/json-object.html
Guard jsonObject access to prevent auth-flow breakage on malformed claims.
Direct jsonObject property access throws IllegalArgumentException when claim values are not JSON objects. The current test only covers malformed nested claims (roles as string when array expected), but not malformed top-level claims (where resource_access itself is a string instead of an object). This leaves the code vulnerable to crashes when processing JWTs with non-object top-level claim values.
💡 Proposed fix
- val clientRolesRoot = resolvePath(idTokenPayload, extractionConfig.clientRolesClaimPath)
- ?.jsonObject
+ val clientRolesRoot = (resolvePath(idTokenPayload, extractionConfig.clientRolesClaimPath) as? JsonObject)
val allClientRoles = clientRolesRoot
?.mapValues { (_, value) ->
- value.jsonObject["roles"]?.asStringSet().orEmpty()
+ (value as? JsonObject)?.get("roles")?.asStringSet().orEmpty()
}
.orEmpty()
.filterValues { it.isNotEmpty() }Consider also expanding the test to include a case where resource_access is a string rather than an object to ensure graceful degradation.
📝 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.
| val clientRolesRoot = resolvePath(idTokenPayload, extractionConfig.clientRolesClaimPath) | |
| ?.jsonObject | |
| val allClientRoles = clientRolesRoot | |
| ?.mapValues { (_, value) -> | |
| value.jsonObject["roles"]?.asStringSet().orEmpty() | |
| } | |
| val clientRolesRoot = (resolvePath(idTokenPayload, extractionConfig.clientRolesClaimPath) as? JsonObject) | |
| val allClientRoles = clientRolesRoot | |
| ?.mapValues { (_, value) -> | |
| (value as? JsonObject)?.get("roles")?.asStringSet().orEmpty() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@waltid-libraries/auth/waltid-ktor-authnz/src/main/kotlin/id/walt/ktorauthnz/methods/OidcExternalRoleExtractor.kt`
around lines 29 - 35, The code assumes JsonElement.jsonObject will always
succeed; guard against non-object top-level and nested claim values by using
safe casts and type checks: replace direct use of resolvePath(...)? .jsonObject
with a safe cast like (resolvePath(idTokenPayload,
extractionConfig.clientRolesClaimPath) as? JsonObject) and in the allClientRoles
mapping ensure each mapped value is treated as a JsonObject (e.g., cast value
as? JsonObject) before accessing ["roles"], then call .asStringSet().orEmpty()
only when the roles element is present and of the expected type; update
references to clientRolesRoot, resolvePath,
extractionConfig.clientRolesClaimPath, and allClientRoles accordingly and add a
unit test where the top-level claim (e.g., resource_access) is a string to
verify graceful degradation.
|
2 participants
Documentation
New Features
Tests