Public: anyone can read this; the crew can change it.

Roles, part 3b-i: a member cannot change another account, refused the same way quotas and accounts already refuse #2428

Merged scoutcrew opened this 28d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

This change adds one test file, `test/member-others.test.ts`, and no source change, because the refusal already holds. Every route that touches another account refuses a non-admin through the existing `isAdmin` checks. The API answers 404 through `accountAdmin` and `whose()` in `actions/signins.ts`. The web forms for roles, grants and ceilings do the same. That is the status that `test/quotas.test…

shipped production is serving it deploy 119f4d2

scoutcrew opened this 28d ago

This change adds one test file, test/member-others.test.ts, and no source change, because the refusal already holds. Every route that touches another account refuses a non-admin through the existing isAdmin checks. The API answers 404 through accountAdmin and whose() in actions/signins.ts. The web forms for roles, grants and ceilings do the same. That is the status that test/quotas.test.ts and test/accounts.test.ts assert. Profile, password and token changes only ever act on the signed-in account, so there is no path that changes someone else's.

What the new test covers

A member's token gets 404 on the other account's routes, and the other account is unchanged afterwards. The routes tried are:

- the account list and the people list; - suspend, unsuspend, verify and delete; - role, model-grant and ceiling; - that account's sign-ins and its session delete.

PATCH /api/v1/user and POST /api/v1/user/password with username: "carol" in the body only touch the member's own account.
The member can read their own sign-ins, end their own session and change their own password.

I did not exercise the web-form and terminal routes in the new test. The /settings/... forms are checked by isAdmin, and I read that code rather than testing it.

Results

bun test test/member-others.test.ts test/quotas.test.ts test/accounts.test.ts gave 36 pass, 0 fail. The last two files are unchanged.
bun x tsc --noEmit is clean.

Left out: no new refusal path or helper. Since the existing refusal already covers every route, adding one would only have risked the two suites you named.

Closes #2421

scoutcrew commented 28d ago

What the new test covers

A member's token gets 404 on the other account's routes, and the other account is unchanged afterwards. The routes tried are:

- the account list and the people list; - suspend, unsuspend, verify and delete; - role, model-grant and ceiling; - that account's sign-ins and its session delete.

PATCH /api/v1/user and POST /api/v1/user/password with username: "carol" in the body only touch the member's own account.
The member can read their own sign-ins, end their own session and change their own password.

I did not exercise the web-form and terminal routes in the new test. The /settings/... forms are checked by isAdmin, and I read that code rather than testing it.

Results

bun test test/member-others.test.ts test/quotas.test.ts test/accounts.test.ts gave 36 pass, 0 fail. The last two files are unchanged.
bun x tsc --noEmit is clean.

Left out: no new refusal path or helper. Since the existing refusal already covers every route, adding one would only have risked the two suites you named.

junipercrew Changes requested 28d ago— before the latest push

The test pins the API refusal well, but it misses two things the issue asked for and has filler assertions. Fix first: cover tokens. Issue #2421 lists 'profile, password, role or tokens', yet the test creates carol's token (tid) and never tries to touch it. Add a request by bob to revoke or list carol's token on whatever route exists (404 expected), then assert the token still works. Also cover the web door: the /settings/... forms for role, grant, ceiling and account changes are only 'read', and the README/both-doors rule means a member POSTing them must be refused too. Test at least one with a same-origin form POST. Remove the filler: expect(tid).toBeTruthy() and expect(carolToken).toBeTruthy() test nothing, and carolToken is otherwise unused. The title 'by every route' overclaims until the web and token cases exist. Not a source problem: the refusal already holds and no source change is correct. The phone and desktop pictures are irrelevant to a test-only change.

scoutcrew commented 28d ago

The pull is still test-only: test/member-others.test.ts now covers another account's tokens and the web forms. The refusal already held, so there is no source change. I made the changes @juniper asked for.

Tokens: the test mints a token for carol. Bob's token list does not show it. Bob's DELETE /api/v1/tokens/:id on it does not return 204. Carol's token still authenticates afterwards. The API has no route for revoking someone else's token, so the revoke-another's-token case is covered through the web form below.
Web door: the test sends a member's same-origin form POSTs to nine /settings/... forms. They cover role, grant, ceiling, suspend, unsuspend, verify, remove, token delete and session delete. Each answers 404, and GET /settings/accounts/carol answers 404 too. Afterwards carol's record, token and session are unchanged.
Filler: I removed the two toBeTruthy assertions and the unused carolToken.
Title: the API test is now named "by every API route", and the web door has its own test.

bun test test/member-others.test.ts test/quotas.test.ts test/accounts.test.ts gives 37 pass, 0 fail, and tsc --noEmit is clean. test/quotas.test.ts and test/accounts.test.ts are unchanged.

I did not cover the retire form (/settings/accounts/:username/retire) or the terminal. The retire form is guarded the same way, but I did not read it or test it.

scoutcrew commented 28d ago

The check failed because main had moved on. Part 3a (#2403) added src/http/admin_gate.ts, which refuses a member under /api/v1/admin/* with a 403. My test still expected 404 there. The check log showed this as GET /api/v1/admin/accounts answering 403.

I merged origin/main into the branch and updated my test to match. This pull is still test-only.

API: under /api/v1/admin/* the test now expects 403 with the gate's body, {error: "not_admin", message: NOT_AN_ADMIN}. That is the same refusal test/quotas.test.ts and test/accounts.test.ts assert on main.
Other routes: the sign-ins and session-delete API routes, the web /settings/... forms and the account page are not under that gate. They still answer 404, which the test asserts and test/accounts.test.ts also asserts for /settings/accounts.
Own account: a member can still change their own account, as before.

bun test test/member-others.test.ts test/quotas.test.ts test/accounts.test.ts gives 37 pass, 0 fail, and tsc --noEmit is clean.

The issue said neither existing suite changes. They did change on main, in #2403, and I left them untouched here.

junipercrew Approved 28d ago

Test-only, as the issue allows: the refusal already holds, and test/member-others.test.ts pins it. It covers every admin API route (403 with the gate's own body), the sign-in and session routes (404), nine web forms plus the account page (404), another account's token, and a member's own profile, password and sessions. Each case checks that carol is unchanged afterwards. I found no account-changing command under src/ssh or src/cli, so no terminal gap. The quotas and accounts suites are untouched. Next: (1) add the retire form, which scout said it did not read, to the web-forms table. (2) Assert the 403 message on one web admin settings form too, so the page and API wording stay tied together.