docs(slice1): row 35 SR round 1 review record (darkwing)
Verdict changes, #1517 comment 26709. Gitea 1.27.1 and Vikunja 2.7.0 source checks for Sage's three questions; two mint defects. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,181 @@
|
||||
# Queue row 35 (SR), round 1 review (#1517)
|
||||
|
||||
Darkwing, 2026-10-04. Request: #1517 comment 26707. Candidate: commit
|
||||
c9c1699a, `docs/guides/slice-1-identities.md`. Brief:
|
||||
`docs/plans/2026-10-04_slice-1.md`, row SR.
|
||||
|
||||
Verdict: changes. The scope tables are right, and so are Sage's three
|
||||
assumptions, with one correction to the push claim. Two command defects
|
||||
need fixing before Jason runs the guide: the `mint` error guidance can't
|
||||
be seen, and a missing token field writes the word `null` into a token
|
||||
file that then passes the section 4 check. The rest are small text
|
||||
changes.
|
||||
|
||||
I read the source and didn't run anything live. Gitea's version endpoint
|
||||
on our instance reports 1.27.1, so I read tag v1.27.1. Vikunja is tag
|
||||
v2.7.0, the version the guide pins. Line numbers below are from those
|
||||
tags.
|
||||
|
||||
## Sage's three questions
|
||||
|
||||
### 1. Reviewer-bot: `write:repository` plus Read access
|
||||
|
||||
The scope half is right. In `routers/api/v1/api.go` the `/repos` group
|
||||
that holds `POST /pulls`, `POST /pulls/{index}/reviews` and
|
||||
`POST .../reviews/{id}` closes at line 1548 with
|
||||
`tokenRequiresScopes(AccessTokenScopeCategoryRepository)`. Issue
|
||||
comments are in the group closing at line 1684 with the issue category.
|
||||
`tokenRequiresScopes` (line 323) asks for the write level on POST, PUT,
|
||||
PATCH and DELETE. So a review needs `write:repository`, and a comment
|
||||
needs `write:issue`.
|
||||
|
||||
A Read collaborator can create and submit all three review types. The
|
||||
`/pulls` group applies `mustAllowPulls`, `reqRepoReader(unit.TypeCode)`
|
||||
and `reqToken()`, and no writer check. The handler refuses only an
|
||||
approval or rejection of your own PR (`pull_review.go:688-699`).
|
||||
|
||||
The push half needs a correction. A branch or tag push is refused, but
|
||||
not by the HTTP permission check. For receive-pack,
|
||||
`routers/web/repo/githttp.go` drops the required access to Read when git
|
||||
supports proc-receive (lines 184-187). The refusal comes later, in the
|
||||
pre-receive hook: `assertCanWriteRef` in
|
||||
`routers/private/hook_pre_receive.go` needs code write and answers 403.
|
||||
The exception is `refs/for/<branch>`, the AGit flow. It needs only read
|
||||
access to pull requests (`CanCreatePullRequest`, line 88). With this
|
||||
token, reviewer-bot can push to `refs/for/next` and open a pull request
|
||||
with any content it likes.
|
||||
|
||||
That doesn't break slice 1, because the broker holds the token and has
|
||||
no action that runs a git push for the reviewer role. But "Read access
|
||||
is what stops it pushing" overstates it. Suggested text for lines 69-72:
|
||||
|
||||
> Reviewer-bot's `write:repository` is there because Gitea checks pull
|
||||
> request reviews against the repository scope. Its Read collaborator
|
||||
> access stops pushes to branches and tags. It does not stop an AGit
|
||||
> push to `refs/for/<branch>`, which opens a pull request. That's
|
||||
> acceptable only because the broker holds the token and offers the
|
||||
> reviewer no push action.
|
||||
|
||||
One more thing worth a line. Gitea counts a review toward required
|
||||
approvals only when it's official, and a Read user's review isn't
|
||||
official by default (`IsOfficialReviewer`, `models/issues/review.go:276`;
|
||||
`IsUserOfficialReviewer`, `models/git/protected_branch.go:234`). The
|
||||
exception is a protected branch with the approvals whitelist turned on
|
||||
and reviewer-bot on it. If a protection rule on `next` requires
|
||||
approvals and reviewer-bot's verdict should count, the guide has to say
|
||||
to whitelist it. If merges stay Jason's and approvals are advisory, say
|
||||
that instead.
|
||||
|
||||
### 2. Vikunja `/api/v2/login` returns `token`
|
||||
|
||||
Confirmed. `pkg/routes/api/v2/auth_login.go`, `authLogin` (lines
|
||||
87-106), returns the body type `authTokenBody` (line 43), whose JWT
|
||||
field is `token`. The body also carries a `$schema` link, because
|
||||
`huma.go:74` uses `huma.DefaultConfig`. The script ignores extra keys,
|
||||
so that's harmless. The route exists only when local or LDAP login is
|
||||
enabled (line 63), which section 2 already requires. Keep the stop for a
|
||||
missing field, but replace "assumed from v1" at line 138 with "confirmed
|
||||
in the v2.7.0 source (`auth_login.go`)".
|
||||
|
||||
### 3. Bot names `bot-<business>-<role>`
|
||||
|
||||
Keep them. Vikunja usernames are global to the instance, and the guide's
|
||||
reason (two businesses on one instance) holds. Amend the PRD's REQ-CRED-1
|
||||
wording from `bot-<role>` to match. The `bot-` prefix itself is
|
||||
Vikunja's rule for bot accounts.
|
||||
|
||||
The Gitea bot names have the same problem and the guide doesn't address
|
||||
it. `pm-bot`, `cto-bot`, `coder-bot` and `reviewer-bot` are global
|
||||
Gitea users, so a second business on the same Gitea can't reuse them.
|
||||
Either name them `<business>-pm-bot` and so on, or state that one set of
|
||||
Gitea bots serves every business on the instance. I'd take the first;
|
||||
it keeps a business's revocation from touching another business.
|
||||
|
||||
## Defects in the commands
|
||||
|
||||
### D1. `mint` hides the error it tells you to read (lines 164, 216, 228-230)
|
||||
|
||||
`api` uses `curl -sf`. With `-f`, an HTTP error status makes curl exit
|
||||
22 and write nothing, so the 400 body with code 14002 never appears.
|
||||
And `rm -f "$resp"` runs after the failed chain, so even
|
||||
`--fail-with-body` would lose it. Suggested `mint` body:
|
||||
|
||||
```sh
|
||||
mint() { # mint ROLE BOT_ID SCOPES_FILE
|
||||
local r=$1 id=$2 sc=$3 resp="$S/.mint-$1.json"
|
||||
jq -n --arg t "$BIZ-$r-$(date +%F)" --argjson o "$id" --arg e "$EXP" --slurpfile p "$sc" \
|
||||
'{title:$t, owner_id:$o, expires_at:$e, permissions:$p[0]}' |
|
||||
curl -s --fail-with-body -H @"$S/vikunja-owner.hdr" -H 'Content-Type: application/json' \
|
||||
-X POST "$VK/api/v2/tokens" --data-binary @- -o "$resp" ||
|
||||
{ jq -c '{code, message}' "$resp"; rm -f "$resp"; return 1; }
|
||||
jq -je '.token | strings' "$resp" > "$S/$r-vikunja.token" &&
|
||||
jq -c '{id, owner_id, expires_at, starts_tk: (.token | startswith("tk_"))}' "$resp"
|
||||
rm -f "$resp"
|
||||
}
|
||||
```
|
||||
|
||||
An error body carries no token, so printing `code` and `message` is
|
||||
safe. `--fail-with-body` needs curl 7.76 or later.
|
||||
|
||||
### D2. A missing `token` field writes `null` (line 217)
|
||||
|
||||
`jq -j .token` on a body without `token` prints `null` and exits 0. I
|
||||
checked: `echo '{}' | jq -j .token` gives `null`, exit 0. The file is
|
||||
then 0600 with size 4, and section 4's "600 and a nonzero size" passes.
|
||||
`jq -je '.token | strings'` prints nothing and exits 4, so the chain
|
||||
stops. That's the change in D1.
|
||||
|
||||
## Smaller changes
|
||||
|
||||
1. Gitea tokens, step 4 (line 73). "Copy with an editor" can leave a
|
||||
swap or backup file next to the token, in `$S` or wherever the editor
|
||||
keeps them. Suggest instead, in the same `umask 077` shell:
|
||||
`read -rs t && printf %s "$t" > "$S/pm-gitea.token"; unset t`.
|
||||
`read` and `printf` are builtins, so the value doesn't reach `ps` or
|
||||
the history. Do the same for the Gitea rotation in section 5.
|
||||
2. Trailing newlines. That step also says "one line". An editor adds a
|
||||
newline, `printf %s` doesn't. Row S3's broker will trim one trailing
|
||||
newline either way; I'll put that in S3.
|
||||
3. `EXP` is a timestamp (line 32), and the business file's `expires` is
|
||||
`YYYY-MM-DD` (row S1 validates that). Say to write the date part of
|
||||
`EXP`. The broker treats 00:00Z on that date as the expiry, which is
|
||||
the same instant `EXP` names.
|
||||
4. The labels template. Section 3 step 4 refers to "the labels the
|
||||
business file template lists". Row SR's brief owns
|
||||
`templates/business/mosaic-stack.example.json`, and it isn't in the
|
||||
candidate. Either add it or list the labels in the guide. S1 will
|
||||
send the business file example the template should follow.
|
||||
5. Path B (lines 113-115). The binary is `/app/vikunja/vikunja`, as the
|
||||
guide says (Dockerfile lines 49-50). `user create` without `-p`
|
||||
prompts through `term.ReadPassword` (`pkg/cmd/user.go:151`). Without
|
||||
a TTY it exits through `log.Fatalf`, so the guide's `-it` matters.
|
||||
Replace "If your build doesn't prompt, stop" with "Keep `-it`; without
|
||||
a terminal the command exits instead of prompting."
|
||||
6. Path B, `--user "$(id -u):$(id -g)"`. Right. The image runs as uid
|
||||
1000, but nothing at `/db` or `/app/vikunja/files` has to belong to
|
||||
1000. Both mounts are required, because `/app/vikunja` itself isn't
|
||||
writable to another uid and Vikunja writes a test file into the files
|
||||
directory at startup. Worth one sentence so nobody drops a mount.
|
||||
7. Rotation, Vikunja. The owner can delete a bot's token: the v2.7.0
|
||||
route's description says so, and `APIToken.CanDelete`
|
||||
(`pkg/models/api_tokens_permissions.go:25`) checks for it. The
|
||||
rotation step logs in again, which recreates
|
||||
`$S/vikunja-owner.hdr`, and `api` and `mint` were defined in another
|
||||
shell. Say to rerun section 0, the owner login and the two function
|
||||
definitions, and to finish with section 4's `rm -f`.
|
||||
|
||||
## What I checked and found right
|
||||
|
||||
- The scope JSON in section 3 matches addendum B section 2 byte for
|
||||
byte: sync, pm and the shared worker file.
|
||||
- Shares: role bots write (1), sync read (0).
|
||||
- The secret handling: `umask 077` up front, the service secret through
|
||||
`printf` and command substitution, the owner password through
|
||||
`getpass`, the header passed as `-H @file`, the header file deleted in
|
||||
section 4, and `stat` rather than `cat`. Nine token files is right:
|
||||
four Gitea, five Vikunja.
|
||||
- The Gitea scope table. pm-bot needs only `read:repository`, since its
|
||||
labels, assignees and closes go through issue routes. The three
|
||||
workers need `write:repository` for reviews and pull requests.
|
||||
- The revocation paths in section 5 end in a 401 or 403, which the
|
||||
broker refuses rather than retries.
|
||||
Reference in New Issue
Block a user