nevzheng opened a new pull request, #14249:
URL: https://github.com/apache/gravitino/pull/14249
### What changes were proposed in this pull request?
- **`UserGroupManager.removeUser` / `removeGroup`** read the principal and
its roles before the delete
(roles that no longer exist are skipped), delete as before, and only after
a successful delete call
`onRevokedRolesFromUser` / `onRevokedRolesFromGroup` on the plugin of each
affected catalog. Roles are
filtered per catalog as in `PermissionManager.revokeRolesFromUser/Group`,
and each catalog is called
once. The bulk `removeUsers` / `removeGroups` go through the same methods.
- **Per-catalog failure handling.** A catalog whose plugin throws, or which
cannot be loaded, is logged
at ERROR with the principal, catalog and role names, and the other
catalogs are still called. All
failures are reported in one `AuthorizationPluginException` (first as
cause, the rest suppressed).
- **`AuthorizationUtils`** gets two package-private helpers:
`getSecurableObjectCatalogs` (the same
catalog selection as `callAuthorizationPluginForSecurableObjects`,
de-duplicated) and
`callAuthorizationPluginForCatalog`.
`callAuthorizationPluginForSecurableObjects` is unchanged.
- `onUserRemoved` / `onGroupRemoved` are not called: for the JDBC plugins
they drop the principal in the
underlying system, and `addUser` / `addGroup` do not call `onUserAdded` /
`onGroupAdded` either.
### Why are the changes needed?
**Removing a principal left its role memberships in the plugin.** The entity
store dropped the role
relations, but Ranger kept the removed user or group in every role granted
through Gravitino, so the
roles' access still applied in the engines. The plugin calls happen after
the delete, so a retry cannot
reach them again; that is why one failing catalog must not stop the others.
Fix: #14247
### Does this PR introduce _any_ user-facing change?
No API change. Removing a user or group now revokes its roles in the
authorization plugins. If a plugin
call fails, the principal stays removed and the caller gets an
`AuthorizationPluginException` naming the
failed catalogs.
### How was this patch tested?
New cases in `TestAccessControlManagerForPermissions` (mock plugin):
- **Revoke reaches the plugin:** single user, single group and bulk removal
get the exact roles and
principal, and never `onUserRemoved` / `onGroupRemoved`.
- **Scope:** roles filtered per catalog with two catalogs; metalake-scoped
roles reach all catalogs; a
role without connector privileges, or deleted earlier, makes no call; no
plugin configured still works.
- **Failures:** two of three catalogs failing still calls all three and
names only the failed ones; an
unloadable catalog does not stop the others; bulk removal reports the
failure per item; a retry after
a failure is a no-op.
- **Negative:** a missing user, a user without roles, a failed delete, and
store read/delete failures
make no plugin call.
Results:
- **On `main`:** 14 of the 35 `TestAccessControlManagerForPermissions` cases
fail (the new revoke, scope and failure cases; the negative cases that check
the remove read the roles before the delete fail too, because `main` never
reads them). The rest pass.
- **With the fix:** all 35 pass, as do `TestAccessControlManager` (24) and
`TestAuthorizationUtils` (19).
`./gradlew -PskipITs :core:test --tests
'*TestAccessControlManagerForPermissions' --tests '*TestAccessControlManager'
--tests '*TestAuthorizationUtils'`
Also adds `RangerHiveRemoveUserIT` (`@Tag("gravitino-docker-test")`, real
Ranger and Hive): removing a
user or group removes it from the Ranger role while the role's policy items
stay. It compiles; it was
not run locally.
--
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]
