feat(auth): External identity providers (Google/Microsoft/GitHub/ORCID) + RLS enforcement fix - #212
Open
KinshukSS2 wants to merge 47 commits into
Open
KinshukSS2 wants to merge 47 commits into
KinshukSS2 wants to merge 47 commits into
Conversation
Closes Issue istSOS#28 — eliminates two-step user provisioning. After POST /Users, a newly created user had no RLS policy and could not access any data until an administrator separately called POST /Policies. This commit fixes that by automatically calling the appropriate policy function inside the same transaction as user creation. Changes: - api/app/v1/endpoints/create/user.py * Add module-level _POLICY_FN_MAP (viewer/editor/obs_manager/sensor). * Capture app_role before get_db_role_for_rbac() remaps it, so the correct policy function can be dispatched. * After GRANT, call sensorthings.<role>_policy([username], policyname) with policyname = '{username}_default'. Administrator is skipped — admins bypass RLS by privilege, not by policy. * Policy functions already exist in the DB (istsos_auth.sql); no migration required. - api/app/v1/endpoints/functions.py * Add docstrings to _validate_role_identifier() and set_role(). - api/app/v1/endpoints/create/data_array_observation.py * Import shared set_role() helper (was already using the correct upstream version; this import makes the dependency explicit). - api/tests/test_rls_policy_creation.py (new) * Tests: correct policy function per role, administrator exclusion, naming convention, users_ as text[]. - api/tests/test_rbac_set_role_safety.py (new) * Tests: identifier validation, injection rejection, shared helper usage in data_array_observation.
- Add auth_provider and external_sub_id columns to sensorthings."User"
via idempotent migration (001_identity_linking.sql) with a partial
unique index on (auth_provider, external_sub_id) WHERE auth_provider
IS NOT NULL, so local password users are completely unaffected.
- Introduce PENDING_ROLE sentinel in rbac_roles.py. The 'pending' state
is intentionally absent from VALID_RBAC_ROLES so it can never be
assigned through the public API; existing role validation is unchanged.
- Gate pending accounts in get_current_user() (oauth.py): after the DB
lookup, any user with role='pending' immediately receives HTTP 403
'Account pending admin activation' before any SET ROLE or handler
body is reached.
- Add oidc_user_crud.py with create_pending_oidc_user() and
get_user_by_provider_sub(). The insert function hardcodes role to
PENDING_ROLE and contains zero DDL (no CREATE ROLE / CREATE USER),
giving new OIDC accounts zero PostgreSQL footprint until activation.
- Add POST /Users/{id}/activate endpoint (activate_user.py), restricted
to administrators. Runs UPDATE role, CREATE ROLE NOLOGIN IN ROLE,
GRANT, and RLS policy assignment inside a single transaction so a
failed step leaves the user still 'pending' with no partial state.
- Register activate_user router in api.py inside the AUTHORIZATION guard.
Local password users (POST /Users) are completely unaffected; no changes
were made to create/user.py.
Relates-to: GSoC 2026 Identity Linking architecture
- Add PasswordUpdateRequest Pydantic v2 schema (models/password.py)
enforcing: min 12 chars, at least 1 uppercase, at least 1 digit.
Violations surface as HTTP 422 before any DB is touched.
- Add update_local_password() CRUD function (db/password_crud.py):
1. Fetch user row by ID → 404 if missing.
2. OIDC guard: block auth_provider IS NOT NULL users with HTTP 400
'External identities cannot update passwords locally'.
3. Verify current_password via asyncpg.connect() (PostgreSQL auth layer)
→ 401 on InvalidPasswordError. No Python-side passlib used.
4. Execute ALTER USER <username> WITH ENCRYPTED PASSWORD <new_password>
using pg_quote_ident / pg_quote_literal to prevent injection.
- Add PATCH /Users/{id}/password endpoint (update/password.py):
owner-or-admin guard; returns 204 No Content on success.
- Register update_password router in api.py inside AUTHORIZATION guard.
- Add test_password_update.py (9 tests, all pass, no live DB required):
schema: valid, too-short, no-uppercase, no-digit
crud: 404, 400 OIDC block, 401 wrong password, 204 ALTER USER issued
endpoint: 403 non-owner/non-admin guard
Depends on: feat/identity-linking-jit-provisioning (requires auth_provider column)
…le-JWT fix
- Add RoleUpdateRequest Pydantic v2 schema (models/role.py):
delegates to validate_rbac_role(); blocks 'administrator' (bootstrap-only)
and 'pending' (internal state) with HTTP 422 before any DB is touched.
Docstring explains the security boundary explicitly.
- Add update_user_role() CRUD function (db/role_crud.py):
All mutations run inside a single asyncpg transaction (FOR UPDATE lock):
1. 404 if user not found.
2. 400 if user is in 'pending' waiting room.
3. No-op early return if current_role == new_role (no DDL issued).
4. 409 if demoting the last administrator (lockout guard).
5. UPDATE sensorthings."User" SET role = new_role.
6. REVOKE <old_pg_group_role> / GRANT <new_pg_group_role> only when the
underlying PostgreSQL group role changes (e.g. viewer→obs_manager).
viewer→editor shares the same 'user' PG role — no DDL issued.
pg_quote_ident used for all identifier interpolation.
- Add PATCH /Users/{id}/role endpoint (update/role.py):
administrator-only guard at router layer; returns 204 No Content.
- Register update_role router in api.py inside AUTHORIZATION guard.
- Add comment to get_current_user() in oauth.py documenting that role is
fetched live from the DB on every request (not from the JWT payload),
eliminating stale-JWT vulnerabilities after role changes. No logic change.
- Add test_role_reassignment.py (12 tests, all pass, no live DB needed):
schema: valid, administrator/pending/unknown blocked
crud: 404, 400 pending, no-op, 409 last-admin, REVOKE+GRANT, no-DDL
endpoint: 403 non-admin guard
Depends on: feat/password-updates (stacked)
…dentials
Users are strictly application-level entities managed via sensorthings."User".
The backend connects to PostgreSQL through a single master service account;
individual users have no PostgreSQL login roles.
- Add shared POLICY_FN_MAP in rbac_roles.py as single source of truth for
RLS policy dispatch (used by create/user.py and activate_user.py)
- Role reassignment (PATCH /Users/{id}/role) is a pure UPDATE on User.role;
last-admin lockout locks all admin rows via SELECT … FOR UPDATE before
counting to prevent concurrent demotion race condition
- User activation (POST /Users/{id}/activate) updates User.role and applies
the corresponding RLS policy function — no PostgreSQL DDL
- User creation stores bcrypt hash in User.password via parameterised UPDATE
Removed: CREATE USER, CREATE ROLE, REVOKE, GRANT, ALTER USER DDL.
Refs: istSOS#190
BREAKING: set_role() now maps app-layer roles to PG group roles via SET LOCAL ROLE instead of SET ROLE <username>. All 72 RESET ROLE instances deleted across 41 endpoint files. Refactor 1 — functions.py::set_role(): - Maps viewer/editor → 'user', sensor/obs_manager → 'sensor', etc. - SET LOCAL ROLE (transaction-scoped, auto-reverts on COMMIT/ROLLBACK) - Removed inner connection.transaction() nested savepoint - Eliminated entire RESET ROLE call class (pool leak prevention) Refactor 2 — password_crud.py: - Removed asyncpg.connect() credential verification - Removed ALTER USER … WITH ENCRYPTED PASSWORD DDL - Modern path: passlib/bcrypt verify + UPDATE User.password - Legacy fallback: get_auth_connection() for NULL password JIT migration Refactor 3 — role_crud.py: - Removed REVOKE/GRANT DDL block (lines 167-196) - Role reassignment is pure UPDATE sensorthings."User" SET role - Removed _ADMIN_PG_ROLE, DB_ROLE_BY_RBAC_ROLE, pg_quote_ident imports Refactor 4 — Test suite alignment: - test_set_role_sql_safety: asserts SET LOCAL ROLE + group role mapping - test_rbac_set_role_safety: parametrised 5 app roles → PG group roles - test_password_update: asserts UPDATE + bcrypt hash (not ALTER USER) - test_role_reassignment: asserts UPDATE only (not REVOKE/GRANT) - test_policy_role_switch: asserts SET LOCAL ROLE + zero RESET ROLE 49 files changed, 298 insertions(+), 442 deletions(-) 63/63 tests passing.
update/user.py lines 119-135 contained live REVOKE/GRANT statements that were missed in the PR5 cleanup pass. These operated on individual usernames as PostgreSQL role identifiers, which no longer exist under the app-layer credential model. Remove the entire DDL block. Role changes are reflected in the sensorthings."User".role UPDATE above; set_role() maps the new role to its PG group role dynamically at request time.
## Summary
Completes the authentication foundation required before building any
new access-control features. Two self-contained changes:
1. database/migrations/002_add_password_status.sql
2. api/app/oauth.py — rewrite authenticate_user()
---
## 1. Migration: 002_add_password_status.sql
Adds two columns to sensorthings."User" inside the existing
custom.authorization guard block (mirrors migration 001):
• password VARCHAR(255) DEFAULT NULL
Stores the passlib/bcrypt hash of the user's local credential.
NULL signals a legacy (pre-migration) account; on next login the
pg_authid JIT fallback fires and backfills this column so
subsequent logins never touch pg_authid again.
• status VARCHAR(50) DEFAULT 'active'
Account lifecycle flag. 'active' is the default so all existing
rows are completely unaffected. Future values: 'suspended',
'deleted'. Application-layer enforcement is a follow-up task.
Both ALTER TABLE statements use ADD COLUMN IF NOT EXISTS so the
migration is idempotent and safe to re-run against instances that
already received the columns via a prior manual hotfix.
---
## 2. authenticate_user() — bcrypt-first with JIT pg_authid fallback
Replaces the legacy pg_authid-only authentication flow with a
three-step process:
Step 1 — Fetch from sensorthings."User"
SELECT id, username, role, password WHERE username = $1.
Unknown users return None immediately; we never attempt pg_authid
for users not present in the application-layer table.
Step 2 — Bcrypt verify (modern path, password IS NOT NULL)
pwd_context.verify() is dispatched via asyncio.to_thread() to
avoid blocking the event loop with bcrypt's intentional CPU cost.
Correct hash → return user dict. Wrong hash → return None.
Step 3 — pg_authid JIT fallback (legacy path, password IS NULL)
get_auth_connection() attempts a raw asyncpg.connect() to let
PostgreSQL validate via pg_authid.
• Failure → return None.
• Success → asyncio.to_thread(pwd_context.hash, password) computes
the bcrypt hash then writes it via the write pool
(POSTGRES_PORT_WRITE pattern from role_crud / password_crud).
Backfill failure is caught, logged, and swallowed — login still
succeeds (best-effort JIT migration, never blocks the user).
Circular import resolution
pwd_context lives in password_crud.py which already imports
get_auth_connection from oauth.py. Both symbols are imported
lazily inside the function body to break the cycle, following the
identical pattern already used in password_crud.py line 115.
---
Breaking changes: none.
Existing users with password IS NULL continue to log in as before;
they are transparently migrated on first login.
Users with a bcrypt hash no longer require a pg_authid LOGIN role.
…ation errors passlib 1.7.4 is incompatible with bcrypt >= 4.1 due to a wrap-bug detection test that passes a >72-byte secret, which bcrypt 4+ rejects with ValueError. Pin bcrypt==4.0.1 — the last version that works with passlib 1.7.4 without triggering the 72-byte guard. A previous refactor left dangling 'if current_user is not None:' guards with no body before the return statement, causing Python to raise IndentationError at import time and crashing the entire API process. Removed the dead guard in each case — the 404 / not-found response should always be returned regardless of auth context. The outer exception handler already enforces auth context where needed. Files fixed: api/app/v1/endpoints/create/bulk_observation.py (ValueError catch) api/app/v1/endpoints/delete/observation.py (404 guard) api/app/v1/endpoints/update/historical_location.py (404 guard) api/app/v1/endpoints/update/location.py (404 guard) api/app/v1/endpoints/update/observation.py (404 guard) api/app/v1/endpoints/update/observed_property.py (404 guard) api/app/v1/endpoints/update/thing.py (404 guard)
BREAKING: set_role() now maps app-layer roles to PG group roles via SET LOCAL ROLE instead of SET ROLE <username>. All 72 RESET ROLE instances deleted across 41 endpoint files. Refactor 1 — functions.py::set_role(): - Maps viewer/editor → 'user', sensor/obs_manager → 'sensor', etc. - SET LOCAL ROLE (transaction-scoped, auto-reverts on COMMIT/ROLLBACK) - Removed inner connection.transaction() nested savepoint - Eliminated entire RESET ROLE call class (pool leak prevention) Refactor 2 — password_crud.py: - Removed asyncpg.connect() credential verification - Removed ALTER USER … WITH ENCRYPTED PASSWORD DDL - Modern path: passlib/bcrypt verify + UPDATE User.password - Legacy fallback: get_auth_connection() for NULL password JIT migration Refactor 3 — role_crud.py: - Removed REVOKE/GRANT DDL block (lines 167-196) - Role reassignment is pure UPDATE sensorthings."User" SET role - Removed _ADMIN_PG_ROLE, DB_ROLE_BY_RBAC_ROLE, pg_quote_ident imports Refactor 4 — Test suite alignment: - test_set_role_sql_safety: asserts SET LOCAL ROLE + group role mapping - test_rbac_set_role_safety: parametrised 5 app roles → PG group roles - test_password_update: asserts UPDATE + bcrypt hash (not ALTER USER) - test_role_reassignment: asserts UPDATE only (not REVOKE/GRANT) - test_policy_role_switch: asserts SET LOCAL ROLE + zero RESET ROLE 49 files changed, 298 insertions(+), 442 deletions(-) 63/63 tests passing.
## Summary
Implements the audit trail foundation for STAC/ODRL access governance.
Two self-contained additions stacked on feat/auth-foundation-phase0:
1. database/migrations/003_audit_log.sql
2. api/app/db/audit_crud.py
---
## 1. Migration: 003_audit_log.sql
Creates sensorthings."AuditLog" inside the custom.authorization guard
block (SET ROLE "administrator" for DDL, RESET ROLE before privileges).
Schema:
id UUID PRIMARY KEY DEFAULT gen_random_uuid()
gen_random_uuid() provided by pgcrypto (already loaded)
actor_id BIGINT FK → sensorthings."User"(id) ON DELETE SET NULL
Nullable for anonymous events (e.g. PUBLIC_READ)
action_type VARCHAR(50) NOT NULL CHECK IN (
'PUBLIC_READ', 'RESTRICTED_REQUEST', 'ADMIN_APPROVAL')
dataset_id TEXT — STAC dataset identifier (nullable)
odrl_policy_id TEXT — ODRL policy reference (nullable)
payload JSONB DEFAULT NULL — arbitrary event metadata
created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
Indexes:
idx_auditlog_action_type (btree) — filter by event category
idx_auditlog_actor_id (btree) — filter by user
Append-only enforcement (after RESET ROLE, following istsos_auth.sql
lines 398-444 pattern):
REVOKE UPDATE, DELETE from: administrator, user, sensor, guest, qc
GRANT INSERT to: user, sensor
---
## 2. Helper: audit_crud.py
async def log_audit_event(conn, action_type, actor_id, dataset_id,
odrl_policy_id, payload) -> None
* Accepts a pre-acquired asyncpg connection so callers can include
the audit INSERT in the same transaction as the action being logged.
* Serialises payload dict via json.dumps() + $5::jsonb cast to avoid
asyncpg TypeError on dict → JSONB (oidc_user_crud.py pattern).
* None payload passes through as SQL NULL unchanged.
* Exports three action-type constants (AUDIT_ACTION_PUBLIC_READ,
AUDIT_ACTION_RESTRICTED_REQUEST, AUDIT_ACTION_ADMIN_APPROVAL)
that match the DB CHECK constraint exactly.
---
Breaking changes: none.
No existing tables or application code are modified.
….sql The 'qc' role only exists when AUTHORIZATION was enabled at DB init time. Replace flat REVOKE statements with a DO block that checks pg_roles before each REVOKE/GRANT so the migration is idempotent and safe on any deployment configuration.
Implements POST /Register (Path B, Step 4 of the architecture plan).
Changes
-------
* api/app/models/register_request.py
- ContactInfo BaseModel: 6 optional string fields (domain, company,
address, telephone, telegram, linkedin).
- RestrictedRegistrationRequest BaseModel: username, password,
dataset_id, odrl_policy_id, explanation, contact_info.
* api/app/v1/endpoints/create/register_request.py
- Public POST /Register handler (no auth dependency).
- bcrypt hash via asyncio.to_thread (non-blocking).
- Merges explanation into contact JSONB blob.
- Single atomic transaction: INSERT User (pending/active),
UPDATE uri, INSERT AuditLog (RESTRICTED_REQUEST).
- Structured error handling: 409 UniqueViolation, 503, 504, 500.
* api/app/v1/api.py
- Import + include_router for register_request.v1 under the
AUTHORIZATION guard alongside other user-management routers.
PATCH /Users/{target_user_id}/policy-approval
* Add AdminApprovalRequest Pydantic model (models/approval_request.py)
- assigned_role validated via validate_rbac_role (field_validator)
- dataset_id + odrl_policy_id forwarded to AuditLog verbatim
* Add admin_approval endpoint (update/admin_approval.py)
- Administrator-only guard (HTTP 403 on non-admin callers)
- Single write-pool transaction:
1. Fetch username for RLS policy name construction
2. UPDATE User SET role=<role>, status='active'
WHERE id=<id> AND role='pending' RETURNING id
→ HTTP 404 if no row returned (not found / not pending)
3. Apply POLICY_FN_MAP RLS function if role has a default policy
4. log_audit_event(ADMIN_APPROVAL) — atomic with UPDATE
* Wire admin_approval.v1 router into api.py under AUTHORIZATION guard
between update_role and delete_user
sensorthings.viewer_policy (and all role policy fns) issue: CREATE POLICY ... TO <username> which requires <username> to be a PostgreSQL role. Self-registered users created via POST /Register have zero DB footprint by architectural design — no CREATE ROLE is ever issued. Calling the policy function for these users raises asyncpg.UndefinedObjectError. Two fixes applied: 1. Catch UndefinedObjectError and log a WARNING — skip RLS gracefully. 2. Wrap the policy call in async with conn.transaction() (savepoint) so the caught error does NOT poison the outer transaction. Without the savepoint, asyncpg marks the entire transaction aborted and the subsequent log_audit_event INSERT fails with InFailedSQLTransactionError. The UPDATE (role/status) and AuditLog INSERT continue to commit atomically when RLS is skipped.
- database/migrations/004_public_access.sql
* ADD COLUMN is_public BOOLEAN NOT NULL DEFAULT false to Datastream
* DROP blanket anonymous_datastream / anonymous_observation policies
* CREATE fine-grained guest RLS policies filtered by is_public
* GRANT INSERT on AuditLog to guest for PUBLIC_READ audit events
- api/app/oauth.py
* Add oauth2_scheme_optional (auto_error=False) for optional auth
* Add get_optional_current_user(): returns user dict or None
(never raises 401/403; covers missing/expired/revoked tokens
and pending accounts — all treated as anonymous)
- api/app/v1/endpoints/read/read.py
* Remove AUTHORIZATION/ANONYMOUS_VIEWER conditional branching
* Use get_optional_current_user universally at module level
* asyncpg_stream_results: always fall back to guest role when
current_user is None (activates is_public RLS policies)
* Log PUBLIC_READ audit event BEFORE SET LOCAL ROLE so the
INSERT runs with pool-user privileges, not restricted guest role
- api/app/v1/endpoints/read/{datastream,observation,thing,...}.py
* All specific-entity read endpoints already migrated to
get_optional_current_user dependency injection
SET LOCAL ROLE (used by set_role()) is transaction-scoped and auto-reverts when connection.transaction() exits. Calling RESET ROLE mid-transaction in the $value early-return path would prematurely escalate back to pool-user privileges before the transaction commits, creating a privilege window inconsistent with all other exit paths.
… errors across endpoints
…ader params Keeps the one genuine improvement from the prior edit (deduplicated revoked-token check in refresh_token) while reverting three regressions: unconditional Redis calls with no REDIS-flag guard, a raw JWT exp value passed straight to redis.set(ex=...) instead of clamped via ttl_from_exp, and Header() (required) instead of Header(default=None), which broke the custom 400 response for a missing Authorization header.
RestrictedRegistrationRequest.password had no validation at all — any string, including a single character, passed. Adds a shared validate_password_strength() helper (app/validators.py) enforcing: - at least 8 characters - at least 1 digit - at least 1 symbol Kept in a separate shared module rather than inline in this model so the same rule can be applied to PasswordUpdateRequest.new_password (feat/password-updates) without duplicating the check in two places and letting them silently drift apart, which is what had happened previously (password-change enforced 12 chars/1 upper/1 digit; registration enforced nothing). Verified live: 422 on <8 chars, 422 on no digit, 422 on no symbol, 201 on a password satisfying all three rules.
…trength()
PasswordUpdateRequest.new_password previously enforced its own inline
rule (12 chars, 1 uppercase, 1 digit). Extracts a shared
validate_password_strength() helper (app/validators.py) so the same
rule can be applied to a brand-new account's initial password at
registration (feat/restricted-registration) without the two checks
silently drifting apart, which is what had already happened (password
update enforced strength, registration enforced nothing at all).
New unified rule:
- at least 8 characters
- at least 1 digit
- at least 1 symbol
Verified live via PATCH /Users/{id}/password: 422 on <8 chars, 422 on
no symbol, 204 on a password satisfying all three rules, old password
rejected afterward, new one accepted.
…oints
Both PATCH /Users/{id}/policy-approval and POST /Users/{id}/activate
only guarded on role == 'pending'. Since rejection (PATCH .../reject)
deliberately leaves role='pending' and only flips status to 'rejected',
a rejected user could be silently approved/activated anyway through
either endpoint, fully bypassing the rejection with no error.
Adds an explicit status == 'rejected' check (400) to both endpoints,
returned before any mutation is attempted.
Also fixes an unrelated crash found while testing this: POST
/Users/{id}/activate had no guard around the RLS policy-creation call
for self-registered ('zero DB footprint') users, so it 500'd with an
unhandled UndefinedObjectError instead of the graceful savepoint-based
skip that update/admin_approval.py already had. Ported that same
try/except savepoint pattern over — now activates self-registered
users correctly (verified live) instead of crashing, independent of
the new rejection guard.
Verified live: policy-approval on a rejected user -> 400 (was: silent
200 bypass). /activate on a rejected user -> 400 (was: 500 crash).
/activate on a fresh, non-rejected self-registered user -> 200,
correctly activated, RLS policy gracefully skipped with a log warning
(was: crashed for this case too, even without rejection involved).
…tion
Adds the negative counterpart to PATCH /Users/{id}/policy-approval:
a pending user's registration request can now be explicitly rejected
rather than only ever approved or left pending indefinitely.
- PATCH /Users/{id}/reject (admin-only): sets status='rejected' on a
pending user, leaving role untouched (still 'pending' — rejection is
a lifecycle transition, not an RBAC role assignment). Guarded by
WHERE role='pending' so an already-approved user can't be silently
rejected by mistake (404 otherwise). Logs an ADMIN_REJECTION audit
event.
- database/migrations/004_admin_rejection.sql extends the
AuditLog.action_type CHECK constraint to allow 'ADMIN_REJECTION'.
No change needed to User.status itself (unconstrained VARCHAR(50)).
- oauth.py authenticate_user(): after password verification succeeds
(both the bcrypt and pg_authid JIT-fallback paths), a rejected
account raises 401 immediately. Password is checked first so a wrong
password on a rejected account still returns the generic "incorrect
username or password" instead of leaking rejection status to an
unauthenticated guesser.
- register_request.py: POST /Register now checks for an existing
'rejected' row (SELECT ... FOR UPDATE, to avoid a re-application
race) before deciding INSERT vs UPDATE. A rejected user re-applying
overwrites password/contact and resets status to 'active', instead
of the blanket 409 Conflict every other existing-username case still
gets.
Full loop verified live: register -> reject -> login blocked (401) ->
re-register (201, overwrite) -> old password rejected -> new password
succeeds. Non-admin reject attempt -> 403. Reject on an already-approved
user -> 404 (guard holds).
RestrictedRegistrationRequest.username had zero validation — any
string passed, including empty, whitespace, or special characters.
The admin-created-user path (create/user.py) already enforces
3-63 chars, letters/digits/underscores only, via validate_username().
Reuses that same function here instead of duplicating the pattern,
so the two entry points can't drift apart.
Verified live: empty string, "bad user" (space), "bad@user",
"bad-user", and "ab" (too short) all -> 422. A valid username
("valid_user_123") -> 201, unaffected.
…ad-end create_pending_oidc_user() previously let any UniqueViolationError bubble up unchanged, with its docstring telling callers to recover via get_user_by_provider_sub() — but that lookup only checks the (auth_provider, external_sub_id) pair. If the actual collision is on username (an OIDC preferred_username claim matching an existing local or other-provider username), that recovery path returns None again, and the account can never be provisioned — every attempt hits the same silent dead end. Scope note: deliberately NOT auto-resolving the collision (e.g. by appending a suffix to the username and retrying). Whether a username collision here should link the OIDC identity to the existing local account, mint a distinct suffixed account, or reject and ask the applicant to pick a different handle is a product decision about identity linking vs. fragmentation — not something to guess at in code that has zero live callers yet (no OIDC callback route exists anywhere in this codebase to design the behavior against). Left as an explicit TODO for whoever wires up that route. Adds OidcUsernameCollisionError, raised only when the UniqueViolationError is specifically on User_username_key. Any other UniqueViolationError (including the genuine (provider, sub) collision) is re-raised unchanged, preserving the existing documented recovery path. Since this function has no HTTP caller to test against, added unit tests (tests/test_oidc_username_collision.py) mocking the connection pool directly, following the same pattern already used in test_oauth_connection_leak.py. All 4 pass; full existing suite run alongside shows one pre-existing, unrelated failure (test_issue7_exception_handling.py) confirmed present before this change too (stashed and re-ran to verify).
…ration This branch already imports the canonical POLICY_FN_MAP correctly in create/user.py and activate_user.py (no local duplicate to remove here) -- just adding the warning that applies wherever this map is defined.
…ymbol rule
Stale from today's password-policy change (shared validate_password_strength:
8 chars, 1 digit, 1 symbol; no uppercase requirement) -- these two tests
still asserted the old 12-char/uppercase rule and were failing:
- test_too_short_raises_422 checked for "12 characters" in the error
- test_no_uppercase_raises_422 asserted an uppercase requirement that
no longer exists; its own test password (18 chars, has digit+symbol)
now correctly passes validation, so the test never even raised
Renamed the latter to test_no_symbol_raises_422 to cover the new third
rule instead. test_no_digit_raises_422 and test_valid_payload_passes
needed no changes -- their fixtures already satisfy both the old and
new rule. Full file: 9/9 passing.
.env now holds real external-provider OAuth secrets (Google/Microsoft/ GitHub/ORCID) alongside local dev defaults. It was already listed in .gitignore but had been tracked since early in the project's history, so the ignore rule had no effect -- untrack it so no future commit can include real credentials. The already-pushed committed version only ever held generic local dev defaults (admin/postgres/a dev JWT key), so nothing sensitive has been exposed by this history. Also ignore the meeting-prep docs and manual verification scripts (demo.py, test_access_matrix.py, etc.) used throughout development -- personal scratch artifacts, not part of the real test suite under api/tests/.
External authentication
------------------------
Adds Google, Microsoft, GitHub, and ORCID (edu-ID wired but dormant,
pending SUPSI's SWITCH federation sponsorship) as login providers,
via one shared provider registry and one generic pair of routes --
GET /auth/{provider}/login and /callback -- instead of five separate
implementations. Google/Microsoft/ORCID/edu-ID are real OIDC, handled
identically; GitHub is OAuth2-only, so its identity comes from a
follow-up REST call instead of an id_token.
A dataset/policy selection made before authenticating is carried
through the redirect via the session (an OAuth login is a bare GET
with no request body), and persisted on the User row for both this
path and local /Register -- previously write-only into the audit log,
invisible to an admin reviewing the queue without a manual join.
Verified live against all four real providers, not just mocked:
real consent screens, real code exchange, real claim extraction, real
provisioning, real admin approval, real tokens issued on the second
login. Two real bugs were found and fixed in the process, not just
credentials plugged into already-correct code:
* os.getenv(key, default) doesn't fall back correctly when Docker
Compose substitutes an unset ${VAR} as an empty string rather than
an absent key -- silently broke Microsoft's discovery URL.
* Multi-tenant Microsoft/Entra apps return a templated `issuer`
("https://login.microsoftonline.com/{tenantid}/v2.0") in their
discovery document, but a real token's `iss` has an actual tenant
GUID substituted in -- Authlib's default exact-match validation
always failed. Fixed with a provider-specific claims validator.
Row-level security fix
-----------------------
Every RLS policy for viewer/editor/obs_manager/sensor/qc was scoped
`TO <username>` -- a specific individual PostgreSQL role. But
set_role() only ever runs SET LOCAL ROLE to a shared group role
("user"/"sensor"/"qc"), and no application code path creates an
individual login role for a real user either. Confirmed live via
pg_policy inspection: these policies could never match any real
session, for any user, ever.
Replaced with static policies created once (not per-approval),
scoped to the actual group role, with per-user/per-dataset
differentiation done via a session claim instead of a role name --
set_role() now also runs
`SELECT set_config('app.current_user_id', $1, true)`, and policies
read it back through three small helper functions. Handles a real
subtlety this surfaced: viewer/editor share one Postgres group, as do
sensor/obs_manager, so telling them apart now happens inside the
USING clause (checking the caller's actual role) rather than by role
membership alone.
Dataset-scoping applies to viewer/editor/obs_manager/sensor/qc on the
Datastream table specifically (dataset_id doesn't exist on other
tables yet -- same limitation odrl_governed already had). odrl_governed
itself is deliberately untouched, per explicit scope agreement --
ODRL document parsing is future work; this fix is for the roles
already in use today.
admin_approval.py, activate_user.py, and create/user.py no longer
need any RLS DDL call for these five roles -- approving or activating
a user into one is now a plain UPDATE. POLICY_FN_MAP and the five
per-user policy functions it dispatched to are retired entirely.
Verified live end-to-end: a viewer scoped to one dataset sees exactly
the matching rows out of the full set; an editor can write within
their own dataset but is invisible to a different one; both proven
against real data, not synthetic fixtures. Full test suite: zero
regressions (same pre-existing unrelated failures before and after).
Resolves conflicts from upstream's exception-import consolidation (ec9d1e1) and the new 'qc' role support (001939c). Resolution principles: - Keep this branch's transaction-scoped SET LOCAL ROLE design: upstream's reintroduced RESET ROLE calls are dropped, since SET LOCAL ROLE reverts automatically at transaction end. - Take upstream's qc-role semantics wholesale (create/user.py qc commit block, update/functions.py qc guard replacing the old sensor guard). - Keep this branch's rewritten test_policy_role_switch.py and test_set_role_sql_safety.py; upstream deleted the originals as obsolete, but these versions cover the SET LOCAL ROLE behavior introduced here. - Restore the Forbidden import in delete/functions.py that the automatic merge dropped while retaining code that raises it.
Carries forward the upstream conflict resolutions made on feat/public-access, plus this branch's own resolutions: - Keep .env untracked (chore d14dd89); upstream ships .env.example. - Fold this branch's ignore entries into upstream's new sectioned .gitignore layout without duplicating .vscode. - Keep upstream's new qc-role commit block in create/user.py while retaining this branch's static-RLS design from migration 007, which supersedes the per-user POLICY_FN_MAP call.
Commit 7026976 referenced Swagger deliverables that are only created later, on docs/swagger-api-documentation, so app/v1/api.py raised ModuleNotFoundError on import and the API could not start on this branch at all. Splits that dependency along PR lines. Kept here, because this branch's own oidc_login.py genuinely uses them: - v1/endpoints/openapi_responses.py (shared response fragments) - models/error.py, models/token.py (its only dependencies) - RegisterResponse / ApprovalResponse doc-only models Returned to the Swagger PR, where they belong: - custom_docs.py and docs_description.py (never referenced outside api.py) - the landing description, tag metadata, custom /docs theme and swagger_ui_parameters block in api.py api.py is now the feat/public-access version plus this PR's own two OIDC lines. Tests: 73 passed (was 64 passed + 9 collection errors).
…r/qc 007_session_scoped_rls_policies.sql DROPs sensorthings.viewer_policy(), editor_policy(), obs_manager_policy(), sensor_policy() and qc_policy() -- those five roles get RLS access automatically from a static policy the moment their role is set, so there is nothing left for these functions to do. But create/policy.py's POST /Policies handler was never updated: it still dispatched to all five, and none of them exist anymore. Reproduced live: POST /Policies with permissions.type "qc" against a real qc user returned the raw Postgres error verbatim -- "function sensorthings.qc_policy(unknown, unknown) does not exist" -- for any of the five role types, not just qc. Rejects any permissions.type other than "custom" up front with an honest message instead. Also drops the now-dead per-user role-match check (its only purpose was validating those five now-removed branches) and updates PAYLOAD_EXAMPLE, which had been advertising "viewer" as a working value. test_policy_role_switch.py was asserting the old, now-broken behavior (201 for permissions.type "viewer") and passed only because it mocks the DB connection entirely -- it never noticed viewer_policy() had been dropped. Updated to assert the corrected rejection. Found while investigating a smaller, now-superseded issue (a missing POLICY_FN_MAP entry for qc from before migration 007 existed).
Every other RLS-adjacent test in this suite mocks the DB connection
entirely, so none of them can prove row-level security enforces
anything -- they only prove which SQL string gets sent. That gap is
exactly how the bug 007_session_scoped_rls_policies.sql fixes went
unnoticed in the first place: the old TO <username> policies looked
correct at the SQL-authoring level and simply never matched a real
session, for any user, ever.
test_datastream_rls_filters_by_dataset_id_per_user connects to the real
database and proves enforcement directly: two viewer users sharing one
PostgreSQL group role ("user") but different individual identities see
different, correctly dataset-filtered Datastream rows. That per-user
differentiation without an individual login role is the entire point of
the fix, and it's exactly what the old design could never achieve.
test_static_policies_are_group_scoped_not_per_user is cheaper and more
direct: every RLS policy on Datastream must be scoped TO a shared group
role, never an individual username. Verified as a real negative control
-- manually created a per-username policy against the live DB and
confirmed this test catches it, then removed it.
Both tests run inside a transaction that is always rolled back, never
committed. A real cleanup DELETE on sensorthings."User" was tried first
and found broken for literally any row, not just these: AuditLog_actor_id_fkey
is ON DELETE SET NULL, and Postgres runs that enforcement trigger with
the *table owner's* privileges (both User and AuditLog are owned by
"administrator", confirmed), not the caller's -- and administrator was
deliberately never granted UPDATE on AuditLog, since it's meant to be
genuinely append-only. That makes DELETE FROM sensorthings."User" fail
unconditionally for every caller, including the real DELETE /Users
endpoint. Separate, real, pre-existing bug -- logged in
UNFINISHED_WORK.md, not fixed here. Rollback-only setup sidesteps it
without papering over it.
Each test opens its own asyncpg connection rather than sharing the app's
get_pool() singleton, which is bound to whichever event loop first
created it -- reusing it across two separate asyncio.run() calls (the
plain-function style already used throughout this suite) raised
"cannot perform operation: another operation is in progress" on the
second test, confirmed unrelated to RLS itself.
Previously RestrictedRegistrationRequest and the OIDC provisioning path
collected dataset_id/odrl_policy_id but no role signal at all: an
administrator reviewing GET /Users had nothing structured to go on beyond
a free-text explanation field, and had to invent a role from nothing at
approval time. Agreed design (never implemented -- picked back up from
where it was paused mid-session): the applicant states the role they
want; the administrator sees it as the default at approval but can
override. The administrator stays the final gatekeeper either way.
Migration 008_requested_role.sql adds a nullable requested_role column to
sensorthings."User", same pattern as dataset_id/odrl_policy_id
(006_user_dataset_policy.sql).
Local path (POST /Register): RestrictedRegistrationRequest gains a
required requested_role field, validated through the same
validate_rbac_role() the approval endpoints already use so "requested"
and "assigned" can never accept different role sets. Persisted on both
INSERT and the re-application UPDATE path, and included in the
RESTRICTED_REQUEST audit payload.
OIDC path: /auth/{provider}/login gains a required requested_role query
param (validated before it ever reaches the session), stashed alongside
dataset_id/odrl_policy_id the same way, read back at /callback and
threaded through create_pending_oidc_user().
Both approval endpoints now treat their role field as a default, not a
mandate:
- AdminApprovalRequest.assigned_role (PATCH .../policy-approval) is now
optional; the handler falls back to the pending user's requested_role
when omitted, and 400s if neither exists.
- activate_user.py's payload["role"] (OIDC path) gets the same
treatment; the user fetch was reordered ahead of role resolution
since the fallback needs requested_role off that same row.
Verified live against the running API, not just the test suite: local
registration + default-on-approval, local registration + explicit
override, local registration with neither role present (400), and the
OIDC/activate_user.py fallback with a directly-seeded pending row.
test_oidc_external_providers.py's 7 /login and /callback tests were all
missing the new required query param -- updated, not skipped. 75/75
passing.
normalize_claims() derives username from preferred_username/email/name/sub
with no format guarantee -- confirmed live, every non-GitHub provider
tested produced something that fails validate_username()'s
^[a-zA-Z0-9_]{3,63}$ rule: Google fell to name ("Kinshuk S", a space),
ORCID fell to sub (the ORCID iD itself, "0009-0000-0287-1000", all
hyphens), Microsoft fell to email. That rule is only ever enforced on the
three local-account paths (POST /Register, POST /Users, DELETE /Users) --
nothing validates an OIDC-derived username today, so all of the above
were stored as-is.
Checked for an actual injection risk first: didn't find one -- every
touch point either uses asyncpg $N parameters or (for the one place a
username feeds into a generated identifier, admin_approval.py's
`f"{username}_default"` policy name) passes through Postgres's own
identifier-quoting inside the SQL function. This is a data-quality/UX
correctness fix, not a security patch.
The real constraint: unlike local registration, there is no form step
where a real person could retype an invalid value -- the username is
whatever the provider handed back on a redirect. Rejecting the login
outright would drop a sincere signup over formatting entirely outside
the applicant's control, so this sanitizes into something valid instead.
sanitize_username() (utils.py, next to validate_username()): collapses
every run of disallowed characters to one underscore, strips leading/
trailing underscores, truncates to 63 chars. If under 3 characters
survive (a pathological all-symbol or CJK-only input), falls back to a
short deterministic hash of the provider's `sub` claim, so the result
always passes validate_username().
Also reorders normalize_claims()'s fallback chain from
preferred_username -> name -> email -> sub to
preferred_username -> email -> name -> sub: email is a stable, meaningful
identifier when a provider shares it, versus name being a raw display
string with no uniqueness guarantee. Whichever wins still goes through
sanitize_username() regardless.
Wired into oidc_login.py's callback immediately after normalize_claims(),
before create_pending_oidc_user() -- keeps normalize_claims() a pure
claim-extraction function, keeps the username policy next to the local
one. Existing collision handling (OidcUsernameCollisionError) is
untouched; a sanitized name that collides hits the same already-flagged,
deliberately-unresolved TODO as any other collision.
test_username_sanitization.py: 13 new unit tests, including every real
case observed live this session, plus deterministic-hash-fallback and
unicode-only edge cases. Verified against real captured data, not just
synthetic cases: ran the actual sanitize_username() from this commit
against the real Google account's stored (name, sub) pair from earlier
in this session ("Kinshuk S" -> "Kinshuk_S", still passes
validate_username()).
test_oidc_external_providers.py's Google/eduID tests asserted the old
fallback order -- updated to expect email over name, not skipped. 88/88
passing.
….yml
Only dev_docker-compose.yml carried the 12 OAuth provider env vars and
the --proxy-headers uvicorn flag; the production compose file had zero
external-auth wiring. A deployment via the plain compose file would have
Google/Microsoft/GitHub/ORCID/eduID login silently not work at all --
oidc_providers.py's per-provider registration would see every CLIENT_ID/
CLIENT_SECRET pair as unset and never enable any of them.
Adds the same 12 env vars (GOOGLE/MICROSOFT/GITHUB/ORCID/EDUID
CLIENT_ID/CLIENT_SECRET, plus the two optional discovery URL overrides)
and --proxy-headers, needed for OAuth callback URLs to come out as
https:// behind TLS-terminating infra.
Deliberately NOT a copy-paste of dev's config: dev hardcodes
--forwarded-allow-ips="*", explicitly flagged in its own comment as
dev-only ("a real deployment should scope this to the actual reverse
proxy's IP, not '*'"). Production instead gets a new FORWARDED_ALLOW_IPS
env var, defaulting to 127.0.0.1 (uvicorn's own built-in default, i.e.
trust nothing external until explicitly configured) rather than an
insecure wildcard.
.env.example documented the 12 provider vars for the first time (it had
none before), plus FORWARDED_ALLOW_IPS, matching the existing per-var
comment style.
Verified end-to-end against the actual production docker-compose.yml,
not just read: built a local image from the current source (the pinned
production image, :1.28, predates this whole external-auth feature and
has none of this code yet -- expected, not a defect in this fix) and
substituted it into a throwaway compose stack via override + --env-file,
isolated from the real dev stack. Confirmed live:
- All 5 providers register (ENABLED_PROVIDERS) through the production
compose wiring, matching dev.
- GET /auth/{provider}/login redirects correctly for all 5, with the
right authorization host, client_id, redirect_uri, and scope per
provider.
- requested_role validation (400), missing required params (422), and
an unconfigured provider (404) all match dev's behavior.
- FORWARDED_ALLOW_IPS is a real, enforced trust boundary, not
cosmetic: X-Forwarded-Proto: https was honored (redirect_uri came
back https://) when the range included the test network's actual
address, and was correctly ignored (redirect_uri stayed http://)
when it didn't.
- Confirmed multi-worker (--workers 2, needed since prod doesn't use
--reload) doesn't break the OAuth session: SessionMiddleware signs
the session into the cookie itself (itsdangerous, no server-side
store), verifiable by any worker sharing the same SECRET_KEY -- login
and callback landing on different workers is safe.
Test stack fully torn down after verification (containers, volumes,
network, local image all removed) -- nothing left running or built
outside the real dev environment.
Re-implements its own local _POLICY_FN_MAP and _simulate_policy_call() rather than importing or calling the real create_user() -- it tests a hand-written copy of the old per-user policy dispatch mechanism, not the app. That mechanism was replaced entirely by 007_session_scoped_rls_policies.sql; POLICY_FN_MAP doesn't exist in the app anymore. This test would keep passing regardless of what the real code does, since it never touches the real code -- false confidence, not coverage. The real behavior it was meant to cover is what test_rls_enforcement.py actually verifies, against a live database.
Carries forward the removal of test_rls_policy_creation.py, a false-confidence test that never touched the real code it claimed to cover.
A real DELETE FROM sensorthings."User" fails for every caller, unconditionally -- reproduced live as a true Postgres superuser, not just as the app's own connection. AuditLog_actor_id_fkey is ON DELETE SET NULL, and Postgres runs that FK enforcement trigger with the *referenced table's owner* privileges (both User and AuditLog are owned by "administrator"), not the caller's -- and administrator was deliberately never granted UPDATE on AuditLog, since it's meant to be genuinely append-only. Two ways to fix this were on the table: loosen the AuditLog grant just enough to let the FK trigger complete, or stop hard-deleting altogether. Chose the latter -- the append-only guarantee staying provably untouched, even by an administrator, is worth more than DELETE working via a real DELETE statement. delete/user.py now sets status = 'deleted' (DELETED_STATUS, added next to PENDING_ROLE in rbac_roles.py) instead of DELETE + DROP ROLE + remove_user_from_policy(). The latter two calls are also dropped entirely: DROP ROLE was calling for an individual PostgreSQL login role that no code path in this app has ever created (see activate_user.py's own architecture note) and always failed as dead code; a deactivated user is rejected at the auth layer before any query runs, so a stale custom-policy reference to their username no longer matters either. Critical gap this surfaced and had to close first: status was not checked ANYWHERE in the login/session path before this. Setting status='deleted' alone would have been cosmetic -- the account would have kept working normally. authenticate_user() already checked status == "rejected" (on both its bcrypt and legacy pg_authid paths); added the identical check for DELETED_STATUS right beside it. get_current_user() and get_optional_current_user() had no status check at all -- added one to each, matching how they already handle the 'pending' role: get_current_user() raises 403, get_optional_current_user() silently returns None (anonymous), so a deactivated user can't sneak through as guest and get the guest RLS view instead of correctly being denied. Role and status are both re-checked live from the database on every request already (see the existing NOTE in get_current_user), so a JWT issued before deactivation stops working on its very next use with no token-revocation step needed. Verified live end-to-end via real HTTP, not just the test suite: an active user's token works, DELETE /Users deactivates them (200), the exact same still-valid token is rejected on its next request (403, "This account has been deactivated"), a fresh login attempt with the correct password is also rejected (401, same message), deactivating again is a clean 409, and the row still exists afterward with status='deleted' -- never physically removed. test_user_deactivation.py: 7 new tests. Three connect to a real database (same reasoning as test_rls_enforcement.py -- mocking the connection can only prove which SQL string gets sent, not that the real auth path rejects a deactivated account): proves deactivation never touches AuditLog at all, proves authenticate_user() rejects the correct password once deactivated, and proves an existing JWT is rejected on its next use with no revocation step. The other four (404 / 409 / self-deactivation guard / UPDATE-not-DELETE shape) mock the connection, matching test_policy_role_switch.py's style. All test data committed by the cross-connection tests is cleaned up afterward via the same temporary AuditLog-grant trick already verified safe earlier in this project's history, confirmed to leave zero residue and the exact prior grant state restored, run twice to confirm idempotency. 51/51 passing.
Carries forward DELETE /Users deactivation. Merged cleanly, including into this branch's own pre-existing get_optional_current_user() (added here for OIDC/guest-fallback support, not present on feat/public-access) -- git's context matching correctly applied the same deactivation check to both get_current_user() and get_optional_current_user().
…ates for admin review Resolves the TODO left by the earlier collision-detection fix: a plain username clash on OIDC signup no longer dead-ends the applicant with a 409. create_pending_oidc_user() now retries with an auto-suffixed candidate (numeric, then a hash of the provider's sub claim) before giving up, matching how GitHub/Slack/Discourse treat handle collisions as cosmetic rather than an identity question. Separately, adds possible_duplicate_of: an advisory-only FK stamped when a new signup's email matches an existing, unrelated account. Never auto-linked, never read by any auth check -- just a hint an administrator sees on the pending queue (GET /Users already returns every column) so a human makes the "same person?" call instead of the system guessing.
KinshukSS2
force-pushed
the
feat/external-identity-providers-rls-fix
branch
from
August 24, 2026 13:13
6058003 to
03f8642
Compare
create_pending_oidc_user() (03f8642) added a duplicate-email lookup before the INSERT, and made a single username collision retry with an auto-suffixed candidate instead of failing immediately. Neither change was reflected in test_oidc_external_providers.py's HTTP-level mocks: _fake_pool_for_provisioning()'s side_effect list was one fetchrow call short, and the old collision test still asserted the pre-auto-suffix 409-on-first-collision behavior. Not caught earlier because this file needs authlib, which isn't installed outside the API container. Fixes the mock helper's call sequence and replaces the outdated collision test with two that match current behavior: a single collision auto-resolving to 202, and the genuine (now much rarer) 409 when every auto-suffixed fallback also collides.
…e write RLS doesn't raise an error when an UPDATE's policy excludes a row -- it just matches zero rows and reports success. update_entity() discarded that signal entirely (fetchval with no RETURNING, return value unused), so a viewer PATCHing a Thing they can SELECT but not write got back a clean 200 even though nothing was written: check_id_exists (SELECT) passes under the shared read policy, then the UPDATE silently no-ops under the separate, editor-only write policy. update_entity() now adds RETURNING id and returns whether a row was actually matched; all nine update_*_entity() wrappers and both the PATCH and PUT handlers propagate that signal and return 403 instead of silently succeeding. Association-only PATCHes (no direct entity-table fields, e.g. a Thing/Locations relink) correctly stay 200 -- update_entity() is never called in that case, so there's nothing to have failed; handlers key off `is False` specifically, not falsy, to keep that distinction. Verified live against the real dev database, not just mocked tests: a throwaway viewer got 403 with the row provably unchanged, an editor/admin PATCH on the same row still succeeded and changed it, and an association-only PATCH stayed 200. New regression tests (test_patch_rls_silent_write_failure.py) reproduce the same three cases against a real RLS-scoped session, mirroring test_rls_enforcement.py's rollback-only pattern.
POST /Users/{id}/activate was the one path into an active RBAC role
that never wrote an AuditLog row. create_pending_oidc_user() already
logs RESTRICTED_REQUEST on signup, and PATCH .../policy-approval (the
local-registration equivalent of this endpoint) already logs
ADMIN_APPROVAL -- this endpoint was the gap, verified by grep before
assuming it, not found by inspection alone.
Adds the same log_audit_event() call admin_approval.py already makes,
inside the same transaction as the role UPDATE so a logging failure
rolls back the activation too. dataset_id/odrl_policy_id are read off
the user's own row (collected at /auth/{provider}/login) rather than
asked for again in the request body.
Verified against a live database: unit test proves the row is written
with the correct actor_id/dataset_id/odrl_policy_id/payload, and a
second test proves a 409 (already-active target) does not fabricate
one. Also confirmed live via curl against the running dev stack --
POST /Users/{id}/activate now leaves a real, queryable ADMIN_APPROVAL
row naming the admin, the activated user, and the granted role.
The audit INSERT for anonymous reads ran before the role switch to guest, on the pool's base login role -- which was never granted INSERT on AuditLog (only 'user'/'sensor' from 003_audit_log.sql, and 'guest' itself from this branch's own 004_public_access.sql). The INSERT failed with InsufficientPrivilegeError on every single anonymous read; the swallowed exception left the transaction poisoned, so the very next statement (SET LOCAL ROLE guest itself) then failed too, uncaught, 500ing the request. Confirmed live: this is why CI conformance was failing on this branch and on everything stacked on top of it (the OGC 18-088 conformance suite runs unauthenticated by default). Swapping the order fixes both problems at once -- guest already has the INSERT grant, so the audit write now succeeds instead of silently failing, and the actual data query is untouched by this change. Verified: 92/92 tests pass; live curl against a running stack shows clean 404s/200s instead of 500s on repeated anonymous requests, and a real PUBLIC_READ row now lands in AuditLog for each one.
…ternal-identity-providers-rls-fix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Title:
feat(auth): external identity providers (OIDC) + session-scoped RLS fixBase:
feat/public-access(#201) — merge that first.Fixes a real RLS bug and adds external login (Google, Microsoft, GitHub, ORCID, SWITCH edu-ID).
Every RLS policy was scoped
TO <username>, but no session ever runs as an individual role —set_role()only issuesSET LOCAL ROLE <group>. So these policies could never match any session, for anyone (confirmed viapg_policy). Fixed by moving to static, group-scoped policies with per-user differentiation via a session variable (app.current_user_id), Supabase-style.Also adds provider-agnostic OIDC login (
/auth/{provider}/login|callback) — external identities land in the samependingapproval queue asPOST /Register, no bypass. Includes dataset-scoping migrations (005, 006) and untracks.envto stop real secrets from landing in the repo.