fix: validate resource type consistency when adding resources to FGAP permissions#48434
Conversation
|
Could you please share the steps you took that led to the wrong state? I do not see any reproducer in the issue. FTR, setting a type to a resource changes the semantics when evaluating permissions. Please, see https://www.keycloak.org/docs/26.6.1/authorization_services/#typed-resources and https://www.keycloak.org/docs/26.6.1/authorization_services/#_permission_typed_resource. In FGAP v2, we handle the "all" and "specific" resources without any parent/child relationship. |
Thank you for the feedback. Let me address both points: ReproducerThe wrong state can be reached through the following API sequence:
A self-contained shell script to reproduce this against a running Keycloak instance is included below: On You are right to flag this. Setting type on per-entity resources using the FGAP type strings (Users, Groups, etc.) could cause those resources to be matched by any typed-resource permission covering the same type, which is an unintended side effect that could affect authorization evaluation. I investigated two alternatives before coming back to this approach:
Given these constraints, I kept resource.setType() as the only reliable mechanism, but I'd appreciate your guidance on
|
There was a problem hiding this comment.
@ThreeMangoTrees thanks for the contribution!
I agree the issue is real — getOrCreateResource() returns any resource found by its internal authz DB ID without validating it belongs to the expected resource type. However, I have concerns about the approach.
resource.setType() is risky. While it happens to be safe today because FGAPPolicyEvaluator overrides evaluateResourceTypePolicies() and never reads resource.getType(), it's relying on an implementation detail. The DefaultPolicyEvaluator.evaluateResourceTypePolicies() does use resource.getType() to drive typed resource inheritance/evaluation, and if FGAP evaluator ever changes and go through that path, setting the type could introduce subtle evaluation side effects. Pedro's concern here is valid — we shouldn't change authz resource semantics when all we need is input validation.
We already have getResourceName(session, resourceType, id) which validates that a UUID resolves to the correct entity type (user, group, client, role). We can reuse it to validate after findById without touching the resource's type field:
Resource resource = resourceStore.findById(resourceServer, id);
if (resource != null) {
String resourceName = resource.getName();
if (!Objects.equals(resourceName, resourceType)) {
// instance resource — validate the name resolves as the expected entity type
getResourceName(session, resourceType, resourceName);
}
return resource;
}This throws ModelValidationException if the resource's name does not match the expected type.
This approach offers same end result, no setType(), no fallback logic for legacy resources, and the isResourceOfType() helper becomes unnecessary.
wdyt?
Also the PR needs test coverage.
… permissions Previously, getOrCreateResource() would return any resource found by its authz DB ID without verifying it belonged to the requested resource type. This allowed resources of one type (e.g. Users) to be silently added to a permission of a different type (e.g. Groups) by passing the internal authz resource ID. - For per-entity resources found by ID, validate the name resolves as the expected entity type via getResourceName(); throw ModelValidationException on mismatch Closes keycloak#37243 Signed-off-by: Vinit Kumar <[email protected]>
ecdd652 to
5e638b8
Compare
Thanks for the detailed review and the concrete suggestion, @vramik! You're right that I've updated the implementation to exactly the approach you proposed: after Test coverage is also added: The branch has been rebased on |
vramik
left a comment
There was a problem hiding this comment.
@ThreeMangoTrees thanks for the update, LGTM!
@pedroigor please run the GHA
|
Thanks, @ThreeMangoTrees. |
Summary
getOrCreateResource()previously returned any resource found by its authz DB ID without verifying it belonged to the requested resource type201instead of the expected400ModelValidationException(HTTP 400) when the found resource does not belong to the expected resource typeresource.setType(resourceType)on creation for fast type checks on subsequent lookupsTest plan
PermissionRESTTest#testResourceTypeMixingNotAllowed— verifies cross-type resource assignment is rejected with400 Bad RequestPermissionRESTTestsuite (8 tests) passes with bothembeddedanddistributionserver modesorg.keycloak.tests.admin.authz.fgap.**suite (167 tests) passes with no regressionsCloses #37243