Skip to content

recursive groups: allow groups to be member or admin of other groups#59593

Open
KiaraGrouwstra wants to merge 5 commits intonextcloud:masterfrom
KiaraGrouwstra:recursive-groups
Open

recursive groups: allow groups to be member or admin of other groups#59593
KiaraGrouwstra wants to merge 5 commits intonextcloud:masterfrom
KiaraGrouwstra:recursive-groups

Conversation

@KiaraGrouwstra
Copy link
Copy Markdown

Summary

Implements #36150: groups can contain other groups as members, and a group can be designated as a sub-admin of another group. Membership composes transitively, so a user in a subgroup is treated as an effective member of every ancestor for share ACLs, app restrictions, tag visibility, settings delegation, sub-admin inheritance, and 2FA enforcement.

Additive: existing getUserGroupIds / getUserGroups semantics are unchanged, and a new getUserEffectiveGroupIds is introduced for call sites that should honor nesting. Only OC\Group\Database implements the new INestedGroupBackend capability interface; LDAP, SAML and other external backends are untouched and compose with database-side nesting as a union.

What is in this PR

Core (OC\Group\Database, OC\Group\Manager)

  • New group_group(parent_gid, child_gid) edge table.
  • BFS transitive closure in Manager with per-request memoization and batched backend queries (one WHERE IN per BFS level, not one query per frontier node).
  • Cycles rejected at insert time inside a serialized transaction so concurrent writers cannot race a cycle into existence.
  • SubGroupAddedEvent / SubGroupRemovedEvent dispatched on edge mutations, plus per-user UserAddedEvent / UserRemovedEvent for every user who gains or loses effective membership of the parent. This is the hook apps/encryption needs to re-key files when nesting shifts the effective recipient set of a group share.
  • Per-user event synthesis is bounded by Manager::MAX_SYNTHESIZED_USER_EVENTS = 500. Beyond that a warning is logged and the per-user events are skipped; admins bulk-nesting on an encrypted instance must run a manual re-key pass (documented).

Sub-admin delegation (OC\SubAdmin)

  • New group_group_admin(admin_gid, gid) table: every effective member of admin_gid is treated as a sub-admin of gid and of all of its subgroups.
  • isSubAdminOfGroup resolves the target's ancestor set via getGroupEffectiveAncestorIds and checks both group_admin and group_group_admin against it, so direct sub-admins of a parent group automatically inherit admin rights over every descendant.

Public API additions

OCP\IGroupManager:

  • getUserEffectiveGroupIds(IUser)
  • addSubGroup(IGroup $parent, IGroup $child) / removeSubGroup(IGroup $parent, IGroup $child)
  • getDirectChildGroupIds(string) / getDirectParentGroupIds(string)
  • getGroupEffectiveDescendantIds(IGroup) / getGroupEffectiveAncestorIds(IGroup)

OCP\Group\ISubAdmin:

  • createGroupSubAdmin(IGroup $adminGroup, IGroup $group) / deleteGroupSubAdmin(IGroup $adminGroup, IGroup $group)
  • getGroupSubAdminsOfGroup(IGroup)

New classes:

  • OCP\Group\Events\SubGroupAddedEvent / SubGroupRemovedEvent
  • OCP\Group\Exception\CycleDetectedException
  • OCP\Group\Exception\NestedGroupsNotSupportedException

Call sites migrated to effective membership

Every place that previously called IGroup::inGroup() for an access-control decision now resolves membership through getUserEffectiveGroupIds, so nested-group edges flow through:

  • Share20\Manager::userCreateChecks (duplicate-share detection)
  • Share20\Manager::groupCreateChecks (shareWithGroupMembersOnly perimeter)
  • Share20\Manager::moveShare
  • Share20\DefaultShareProvider::acceptShare / deleteFromSelf / getSharedWith
  • Share20\ShareDisableChecker::sharingDisabledForUser
  • Files_Sharing\Controller\ShareAPIController::canAccessShare / canDeleteShareFromSelf
  • Files_Sharing\Listener\UserShareAcceptanceListener (auto-accept now walks descendants)
  • Files_Sharing\Notification\Notifier (pending-share notifications)
  • Files_Sharing\External\Manager (federated group-share accept)
  • App\AppManager::isEnabledForUser (app restrictions)
  • SystemTag\SystemTagManager::isSystemTagAccessible (tag visibility)
  • Settings\AuthorizedGroupMapper::findAllClassesForUser (settings delegation)
  • Authentication\TwoFactorAuth\MandatoryTwoFactor (see 2FA note below)
  • Provisioning_API\Controller\GroupsController::getGroupUsers / getGroupUsersDetails — now walks getGroupEffectiveDescendantIds so a sub-admin clicking a parent group in the Users UI sees members of every descendant, not "No accounts".

OCS endpoints (apps/provisioning_api)

Additive, namespaced under cloud/groups/{groupId}:

  • GET /subgroups
  • POST /subgroups body: {subGroupId}
  • DELETE /subgroups/{subGroupId}
  • GET /subadmins/groups
  • POST /subadmins/groups body: {adminGroupId}
  • DELETE /subadmins/groups/{adminGroupId}

