fix(backend): refresh claims, startup table defs, tz-aware datetime, logged errors, safe admin seed (OM4/M1/M2/M3/H2)#62
Open
PenguinzTech wants to merge 1 commit into
Conversation
…time, logged errors, safe admin seed (OM4/M1/M2/M3/H2) - OM4: refresh grant (both /refresh and /oauth/token) now resolves tenant_id + scopes like the password grant, so refreshed access tokens keep their tenant/scope claims. - M1: define rbac + oidc tables (and the former inline create_user table) once at startup in init_db, eliminating the request-time check-then-define race on the shared DAL across executor threads. - M2: datetime.utcnow()/utcfromtimestamp() -> timezone-aware datetime.now(timezone.utc) in token exp/iat and expiry comparisons. - M3: replace silent except: pass around tenant/scope resolution and revoke storage with logged warnings (no PII/secrets), so degraded-permission failures are visible. - H2: in production, refuse to seed the default admin with the built-in weak password; require DEFAULT_ADMIN_PASSWORD to be set or skip seeding. Dev keeps the convenience default with a prominent warning. Suite: 467 passed, 90.44% coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reviewer's GuideBackend hardening PR that makes refresh-token grants preserve tenant/scopes, defines RBAC/OIDC tables eagerly at startup, switches token timestamps and expiry checks to timezone-aware datetimes, replaces previously-silent exception swallowing with structured logging, and prevents seeding a production admin user with a built-in weak password, plus tests for these behaviors. Sequence diagram for refresh token grant preserving tenant and scopessequenceDiagram
actor Client
participant AuthRefresh as refresh
participant DB as get_db
participant RBAC as _define_rbac_tables
participant Scopes as get_user_scopes
participant Tokens as create_access_token
participant Refresh as create_refresh_token
participant AsyncDB as run_sync
participant Revoke as revoke_refresh_token
Client->>AuthRefresh: POST /auth/refresh
AuthRefresh->>AsyncDB: run_sync(revoke_refresh_token, token_hash)
AsyncDB->>Revoke: revoke_refresh_token(token_hash)
Revoke-->>AsyncDB: done
AsyncDB-->>AuthRefresh: done
AuthRefresh->>AsyncDB: run_sync(_resolve_tenant, user.id)
AsyncDB->>DB: get_db()
DB-->>AsyncDB: db
AsyncDB->>RBAC: _define_rbac_tables(db)
RBAC-->>AsyncDB: tables ready
AsyncDB-->>AuthRefresh: tenant_id, tenant_slug
AuthRefresh->>AsyncDB: run_sync(get_user_scopes, user.id, tenant_id)
AsyncDB->>Scopes: get_user_scopes(user.id, tenant_id)
Scopes-->>AsyncDB: user_scopes
AsyncDB-->>AuthRefresh: user_scopes
AuthRefresh->>Tokens: create_access_token(user.id, user.role, tenant_id, tenant_slug, scopes=user_scopes)
Tokens-->>AuthRefresh: access_token
AuthRefresh->>AsyncDB: run_sync(create_refresh_token, user.id)
AsyncDB->>Refresh: create_refresh_token(user.id)
Refresh-->>AsyncDB: new_refresh_token, refresh_expires
AsyncDB-->>AuthRefresh: new_refresh_token, refresh_expires
AuthRefresh-->>Client: JSON(access_token, new_refresh_token)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The default-admin seeding logic is now implemented both in
app.models._seed_default_adminandrun.create_default_adminwith slightly different behaviors (logging vs prints); consider consolidating this into a single helper or ensuring one is the canonical path to avoid drift. - The tenant and scope resolution logic for refresh/password grants and login (e.g.,
_resolve_tenantinauth.refresh,auth.login, andoauth._handle_*_grant) is nearly identical; extracting a shared helper to encapsulate this behavior would reduce duplication and make future changes easier. - In
create_user, the new guard onuser_role_assignmentssilently skips inserting a global role when the table is missing; if this situation is unexpected in normal operation, you might want to log a warning to make misconfigured RBAC setups visible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The default-admin seeding logic is now implemented both in `app.models._seed_default_admin` and `run.create_default_admin` with slightly different behaviors (logging vs prints); consider consolidating this into a single helper or ensuring one is the canonical path to avoid drift.
- The tenant and scope resolution logic for refresh/password grants and login (e.g., `_resolve_tenant` in `auth.refresh`, `auth.login`, and `oauth._handle_*_grant`) is nearly identical; extracting a shared helper to encapsulate this behavior would reduce duplication and make future changes easier.
- In `create_user`, the new guard on `user_role_assignments` silently skips inserting a global role when the table is missing; if this situation is unexpected in normal operation, you might want to log a warning to make misconfigured RBAC setups visible.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
Security fixes (Track A5b) — backend hygiene
Stacked on #61 (base =
fix/token-revocation-ratelimit). Targetsrelease/v0.1.x. (Companion to A5a #60, which covers licensing/OIDC-secret/provider gating on the OIDC branch.)/refreshand/oauth/token) resolves tenant_id + scopes like the password grant, so refreshed tokens keep their claims (previously de-scoped/de-tenanted).create_usertable) are defined once at startup ininit_db, eliminating the request-time check-then-define race on the shared DAL across executor threads.datetime.utcnow()/utcfromtimestamp()→ timezone-awaredatetime.now(timezone.utc)in token exp/iat + expiry comparisons.except Exception: passaround tenant/scope resolution + revoke storage → logged warnings (no PII/secrets), so degraded-permission failures are visible.DEFAULT_ADMIN_PASSWORDor skips seeding. Dev keeps the convenience default with a warning.Tests
Suite: 467 passed, 90.44% coverage.
🤖 Generated with Claude Code
Summary by Sourcery
Harden backend auth and token handling by preserving claims on refresh, making token timestamps timezone-aware, removing request-time table definition races, logging previously silent failures, and enforcing safer default-admin seeding behavior.
Bug Fixes:
Enhancements:
Tests:
Chores: