Skip to content

fix: validate resource type consistency when adding resources to FGAP permissions#48434

Merged
pedroigor merged 1 commit intokeycloak:mainfrom
ThreeMangoTrees:threemangotrees/issue-37243
Apr 27, 2026
Merged

fix: validate resource type consistency when adding resources to FGAP permissions#48434
pedroigor merged 1 commit intokeycloak:mainfrom
ThreeMangoTrees:threemangotrees/issue-37243

Conversation

@ThreeMangoTrees
Copy link
Copy Markdown
Contributor

@ThreeMangoTrees ThreeMangoTrees commented Apr 23, 2026

Summary

  • getOrCreateResource() previously returned any resource found by its authz DB ID without verifying it belonged to the requested resource type
  • This allowed a resource 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, resulting in a 201 instead of the expected 400
  • Fix throws a ModelValidationException (HTTP 400) when the found resource does not belong to the expected resource type
  • New resources are now stamped with resource.setType(resourceType) on creation for fast type checks on subsequent lookups
  • A fallback via name-based entity resolution handles legacy resources that pre-date the type field being set

Test plan

  • PermissionRESTTest#testResourceTypeMixingNotAllowed — verifies cross-type resource assignment is rejected with 400 Bad Request
  • Full PermissionRESTTest suite (8 tests) passes with both embedded and distribution server modes
  • Full org.keycloak.tests.admin.authz.fgap.** suite (167 tests) passes with no regressions

Closes #37243

@pedroigor
Copy link
Copy Markdown
Contributor

pedroigor commented Apr 23, 2026

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.

@pedroigor pedroigor added the status/needs-discussion PR needs discussion on developer mailing list label Apr 23, 2026
@ThreeMangoTrees
Copy link
Copy Markdown
Contributor Author

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:

Reproducer

The wrong state can be reached through the following API sequence:

  1. Enable FGAP on a realm and create a user alice.
  2. Create a Users FGAP permission for alice. This internally creates an authz resource whose name is alice's UUID.
  3. Fetch the authz resource's internal ID: GET /admin/realms/{realm}/clients/{admin-permissions-client-id}/authz/resource-server/resource?name={alice-uuid}&exactName=true&deep=true and extract _id.
  4. Create a Groups FGAP permission, passing alice's authz resource _id in the resources field instead of a group ID.
  5. The server returns 201 Created. A Users resource is now silently associated with a Groups permission.

A self-contained shell script to reproduce this against a running Keycloak instance is included below:

#!/usr/bin/env bash
# Reproducer for issue #37243
# Demonstrates that a resource of one FGAP type (Users) can be silently
# associated with a permission of a different type (Groups), returning 201
# instead of the expected 400.
#
# Prerequisites:
#   - Keycloak running at http://localhost:8080 in dev mode
#   - jq installed
#   - Admin credentials: admin / admin

set -e

BASE_URL="http://localhost:8080"
REALM="test"
TS=$(date +%s)

refresh_token() {
  TOKEN=$(curl -s -X POST "$BASE_URL/realms/master/protocol/openid-connect/token" \
    -H "Content-Type: application/x-www-form-urlencoded" \
    -d "client_id=admin-cli" \
    -d "username=admin" \
    -d "password=admin" \
    -d "grant_type=password" | jq -r '.access_token')
  echo "[token refreshed]"
}

# ─── Step 1: Create realm ────────────────────────────────────────────────────
echo ""
echo "=== Step 1: Create realm '$REALM' ==="
refresh_token
curl -s -o /dev/null -w "HTTP %{http_code}\n" -X POST "$BASE_URL/admin/realms" \
  -H "Authorization: Bearer $TOKEN" \
  -H "Content-Type: application/json" \
  -d "{\"realm\":\"$REALM\",\"enabled\":true}"

# ─── Step 2: Enable FGAP ─────────────────────────────────────────────────────
echo ""
echo "=== Step 2: Enable FGAP on realm '$REALM' ==="
refresh_token
curl -s -o /dev/null -w "HTTP %{http_code}\n" -X PUT "$BASE_URL/admin/realms/$REALM" \
  -H "Authorization: Bearer $TOKEN" \
  -H "Content-Type: application/json" \
  -d '{"adminPermissionsEnabled":true}'

# ─── Step 3: Create user alice ───────────────────────────────────────────────
echo ""
echo "=== Step 3: Create user 'alice' ==="
refresh_token
curl -s -o /dev/null -w "HTTP %{http_code}\n" -X POST "$BASE_URL/admin/realms/$REALM/users" \
  -H "Authorization: Bearer $TOKEN" \
  -H "Content-Type: application/json" \
  -d '{"username":"alice","enabled":true}'

# ─── Step 4: Get alice's UUID ────────────────────────────────────────────────
echo ""
echo "=== Step 4: Get alice's UUID ==="
refresh_token
ALICE_ID=$(curl -s "$BASE_URL/admin/realms/$REALM/users?username=alice" \
  -H "Authorization: Bearer $TOKEN" | jq -r '.[0].id')
echo "Alice UUID: $ALICE_ID"

# ─── Step 5: Get the dedicated FGAP client ID from the realm ─────────────────
# When adminPermissionsEnabled is set, Keycloak creates a dedicated
# adminPermissionsClient — it is NOT realm-management.
echo ""
echo "=== Step 5: Get FGAP adminPermissionsClient ID ==="
refresh_token
FGAP_CLIENT_ID=$(curl -s "$BASE_URL/admin/realms/$REALM" \
  -H "Authorization: Bearer $TOKEN" | jq -r '.adminPermissionsClient.id')
echo "FGAP Client ID: $FGAP_CLIENT_ID"

# ─── Step 6: Create a group ──────────────────────────────────────────────────
echo ""
echo "=== Step 6: Create group 'test-group' ==="
refresh_token
curl -s -o /dev/null -w "HTTP %{http_code}\n" -X POST "$BASE_URL/admin/realms/$REALM/groups" \
  -H "Authorization: Bearer $TOKEN" \
  -H "Content-Type: application/json" \
  -d '{"name":"test-group"}'

refresh_token
GROUP_ID=$(curl -s "$BASE_URL/admin/realms/$REALM/groups?search=test-group" \
  -H "Authorization: Bearer $TOKEN" | jq -r '.[0].id')
echo "Group ID: $GROUP_ID"

# ─── Step 7: Create a Users permission for alice ─────────────────────────────
# This internally creates an authz resource whose name = alice's UUID
echo ""
echo "=== Step 7: Create Users permission for alice ==="
refresh_token
curl -s -o /dev/null -w "HTTP %{http_code}\n" \
  -X POST "$BASE_URL/admin/realms/$REALM/clients/$FGAP_CLIENT_ID/authz/resource-server/permission/scope" \
  -H "Authorization: Bearer $TOKEN" \
  -H "Content-Type: application/json" \
  -d "{
    \"name\": \"alice-users-permission-$TS\",
    \"resourceType\": \"Users\",
    \"resources\": [\"$ALICE_ID\"],
    \"scopes\": [\"view\", \"manage\", \"impersonate\", \"map-roles\", \"manage-group-membership\", \"reset-password\"]
  }"

# ─── Step 8: Fetch alice's internal authz resource ID ────────────────────────
echo ""
echo "=== Step 8: Fetch alice's internal authz resource ID ==="
refresh_token
ALICE_AUTHZ_RESOURCE_ID=$(curl -s \
  "$BASE_URL/admin/realms/$REALM/clients/$FGAP_CLIENT_ID/authz/resource-server/resource?name=$ALICE_ID&exactName=true&deep=true" \
  -H "Authorization: Bearer $TOKEN" | jq -r '.[0]._id')
echo "Alice authz resource ID: $ALICE_AUTHZ_RESOURCE_ID"

# ─── Step 9: Create a Groups permission for the group ────────────────────────
# This creates a separate authz resource for the group
echo ""
echo "=== Step 9: Create Groups permission for test-group ==="
refresh_token
curl -s -o /dev/null -w "HTTP %{http_code}\n" \
  -X POST "$BASE_URL/admin/realms/$REALM/clients/$FGAP_CLIENT_ID/authz/resource-server/permission/scope" \
  -H "Authorization: Bearer $TOKEN" \
  -H "Content-Type: application/json" \
  -d "{
    \"name\": \"test-group-permission-$TS\",
    \"resourceType\": \"Groups\",
    \"resources\": [\"$GROUP_ID\"],
    \"scopes\": [\"view\", \"manage\", \"manage-membership\", \"manage-members\", \"view-members\"]
  }"

