refactor(bots): extract shared bot identity, share labels between sibling bots - #3637
Conversation
Preview DeploymentPreview deployments for this PR are available at:
The preview environment will start automatically on first visit. Subsequent pushes to this PR will update the Run locally with Dockerdocker pull ghcr.io/go-vikunja/vikunja:pr-3637
docker run -p 3456:3456 ghcr.io/go-vikunja/vikunja:pr-3637Last updated for commit 8748ac2 |
tink-bot
left a comment
There was a problem hiding this comment.
🤖 PR Swarm — automated review, round 1. See inline comments.
tink-bot
left a comment
There was a problem hiding this comment.
🤖 PR Swarm — automated review, round 1: deferred items needing your call.
tink-bot
left a comment
There was a problem hiding this comment.
🤖 PR Swarm — automated review, round 2. Re-reviewed round 1's fixes; one of them had swapped a blind spot rather than removing it.
tink-bot
left a comment
There was a problem hiding this comment.
🤖 PR Swarm — automated review, round 3 (convergence check). One finding, in a comment the earlier rounds approved.
|
🤖 PR Swarm — automated review. Panel: bughunter · security · conventions · design · verifier — round 5 @ Verdict: ✅ APPROVEMaintainer accepted sibling-label sharing for now. Docs PR #394 records owner-fleet label access and task/link-share visibility. No deferred swarm items remain. ❓ Needs your input (0)None. ✅ Fixed this round (1)
💤 Resolved as nit (0)None. 🔎 Runtime verification
Reviewer summaries
Previous rounds (4)round 4 @ |
tink-bot
left a comment
There was a problem hiding this comment.
🤖 PR Swarm — automated review, round 4. See inline comments.
IsBotOwnedBy and SameBotIdentityCond replace the ad-hoc bot ownership checks scattered across labels and api tokens.
Replaces the four hand-rolled IsBot() && BotOwnerID == caller checks in labels and api tokens. No behaviour change.
The label access condition covered owner-to-bot and bot-to-owner, so two bots of the same human could not use each other's labels: bot A seeded a label and bot B got a 403 on it. Replacing it with SameBotIdentityCond resolves every caller to an identity root - a bot's owner, or a human themselves - and matches everyone under that root, which covers all three directions in a single condition. Write access is unchanged: only the creator and the human owner can update or delete a label.
Both helpers compared a.GetID() against users.id, but *LinkSharing also satisfies web.Auth and its GetID() is a link_shares.id - a different id namespace. Passing one would have emitted users.id = <share id> and lent that share another user's identity, which is the confusion behind GHSA-vvcv-vpph-h844. Guarded at every call site, but the helpers moved out of models and into user, so the guard drifted away from the code needing it. Taking *User makes the case unrepresentable. Also fail closed on a zero id: humans store bot_owner_id = 0, so an unguarded zero would have matched all of them. The doc claimed JWT and CalDAV auth carry no BotOwnerID, but bots can reach neither path, and claimed bots-cannot-own-bots as an invariant when the guard is inert for JWT principals. Reworded to chain-safety by construction, which holds regardless.
4e3ae00 to
8748ac2
Compare
Follow-up to #3609, as agreed in review. "These two user rows are the same identity" was encoded in five places, each slightly differently and each picking its own direction. Extracted into one place, then rewired every site onto it.
Also closes the sibling-bot gap found while reviewing #3609 — see the behavior change below, it is the one non-mechanical part of this PR.
The helper
pkg/user/bot_identity.go, two forms because callers need both:Organising idea is the identity root: a bot's
bot_owner_id, or a human's own id. Two users share an identity when their roots match. Bots cannot own bots (CreateBotUser), so the root is at most one hop and no recursion is needed.The root is resolved in SQL, not Go —
hasAccessToLabelruns once per label insideTask.UpdateTaskLabels, and auth values built from a JWT or a CalDAV login carry noBotOwnerID, so the struct field cannot be trusted. Builder-only, no raw SQL.Rewired:
isLabelOwner,hasAccessToLabel,GetLabelsByTaskIDs,APIToken.CanDelete,APITokencreate/read-all,BotUser.isOwner. Each site got shorter;labelCreatedByBotIdentityCondis gone.At the two cond sites the separate
created_by_id = callerbranch also went away — the identity set contains the caller by construction, so keeping it encoded ownership twice.Behavior change: sibling bots
One human owning two bots meant bot A seeded a label and bot B got a 403 — the #3592 trap one hop over, reproduced against a running server during the #3609 review. The identity-root formulation covers owner→bot, bot→owner and bot→sibling in one expression, so this closes as a side effect of the refactor rather than as a fourth branch.
Reads and label use become symmetric across an owner's bot fleet. Writes stay directional on purpose:
isLabelOwnerkeepsIsBotOwnedBy)Still denied, with tests: a different owner's bot, an unrelated user, and link shares (which remain excluded from the identity branches while still reading labels through the attached-task branch).
Out of scope
saved_filters.go:97has the same gap — a bot cannot see filters its owner created. It becomess.Where(user.SameBotIdentityCond(auth, "owner_id"))once this lands, but changing filter visibility is a product decision, not a refactor, so it is left alone deliberately.users_project.gowas also left alone: its conds answer "may this bot row appear in search results", andnotSomeoneElsesBotdeliberately includes all non-bots, which an identity cond must never do. Forcing the helper in there would widen user search.How to verify
PUT /api/v1/user/bots), and an API token for each (PUT /api/v1/tokenswithowner_id).POST /api/v2/labels {"title":"shared"}→ note idN.GET /api/v2/labels, thenPOST /api/v2/tasks/{taskID}/labels {"label_id": N}.Expected: label
Nis listed, and the attach returns201.Before this PR: the label was absent from the listing and the attach returned
403.PUT /api/v2/labels/{N}. Expected:403— reads are shared, writes are not.GET /api/v2/labels/{N}. Expected:403.