jerryshao commented on code in PR #13006:
URL: https://github.com/apache/gravitino/pull/13006#discussion_r4025448848
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/UserMetaService.java:
##########
@@ -271,6 +283,9 @@ public <E extends Entity & HasIdentifier> UserEntity
updateUser(
throw userWriteFailure(identifier, oldUserPO, UserLookup.NAME);
}
},
+ () ->
+ RoleMetaService.getInstance()
+ .lockRolesForMembership(oldUserPO.getMetalakeId(),
insertRoleIds),
Review Comment:
This fence checks that the added role IDs are still active, but those IDs
come from `PermissionManager`'s updater. The updater looks up the user's
**existing** roles again by name (`roleManager.getRole(metalake, role)` for
each of `userEntity.roleNames()`), so a same-name replacement can still get
through:
1. User U has role `r` (id 1). An admin grants role `g` to U, and
`updateUser` reads U's roles ({`r`: 1}) outside the transaction.
2. Before the updater runs, `r` is deleted (its rels are soft-deleted) and
recreated with different privileges (id 2).
3. The updater resolves `r` by name to id 2, so `insertRoleIds` = {2, g} and
`deleteRoleIds` = {1}.
4. The user CAS still passes because `deleteRole` doesn't bump the user
row's version. `lockRolesForMembership(metalakeId, {2, g})` passes too, because
both roles are active.
5. `(U, 2)` is inserted, and U is now a member of the new `r`, which nobody
granted.
`revokeRolesFromUser` rebuilds the role list the same way, so revoking an
unrelated role can trigger this as well. `GroupMetaService.updateGroup` has the
same path for groups.
The root cause is in `PermissionManager` rather than in this diff, but it
undercuts the PR's guarantee that stale IDs never retarget same-name
replacements. Could we carry the observed memberships forward by ID in the
updater, or reject the update when an existing role name now resolves to a
different ID?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