# ─── Step 10: THE BUG — cross-type resource assignment ───────────────────────
# Pass alice's Users authz resource ID to a Groups permission.
# WITHOUT the fix: returns 201 (wrong — Users resource silently added to Groups permission)
# WITH the fix:    returns 400 Bad Request
echo ""
echo "=== Step 10: THE BUG — create Groups permission using alice's Users resource ID ==="
echo "Expected: HTTP 400"
echo "Without fix: HTTP 201  ← bug"
refresh_token
curl -s -o /dev/null -w "HTTP %{http_code}\n" \
  -X POST "$BASE_URL/admin/realms/$REALM/clients/$FGAP_CLIENT_ID/authz/resource-server/permission/scope" \
  -H "Authorization: Bearer $TOKEN" \
  -H "Content-Type: application/json" \
  -d "{
    \"name\": \"bad-groups-permission-$TS\",
    \"resourceType\": \"Groups\",
    \"resources\": [\"$ALICE_AUTHZ_RESOURCE_ID\"],
    \"scopes\": [\"view\", \"manage\", \"manage-membership\", \"manage-members\", \"view-members\"]
  }"

echo ""
echo "=== Done ==="
echo "If Step 10 returned HTTP 201, the bug is present (unfixed code)."
echo "If Step 10 returned HTTP 400, the fix is working correctly."
                                                                                                                   

On resource.setType() and evaluation semantics

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:

  • Name-based resolution only (no stored type): This fails when the caller passes the authz resource's own internal UUID as the resource ID, findById locates the resource, but the subsequent name-based resolution in isResourceOfType cannot reliably determine the type from the authz UUID alone.
  • Custom resource attribute (resource.setAttribute): This is wired directly to the user-visible REST API, meaning the
    attribute would be publicly exposed and could be overwritten by any admin, wrong for internal metadata.

Given these constraints, I kept resource.setType() as the only reliable mechanism, but I'd appreciate your guidance on
the preferred approach here:

  • Is there an internal mechanism in FGAP v2 for associating metadata with a resource that doesn't affect typed-resource permission evaluation?
  • Or should the validation check itself (ModelValidationException on type mismatch) be implemented at a different layer, for example, in the REST endpoint rather than in getOrCreateResource?
  • Alternatively, is this cross-type assignment scenario something FGAP v2 considers out of scope for validation, and the fix should simply not be pursued in this form?

Copy link
Copy Markdown
Contributor

@vramik vramik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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]>
@ThreeMangoTrees ThreeMangoTrees force-pushed the threemangotrees/issue-37243 branch from ecdd652 to 5e638b8 Compare April 25, 2026 23:42
@ThreeMangoTrees
Copy link
Copy Markdown
Contributor Author

@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.

Thanks for the detailed review and the concrete suggestion, @vramik!

You're right that resource.setType() was overreaching. The goal was validation, not mutating authz resource
semantics, and relying on FGAPPolicyEvaluator never reading that field is a fragile assumption.

I've updated the implementation to exactly the approach you proposed: after findById, if the resource name
doesn't equal the resource type (i.e. it's a per-entity resource, not the wildcard), getResourceName(session, resourceType, resourceName) is called purely for validation, throwing ModelValidationException on a type
mismatch. The isResourceOfType() helper and the setType() call are both removed.

Test coverage is also added: testResourceTypeMixingNotAllowed in PermissionRESTTest verifies that
attempting to create a permission of one type using an authz resource ID belonging to a different type returns
400 Bad Request.

The branch has been rebased on main. Please take another look when you get a chance!

Copy link
Copy Markdown
Contributor

@vramik vramik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThreeMangoTrees thanks for the update, LGTM!

@pedroigor please run the GHA

@pedroigor pedroigor merged commit 217d62c into keycloak:main Apr 27, 2026
85 checks passed
@pedroigor
Copy link
Copy Markdown
Contributor

Thanks, @ThreeMangoTrees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flaky-test status/needs-discussion PR needs discussion on developer mailing list

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure all resources in the permission is of the same type

3 participants