Skip to content

Password Updates & Input Validation for Local Users - #189

Open
KinshukSS2 wants to merge 12 commits into
istSOS:mainfrom
KinshukSS2:feat/password-updates
Open

Password Updates & Input Validation for Local Users#189
KinshukSS2 wants to merge 12 commits into
istSOS:mainfrom
KinshukSS2:feat/password-updates

Conversation

@KinshukSS2

@KinshukSS2 KinshukSS2 commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Password Updates & Input Validation for Local Users

Stacked on: feat/identity-linking-jit-provisioning — requires the auth_provider column from that migration. Please merge that first, or set it as the base branch for this PR.

Why

Local users needed a way to change their istSOS password via the API. Since the codebase now supports both local and external OIDC users sharing the same User table, the endpoint explicitly guards against OIDC users attempting to update a local credential — a concept that does not apply to them.

Passwords are stored as bcrypt hashes in a new password column on sensorthings."User" and are hashed entirely on the Python side using passlib. The login flow (/Login) and the password-update flow both verify and write credentials against this column via the shared connection pool.

For accounts created before this migration (password IS NULL), a transparent JIT (Just-In-Time) upgrade path is provided: legacy credentials are verified against PostgreSQL's pg_authid via a direct connection, and the bcrypt hash is written immediately on first successful authentication — zero disruption, no admin intervention required.

What changed

File Change
database/migrations/002_user_password_column.sql New migration — adds nullable password VARCHAR(255) column to sensorthings."User" for bcrypt hashes
api/app/oauth.py get_auth_connection() retained as a legacy fallback; authenticate_user() now checks bcrypt hash for modern accounts and falls back to pg_authid + JIT hash write for legacy accounts; OIDC guard added
api/app/db/password_crud.py update_local_password() — OIDC guard, bcrypt verify for modern accounts, pg_authid fallback for legacy accounts (simultaneous upgrade); parameterised UPDATE
api/app/v1/endpoints/create/user.py Registration writes bcrypt hash to User.password immediately after INSERT (same transaction)
api/app/models/password.py PasswordUpdateRequest Pydantic v2 schema — enforces min 12 chars, 1 uppercase, 1 digit via @field_validator
api/app/v1/endpoints/update/password.py PATCH /Users/{id}/password — owner-or-admin authorization guard, returns 204
api/tests/test_password_update.py 11 tests — schema validation, CRUD guards including legacy-path upgrade; all pass without a live database
api/tests/test_oauth_connection_leak.py 6 tests — authenticate_user() modern + legacy paths, OIDC guard, JIT hash-write assertion
requirements-test.txt Added passlib[bcrypt]==1.7.4

Edge cases handled

  • Weak password → 422 — Pydantic rejects before any DB is touched
  • OIDC user → 400 — auth_provider IS NOT NULL check fires immediately: "External identities cannot update passwords locally."
  • Wrong current password → 401 — verified via pwd_context.verify() (modern) or get_auth_connection() fallback (legacy)
  • Non-owner / non-admin → 403 — users cannot change someone else's password
  • Legacy local user (password IS NULL) → JIT upgrade — authenticated via PostgreSQL pg_authid fallback; bcrypt hash written inline so the next login uses pwd_context.verify() directly
  • OIDC user attempts /Login → 401 — auth_provider IS NOT NULL guard in authenticate_user() returns None

How to test

# 1. Run unit tests (no live DB needed)
cd api && python3 -m pytest tests/test_password_update.py tests/test_oauth_connection_leak.py -v
# → 17 passed

# 2. Apply the migration
psql -U <admin_user> -d <db> -f database/migrations/002_user_password_column.sql

# 3. Happy path — local user changes their own password
curl -X PATCH http://localhost:8018/v1.1/Users/<id>/password \
  -H "Authorization: Bearer $TOKEN" \
  -H "Content-Type: application/json" \
  -d '{"current_password": "OldPass1!", "new_password": "NewSecure1Pass!"}'
# → 204 No Content

# 4. Legacy user — first login triggers JIT upgrade (password IS NULL before)
curl -X POST http://localhost:8018/Login \
  -d "username=legacyuser&password=TheirOldPass1!"
# → 200 OK + JWT; User.password now contains bcrypt hash

# 5. OIDC user blocked
# Set auth_provider = 'google' on a test user row, then attempt the same call
# → 400 Bad Request: "External identities cannot update passwords locally."

# 6. Weak password rejected
curl -X PATCH http://localhost:8018/v1.1/Users/<id>/password \
  -H "Authorization: Bearer $TOKEN" \
  -H "Content-Type: application/json" \
  -d '{"current_password": "OldPass1!", "new_password": "weak"}'
# → 422 Unprocessable Entity

@massimiliano-cannata

massimiliano-cannata commented Jun 9, 2026 via email

Copy link
Copy Markdown
Member

@KinshukSS2

KinshukSS2 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the clarification.I have updated the PR to use application-layer credentials (passlib bcrypt hashes) and removed all PostgreSQL password logic entirely.

I also refactored oauth.py to authenticate against these new hashes instead of pg_authid and implemented a JIT migration to seamlessly upgrade legacy test users upon their next login.

@KinshukSS2
fix(rbac): auto-create RLS policy on user creation
1ea2f91 
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.
@KinshukSS2
feat(auth): implement identity linking & JIT provisioning for OIDC users
8b4e82c 
- 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
@KinshukSS2
feat(auth): password update endpoint with OIDC guard & input validation
e787657 
- 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)
@KinshukSS2
refactor(auth): pivot password update to app-level bcrypt credentials
f7572e8 
The original implementation incorrectly treated each istSOS user as an
individual PostgreSQL login role, verifying passwords via asyncpg.connect()
and changing them with ALTER USER ... WITH ENCRYPTED PASSWORD DDL.

