qwc-open/asiakirjat

Watch

Fork 0

Make global access rules that name a user actually grant access #135

Merged

qwc merged 1 commit from fix/global-access-user-rules into main

2026-08-30 18:06:33 +02:00

qwc commented

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

Owner

Groundwork found while starting #125. Independent of #132 and #134 — different files, any merge order works.

The bug

Admin > Global Access accepts three subject types, and docs/how-to/manage-global-access.md already promises that naming a user is one of the ways in:

Does the global access list include the user directly?

It never worked. Grants are only ever written by the LDAP and OAuth2 login sync (auth/ldap.go:324, auth/oauth2.go:469), and both skip anything that isn't ldap_group / oauth2_group. access.Checker decides private-project access purely from GetGrantByUser, so a rule with subject_type='user' had no path to a grant and no path to the checker.

main.go looked like it covered this — there was a loop over the config rules under the comment "Resolve user-type rules into direct grants" — but the body was:

if rule.SubjectType == "user" {
    // ... we'll handle it via the admin UI and auth sync instead
    continue
}

The admin UI creates a rule and stops; the auth sync ignores user rules. So an admin adding "alice — viewer" (UI or access.private.viewers.users: [alice]) got a row in the table, a rule visible in the admin list, and alice still got 403 on every private project. Silent, and it looks exactly like a working configuration.

Tests first

internal/access/user_rule_test.go states the promise the docs make. Both fail on main:

--- FAIL: TestUserRuleGrantsPrivateAccess
    a user named in a global access rule should be able to view private projects
--- FAIL: TestUserRuleEditorCanUpload
    a user named as editor in a global access rule should be able to upload

The view test also asserts the negative — a user with neither rule nor grant still can't reach private projects — so it can't pass by making the checker permissive.

The fix

Materializing grants would need a resolution step in three places (rule creation, startup config sync, and again whenever a user is created later — a config rule can name someone who doesn't exist yet), each able to drift out of sync. So the checker matches user rules at check time instead, via one new store method GetUserRule (case-insensitive, matching how group rules are compared in the authenticators).

Grants and rules now resolve together in Checker.globalRole, with the stronger role winning if a user is covered by both. The three sites that each poked at the grant store — CanView, CanUpload, FilterAccessible — share it, so private-access policy lives in one function rather than three copies. That was the drift the access package was extracted to prevent.

main.go's dead loop is gone, replaced by a comment saying where user rules are handled.

Cost

One extra indexed lookup on private-project checks for users without a grant. FilterAccessible still does a single global lookup for the whole list, unchanged.

Full suite green. (go vet flags handler_test.go:1671 — pre-existing on main, untouched here.)

On #125

This is why the issue is worth doing carefully: named lists will have the same "a member can be a user or a group" question, and now there's one resolution path to extend rather than a broken one to copy. Next up is the schema for named lists, per your note that a list must be able to be an LDAP group, or an LDAP group plus extra users.

Assisted by Claude Opus 5.

Groundwork found while starting #125. Independent of #132 and #134 — different files, any merge order works. ## The bug **Admin > Global Access** accepts three subject types, and `docs/how-to/manage-global-access.md` already promises that naming a user is one of the ways in: > 2. Does the global access list include the user directly? It never worked. Grants are only ever written by the LDAP and OAuth2 login sync (`auth/ldap.go:324`, `auth/oauth2.go:469`), and both skip anything that isn't `ldap_group` / `oauth2_group`. `access.Checker` decides private-project access purely from `GetGrantByUser`, so a rule with `subject_type='user'` had no path to a grant and no path to the checker. `main.go` looked like it covered this — there was a loop over the config rules under the comment *"Resolve user-type rules into direct grants"* — but the body was: ```go if rule.SubjectType == "user" { // ... we'll handle it via the admin UI and auth sync instead continue } ``` The admin UI creates a rule and stops; the auth sync ignores user rules. So an admin adding "alice — viewer" (UI or `access.private.viewers.users: [alice]`) got a row in the table, a rule visible in the admin list, and alice still got 403 on every private project. Silent, and it looks exactly like a working configuration. ## Tests first `internal/access/user_rule_test.go` states the promise the docs make. Both fail on `main`: ``` --- FAIL: TestUserRuleGrantsPrivateAccess a user named in a global access rule should be able to view private projects --- FAIL: TestUserRuleEditorCanUpload a user named as editor in a global access rule should be able to upload ``` The view test also asserts the negative — a user with neither rule nor grant still can't reach private projects — so it can't pass by making the checker permissive. ## The fix Materializing grants would need a resolution step in three places (rule creation, startup config sync, and again whenever a user is created later — a config rule can name someone who doesn't exist yet), each able to drift out of sync. So the checker matches user rules at check time instead, via one new store method `GetUserRule` (case-insensitive, matching how group rules are compared in the authenticators). Grants and rules now resolve together in `Checker.globalRole`, with the stronger role winning if a user is covered by both. The three sites that each poked at the grant store — `CanView`, `CanUpload`, `FilterAccessible` — share it, so private-access policy lives in one function rather than three copies. That was the drift the `access` package was extracted to prevent. `main.go`'s dead loop is gone, replaced by a comment saying where user rules are handled. ## Cost One extra indexed lookup on private-project checks for users without a grant. `FilterAccessible` still does a single global lookup for the whole list, unchanged. Full suite green. (`go vet` flags `handler_test.go:1671` — pre-existing on `main`, untouched here.) ## On #125 This is why the issue is worth doing carefully: named lists will have the same "a member can be a user or a group" question, and now there's one resolution path to extend rather than a broken one to copy. Next up is the schema for named lists, per your note that a list must be able to be an LDAP group, or an LDAP group plus extra users. --- Assisted by Claude Opus 5.

qwc added 2 commits

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

Drop the leftover per-user unique constraint on SQLite (fixes #133 )

CI / test (pull_request) Successful in 1m20s

Details

CI / build (pull_request) Successful in 49s

Details

CI / docker (pull_request) Has been skipped

Details

58b78a8945

Migration 002 meant to widen uniqueness on project_access to include
source, so a user can hold a manual grant alongside a synced LDAP or
OAuth2 one. On SQLite it never took: the constraint came from the
table-level UNIQUE(project_id, user_id) in 001_initial, and DROP INDEX
cannot remove the auto-index behind an inline constraint, so the
statement matched nothing and succeeded silently.

The old constraint therefore stayed live on the default driver. Any
second-source Grant failed with "UNIQUE constraint failed:
project_access.project_id, project_access.user_id", so group mappings
never applied to users who already had a manual grant, and vice versa.

Migration 011 rebuilds the table without the inline constraint, which
is the only way to drop one in SQLite. Existing rows are already unique
per (project_id, user_id) — stricter than the target — so all of them
carry over. Postgres and MySQL dropped theirs correctly in 002; their
011 is an idempotent repair and a no-op respectively, kept only so the
numbering stays aligned across dialects.

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>

Make global access rules that name a user actually grant access

CI / test (pull_request) Successful in 1m24s

Details

CI / build (pull_request) Successful in 50s

Details

CI / docker (pull_request) Has been skipped

Details

ceeb594404

Admin > Global Access accepts three subject types, and the docs promise
that naming a user directly is one way to reach private projects. It
never worked: only ldap_group and oauth2_group rules were resolved into
grants, by the LDAP/OAuth2 login sync. Nothing resolved user rules.
main.go had a loop that looked like it did, but its body was `continue`
under a comment saying the admin UI and auth sync would handle it —
neither did. Adding a user by name, in the UI or via
access.private.*.users, silently granted nothing.

Rather than materialize grants (which would need a resolution step at
rule creation, at startup, and again whenever a user is created later),
the checker now matches user rules by username at check time. Grants and
rules are consulted together in one place, access.Checker.globalRole,
with the stronger role winning; the three call sites that each poked at
the grant store now share it.

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 merged commit a0af80341d into main

2026-08-30 18:06:33 +02:00

qwc deleted branch fix/global-access-user-rules

2026-08-30 18:06:33 +02:00

qwc referenced this pull request from a commit

2026-08-30 18:06:34 +02:00

Merge pull request 'Make global access rules that name a user actually grant access' (#135) from fix/global-access-user-rules into main

qwc referenced this pull request

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

Add schema and store for named access lists (#125) #136

qwc referenced this pull request

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