Skip to content

feat(groups): transitive group-in-group membership#59859

Open
KiaraGrouwstra wants to merge 1 commit intonextcloud:masterfrom
KiaraGrouwstra:recursive-groups-1
Open

feat(groups): transitive group-in-group membership#59859
KiaraGrouwstra wants to merge 1 commit intonextcloud:masterfrom
KiaraGrouwstra:recursive-groups-1

Conversation

@KiaraGrouwstra
Copy link
Copy Markdown

@KiaraGrouwstra KiaraGrouwstra commented Apr 22, 2026

Adds transitive group-in-group membership to the database group backend. A group can be made a direct subgroup of another group via a new group_group(parent_gid, child_gid) edge table. Membership composes transitively: a user in a subgroup is an effective member of every ancestor.

Existing getUserGroupIds / getUserGroups semantics are unchanged; a new getUserEffectiveGroupIds is introduced for call sites that should honor nesting. Only OC\Group\Database implements the new internal INestedGroupBackend capability interface; LDAP, SAML and other external backends are untouched.

Part 1 of a series implementing #36150. Follow-up PRs layer sub-admin delegation, access-check migration, OCS endpoints, and admin UI on top.

What is in this PR

  • Migration Version34000Date20260410120000 -- creates group_group(parent_gid, child_gid) and group_group_admin(admin_gid, gid) tables.
  • OC\Group\Database -- addGroupToGroup / removeGroupFromGroup / getChildGroups / getParentGroups / getChildGroupsBatch / getParentGroupsBatch / groupInGroup / isDescendantOf. Cycle check + insert wrapped in a serialized transaction so concurrent writers cannot race a cycle into existence.
  • OC\Group\Manager -- BFS transitive closure with per-request memoization and batched backend queries (one WHERE IN per BFS level). addSubGroup / removeSubGroup dispatch SubGroupAddedEvent / SubGroupRemovedEvent plus per-user UserAddedEvent / UserRemovedEvent for every user who gains or loses effective membership (bounded by MAX_SYNTHESIZED_USER_EVENTS = 500).
  • Public API on OCP\IGroupManager: getUserEffectiveGroupIds, addSubGroup / removeSubGroup, getDirectChildGroupIds / getDirectParentGroupIds, getGroupEffectiveDescendantIds / getGroupEffectiveAncestorIds.
  • Events: SubGroupAddedEvent, SubGroupRemovedEvent.
  • Exceptions: CycleDetectedException, NestedGroupsNotSupportedException.
  • Documentation: lib/private/Group/NESTED_GROUPS.md covering concept, cycle prevention, event synthesis, and caveats.

Caveats (documented in NESTED_GROUPS.md)

  • Encryption re-keying cap -- beyond 500 users, per-user events are skipped and a warning is logged; admin must run a manual re-key pass.
  • Delete-middle-group -- deleting B in A -> B -> C drops both edges without splicing.
  • LDAP enumeration cost -- collectEffectiveUserIds walks descendants via searchUsers(''); nesting a large LDAP group may block the request.

Test plan

  • tests/lib/Group/NestedGroupsTest.php -- transitive closure, diamond dedup, cycle rejection, idempotent add, event dispatch, cache invalidation, direct-child listing.
  • tests/lib/Group/DatabaseTest.php -- nested CRUD, self-edge rejection, cycle rejection, edge cleanup on group deletion.

Checklist

AI (if applicable)

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

Copy link
Copy Markdown
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Sorry, meant to comment already last week.
I'm having some fear with this change from security and app-ecosystem perspective.

There will be a huge mess if this is not dealt with correctly by each app.
So from my POV the existing API must reflect the state without any changes by calling parties, to ensure no additional/unexpected data/access whatever is leaked.

Server-side encryption

If apps/encryption is enabled, nested-group mutations may leave the
key distribution out of sync for effective members gained or lost past
the MAX_SYNTHESIZED_USER_EVENTS cap. This is not addressed
automatically; the admin must run a manual re-key pass after bulk
nesting changes on encrypted instances. A prominent warning in
nextcloud.log indicates when this is required.

E.g. this is not acceptable from my POV

Comment thread lib/private/Group/INestedGroupBackend.php Outdated
@KiaraGrouwstra KiaraGrouwstra force-pushed the recursive-groups-1 branch 3 times, most recently from e7cbb43 to f56acad Compare April 27, 2026 16:53
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@provokateurin provokateurin removed their request for review May 7, 2026 07:10
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants