feat(backend): keycloak permissions authorities #1158
Conversation
WalkthroughThis PR replaces the deprecated UserInfoAuthoritiesConverter with KeycloakPermissionsAuthoritiesConverter, switches profile usage from "userinfo-authorities" to "keycloak-permissions", updates SecurityProperties (permissionsUri, permissionCacheLifetime), updates beans and wiring, adjusts tests and configs, and removes related Keycloak migration and volume mapping. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SecurityConfig as Security Config
participant Converter as KeycloakPermissionsAuthoritiesConverter
participant Cache as Caffeine Cache
participant Keycloak as Keycloak Endpoint
Note over Client,Keycloak: Permission-fetch flow (POST, caching)
Client->>SecurityConfig: request with JWT
SecurityConfig->>Converter: convert(jwt)
Converter->>Cache: lookup(jwt.subject)
alt cache hit
Cache-->>Converter: cached authorities
Converter-->>SecurityConfig: authorities
else cache miss
Converter->>Keycloak: POST /permissions (form + Bearer JWT)
alt success
Keycloak-->>Converter: List of permission maps (rsid, rsname)
Converter->>Converter: map to GrantedAuthority[]
Converter->>Cache: store authorities (TTL)
Converter-->>SecurityConfig: authorities
else error
Keycloak-->>Converter: error
Converter-->>SecurityConfig: empty authorities
end
end
SecurityConfig-->>Client: continue processing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
github-actions
bot
added
Template: Backend
simonhir
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java
Dismissed
Show dismissed
Hide dismissed
...java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
An alternative would be to merge both AuthoritiesConverter into one and allow enable/disable of the Keycloak permissions one.
...java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java
Outdated
Show resolved
Hide resolved
For Testing
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java (3)
81-81: Replaceassertkeyword with JUnit'sassertNotNull.Java's
assertkeyword can be disabled at runtime with JVM flags, which would silently skip this check. Use JUnit'sassertNotNullfor consistent test behavior.- assert authorities != null; + assertNotNull(authorities);Also add the import if not present:
import static org.junit.jupiter.api.Assertions.assertNotNull;
102-102: Same issue: ReplaceassertwithassertNotNull.- assert authorities != null; + assertNotNull(authorities);
119-119: Same issue: ReplaceassertwithassertNotNull.- assert authorities != null; + assertNotNull(authorities);
♻️ Duplicate comments (2)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java (2)
40-40: Hardcoded cache TTL should be configurable.The 60-second cache expiry is hardcoded. Consider making this configurable via
SecurityProperties.
85-85: Log injection risk with user-controlled data.Logging the raw permissions response could allow log injection if the permissions endpoint returns malicious content. Consider sanitizing or limiting the logged output.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
stack/keycloak/plugins/authority-mapper-1.1.2-SNAPSHOT.jaris excluded by!**/*.jar
📒 Files selected for processing (10)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/AuthoritiesConverterConfiguration.java(1 hunks)refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java(1 hunks)refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityConfiguration.java(3 hunks)refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityProperties.java(1 hunks)refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/UserInfoAuthoritiesConverter.java(0 hunks)refarch-backend/src/main/resources/application-local.yml(1 hunks)refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java(5 hunks)refarch-backend/src/test/resources/application-test.yml(1 hunks)stack/docker-compose.yml(0 hunks)stack/keycloak/migration/permissions/P01_init_permissions.yml(0 hunks)
💤 Files with no reviewable changes (3)
- refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/UserInfoAuthoritiesConverter.java
- stack/keycloak/migration/permissions/P01_init_permissions.yml
- stack/docker-compose.yml
🧰 Additional context used
🧬 Code graph analysis (1)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java (1)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityConfiguration.java (1)
RequiredArgsConstructor(27-88)
🔇 Additional comments (8)
refarch-backend/src/main/resources/application-local.yml (1)
57-58: Review comment is based on incorrect assumptions about the endpoint usage.After examining the implementation, the original concerns are not valid:
Endpoint URL is correct:
KeycloakPermissionsAuthoritiesConvertermakes a POST request to the configured URL with UMA 2.0 parameters (grant_type=urn:ietf:params:oauth:grant-type:uma-ticketandresponse_mode=permissions). The/protocol/openid-connect/tokenendpoint is Keycloak's standard endpoint for UMA 2.0 token requests—the correct choice for fetching permissions.Configuration is consistent:
client-idandpermissions-uriare properly defined in active profiles (local and test), while the baseapplication.ymlcorrectly omits therefarch.securitysection. The test profile intentionally uses a mock endpoint (http://localhost/permissions).Likely an incorrect or invalid review comment.
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityConfiguration.java (1)
71-81: LGTM!The authority converter selection logic correctly prioritizes
KeycloakRolesAuthoritiesConverteroverKeycloakPermissionsAuthoritiesConverter, with a graceful fallback to defaults.refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/AuthoritiesConverterConfiguration.java (1)
16-27: LGTM!The profile-based bean configuration ensures mutual exclusivity between the two converter types. The
keycloak-permissionsprofile correctly activates only the permissions-based converter while defaulting to the roles-based converter otherwise.refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java (2)
71-91: Verify cache key strategy handles permission changes correctly.Using
jwt.getSubject()as the cache key means permission changes for a user won't be reflected until the cache entry expires (60 seconds). If permissions can change during a user's session (e.g., via admin action), consider including the token'siat(issued-at) claim in the cache key, or document this as expected behavior.
92-94: Error handling approach is sound.The layered exception handling gracefully degrades to empty authorities on errors, which is a safe security default. The outer catch handles unexpected errors while
fetchPermissionshandles HTTP-specific failures.refarch-backend/src/test/resources/application-test.yml (1)
12-18: LGTM!Test configuration correctly updated to reflect the new
permissionsUriproperty and includes appropriate placeholder values for testing.refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java (2)
36-39: LGTM!The test configuration correctly combines
@SpringBootTestfor loadingSecurityPropertieswith@ExtendWith(MockitoExtension.class)for mock support. The active profile ensures test-specific properties are loaded.
43-57: No issues found—properties correctly configured.Verification of
application-test.ymlconfirms thatpermissions-uri: http://localhost/permissionsis explicitly configured, exactly matching the hardcodedPERMISSIONS_URIconstant. The converter will correctly read this value fromsecurityProperties, and the mock setup usingeq(PERMISSIONS_URI)will match as expected. The test is correctly implemented.
...java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java
Show resolved
Hide resolved
...java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java
Outdated
Show resolved
Hide resolved
...-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityConfiguration.java
Outdated
Show resolved
Hide resolved
...rch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityProperties.java
Show resolved
Hide resolved
.../de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityConfiguration.java (1)
69-79: Converter precedence is clear; consider logging which converter is active (optional).The fallback from roles-based authorities to the Keycloak permissions endpoint is straightforward and safe. For easier operational debugging, you could log which converter is actually being used:
- // authorities via keycloak roles scope - if (keycloakRolesAuthoritiesConverter.isPresent()) { - jwtAuthenticationConverter.setJwtGrantedAuthoritiesConverter( - keycloakRolesAuthoritiesConverter.get()); - } - // authorities via keycloak permissions endpoint - else if (keycloakPermissionsAuthoritiesConverter.isPresent()) { - jwtAuthenticationConverter.setJwtGrantedAuthoritiesConverter( - keycloakPermissionsAuthoritiesConverter.get()); - } else { + // authorities via keycloak roles scope + if (keycloakRolesAuthoritiesConverter.isPresent()) { + log.info("Configuring JwtAuthenticationConverter with KeycloakRolesAuthoritiesConverter."); + jwtAuthenticationConverter.setJwtGrantedAuthoritiesConverter( + keycloakRolesAuthoritiesConverter.get()); + } + // authorities via keycloak permissions endpoint + else if (keycloakPermissionsAuthoritiesConverter.isPresent()) { + log.info("Configuring JwtAuthenticationConverter with KeycloakPermissionsAuthoritiesConverter."); + jwtAuthenticationConverter.setJwtGrantedAuthoritiesConverter( + keycloakPermissionsAuthoritiesConverter.get()); + } else { log.warn("No custom authority converter available, falling back to default."); }Purely a convenience improvement; behavior is already correct.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java(1 hunks)refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityConfiguration.java(3 hunks)refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityProperties.java(2 hunks)refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze Java source files (./refarch-eai)
- GitHub Check: Analyze Java source files (./refarch-backend)
- GitHub Check: build (refarch-backend)
🔇 Additional comments (7)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityProperties.java (3)
8-8: Import forDurationis correct and scoped.The added
java.time.Durationimport cleanly supports the new configuration field without introducing unused dependencies.
30-35:permissionsUriproperty and Javadoc look consistent with the new permissions flow.The field is correctly marked
@NotBlank, and the Javadoc now matches the permissions endpoint semantics and referencesKeycloakPermissionsAuthoritiesConverterappropriately.
36-40:permissionCacheLifetimedefault and validation are appropriate.Using a
Durationwith a sane default of 60s and@NotNullprovides safe, explicit cache control for permissions while remaining easily overridable via configuration.refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java (1)
52-62: LGTM: Constructor properly initializes dependencies.The constructor correctly builds the RestTemplate and configures the Caffeine cache with the provided expiration lifetime from SecurityProperties.
refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java (1)
44-44: Hardcoded PERMISSIONS_URI constant matches test configuration.The test configuration in
application-test.ymlsetspermissions-uri: http://localhost/permissions, which is loaded intoSecurityPropertiesand returned bygetPermissionsUri(). This matches the hardcodedPERMISSIONS_URIconstant used in the RestTemplate stubs, so the mock will be invoked correctly.The test is properly configured and does not have a URI mismatch issue. While using a hardcoded constant is a minor DRY violation, the suggested refactoring is not necessary for test correctness.
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityConfiguration.java (2)
24-25: Javadoc update correctly reflects the permissions-based converter.The Javadoc now accurately mentions
KeycloakPermissionsAuthoritiesConverterand itskeycloak-permissionsprofile without labeling it deprecated, which aligns with the new permissions-based approach. No further changes needed here.
35-36: Optional injection for the permissions converter is consistent and clear.Using
Optional<KeycloakPermissionsAuthoritiesConverter>mirrors the existing pattern for the roles converter and keeps the configuration flexible. Just be aware that, given the currentif/else iflogic, if both beans are ever present, the roles converter will always be chosen; profiles/conditions should ensure that matches your intent.
...java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java
Show resolved
Hide resolved
...java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java
Show resolved
Hide resolved
...java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java
Show resolved
Hide resolved
.../de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java
Show resolved
Hide resolved
.../de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java (1)
82-82: Replace Javaassertstatements with JUnitassertNotNull()for test reliability.The code uses Java
assertstatements at lines 82, 103, and 120 to check ifauthorities != null. These assertions rely on JVM assertions, which are disabled by default unless tests run with-ea. Since the file already imports JUnit assertions likeassertEqualsandassertTrue, replace all threeassert authorities != null;statements withassertNotNull(authorities);to ensure the checks always execute and maintain consistency with the rest of the test suite.
♻️ Duplicate comments (1)
refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java (1)
44-45: Align test URI stubbing withSecurityPropertiesto avoid duplication.The converter calls
fetchPermissions(securityProperties.getPermissionsUri(), ...), but the mocks stubrestTemplate.exchange(eq(PERMISSIONS_URI), ...). If the configured URI ever diverges from the hard‑coded constant, the stubbing will no longer match. Prefer stubbing witheq(securityProperties.getPermissionsUri())(and then you can drop or repurpose thePERMISSIONS_URIconstant).- when(restTemplate.exchange(eq(PERMISSIONS_URI), eq(HttpMethod.POST), any(HttpEntity.class), any(ParameterizedTypeReference.class))) + when(restTemplate.exchange(eq(securityProperties.getPermissionsUri()), eq(HttpMethod.POST), any(HttpEntity.class), any(ParameterizedTypeReference.class))) .thenReturn(new ResponseEntity<>(response, HttpStatus.OK)); ... - when(restTemplate.exchange(eq(PERMISSIONS_URI), eq(HttpMethod.POST), any(HttpEntity.class), any(ParameterizedTypeReference.class))) + when(restTemplate.exchange(eq(securityProperties.getPermissionsUri()), eq(HttpMethod.POST), any(HttpEntity.class), any(ParameterizedTypeReference.class))) .thenReturn(new ResponseEntity<>(response, HttpStatus.OK));Also applies to: 75-76, 95-97
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java(1 hunks)refarch-backend/src/test/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverterTest.java(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java (1)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/SecurityConfiguration.java (1)
RequiredArgsConstructor(27-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze Java source files (./refarch-backend)
- GitHub Check: Analyze Java source files (./refarch-eai)
- GitHub Check: build (refarch-backend)
🔇 Additional comments (2)
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java (2)
73-80: Verify that caching solely by JWT subject is correct for your permission model.Authorities are cached under
jwt.getSubject()only; if permissions can differ for the same subject based on other dimensions (e.g. clientId, tenant, realm) within the cache lifetime, this could serve stale or cross‑context authorities. If such variation exists, include the additional dimensions in the cache key (e.g. composite key of subject + clientId/tenant) or shorten the TTL accordingly.Also applies to: 93-93
101-107: Robust, type‑safe permissions mapping looks good.Using
ParameterizedTypeReference<List<Map<String, String>>>forRestTemplate.exchangeplusasAuthoritiesfiltering out null/emptyrsnamevalues avoidsSimpleGrantedAuthorityerrors and gives a clean, type‑safe mapping from the Keycloak response to authorities.Also applies to: 124-126
| public KeycloakPermissionsAuthoritiesConverter(final SecurityProperties securityProperties, final RestTemplateBuilder restTemplateBuilder) { | ||
| this( | ||
| securityProperties, | ||
| restTemplateBuilder.build(), | ||
| new CaffeineCache( | ||
| AUTHENTICATION_CACHE_NAME, | ||
| Caffeine.newBuilder() | ||
| .expireAfterWrite(securityProperties.getPermissionCacheLifetime()) | ||
| .ticker(Ticker.systemTicker()) | ||
| .build())); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider bounding the Caffeine cache size to avoid unbounded growth.
The cache currently uses only expireAfterWrite(permissionCacheLifetime); with many distinct sub values inside that TTL window, entries can grow without an upper bound. Consider adding a reasonable maximumSize(...) (or making it configurable) to cap memory usage for long‑lived services.
🤖 Prompt for AI Agents
In
refarch-backend/src/main/java/de/muenchen/refarch/configuration/security/KeycloakPermissionsAuthoritiesConverter.java
around lines 52 to 61, the Caffeine cache is built with only
expireAfterWrite(...) which allows unbounded growth; update the cache builder to
include a maximumSize(...) cap (make value configurable via SecurityProperties,
e.g., getPermissionCacheMaxSize() with a sensible default) so the cache cannot
grow indefinitely, validate/configure the property in SecurityProperties and use
that value when building the Caffeine.newBuilder() chain.
Pull Request
Changes
Reference
Issue: https://git.muenchen.de/ccse/refarch-security/-/issues/8
Checklist
Note: If some checklist items are not relevant for your PR, just remove them.
General
Code
console.log), see code quality toolingDevelopment Stack
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.