Mentor feedback clarified that istSOS users are strictly application-level
entities. The backend connects to PostgreSQL via a single master service
account (ISTSOS_ADMIN); individual users have no PostgreSQL login role.

This commit pivots to Python-side passlib/bcrypt credential management:

Schema:
- Add migration 002: nullable password VARCHAR(255) column in
  sensorthings.User to store bcrypt hashes.

Registration (create/user.py):
- After INSERT, hash the plaintext password with pwd_context.hash() and
  store the bcrypt hash in the new User.password column. The CREATE USER
  DDL is kept because PostgreSQL roles are still required for RLS.

Password update (password_crud.py):
- Remove asyncpg.connect()-as-user and ALTER USER DDL entirely.
- SELECT now includes the password column.
- Verify via pwd_context.verify(current_password, stored_hash).
- Hash new password via pwd_context.hash(new_password).
- Persist with parameterised UPDATE sensorthings.User SET password = $1.
- Add HTTP 400 guard for accounts with no local credential (NULL password).

Tests (test_password_update.py):
- Remove all asyncpg.connect mocks.
- Mock pwd_context.verify / pwd_context.hash instead.
- Add test for NULL password guard (new 400 case).
- Happy-path assertion now verifies UPDATE (not ALTER) with hash as param.
- Total: 10 tests, all passing.

Docs: update model and endpoint docstrings to say local istSOS credential
instead of PostgreSQL password.
@KinshukSS2
refactor(auth): remove pg_authid dependency from login flow
11bcba2 
Replace asyncpg.connect()-as-user authentication in oauth.py with
Python-side passlib/bcrypt verification against the application-level
sensorthings."User"."password" column.

Changes:
- Delete get_auth_connection() context manager entirely
- Rewrite authenticate_user() to use a single pooled SELECT that reads
  id, role, and password, then verifies via pwd_context.verify()
- Remove asyncpg, POSTGRES_DB/HOST/PORT imports from oauth.py (no longer
  needed — all auth is now pool-based)
- NULL password (OIDC user or pre-migration account) returns None → 401
  at the login endpoint, avoiding a crash or misleading error
- Replace test_oauth_connection_leak.py (which asserted asyncpg.connect
  was called) with TestAuthenticateUser covering: unknown user, NULL hash,
  wrong password, correct password, and a regression guard confirming
  asyncpg.connect is never invoked in the new flow

15 tests passing (10 password update + 5 oauth).
@KinshukSS2
feat(auth): add JIT credential migration for legacy accounts
3aeee8d 
Legacy local accounts (User.password IS NULL) previously blocked login
and the password-update endpoint. This commit adds a transparent
Just-In-Time upgrade path so these accounts work without any manual
migration step.

oauth.py — authenticate_user():
- Restored get_auth_connection() as a legacy fallback helper
- Added OIDC guard: auth_provider IS NOT NULL → return None
- Modern accounts (password IS NOT NULL): verify via pwd_context.verify()
- Legacy accounts (password IS NULL): fall back to get_auth_connection();
  on success, write bcrypt hash inline (pool UPDATE) so the next login
  uses the fast bcrypt path directly

password_crud.py — update_local_password():
- Removed HTTP 400 block for NULL password accounts
- Modern accounts: verify via pwd_context.verify()
- Legacy accounts: verify via get_auth_connection() fallback; the new
  bcrypt hash written by the UPDATE doubles as the migration

Tests:
- test_oauth_connection_leak.py: +2 tests (legacy wrong/correct password,
  JIT hash-write assertion); +1 OIDC guard test → 6 total
- test_password_update.py: replaced null-password 400 test with
  legacy wrong-password 401 + legacy upgrade UPDATE tests → 11 total
- Total: 17 passed
@KinshukSS2
KinshukSS2 force-pushed the feat/password-updates branch from 5896332 to 3aeee8d Compare July 21, 2026 13:21
@KinshukSS2
feat(auth): move password strength rule to shared validate_password_s…
39b808b 
…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.
@KinshukSS2
refactor(rbac): deduplicate POLICY_FN_MAP into rbac_roles.py
f122910 
Both create/user.py and create/activate_user.py defined their own
local _POLICY_FN_MAP -- activate_user.py's copy even carried a comment
admitting the exact risk this fix closes: "Must stay in sync with
create/user.py's _POLICY_FN_MAP." Never actually enforced.

Moves the map to rbac_roles.py as the single source of truth; both
files now import it. Adds an explicit WARNING that this whole map is
legacy scaffolding pending the move to dynamic ODRL-based policy
generation.
@KinshukSS2
test(auth): update test_password_update.py for the new 8-char/digit/s…
2bc10a7 
…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.
@KinshukSS2
Merge upstream/main into feat/password-updates
7424a0c 
Upstream (789d3fd) deleted test_oauth_connection_leak.py as obsolete.
This branch had already replaced its contents with a real unit-test
suite for authenticate_user(), including the legacy-password JIT bcrypt
upgrade path this PR introduces, so the rewritten file is kept.
@KinshukSS2
test: remove test_rls_policy_creation.py, a false-confidence test
aedeb3f 
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.
@KinshukSS2
fix(auth): DELETE /Users deactivates instead of hard-deleting
37a07db 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@KinshukSS2 @massimiliano-cannata