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

Merged

qwc merged 1 commit from fix/sqlite-project-access-unique into main

2026-08-30 18:04:52 +02:00

qwc commented

2026-08-30 17:34:43 +02:00

Owner

Fixes #133. Independent of #132 — different files, either order merges cleanly.

The bug

Migration 002_auth_groups widens 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 effect:

DROP INDEX IF EXISTS idx_project_access_unique;   -- matches nothing

The constraint actually came from the table-level UNIQUE(project_id, user_id) declared inside CREATE TABLE in 001_initial. DROP INDEX cannot remove the auto-index behind an inline constraint (sqlite_autoindex_project_access_1), so the statement succeeded silently and both constraints stayed live.

SQLite is the default driver, so most deployments carry the wrong schema. Any second-source Grant fails:

UNIQUE constraint failed: project_access.project_id, project_access.user_id (2067)

The ON CONFLICT(project_id, user_id, source) upsert targets the other index, so the violation is never handled. Group mappings silently fail to apply to users who already hold a manual grant, and granting manual access to an LDAP-granted user fails the same way.

Tests first

All three tests were written against main and fail there with the exact production error:

internal/database/project_access_schema_test.go — TestProjectAccessUniqueOnSource reads PRAGMA index_list / index_info and asserts no unique index constrains (project_id, user_id) without source. On main: unique index "sqlite_autoindex_project_access_1" still constrains (project_id, user_id) without source.
internal/store/sql/access_source_test.go — TestGrantAllowsOneRowPerSource and TestRevokeBySourceLeavesOtherSources grant the same user from two sources. On main: UNIQUE constraint failed.
TestGrantUpdatesRoleWithinSource passes both before and after — it guards the upsert path the new key backs, so the rebuild can't silently turn re-grants into duplicates.

I re-ran the suite with the new SQLite migration files moved aside to confirm they go red without the fix and green with it.

The fix

011_project_access_source_unique rebuilds project_access without the inline constraint — the only way to drop one in SQLite — and recreates idx_project_access_source. Existing rows are already unique per (project_id, user_id), which is stricter than the target constraint, so every row carries over; no data is dropped or merged on the way up.

The down migration collapses multi-source rows back to one per user (keeping the manual grant where there is one) so the old constraint can hold again. TestProjectAccessSourceUniqueRoundTrip migrates down to 010 and back up, asserting both the collapse and that the constraint doesn't reappear.

On the other dialects

Postgres and MySQL dropped their old constraint correctly in 002, so nothing is broken there. Their 011 exists only to keep numbering aligned: Postgres runs an idempotent DROP CONSTRAINT IF EXISTS (a no-op on a correct database, a repair on a stale one), MySQL is a bare SELECT 1 since DROP INDEX has no IF EXISTS form to make a repair safe.

Both are trivial, but neither is exercised locally — the multi-dialect migration test is still blocked on CI Docker access (audit item H-12). If you'd rather not run untested SQL against Postgres/MySQL at all, say so and I'll make 011 SQLite-only and accept the numbering gap.

Assisted by Claude Opus 5.

Fixes #133. Independent of #132 — different files, either order merges cleanly. ## The bug Migration `002_auth_groups` widens 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 effect: ```sql DROP INDEX IF EXISTS idx_project_access_unique; -- matches nothing ``` The constraint actually came from the table-level `UNIQUE(project_id, user_id)` declared inside `CREATE TABLE` in `001_initial`. `DROP INDEX` cannot remove the auto-index behind an inline constraint (`sqlite_autoindex_project_access_1`), so the statement succeeded silently and both constraints stayed live. SQLite is the default driver, so most deployments carry the wrong schema. Any second-source `Grant` fails: ``` UNIQUE constraint failed: project_access.project_id, project_access.user_id (2067) ``` The `ON CONFLICT(project_id, user_id, source)` upsert targets the *other* index, so the violation is never handled. Group mappings silently fail to apply to users who already hold a manual grant, and granting manual access to an LDAP-granted user fails the same way. ## Tests first All three tests were written against `main` and fail there with the exact production error: - `internal/database/project_access_schema_test.go` — `TestProjectAccessUniqueOnSource` reads `PRAGMA index_list` / `index_info` and asserts no unique index constrains `(project_id, user_id)` without `source`. On `main`: `unique index "sqlite_autoindex_project_access_1" still constrains (project_id, user_id) without source`. - `internal/store/sql/access_source_test.go` — `TestGrantAllowsOneRowPerSource` and `TestRevokeBySourceLeavesOtherSources` grant the same user from two sources. On `main`: `UNIQUE constraint failed`. - `TestGrantUpdatesRoleWithinSource` passes both before and after — it guards the upsert path the new key backs, so the rebuild can't silently turn re-grants into duplicates. I re-ran the suite with the new SQLite migration files moved aside to confirm they go red without the fix and green with it. ## The fix `011_project_access_source_unique` rebuilds `project_access` without the inline constraint — the only way to drop one in SQLite — and recreates `idx_project_access_source`. Existing rows are already unique per `(project_id, user_id)`, which is stricter than the target constraint, so every row carries over; no data is dropped or merged on the way up. The down migration collapses multi-source rows back to one per user (keeping the manual grant where there is one) so the old constraint can hold again. `TestProjectAccessSourceUniqueRoundTrip` migrates down to 010 and back up, asserting both the collapse and that the constraint doesn't reappear. ## On the other dialects Postgres and MySQL dropped their old constraint correctly in 002, so nothing is broken there. Their `011` exists only to keep numbering aligned: Postgres runs an idempotent `DROP CONSTRAINT IF EXISTS` (a no-op on a correct database, a repair on a stale one), MySQL is a bare `SELECT 1` since `DROP INDEX` has no `IF EXISTS` form to make a repair safe. Both are trivial, but neither is exercised locally — the multi-dialect migration test is still blocked on CI Docker access (audit item H-12). If you'd rather not run untested SQL against Postgres/MySQL at all, say so and I'll make 011 SQLite-only and accept the numbering gap. --- Assisted by Claude Opus 5.

qwc added 1 commit

2026-08-30 17:34:43 +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>

qwc referenced this pull request

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

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

qwc merged commit e879729148 into main

2026-08-30 18:04:52 +02:00

qwc deleted branch fix/sqlite-project-access-unique

2026-08-30 18:04:52 +02:00

qwc referenced this pull request from a commit

2026-08-30 18:04:53 +02:00

Merge pull request 'Drop the leftover per-user unique constraint on SQLite (fixes #133)' (#134) from fix/sqlite-project-access-unique into main