Constrain access roles and sources at store entry (audit L-1, L-7) #145

Merged

qwc merged 1 commit from fix/constrain-access-roles into main

2026-08-30 20:42:47 +02:00

qwc commented

2026-08-30 20:20:56 +02:00

Owner

Audit items L-1 and L-7, done together as the audit suggested — they're the same job seen from two sides. Independent of #144.

What was loose

Every path that writes an access role already coerces it to viewer or editor (admin.go:395, admin.go:980, main.go:287), and every path that writes a source sets one explicitly. But the stores trusted their callers and the column default:

ProjectAccessStore.Grant defaulted an empty source to manual and validated nothing else.
GlobalAccessStore.CreateRule / UpsertGrant took any role and any subject type.
AuthGroupMappingStore.Create / Update took any role — and a mapping's role is what reaches project_access on every LDAP/OAuth2 login.

So the invariant held only by convention, in four handlers, and a future caller reintroducing a typo would write a row that nothing matches and no one can see — the silent-no-op shape that produced #126 and #133.

What changed

All of those reject an unrecognised role, source or subject type outright, using the validators that already exist (database.ValidAccessRole, ValidAccessSource, ValidSubjectType — added incidentally with #132 and #125, which is what made these items cheap now).

With the values constrained, the admin branches in the read paths are dead — an access rule confers viewer or editor, while "admin" describes a user. CanUpload, GetEffectiveRole, roleRank and the role-priority SQL all drop it. user.Role == "admin" is untouched; that's the legitimate one.

The migration matters here

Migration 015 normalises any existing role = 'admin' row to editor across project_access, global_access, global_access_grants and auth_group_mappings.

Without it, dropping the read-side branches would be a silent access revocation on any deployment that has such a row rather than a no-op — exactly the class of bug this audit item is meant to close. The down migration is a no-op and says why: a normalised row is indistinguishable from an editor that was always an editor.

Tests

internal/store/sql/access_role_validation_test.go — each store rejecting admin, an unknown role, an empty role, an unknown source, an unknown subject type; and the valid combinations still working, including an empty source still defaulting to manual.
internal/database/normalize_roles_test.go — inserts admin rows directly (the stores now refuse them), migrates down and back up, and asserts all four tables come out as editors.

Full suite green, including the existing checker tests that pin who can upload.

Assisted by Claude Opus 5.

Audit items **L-1** and **L-7**, done together as the audit suggested — they're the same job seen from two sides. Independent of #144. ## What was loose Every path that writes an access role already coerces it to viewer or editor (`admin.go:395`, `admin.go:980`, `main.go:287`), and every path that writes a source sets one explicitly. But the *stores* trusted their callers and the column default: - `ProjectAccessStore.Grant` defaulted an empty source to `manual` and validated nothing else. - `GlobalAccessStore.CreateRule` / `UpsertGrant` took any role and any subject type. - `AuthGroupMappingStore.Create` / `Update` took any role — and a mapping's role is what reaches `project_access` on every LDAP/OAuth2 login. So the invariant held only by convention, in four handlers, and a future caller reintroducing a typo would write a row that nothing matches and no one can see — the silent-no-op shape that produced #126 and #133. ## What changed All of those reject an unrecognised role, source or subject type outright, using the validators that already exist (`database.ValidAccessRole`, `ValidAccessSource`, `ValidSubjectType` — added incidentally with #132 and #125, which is what made these items cheap now). With the values constrained, the `admin` branches in the read paths are dead — an access rule confers viewer or editor, while "admin" describes a *user*. `CanUpload`, `GetEffectiveRole`, `roleRank` and the role-priority SQL all drop it. `user.Role == "admin"` is untouched; that's the legitimate one. ## The migration matters here Migration 015 normalises any existing `role = 'admin'` row to `editor` across `project_access`, `global_access`, `global_access_grants` and `auth_group_mappings`. Without it, dropping the read-side branches would be a **silent access revocation** on any deployment that has such a row rather than a no-op — exactly the class of bug this audit item is meant to close. The down migration is a no-op and says why: a normalised row is indistinguishable from an editor that was always an editor. ## Tests - `internal/store/sql/access_role_validation_test.go` — each store rejecting `admin`, an unknown role, an empty role, an unknown source, an unknown subject type; and the valid combinations still working, including an empty source still defaulting to `manual`. - `internal/database/normalize_roles_test.go` — inserts `admin` rows directly (the stores now refuse them), migrates down and back up, and asserts all four tables come out as editors. Full suite green, including the existing checker tests that pin who can upload. --- Assisted by Claude Opus 5.

qwc added 1 commit

2026-08-30 20:20:57 +02:00

Constrain access roles and sources at store entry (audit L-1, L-7)

CI / test (pull_request) Successful in 1m23s

Details

CI / build (pull_request) Successful in 49s

Details

CI / docker (pull_request) Has been skipped

Details

93546dcde6

Every path that writes an access role already coerced it to viewer or
editor, and every path that writes a source set it explicitly — but the
stores trusted their callers and the column default, so a future caller
could write a row that nothing matches and no one can see. They now
reject an unrecognised role, source or subject type outright:
project_access grants, global access rules and grants, and group
mappings.

With the values constrained, the 'admin' branches the read paths carried
are dead: an access rule confers viewer or editor, while "admin"
describes a user. CanUpload, GetEffectiveRole, roleRank and the
role-priority SQL drop it.

Migration 015 normalises any 'admin' row already in the database to
'editor' first. Without that, dropping the read-side branches would
revoke access from an existing deployment instead of being a no-op.

Assisted-by: Claude Opus 5

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Marcel M. Otte <marcel.otte@mmo.to>

qwc referenced this pull request

2026-08-30 20:24:02 +02:00

Stop putting raw errors on screen (audit L-4) #146

qwc referenced this pull request

2026-08-30 20:26:24 +02:00

Stream non-HTML responses past the overlay injector (audit L-5) #147

qwc merged commit 6cb2e56a62 into main

2026-08-30 20:42:47 +02:00

qwc deleted branch fix/constrain-access-roles

2026-08-30 20:42:47 +02:00

qwc referenced this pull request from a commit

2026-08-30 20:42:49 +02:00