Resolve named access lists in the access checker (#125) #137

Merged

qwc merged 1 commit from feature/access-list-resolution into main

2026-08-30 18:26:41 +02:00

qwc commented

2026-08-30 18:16:11 +02:00

Owner

Second of the #125 series. Stacked on #136 — the base branch is feature/named-access-lists, so this diff shows only the resolution work. Merge #136 first and this retargets to main cleanly.

How a list admits someone

Two routes in, mirroring the split global_access already uses:

Member	Resolved	Why
`user`	by username, at check time	Applies immediately, and to users created after the list
`ldap_group` / `oauth2_group`	via `access_list_grants`, written at login	Group membership is only known while signing in — it isn't persisted anywhere

The stronger of the two wins if someone is admitted both ways. Migration 013 adds access_list_grants (unique per list + user + source), which the login sync will populate in the next PR.

I checked whether group membership could be resolved at check time instead, which would avoid the table entirely — it can't: users stores no group columns, and memberOf is computed during LDAP bind and discarded. So the grant table is the same compromise #135 landed on, not a new mechanism.

Fail-closed cases

visibility = 'list' with no list pointer admits nobody (admins excepted, as everywhere). The list is the only thing that could let anyone in, so a missing pointer is not a reason to fall back to something broader. TestListProjectWithoutListFailsClosed pins it.
A per-project grant still counts on a list project, matching how private behaves — naming someone on the project itself is more specific than the list, never less.

Drift protection

CanView, CanUpload and FilterAccessible all resolve through the same RolesForUser lookup. TestFilterAccessibleIncludesListProjects asserts the list view and the per-project check agree on the same two projects — that drift is the bug this package was extracted to prevent, so it seemed worth a test rather than a convention.

FilterAccessible does one lookup for all of a user's lists, not one per project, so long project lists don't get slower.

Visibility validation

Moved into projects.ValidateVisibility(visibility, accessListID, creator) so the HTTP layer can apply the same rule in the next PR instead of re-deriving it — admin.go:236 currently has its own copy of the three-literal check. list visibility requires a list id; public stays admin-only; unknown values are still rejected.

Tests

internal/access/access_list_test.go covers the issue's shape directly — a list holding an LDAP group plus two named users with different roles — plus grant-based admission and revocation when the grant is dropped, the fail-closed orphan, and the filter/check agreement. internal/projects/service_test.go covers the validation rule.

Full suite green.

Still to come

Step 3 — LDAP/OAuth2 login sync writes and revokes list grants, so the group half of a list actually fills in.
Step 4 — admin UI for managing lists and members, the visibility picker, and docs.

Until step 4, visibility = 'list' still can't be set through the UI, so merging this changes no behaviour on its own.

Assisted by Claude Opus 5.

Second of the #125 series. **Stacked on #136** — the base branch is `feature/named-access-lists`, so this diff shows only the resolution work. Merge #136 first and this retargets to `main` cleanly. ## How a list admits someone Two routes in, mirroring the split `global_access` already uses: | Member | Resolved | Why | |---|---|---| | `user` | by username, at check time | Applies immediately, and to users created after the list | | `ldap_group` / `oauth2_group` | via `access_list_grants`, written at login | Group membership is only known while signing in — it isn't persisted anywhere | The stronger of the two wins if someone is admitted both ways. Migration 013 adds `access_list_grants` (unique per list + user + source), which the login sync will populate in the next PR. I checked whether group membership could be resolved at check time instead, which would avoid the table entirely — it can't: `users` stores no group columns, and `memberOf` is computed during LDAP bind and discarded. So the grant table is the same compromise #135 landed on, not a new mechanism. ## Fail-closed cases - **`visibility = 'list'` with no list pointer admits nobody** (admins excepted, as everywhere). The list is the only thing that could let anyone in, so a missing pointer is not a reason to fall back to something broader. `TestListProjectWithoutListFailsClosed` pins it. - **A per-project grant still counts** on a list project, matching how `private` behaves — naming someone on the project itself is more specific than the list, never less. ## Drift protection `CanView`, `CanUpload` and `FilterAccessible` all resolve through the same `RolesForUser` lookup. `TestFilterAccessibleIncludesListProjects` asserts the list view and the per-project check agree on the same two projects — that drift is the bug this package was extracted to prevent, so it seemed worth a test rather than a convention. `FilterAccessible` does one lookup for all of a user's lists, not one per project, so long project lists don't get slower. ## Visibility validation Moved into `projects.ValidateVisibility(visibility, accessListID, creator)` so the HTTP layer can apply the same rule in the next PR instead of re-deriving it — `admin.go:236` currently has its own copy of the three-literal check. `list` visibility requires a list id; public stays admin-only; unknown values are still rejected. ## Tests `internal/access/access_list_test.go` covers the issue's shape directly — a list holding an LDAP group plus two named users with different roles — plus grant-based admission and revocation when the grant is dropped, the fail-closed orphan, and the filter/check agreement. `internal/projects/service_test.go` covers the validation rule. Full suite green. ## Still to come **Step 3** — LDAP/OAuth2 login sync writes and revokes list grants, so the group half of a list actually fills in. **Step 4** — admin UI for managing lists and members, the visibility picker, and docs. Until step 4, `visibility = 'list'` still can't be set through the UI, so merging this changes no behaviour on its own. --- Assisted by Claude Opus 5.

qwc added 1 commit

2026-08-30 18:16:11 +02:00

Resolve named access lists in the access checker (#125 ) 25ee0ec74a

Second step of named access lists: teach the checker to decide access
for projects whose visibility is 'list'.

A list member that names a user is matched by username when access is
checked, so it applies immediately and to users created later. A member
that names an LDAP or OAuth2 group cannot be: group membership is only
known while the user signs in and is not persisted, so migration 013
adds access_list_grants for the login sync to write. That is the same
split global_access already uses, and the stronger of the two routes
wins.

CanView, CanUpload and FilterAccessible all go through it, so the list
view and the per-project check cannot drift. A project whose visibility
is 'list' but whose list pointer is missing admits nobody: the list is
the only thing that could let anyone in.

Visibility validation moves into projects.ValidateVisibility so the
HTTP layer can apply the same rule instead of re-deriving it, and
list visibility requires the list it points at.

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 changed target branch from feature/named-access-lists to main

2026-08-30 18:26:15 +02:00

qwc merged commit 94a525d0dd into main

2026-08-30 18:26:41 +02:00

qwc referenced this pull request from a commit

2026-08-30 18:26:41 +02:00

Merge pull request 'Resolve named access lists in the access checker (#125)' (#137) from feature/access-list-resolution into main

qwc deleted branch feature/access-list-resolution

2026-08-30 18:26:41 +02:00

qwc referenced this pull request

2026-08-30 18:30:32 +02:00

Resolve access list group membership at login (#125) #138