[#10411] feat(core): Extend metadata object owner to support group#10848
Merged
roryqi merged 2 commits intoapache:mainfrom Apr 24, 2026
Merged
[#10411] feat(core): Extend metadata object owner to support group#10848roryqi merged 2 commits intoapache:mainfrom
roryqi merged 2 commits intoapache:mainfrom
Conversation
bharos
force-pushed
the
feat/extend-owner-support-group
branch
from
e09a718 to
290bb94
Compare
bharos
commented
Apr 22, 2026
| import org.junit.jupiter.api.TestMethodOrder; | ||
| import org.mockito.Mockito; | ||
|
|
||
| @TestMethodOrder(MethodOrderer.OrderAnnotation.class) |
Contributor
Author
There was a problem hiding this comment.
The tests share a single in-memory H2 entity store that persists state across methods. testOwner() asserts the metalake has no owner initially, but testGroupTypeOwner() sets an owner on the same metalake. Without ordering, JUnit could run them in any order, causing testOwner() to fail if it runs second.
bharos
force-pushed
the
feat/extend-owner-support-group
branch
from
290bb94 to
d54eca6
Compare
Code Coverage Report
Files
|
…cting it The PR extends owner support to groups, but testOwnerDrop() still asserted that setting a GROUP owner throws IllegalArgumentException. Updated the test to set a group owner, verify ownership, drop the group, and confirm the owner is removed.
4 tasks
roryqi
pushed a commit
that referenced
this pull request
Apr 25, 2026
…rizer (#10867) ### What changes were proposed in this pull request? Extend `JcasbinAuthorizer` to recognize group-based ownership. When a metadata object is owned by a group, all members of that group are now treated as owners and granted owner privileges. Key changes: - **`OwnerInfo` inner class**: Replaces the raw `Long` owner ID in the cache with a struct that stores `id`, `type` (USER/GROUP), and `name`. - **`ownerRel` cache type**: Changed from `Cache<Long, Optional<Long>>` to `Cache<Long, Optional<OwnerInfo>>`. - **`loadOwnerPolicy()`**: Now handles `GroupEntity` alongside `UserEntity` when populating the owner cache. - **`checkOwnership()`**: New method that resolves both user and group owners — for USER owners it compares entity IDs, for GROUP owners it checks whether the principal's groups include the owning group. - **`isOwner()` and `authorizeByJcasbin()`**: Refactored to delegate to `checkOwnership()`, eliminating duplicated ownership logic. - **Documentation**: Removed "group ownership not supported" info boxes from `docs/security/access-control.md` and added group ownership bullet. ### Why are the changes needed? Currently, when a metadata object's owner is set to a group (supported since #10848), the `JcasbinAuthorizer` does not recognize group ownership — only individual user ownership is checked. This means group members are denied owner privileges even when the group is the registered owner. This PR is part 2 of a 3-PR series for #10412: 1. **#10848** — Core/API: `OwnerManager.setOwner` accepts GROUP (**merged**) 2. **This PR** — Enforcement: `JcasbinAuthorizer` recognizes group ownership 3. **Planned** — Role inheritance: `loadRolePrivilege` queries `ROLE_GROUP_REL` Fix: #10412 ### Does this PR introduce _any_ user-facing change? Yes. Users who assign group ownership to metadata objects will now have all group members recognized as owners by the JCasbin authorization plugin. ### How was this patch tested? Unit tests in `TestJcasbinAuthorizer`: - `testAuthorizeByGroupOwner`: Verifies a principal whose groups include the owning group is recognized as owner, a non-member group is denied, and clearing ownership returns false. - Existing tests (`testIsOwner`, `testOwnerCacheInvalidation`, `testCacheInitialization`) updated for `OwnerInfo` type. - All 8 tests pass: `./gradlew :server-common:test --tests "...TestJcasbinAuthorizer"`
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Remove the USER-only restriction in
OwnerManager.setOwner()and add GROUP support tonotifyOwnerChange(), enabling groups to be set as metadata object owners.Changes:
Preconditions.checkArgument(ownerType == Owner.Type.USER)guard inOwnerManager.setOwner()notifyOwnerChange()to resolve group entity IDsGroupEntitynamespace in test (was usingofUserNamespace, corrected toofGroupNamespace)testGroupTypeOwnerNotSupportedwith tests that verify group ownership worksWhy are the changes needed?
The owner field was implicitly assumed to point to a User. In practice, the logical owner of a resource (Schema, Table, etc.) is often a team (Group) rather than an individual. The downstream layers (REST API, DTOs, relational store) already support GROUP — only
OwnerManagerhad a guard blocking it.Fix: #10411
Does this PR introduce any user-facing change?
Yes. The
setOwnerAPI now acceptsOwner.Type.GROUPin addition toOwner.Type.USER.How was this patch tested?
Unit tests in
TestOwnerManager:testGroupTypeOwner: Sets group as owner, verifies, then replaces with user owner (exercisesnotifyOwnerChangeGROUP path)testSetNonExistentGroupAsOwner: VerifiesNotFoundExceptionfor non-existent group