From 5f3cfe8bb713dfa7b222f77086290449c833af87 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 7 Oct 2026 18:25:16 -0400 Subject: [PATCH] groups: admin-managed groups, sharing a note with one, and membership in the feed /api/groups (admin) creates, renames and deletes groups and adds or removes members. The member directory lists every group, and a share may name a group_id instead of a user_id; a note's shares answer with `member` or `group`. A note shared with a group reaches whoever is in it now, so membership is what the feed follows: joining grants each of the group's notes to the new member's devices, and leaving (or the group being deleted) revokes them unless a direct share or another group still reaches that person. recipients() never counts the note's own owner, who may sit in a group it is shared with. Co-Authored-By: Claude Opus 5.5 --- docs/sync.md | 6 +- src/inkwell/app.py | 2 + src/inkwell/groups_api.py | 166 ++++++++++++++++++++++++++++++++++++++ src/inkwell/share_sync.py | 38 ++++++++- src/inkwell/shares_api.py | 98 +++++++++++++++------- tests/test_integration.py | 82 ++++++++++++++++++- 6 files changed, 358 insertions(+), 34 deletions(-) create mode 100644 src/inkwell/groups_api.py diff --git a/docs/sync.md b/docs/sync.md index c7c2233..86838c4 100644 --- a/docs/sync.md +++ b/docs/sync.md @@ -221,8 +221,10 @@ labels are personal, and a recipient files nothing under the owner's tags. Granting or changing a share moves the note to a new revision (without touching `updated_at`, so last-write-wins is unaffected), which is how it passes a -recipient's cursor. Ending a share — or the owner purging the note — adds the note -to the recipient's `revoked` list: +recipient's cursor. Joining a group a note is shared with does the same for the +new member (#5177). Ending a share — or the owner purging the note, the person +leaving the group, or the admin deleting it — adds the note to the `revoked` list of +each person who can no longer see it: ```json { "notes": [ ... ], "labels": [ ... ], "revoked": ["", ...], diff --git a/src/inkwell/app.py b/src/inkwell/app.py index 214ea23..6a438c1 100644 --- a/src/inkwell/app.py +++ b/src/inkwell/app.py @@ -14,6 +14,7 @@ from quart.sessions import SecureCookieSessionInterface from .accounts_api import bp as accounts_bp from .auth import bp as auth_bp from .client_dist import advertisement as client_advertisement, bp as client_bp +from .groups_api import bp as groups_bp from .config import Config from .db import session_scope from .invites_api import bp as invites_bp @@ -96,6 +97,7 @@ def create_app() -> Quart: app.register_blueprint(settings_bp) app.register_blueprint(invites_bp) app.register_blueprint(accounts_bp) + app.register_blueprint(groups_bp) app.register_blueprint(shares_bp) app.register_blueprint(sync_bp) app.register_blueprint(saved_filters_bp) diff --git a/src/inkwell/groups_api.py b/src/inkwell/groups_api.py new file mode 100644 index 0000000..ec7d880 --- /dev/null +++ b/src/inkwell/groups_api.py @@ -0,0 +1,166 @@ +"""Admin routes over groups: named sets of people a note can be shared with (#5177). + +A group is the instance's, not anyone's: the admin makes it and says who is in it, +and every member of the instance can share a note with it. Sharing with a group +reaches whoever is in it at the time, so adding or removing someone is what changes +their access, and `share_sync` tells their devices. +""" +from __future__ import annotations + +import uuid + +from quart import Blueprint, jsonify, request +from sqlalchemy import func, select +from sqlalchemy.dialects.postgresql import insert + +from .auth import require_admin +from .db import session_scope +from .models.group import Group, GroupMember +from .models.user import User +from .responses import json_error, not_found, parse_uuid +from .share_sync import bump_note, group_notes, joined_group, left_group, recipients, revoke + +bp = Blueprint("groups", __name__, url_prefix="/api/groups") + +NAME_CAP = 80 + + +def _name(data: dict) -> str | None: + name = (data.get("name") or "").strip() if isinstance(data.get("name"), str) else "" + return name[:NAME_CAP] or None + + +async def _name_taken(db, name: str, but: uuid.UUID | None = None) -> bool: + stmt = select(Group.id).where(func.lower(Group.name) == name.lower()) + if but is not None: + stmt = stmt.where(Group.id != but) + return await db.scalar(stmt) is not None + + +async def _serialize_groups(db, groups: list[Group]) -> list[dict]: + rows = ( + await db.execute( + select(GroupMember.group_id, User) + .join(User, User.id == GroupMember.user_id) + .where(GroupMember.group_id.in_([gr.id for gr in groups])) + .order_by(User.display_name, User.email) + ) + ).all() if groups else [] + members: dict = {} + for group_id, user in rows: + members.setdefault(group_id, []).append( + {"id": str(user.id), "display_name": user.display_name, "email": user.email} + ) + return [{"id": str(gr.id), "name": gr.name, "members": members.get(gr.id, [])} for gr in groups] + + +async def _answer(db, group: Group, status: int = 200): + return jsonify((await _serialize_groups(db, [group]))[0]), status + + +@bp.get("") +@require_admin +async def list_groups(): + async with session_scope() as db: + groups = (await db.scalars(select(Group).order_by(func.lower(Group.name)))).all() + return jsonify({"groups": await _serialize_groups(db, list(groups))}) + + +@bp.post("") +@require_admin +async def create_group(): + name = _name(await request.get_json(silent=True) or {}) + if name is None: + return json_error("give the group a name", 400) + async with session_scope() as db: + if await _name_taken(db, name): + return json_error("there is already a group with that name", 409) + group = Group(name=name) + db.add(group) + await db.commit() + return await _answer(db, group, 201) + + +@bp.patch("/") +@require_admin +async def rename_group(group_id: str): + name = _name(await request.get_json(silent=True) or {}) + if name is None: + return json_error("give the group a name", 400) + gid = parse_uuid(group_id) + async with session_scope() as db: + group = await db.get(Group, gid) if gid else None + if group is None: + return not_found() + if await _name_taken(db, name, but=group.id): + return json_error("there is already a group with that name", 409) + group.name = name + await db.commit() + return await _answer(db, group) + + +@bp.delete("/") +@require_admin +async def delete_group(group_id: str): + """Delete the group and every share made to it. Whoever could see a note only + through it loses the note, and their devices are told.""" + gid = parse_uuid(group_id) + async with session_scope() as db: + group = await db.get(Group, gid) if gid else None + if group is None: + return not_found() + note_ids = await group_notes(db, group.id) + before = {nid: await recipients(db, nid) for nid in note_ids} + await db.delete(group) # members and shares go with it (ON DELETE CASCADE) + await db.flush() + for nid in note_ids: + await revoke(db, nid, before[nid] - await recipients(db, nid)) + # The owner's devices learn whether the note is still shared at all. + await bump_note(db, nid) + await db.commit() + return jsonify({"ok": True}) + + +@bp.post("//members") +@require_admin +async def add_member(group_id: str): + data = await request.get_json(silent=True) or {} + gid = parse_uuid(group_id) + uid = parse_uuid(str(data.get("user_id") or "")) + async with session_scope() as db: + group = await db.get(Group, gid) if gid else None + if group is None: + return not_found() + if uid is None or await db.get(User, uid) is None: + return json_error("that person isn't on this instance", 400) + added = await db.execute( + insert(GroupMember) + .values(id=uuid.uuid4(), group_id=group.id, user_id=uid) + .on_conflict_do_nothing(constraint="uq_group_members_group_user") + ) + if added.rowcount: + await joined_group(db, group.id, uid) + await db.commit() + return await _answer(db, group, 201) + + +@bp.delete("//members/") +@require_admin +async def remove_member(group_id: str, user_id: str): + gid = parse_uuid(group_id) + uid = parse_uuid(user_id) + async with session_scope() as db: + group = await db.get(Group, gid) if gid else None + if group is None or uid is None: + return not_found() + membership = await db.scalar( + select(GroupMember).where(GroupMember.group_id == group.id, GroupMember.user_id == uid) + ) + if membership is None: + return not_found() + note_ids = await group_notes(db, group.id) + await db.delete(membership) + await db.flush() + await left_group(db, group.id, uid, note_ids) + await db.commit() + return await _answer(db, group) diff --git a/src/inkwell/share_sync.py b/src/inkwell/share_sync.py index fb0fa13..9d4451c 100644 --- a/src/inkwell/share_sync.py +++ b/src/inkwell/share_sync.py @@ -16,6 +16,7 @@ from sqlalchemy import select, text from sqlalchemy.dialects.postgresql import insert from .models.group import GroupMember +from .models.note import Note from .models.share import Share from .models.share_revocation import ShareRevocation @@ -31,7 +32,8 @@ async def bump_note(db, note_id: uuid.UUID) -> None: async def recipients(db, note_id: uuid.UUID) -> set[uuid.UUID]: - """Everyone the note is shared with, directly or through a group.""" + """Everyone the note is shared with, directly or through a group. Never its owner, + who may be in a group it is shared with and holds it as their own regardless.""" direct = await db.scalars( select(Share.shared_with_user_id).where( Share.resource_type == "note", Share.resource_id == note_id, Share.shared_with_user_id.is_not(None) @@ -42,7 +44,8 @@ async def recipients(db, note_id: uuid.UUID) -> set[uuid.UUID]: .join(Share, Share.shared_with_group_id == GroupMember.group_id) .where(Share.resource_type == "note", Share.resource_id == note_id) ) - return set(direct.all()) | set(grouped.all()) + owner = await db.scalar(select(Note.owner_id).where(Note.id == note_id)) + return (set(direct.all()) | set(grouped.all())) - {owner} async def revoke(db, note_id: uuid.UUID, user_ids) -> None: @@ -67,3 +70,34 @@ async def granted(db, note_id: uuid.UUID, user_id: uuid.UUID) -> None: sa_delete(ShareRevocation).where(ShareRevocation.note_id == note_id, ShareRevocation.user_id == user_id) ) await bump_note(db, note_id) + + +# --- Group membership (#5177) ------------------------------------------------ +# +# A note shared with a group reaches whoever is in it NOW, so joining or leaving +# changes what that one person can see without any share changing. Their devices +# hear it the same way as for a direct share: a grant moves the note past their +# cursor, and losing it leaves a revocation. + + +async def group_notes(db, group_id: uuid.UUID) -> list[uuid.UUID]: + """The notes shared with this group.""" + rows = await db.scalars( + select(Share.resource_id).where(Share.resource_type == "note", Share.shared_with_group_id == group_id) + ) + return list(rows.all()) + + +async def joined_group(db, group_id: uuid.UUID, user_id: uuid.UUID) -> None: + """`user_id` was added to the group: each of its notes reaches their devices.""" + for note_id in await group_notes(db, group_id): + await granted(db, note_id, user_id) + + +async def left_group(db, group_id: uuid.UUID, user_id: uuid.UUID, note_ids: list[uuid.UUID]) -> None: + """`user_id` is no longer in the group (the membership row is gone and flushed): + each of `note_ids`, the group's notes from before, leaves their devices unless a + direct share or another group still reaches them.""" + for note_id in note_ids: + if user_id not in await recipients(db, note_id): + await revoke(db, note_id, [user_id]) diff --git a/src/inkwell/shares_api.py b/src/inkwell/shares_api.py index c688507..ce44a11 100644 --- a/src/inkwell/shares_api.py +++ b/src/inkwell/shares_api.py @@ -2,7 +2,8 @@ `acl.visible_to_user` has gated every read since M0, but nothing wrote a share. This is the writing half: the note's owner lists, grants and revokes access, and anyone -signed in can read the member directory the Share dialog picks people from. +signed in can read the member directory the Share dialog picks people from. A share +goes to one person or to a group the admin made (#5177). What a share grants is in `acl` (view, or edit: the body and checklist) and is enforced by the note routes (`notes.helpers._get_owned` / `_get_editable`). @@ -12,13 +13,14 @@ from __future__ import annotations import uuid from quart import Blueprint, g, jsonify, request -from sqlalchemy import select +from sqlalchemy import func, select from sqlalchemy.dialects.postgresql import insert from .acl import PERMISSIONS from .auth import login_required from .common import iso from .db import session_scope +from .models.group import Group, GroupMember from .models.share import Share from .models.user import User from .notes.helpers import _get_owned @@ -32,32 +34,61 @@ def _member(user: User) -> dict: return {"id": str(user.id), "display_name": user.display_name, "email": user.email} +def _group(group: Group, counts: dict) -> dict: + return {"id": str(group.id), "name": group.name, "member_count": counts.get(group.id, 0)} + + +async def _member_counts(db, group_ids: list) -> dict: + if not group_ids: + return {} + rows = await db.execute( + select(GroupMember.group_id, func.count()) + .where(GroupMember.group_id.in_(group_ids)) + .group_by(GroupMember.group_id) + ) + return dict(rows.all()) + + @bp.get("/users/directory") @login_required async def directory(): - """The people on this instance a note can be shared with: everyone but you. + """Who on this instance a note can be shared with: everyone but you, and every + group the admin has made (#5177). - Signed-in only. Each entry is a name and an email and nothing more, which is what - choosing someone takes and all a fellow member needs to know.""" + Signed-in only. Each person is a name and an email and nothing more, which is + what choosing someone takes and all a fellow member needs to know; a group is its + name and how many people are in it.""" async with session_scope() as db: users = ( await db.scalars(select(User).where(User.id != g.user_id).order_by(User.display_name, User.email)) ).all() - return jsonify({"members": [_member(u) for u in users]}) + groups = (await db.scalars(select(Group).order_by(func.lower(Group.name)))).all() + counts = await _member_counts(db, [gr.id for gr in groups]) + return jsonify({"members": [_member(u) for u in users], "groups": [_group(gr, counts) for gr in groups]}) async def _shares_of(db, note_id: uuid.UUID) -> list[dict]: + """Each share of the note, to a person (`member`) or to a group (`group`); the + other of the two is null.""" rows = ( await db.execute( - select(Share, User) - .join(User, User.id == Share.shared_with_user_id) + select(Share, User, Group) + .outerjoin(User, User.id == Share.shared_with_user_id) + .outerjoin(Group, Group.id == Share.shared_with_group_id) .where(Share.resource_type == "note", Share.resource_id == note_id) .order_by(Share.created_at) ) ).all() + counts = await _member_counts(db, [gr.id for _, _, gr in rows if gr is not None]) return [ - {"id": str(share.id), "member": _member(user), "permission": share.permission, "created_at": iso(share.created_at)} - for share, user in rows + { + "id": str(share.id), + "member": _member(user) if user is not None else None, + "group": _group(group, counts) if group is not None else None, + "permission": share.permission, + "created_at": iso(share.created_at), + } + for share, user, group in rows ] @@ -74,39 +105,48 @@ async def list_shares(note_id: str): @bp.post("/notes//shares") @login_required async def share_note(note_id: str): - """Share the note with one member at `view` or `edit`. Sharing again with the same - person changes their permission rather than adding a second grant.""" + """Share the note at `view` or `edit` with one member (`user_id`) or with a group + (`group_id`). Sharing again with the same person or group changes the permission + rather than adding a second grant.""" data = await request.get_json(silent=True) or {} permission = data.get("permission") or "view" if permission not in PERMISSIONS: return json_error("permission must be view or edit", 400) - target = parse_uuid(str(data.get("user_id") or "")) - if target is None: - return json_error("choose someone to share with", 400) + user_id = parse_uuid(str(data.get("user_id") or "")) + group_id = parse_uuid(str(data.get("group_id") or "")) + if (user_id is None) == (group_id is None): + return json_error("choose someone or a group to share with", 400) async with session_scope() as db: note = await _get_owned(db, note_id) if note is None: return not_found() - if target == g.user_id: - return json_error("this note is already yours", 400) - if await db.get(User, target) is None: - return json_error("that person isn't on this instance", 400) + if user_id is not None: + if user_id == g.user_id: + return json_error("this note is already yours", 400) + if await db.get(User, user_id) is None: + return json_error("that person isn't on this instance", 400) + target_column, target = Share.shared_with_user_id, {"shared_with_user_id": user_id} + reached = {user_id} + else: + if await db.get(Group, group_id) is None: + return json_error("that group isn't on this instance", 400) + target_column, target = Share.shared_with_group_id, {"shared_with_group_id": group_id} + reached = set( + (await db.scalars(select(GroupMember.user_id).where(GroupMember.group_id == group_id))).all() + ) await db.execute( insert(Share) - .values( - id=uuid.uuid4(), - resource_type="note", - resource_id=note.id, - shared_with_user_id=target, - permission=permission, - ) + .values(id=uuid.uuid4(), resource_type="note", resource_id=note.id, permission=permission, **target) .on_conflict_do_update( - index_elements=[Share.resource_type, Share.resource_id, Share.shared_with_user_id], - index_where=Share.shared_with_user_id.is_not(None), + index_elements=[Share.resource_type, Share.resource_id, target_column], + index_where=target_column.is_not(None), set_={"permission": permission}, ) ) - await granted(db, note.id, target) + for uid in reached - {g.user_id}: + await granted(db, note.id, uid) + # The owner's devices learn the note is shared, even with an empty group. + await bump_note(db, note.id) await db.commit() return jsonify({"shares": await _shares_of(db, note.id)}), 201 diff --git a/tests/test_integration.py b/tests/test_integration.py index b79677c..efaf04f 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -51,7 +51,7 @@ pytestmark = pytest.mark.integration # Every table the tests touch, child-first so FKs never block the truncate. # RESTART IDENTITY + CASCADE keeps this honest if a table gains children later. -_TABLES = "notes, note_revisions, note_labels, note_link_previews, labels, shares, share_revocations, note_user_state, invites, password_resets, users" +_TABLES = "notes, note_revisions, note_labels, note_link_previews, labels, shares, share_revocations, note_user_state, group_members, groups, invites, password_resets, users" @pytest_asyncio.fixture @@ -1511,6 +1511,86 @@ async def test_deleting_a_shared_note_reaches_the_recipients_devices(app_client, assert _feed_note(gone, nid) is None +# --- Groups (#5177) ------------------------------------------------------------ + + +async def _group(app_client, name: str, *member_ids: str) -> str: + made = await app_client.post("/api/groups", json={"name": name}) + assert made.status_code == 201, await made.get_data(as_text=True) + gid = (await made.get_json())["id"] + for uid in member_ids: + added = await app_client.post(f"/api/groups/{gid}/members", json={"user_id": uid}) + assert added.status_code == 201, await added.get_data(as_text=True) + return gid + + +async def test_only_an_admin_manages_groups_and_everyone_can_share_with_one(app_client, db): + recipient, _, people = await _three_people(app_client) + assert (await recipient.get("/api/groups")).status_code == 403 + assert (await recipient.post("/api/groups", json={"name": "Mine"})).status_code == 403 + + gid = await _group(app_client, "Family", people["recipient"]) + assert (await app_client.post("/api/groups", json={"name": "family"})).status_code == 409 + assert (await app_client.post("/api/groups", json={"name": " "})).status_code == 400 + listed = (await (await app_client.get("/api/groups")).get_json())["groups"] + assert [(gr["name"], [m["email"] for m in gr["members"]]) for gr in listed] == [ + ("Family", ["recipient@example.test"]) + ] + renamed = await app_client.patch(f"/api/groups/{gid}", json={"name": "Household"}) + assert (await renamed.get_json())["name"] == "Household" + + # Anyone signed in sees each group to share with, and how many are in it. + directory = await (await recipient.get("/api/users/directory")).get_json() + assert directory["groups"] == [{"id": gid, "name": "Household", "member_count": 1}] + + +async def test_a_note_shared_with_a_group_follows_who_is_in_it(app_client, db): + recipient, stranger, people = await _three_people(app_client) + gid = await _group(app_client, "Family", people["recipient"]) + nid = await _owners_note(app_client) + start = (await _feed(stranger))["cursor"] + + shared = await app_client.post(f"/api/notes/{nid}/shares", json={"group_id": gid, "permission": "edit"}) + assert shared.status_code == 201, await shared.get_data(as_text=True) + [entry] = (await shared.get_json())["shares"] + assert (entry["member"], entry["group"]["name"], entry["group"]["member_count"]) == (None, "Family", 1) + assert (await (await recipient.get(f"/api/notes/{nid}")).get_json())["permission"] == "edit" + assert (await stranger.get(f"/api/notes/{nid}")).status_code == 404 + both = await app_client.post(f"/api/notes/{nid}/shares", json={"group_id": gid, "user_id": people["stranger"]}) + assert both.status_code == 400 + + # Joining reaches the new member's devices; leaving takes it back. + await app_client.post(f"/api/groups/{gid}/members", json={"user_id": people["stranger"]}) + joined = await _feed(stranger, start) + assert _feed_note(joined, nid)["permission"] == "edit" + left = await app_client.delete(f"/api/groups/{gid}/members/{people['stranger']}") + assert left.status_code == 200 + gone = await _feed(stranger, joined["cursor"]) + assert gone["revoked"] == [nid] + assert (await stranger.get(f"/api/notes/{nid}")).status_code == 404 + + # Someone also shared with directly keeps the note when they leave the group. + await _share(app_client, nid, people["recipient"], "view") + before = (await _feed(recipient))["cursor"] + await app_client.delete(f"/api/groups/{gid}/members/{people['recipient']}") + assert (await _feed(recipient, before))["revoked"] == [] + assert (await (await recipient.get(f"/api/notes/{nid}")).get_json())["permission"] == "view" + + +async def test_deleting_a_group_ends_every_share_made_to_it(app_client, db): + recipient, _, people = await _three_people(app_client) + gid = await _group(app_client, "Family", people["recipient"]) + nid = await _owners_note(app_client) + await app_client.post(f"/api/notes/{nid}/shares", json={"group_id": gid}) + seen = await _feed(recipient) + assert _feed_note(seen, nid) is not None + + assert (await app_client.delete(f"/api/groups/{gid}")).status_code == 200 + assert (await _feed(recipient, seen["cursor"]))["revoked"] == [nid] + assert (await recipient.get(f"/api/notes/{nid}")).status_code == 404 + assert (await (await app_client.get(f"/api/notes/{nid}")).get_json())["shared"] is False + + # --- Password reset by email (#5266) ------------------------------------------ # # The SMTP hand-off is replaced by a list; everything up to it is real.