All require the Users admin delegation. Cycle and unsupported-backend cases surface as typed OCS errors. OpenAPI spec regenerated.

Settings admin UI

A new GroupNestingModal reachable from each row in the groups list via a "Manage nested groups" action. Uses NcSelect pickers backed by the existing searchGroups service, excluding self and already-added entries from autocomplete. Compiled bundle is in the PR.

Security-relevant design notes

2FA enforced vs. excluded lists

MandatoryTwoFactor expands the enforced groups list along nested edges (strictly more secure) but keeps the excluded list direct-only. Expanding the excluded list transitively would let an admin silently exempt an arbitrary population from 2FA by nesting groups under an excluded one — a one-way security weakening via hierarchy changes. Admins who want subgroups exempt must mark each subgroup on the excluded list explicitly. Covered by testIsEnforcedForMemberOfExcludedGroupViaNestingOnly.

shareWithGroupMembersOnly

The restriction now permits sharing whenever sharer and sharee are effective members of the same group, not only direct members. This is the intended behaviour of the feature but represents a policy change from previous releases. Admins relying on direct-only membership as a sharing perimeter should review their group hierarchy before enabling nesting. Documented in NESTED_GROUPS.md.

Server-side encryption

apps/encryption listens for UserAddedEvent / UserRemovedEvent to re-key files. We synthesize those events on nesting edge mutations so encrypted shares stay consistent. Beyond the 500-user cap the events are skipped and a prominent warning is logged to nextcloud.log; the admin must then run a manual re-key pass. The cap bounds worst-case request duration when nesting a group that already contains thousands of users. Admins bulk-nesting on an encrypted instance should do so via occ during off-hours.

Cycle prevention

Cycles are rejected at insert time in OC\Group\Database::addGroupToGroup via a BFS reachability check run inside a serialized transaction so that concurrent inserts cannot race a cycle into existence. Self-edges and duplicate edges are rejected the same way.

Backwards compatibility

  • getUserGroupIds / getUserGroups remain direct-membership-only. No existing call site changes semantics unless it has been explicitly migrated in this PR.
  • Existing group backends that do not implement INestedGroupBackend are unaffected and simply cannot participate on the edge side of nesting, but their groups can still appear in hierarchies managed by the database backend.
  • Migration Version34000Date20260410120000 creates the two new tables idempotently with hasTable guards.

Caveats (documented in lib/private/Group/NESTED_GROUPS.md)

  • Delete-middle-group — deleting B in A -> B -> C transparently drops the two edges touching B; the remaining groups are disconnected (no splice to A -> C). Admin is not warned.
  • LDAP enumeration costcollectEffectiveUserIds walks descendants and calls Group::searchUsers(''), which on LDAP triggers a paginated backend query. Nesting a large LDAP group may block the edge-mutation request for several seconds; run via occ during off-hours for large LDAP groups.
  • Sub-admin inheritance — revoking a group sub-admin designation does not automatically revoke rights that were inherited via ancestry; re-check the hierarchy after mutations.

Out of scope

  • Closure table / recursive CTE optimization. BFS is O(depth * fan-out) per request with per-request memoization; acceptable for shallow hierarchies, suboptimal for deep ones. Add a closure table if profiling shows it.
  • Audit log integration (events are dispatched so an app can listen, but nothing ships in core).
  • UI splice-on-delete for intermediate groups.
  • Circles app interop.

Commits

  1. feat(groups): transitive group-in-group membership — migration, backend, Manager, public API, events, exceptions, docs, backend + nesting tests.
  2. feat(subadmin): group-level delegation and ancestor inheritanceSubAdmin, ISubAdmin, sub-admin tests.
  3. feat(groups): honor effective membership in access checks — Share20 / AppManager / SystemTag / AuthorizedGroupMapper / 2FA / files_sharing / provisioning_api call-site migrations, with test mock updates and dedicated regression tests (testGroupCreateChecksShareWithGroupMembersOnlyViaNestedGroup, testAcceptShareViaNestedGroup, testGetGroupUsersUnionsNestedDescendants, testGetGroupUsersDetailsUnionsNestedDescendants).
  4. feat(provisioning_api): OCS endpoints for nested groups and group sub-admins — routes, controller, regenerated OpenAPI.
  5. feat(settings): admin UI for managing nested groupsGroupListItem, GroupNestingModal, store actions, compiled bundle.

Screenshots

user set-up (note dave is member of no groups, so may not be managed using inherited group sub-admin status):

Screenshot from 2026-04-11 22-16-46

adding group associations:

Screenshot from 2026-04-11 16-48-09 Screenshot from 2026-04-11 16-52-44 Screenshot from 2026-04-11 21-15-58

files shared through groups the users are effectively members of:

Screenshot from 2026-04-11 22-07-10

being able to manage users of groups the user is effectively a sub-admin of:

Screenshot from 2026-04-11 22-26-06

Test plan

  • tests/lib/Group/NestedGroupsTest.php — transitive closure, diamond hierarchy dedup, cycle rejection, idempotent edge add, event dispatch on add/remove, cache invalidation after removal, shallow direct-child listing.
  • tests/lib/Group/DatabaseTest.php — nested CRUD, self-edge rejection, cycle rejection, edge cleanup on group deletion.
  • tests/lib/SubAdminTest.php — direct sub-admin inherits across ancestors, group-level delegation composes with hierarchy, getSubAdminsGroupIds descends.
  • tests/lib/Authentication/TwoFactorAuth/MandatoryTwoFactorTest.php — enforced list expanded transitively, excluded list direct-only, testIsEnforcedForMemberOfExcludedGroupViaNestingOnly.
  • tests/lib/Share20/ManagerTest.php, DefaultShareProviderTest.php, lib/App/AppManagerTest.php, lib/SystemTag/SystemTagManagerTest.php — mocks updated to assert getUserEffectiveGroupIds, dedicated nested-group regression tests added, all passing.
  • apps/files_sharing/tests/Controller/ShareAPIControllerTest.php, apps/provisioning_api/tests/Controller/GroupsControllerTest.php — controller call-site migrations and descendant-walk regression tests.
  • Manual: added subgroups via settings UI, verified Manage nested groups action and picker UX.
  • Manual: bob shared a file with engineering; alice (direct member of backend, nested under engineering) received it under Files without intervention via the UserShareAcceptanceListener descendant walk; dave (ungrouped) did not.
  • Manual: designated managers as admin group of engineering; carol (direct member of managers) gained sub-admin rights over engineering, backend and frontend in the Users UI.
  • Manual on encrypted instance: verify re-key happens for small nests and that the cap-exceeded warning appears in nextcloud.log for large ones.
  • Manual on LDAP instance: verify edge mutations complete and that external group membership composes as a union.

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

Add a group_group edge table maintained by OC\Group\Database, an
internal INestedGroupBackend capability interface, and BFS-based
transitive closure in OC\Group\Manager with per-request memoization
and batched backend queries.

Public API gains getUserEffectiveGroupIds, addSubGroup/removeSubGroup,
getDirectChildGroupIds/getDirectParentGroupIds, and
getGroupEffectiveDescendantIds/getGroupEffectiveAncestorIds.
Cycles are rejected inside a serialized transaction.
SubGroupAdded/SubGroupRemovedEvent are dispatched along with per-user
UserAdded/UserRemovedEvent (bounded by MAX_SYNTHESIZED_USER_EVENTS) so
listeners such as apps/encryption stay consistent when nesting shifts
the effective recipient set of a group share.

See lib/private/Group/NESTED_GROUPS.md for caveats (encryption
re-keying cap, LDAP enumeration cost, delete-middle-group semantics).

Refs nextcloud#36150.

Signed-off-by: Kiara Grouwstra <cinereal@riseup.net>
A group can now be designated as sub-admin of another group via a
new group_group_admin(admin_gid, gid) table: every effective member
of admin_gid is treated as a sub-admin of gid and, by ancestry, of
all of its subgroups. Direct sub-admins of a parent group inherit
admin rights over every descendant.

isSubAdminOfGroup resolves the target's ancestor set via
getGroupEffectiveAncestorIds and checks both group_admin and
group_group_admin against that set, so rights propagate without
reimplementing Manager's BFS.

Adds createGroupSubAdmin / deleteGroupSubAdmin /
getGroupSubAdminsOfGroup on OCP\Group\ISubAdmin.

Refs nextcloud#36150.

Signed-off-by: Kiara Grouwstra <cinereal@riseup.net>
Migrate Share20, AppManager, SystemTagManager, AuthorizedGroupMapper,
ShareDisableChecker, and MandatoryTwoFactor to resolve group
membership via getUserEffectiveGroupIds so nested-group edges flow
through to share ACLs, app restrictions, tag visibility, settings
delegation, and 2FA enforcement.

MandatoryTwoFactor deliberately keeps the *excluded* groups list on
direct membership: expanding it transitively would let an admin
silently exempt an arbitrary population from 2FA by nesting groups
under an excluded one, a one-way security weakening. Enforced groups
are expanded (strictly more secure).

Refs nextcloud#36150.

Signed-off-by: Kiara Grouwstra <cinereal@riseup.net>
…-admins

Additive routes under cloud/groups/{groupId}:

- GET/POST/DELETE subgroups
- GET/POST/DELETE subadmins/groups

All require the Users admin delegation. Cycle and
unsupported-backend cases are surfaced as typed HTTP errors.

OpenAPI spec regenerated.

Signed-off-by: Kiara Grouwstra <cinereal@riseup.net>
Add a GroupNestingModal reachable from each row in the groups list
via a new "Manage nested groups" action. The modal uses NcSelect
pickers backed by the existing searchGroups service to add or remove
subgroups and admin groups, excluding self and already-added entries
from autocomplete.

Store actions fetchSubGroups / addSubGroup / removeSubGroup and
fetchGroupSubAdmins / addGroupSubAdmin / removeGroupSubAdmin wrap
the new OCS endpoints.

Signed-off-by: Kiara Grouwstra <cinereal@riseup.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant