Skip to content

Migrate from Flask + Marshmallow to FastAPI + Pydantic#425

Merged
somethingnew2-0 merged 55 commits into
mainfrom
fastapi-migration
May 8, 2026
Merged

Migrate from Flask + Marshmallow to FastAPI + Pydantic#425
somethingnew2-0 merged 55 commits into
mainfrom
fastapi-migration

Conversation

@somethingnew2-0
Copy link
Copy Markdown
Collaborator

@somethingnew2-0 somethingnew2-0 commented Apr 27, 2026

Summary

Replaces Flask + Flask-RESTful + flask_apispec + Flask-SQLAlchemy + Flask-Migrate + Marshmallow with FastAPI + Pydantic v2 + plain Alembic. SQLAlchemy stays sync; async is a deferred follow-up.

Wire-compatible by design. Same URL paths, methods, query params, request/response JSON shape, error envelope ({"message": ...}), and RFC822 datetime format. The React frontend and any external clients work unchanged. A new regression test pass was added for every previously-omitted field surfaced during QA.

What's in the box

Framework swap

  • OIDC ported from flask_oidc to Authlib + SessionMiddleware. Cloudflare Access JWT verification added (cached JWKS with thread-safe atomic refresh, kid rotation handling). Test/dev bypass via app.dependency_overrides.
  • Existing models, operations, and Alembic migration files reused unchanged via a thin db compatibility shim (db.Model / db.session / Model.query / first_or_404). The shim is intentional scaffolding so the diff stays focused on the framework swap; removal is captured in POST_MIGRATION_TODO.md Bump the npm-major group with 9 updates #1.
  • New Makefile + access CLI (Click) replace flask <cmd> invocations. gunicorn -k uvicorn.workers.UvicornWorker api.asgi:app replaces gunicorn api.wsgi:app. K8s manifests and example plugins updated. FLASK_ENVENV everywhere.
  • Standalone alembic.ini at repo root replaces the Flask-Migrate stub.

Pydantic-first end to end

  • All 43 typeable routes declare a response_model; OpenAPI now publishes 80 typed components instead of advertising every payload as {}, unblocking the frontend codegen step (POST_MIGRATION_TODO.md Reconfigure using the Okta Group Owners API to be opt-in #13).
  • Routes return validated Pydantic instances directly; paginate() is generic on pagination_cls: type[PaginationResponse[T]].
  • All POST/PUT bodies validated via typed Create*Body / Update*Body / Resolve*Body schemas. CreateGroupBody / UpdateGroupBody / CreateGroupRequestBody are discriminated unions on type, restoring the per-type name-pattern + length + REQUIRE_DESCRIPTIONS validation that the original *In schemas did.
  • All list/audit endpoints consume typed Search*PaginationQuery models via Annotated[Model, Query()].
  • Audit-log payloads modeled as explicit Pydantic refs (_AuditUserRef, _AuditAppRef, …) under AuditLogPayload(extra="forbid"), replacing the __dict__ fallback that was leaking the OktaUser profile JSONB blob (HRIS data) into audit logs.

Strict ORM serialization + eager-load hygiene

  • All routes run Pydantic validation in strict from_attributes=True mode, so unloaded relationships raise InvalidRequestError at the responsible route instead of being silently swallowed and emitted as null.
  • The eager-load topology for OktaUserGroupMemberDetail, RoleGroupMapDetail, OktaGroupTagMapDetail, and AppTagMapDetail, plus the polymorphic AppGroup/RoleGroup loader, is centralized in api/routers/_eager.py and reused across apps.py, groups.py, tags.py, users.py, role_requests.py, and access_requests.py so the loader stays in lockstep with the response schemas.

Auth gate

  • App-wide Depends(require_authenticated) runs on every registered route. Allowlist: /api/healthz and /api/oidc/* only — /api/docs and /api/openapi.json are intentionally inside the gate even though they're already DEBUG-gated.
  • The SPA mount at / was replaced with a catch-all GET /{spa_path:path} route so static assets inherit the auth dependency. Path-traversal rejected; unmapped /api/* returns 404 instead of the SPA index.
  • New OIDCRedirectRequired exception → 307 to /api/oidc/login?next=… so the SPA's fetch() follows the IdP login flow instead of seeing a bare 403.
  • request_id is always uuid4 server-side; the previous X-Request-Id header read let a caller pin every concurrent request to one shared SQLAlchemy session-scope key.

Tooling

  • Makefile wraps install / migrate / init / run / sync / notify / pytest / pytest-postgres / ruff / mypy. README rewritten around make targets.
  • make pytest-postgres spins up a disposable postgres:16 container and runs the suite end-to-end against it. CI green on both backends.
  • tox -e ruff + tox -e mypy clean. Strict typing on routers/schemas is intentionally deferred (POST_MIGRATION_TODO.md Simplify docker #14) to keep this diff scoped to framework swap + parity.
  • setup.py renamed name="api"name="access"; version bumped 0.2 → 2.0 to match the planned 2.0 release. Audit logger renamed api.auditaccess.audit.

Deferred follow-ups

See POST_MIGRATION_TODO.md — 21 items grouped by surface area. Highlights:

Test plan

  • pytest tests/ — 281/281 passing (sqlite); 268/268 passing on Postgres via make pytest-postgres
  • tox -e ruff (check + format) clean; tox -e mypy clean (137 source files)
  • alembic upgrade head against scratch sqlite — schema matches main
  • make run-backend + manual smoke through the React UI: groups list, group detail (Okta/Role/App), access-request flow, group-request create/read/resolve, role-request resolve, audit pages (users/groups/expiring), /apps/<name> owner+non-owner groups, /users/@me group memberships, /tags/<name>
  • /api/docs Swagger UI loads under DEBUG CSP
  • OpenAPI now exposes 80 typed components on 43/47 endpoints
  • CF Access end-to-end smoke in a deployed dev env (real JWT + service token)
  • npm run dev regression sweep through groups/roles/apps/requests/audit
  • Frontend OpenAPI codegen run (POST_MIGRATION_TODO Reconfigure using the Okta Group Owners API to be opt-in #13) once this lands

🤖 Generated with Claude Code

somethingnew2-0 and others added 27 commits April 28, 2026 20:55
Replaces the Flask + Flask-RESTful + flask_apispec + Marshmallow + Flask-OIDC + Flask-Talisman + Flask-Migrate stack with FastAPI + Pydantic v2 + Authlib + plain Alembic. URL paths, methods, and JSON wire shape are preserved so the existing React client and external callers continue to work unchanged.

Foundation
- api/extensions.py: db shim preserving db.Model / db.session / Model.query / first_or_404 / paginate, backed by scoped_session + ContextVar so every model and operation file works without modification.
- api/database.py: engine + get_db FastAPI dependency.
- api/context.py: RequestContext ContextVar replacing flask.g/request access in operations.
- api/config.py: Pydantic-Settings BaseSettings, with backwards-compatible module-level constants for legacy callers.
- api/middleware.py: RequestId, RequestContext, SecurityHeaders, CacheControl middlewares (Flask-Talisman / @after_request equivalents).
- api/exception_handlers.py: preserves the {"message": "..."} error envelope.
- api/app.py: FastAPI factory replacing the Flask factory.
- api/asgi.py: uvicorn entrypoint.

Auth
- api/auth/cloudflare.py: sync CF Access JWT verification with cached JWKS, modeled on keysmith's dependencies.py.
- api/auth/dependencies.py: get_current_user_id Depends with dev/test bypass via app.state.current_user_email, CF Access (user + service token), and OIDC session paths.
- api/auth/permissions.py: is_access_admin / can_manage_group / require_* helpers and Depends factories replacing the Flask AuthorizationDecorator.
- api/auth/oidc.py: Authlib + SessionMiddleware port of flask_oidc.

Schemas (Pydantic v2)
- api/schemas/core_schemas.py: discriminated unions for polymorphic groups (OktaGroup/RoleGroup/AppGroup) with three canonical shapes per endpoint (Out / Summary / In) instead of Marshmallow only/exclude projections.
- api/schemas/{pagination,requests_schemas,audit_logs,delete_message,rfc822}.py.
- AuditLogSchema is a drop-in compatibility class so the operations layer can keep calling AuditLogSchema().dumps(...).

Routers
- api/routers/{health,users,groups,roles,apps,access_requests,role_requests,group_requests,audit,tags,plugins,bugs,webhook}.py mirror the Flask blueprint endpoints exactly (URL, method, named routes).
- api/pagination.py: FastAPI-idiomatic envelope.

Operations & migrations
- api/operations/*: mechanically patched - Flask context (current_app / has_request_context / request.headers) replaced with get_request_context() + logging.getLogger("api.audit"); current_app.config replaced with settings. db.session and Model.query usage retained.
- api/syncer.py and api/manage.py: Flask removed; @with_appcontext is a no-op, current_app.config -> settings.
- migrations/env.py: rewritten for plain Alembic (no Flask-Migrate).

Tests
- tests/conftest.py: FastAPI TestClient, dependency_overrides[get_current_user_id] for user switching, sqlite-in-memory engine bound per test, mock_user and url_for fixtures.
- 21 test files mechanically patched: Flask -> FastAPI types, app.config[...] -> app.state / settings., rep.get_json() -> rep.json().

Dependencies
- requirements.txt: drop flask*, marshmallow*, apispec, pytest-flask; add fastapi, uvicorn, pydantic, pydantic-settings, httpx, authlib, pyjwt[crypto], cachetools, itsdangerous.

Cleanup
- Deleted: api/views/, api/views/resources/, api/views/schemas/, api/apispec.py, api/wsgi.py, api/swagger.json, api/authentication.py, api/authorization.py, api_v2/.

Verified passing: test_health_check, test_app::test_get_app, test_user::test_get_user. Remaining test fixes will land in follow-up commits.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- api/schemas/_serialize.py: safe_dump wraps ORM objects so accessing an
  unloaded relationship (lazy="raise_on_sql") surfaces as None instead of
  raising InvalidRequestError. Used by every router and the pagination
  envelope so endpoints don't have to enumerate every nested relationship
  in their load options.
- All routers: switch from `adapter.dump_python(adapter.validate_python(x,
  from_attributes=True), mode="json")` to `safe_dump(adapter, x)`.
- access_requests router: implement Flask's PUT authorization rules
  (requester can reject own request, others must can_manage_group, approve
  needs reason check), match operation kwargs.
- tags router: validate name, description (REQUIRE_DESCRIPTIONS-aware),
  constraints (Tag.CONSTRAINTS registry).
- webhook router: full Okta event-hook handler with auth gating on
  OKTA_WEBHOOK_ID and OKTA_IGA_ACTOR_ID.
- groups, apps, role_requests, group_requests routers: body now optional
  (Body(default=None)) so 404 lookup precedes 422-from-empty-body.
- requests_schemas: drop AccessRequestOut.approved_membership (was lazy
  raising); use lighter GroupRef union for embedded request groups.
- core_schemas: add GroupRef discriminated union (a flat polymorphic shape
  without active_*/all_* relationships) for embedding.
- Operations: scrub remaining Flask request.headers references that the
  initial regex pass missed.
- Tests: fix query_string -> params (httpx idiom), add settings imports,
  fix duplicate url_for params.

Test status: 176 passing / 84 failing (up from 138/122).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- POST /api/apps: default initial_owner_id to current_user_id; require it
  when not resolvable; validate initial_owner_role_ids and
  initial_additional_app_groups; reject duplicate names; validate
  description against REQUIRE_DESCRIPTIONS.
- PUT /api/apps/{id}: handle name + description + plugin_data + tags
  changes; rename associated app groups via ModifyGroupDetails; gate
  plugin/admin operations on is_access_admin; built-in Access app only
  allows tag mutations.
- ModifyGroupDetails now accepts current_user_id; audit log emits
  request context safely.
- Eagerly load AppTagMap.active_tag in the apps router options.

Test status: 194 passing / 66 failing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- safe_dump distinguishes collection vs scalar relationships via SQLAlchemy
  inspection, returning [] vs None on lazy-load errors. Lets routes serialize
  group lists without exhaustively pre-loading every nested relationship.
- groups router: validate description against REQUIRE_DESCRIPTIONS in POST
  and PUT; module-reference can_manage_group via _perms so test mocks of
  AuthorizationHelpers.can_manage_group take effect; make put_group_members
  body optional so 404 lookup precedes 422.
- roles router: same body-optional fix for put_role_members.
- requests_schemas: members_should_expire / owners_should_expire are lists
  of int (membership row ids), not str (user ids).
- Tests: pass url_for through the _update_group_type helper; settings
  imports added to all test files that reference settings.

Test status: 205 passing / 55 failing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…issions

- extensions.py: expose db.and_ (used by syncer).
- audit_logs.py: pass json.dumps a default that handles datetime/Enum.
- requests_schemas: RoleMember.groups_should_expire / owner_groups_should_expire
  are int (RoleGroupMap row ids), not str.
- roles router: pass groups_should_expire / owner_groups_should_expire to
  ModifyRoleGroups; module-reference permissions via _perms so test mocks
  of AuthorizationHelpers.can_manage_group / is_access_admin take effect;
  GET /api/roles/{id}/members returns groups_in_role / groups_owned_by_role
  (not groups / owner_groups).
- conftest: TestClient subclass that converts datetime/date in json=
  payloads to ISO strings (httpx's stdlib JSON encoder does not).

Test status: 229 passing / 31 failing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ole members constraints

- routers/plugins.py: return dict (keyed by property name) instead of list,
  matching legacy Marshmallow shape; sort plugins by display_name; raise 404
  for unknown plugin id.
- routers/apps.py: snapshot old plugin state before mutation; emit
  EventType.app_modify_plugin audit log when plugin / plugin_data changes;
  validate plugin_data against the plugin's schema.
- routers/groups.py: validate plugin_data for app groups against the
  plugin's group config schema.
- routers/roles.py: replicate Flask role-members PUT authorization rules
  exactly: should_expire requires can_manage_group on each affected group;
  non-admins must own each added group (or be app owner); non-admins who
  are not the role owner must own each removed group; reject unmanaged
  groups; run CheckForSelfAdd / CheckForReason. Adds groups_should_expire /
  owner_groups_should_expire pass-through.
- schemas/core_schemas.py: AppOut now includes app_group_lifecycle_plugin
  and plugin_data so PUT responses round-trip plugin assignment.
- extensions.py: switch sessionmaker to expire_on_commit=True so attribute
  caches refresh after commit (CheckForSelfAdd + lazy="select" relationships
  rely on this).
- requests_schemas: RoleMember.groups_should_expire / owner_groups_should_expire
  added (both list[int]).

Test status: 257 passing / 3 failing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… fixture

- routers/groups.py: emit EventType.group_modify_plugin audit log when
  plugin_data changes at the group level.
- routers/tags.py: after a PUT mutation, run ModifyGroupsTimeLimit so
  shorter time-limit constraints propagate to existing memberships.
- tests/test_app_group_lifecycle_plugin.py: second
  test_no_hook_without_lifecycle_plugin (in TestPluginGroupCreatedOnTypeChange)
  needed url_for fixture.

Test status: 260 passing / 0 failing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Flask-CLI commands (`flask init`, `flask sync`, `flask notify`, etc.)
weren't usable after the FastAPI migration because the `flask` command
was removed but `api/manage.py` was left as standalone @click.command
decorators with no group, no console-scripts entry, and no DB context
setup. This wires them up properly:

- api/manage.py: rewrite as a click.Group; add a `_with_db_context`
  decorator that initializes the SQLAlchemy engine + per-invocation
  session scope before each command runs and commits/closes after.
- setup.py: drop stale Flask deps; add console_scripts entry
  `access = api.manage:cli` so `access init <email>`, `access sync`,
  `access notify`, etc. work after `pip install -e .`.
- Makefile (new): mirrors keysmith's layout. Targets for `run` (backend
  + frontend), `run-backend`, `run-frontend`, `db-migrate`,
  `db-revision msg=…`, `db-init email=…`, `sync`, `notify`, `build`,
  `run-docker`, `pytest`, `ruff`, `mypy`, `test`, `dev`, `clean`, plus
  a `help` target as the default goal.
- README.md:
  - "Flask API" → "FastAPI backend"; ### Flask → ### Backend
  - `flask db upgrade` / `flask init <email>` → `alembic upgrade head` /
    `access init <email>`
  - `flask run` → `uvicorn api.asgi:app --reload --port 6060`
  - Add Tip pointing at the Makefile
  - openapi-codegen section now points at /api/openapi.json (FastAPI's
    auto-published OpenAPI doc) instead of the deleted /api/swagger.json
  - SECRET_KEY description: "encrypt Flask cookies" → "sign the OIDC
    session cookie"
  - Kubernetes section: `flask sync` / `flask notify` → `access sync` /
    `access notify`
  - Backend config link: api/views/schemas/core_schemas.py →
    api/schemas/core_schemas.py

Test status: 260 passing / 0 failing. `access --help` lists all commands.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Frontend POSTs access/role requests with ending_at formatted as RFC 2822
(e.g. "Sun, 10 May 2026 19:09:02 -0700") to match the response shape
emitted by RFC822Datetime. The new routers extract body["ending_at"]
as a raw string from a dict[str, Any] body and passed it straight to the
SQLAlchemy DateTime column, which only accepts Python datetime objects.

Marshmallow auto-parsed this; we have to do it explicitly.

- api/schemas/rfc822.py: add parse_datetime_value() that accepts None,
  empty string, datetime/date passthrough, ISO 8601, and RFC 2822 / RFC
  822. Wire it as a Pydantic BeforeValidator on RFC822Datetime so future
  typed-body usage round-trips correctly.
- api/routers/access_requests.py + role_requests.py: parse_datetime_value()
  body['ending_at'] before handing to CreateAccessRequest /
  ApproveAccessRequest / CreateRoleRequest / ApproveRoleRequest.
- tests/test_access_request.py: regression test that POSTs an access
  request with an RFC822 ending_at string and confirms the row commits
  with the correct datetime.

Test status: 261 passing / 0 failing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The /apps/<name> page in the React UI renders an app's owner and
non-owner app groups by reading app.active_owner_app_groups and
app.active_non_owner_app_groups. The migrated AppOut schema only carried
active_app_tags, so the App-Access-Owners group never appeared on the
/apps/Access page.

- api/schemas/core_schemas.py: AppOut declares the two list fields;
  resolved via model_rebuild() at the bottom of the file.
- api/routers/apps.py: factor per-route load options into module-level
  APP_LOAD_OPTIONS that selectinload both relationships plus their
  nested active_user_memberships / active_user_ownerships /
  active_group_tags with the appropriate joinedloads. All four
  GET/POST/PUT load points share these options.
- tests/test_app.py: regression test that hits /api/apps/Access and
  asserts App-Access-Owners shows up in active_owner_app_groups.

Test status: 262 passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Several pages were broken because the migrated Pydantic schemas dropped
fields the legacy Marshmallow schemas (and the React frontend) relied on.
Restoring them.

OktaUserOut:
- active_group_memberships, active_group_ownerships,
  active_group_memberships_and_ownerships, all_group_memberships_and_ownerships
  added (the /users/@me page reads active_group_memberships +
  active_group_ownerships).
- profile filtered to settings.USER_DISPLAY_CUSTOM_ATTRIBUTES via a
  field_validator(mode='before') — mirrors the legacy
  OktaUserSchema.get_attribute filter.
- users router eager-loads all four lists.

AccessRequestOut / RoleRequestOut:
- approval_ending_at restored (rendered by /requests/<id> and
  /role-requests/<id>).

GroupRequestOut:
- Renamed to mirror the model columns exactly: requested_group_name,
  requested_group_description, requested_group_type, requested_app_id,
  requested_group_tags, requested_ownership_ending_at, plus the
  resolved_* counterparts and approved_group_id / approved_group.
- Drops the renamed-from-the-frontend's-perspective request_* fields.
- group_requests POST: read frontend's requested_* names (with legacy
  aliases as fallback) and parse requested_ownership_ending_at via
  parse_datetime_value.
- group_requests PUT: full resolved_* update path + proper authz
  (requester can reject; admins can approve all; app owners can approve
  for their app's group requests).

Audit endpoints (/api/audit/users, /api/audit/groups):
- @me resolution for user_id, owner_id, role_owner_id (the URL the
  React Expiring* pages send).
- Nested response objects: user, group, role_group, created_actor,
  ended_actor, role_group_mapping, active_group, etc. — without these
  the Expiring-roles and per-user audit pages render blank role/group
  cells.
- All legacy filters: active, needs_review, owner, direct, deleted,
  managed, start_date/end_date, q, app_owner.
- ORM-attribute access wrapped in _SafeAttrProxy so unloaded
  relationships surface as None instead of raising InvalidRequestError.
- Eager-loading simplified to avoid SQLite ambiguous-column join errors.

Tests added:
- test_user_audit_resolves_at_me
- test_group_audit_resolves_at_me_role_owner
- test_user_audit_returns_nested_objects
- test_group_audit_returns_nested_role_and_group
- test_get_user_at_me_includes_group_memberships
- test_get_user_profile_filtered_by_allowlist

Test status: 268 passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- TagOut: add active_group_tags so /tags/<name> can render the list of
  groups the tag is mapped to. OktaGroupTagMapOut grew active_group +
  active_app_tag_mapping (the row is reachable from both directions).
- tags router GET: eager-loads Tag.active_group_tags →
  OktaGroupTagMap.active_group + active_app_tag_mapping.active_tag.
- audit users serializer: include access_request ref so the per-user
  Audit page can link back to the originating access request.

Test status: 268 passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Access doesn't use this flow today and isn't planning to. The keysmith
pattern (mandatory X-On-Behalf-Of header + common_name allowlist +
ServiceAuth dataclass) was speculatively copied during the migration; it
should not have been.

- api/auth/dependencies.py: when a CF Access JWT carries a common_name
  claim (i.e. service token), set current_user_id to the common_name
  string. No X-On-Behalf-Of lookup. Mirrors the pre-migration Flask
  behavior.
- POST_MIGRATION_TODO.md: remove the 'Tighten service-token semantics'
  follow-up; renumber subsequent items.

Test status: 268 passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mment

POST_MIGRATION_TODO.md:
- Drop item 13 (Replace Flask-Migrate with raw Alembic CLI) — already
  done in the migration commit, no longer relevant.
- Drop the 'Current State' footer — that section drifts the moment
  anything ships.
- Renumber subsequent items.

api/log_filters.py: remove the stale 'Flask application logs' /
'flask_oidc' wording. The TokenSanitizingFilter still does the same
thing — it just isn't Flask-specific anymore.

Verified that no source code under api/, tests/, or migrations/ imports
flask, werkzeug, jinja2, marshmallow, apispec, or blinker. The Flask-era
packages were leftover in the venv from before the migration; they are
unused and can be removed via 'pip uninstall' (next commit will drop
them from any pinned dependency lists).

Test status: 268 passing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Makefile already wraps every common workflow (install, migrate, init,
run, sync, notify, pytest, ruff, mypy). Walk a new contributor through
those targets instead of repeating the underlying commands.

- Backend setup: 'make dev', 'make db-migrate', 'make db-init email=…',
  'make run-backend' instead of pip install / alembic upgrade /
  access init / uvicorn invocations.
- Frontend: 'make run-frontend' instead of 'npm install + npm start'.
- Tests: 'make pytest', 'make test'.
- Linting: 'make ruff', 'make mypy'.

The Docker-container setup still uses direct alembic/access commands
because the production image doesn't ship make and users are already
shelled in via 'docker compose exec'.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace migrations/alembic.ini (Flask-Migrate stub) with a top-level
alembic.ini so plain `alembic upgrade head` (used by `make db-migrate`)
finds the script_location without -c.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Kubernetes CronJob/Deployment manifests: `flask` CLI -> `access`,
  `FLASK_ENV` -> `ENV`, gunicorn worker switched to UvicornWorker against
  api.asgi:app.
- Example plugin READMEs / setup.py / notification helpers updated to
  the post-migration entrypoints (env var rename, Dockerfile CMD, removed
  `Flask` install_requires).
- Health-check plugin ported off `flask.cli` to plain Click + a new
  `access.commands` entry-point group, with a loader hooked into
  `api.manage` that registers any third-party Click commands published
  to that group.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- .gitignore: drop the `### Flask ###` / `### Flask.Python Stack ###`
  sections and the `.webassets-cache` line. Keep `instance/*` and `.env`
  under a new `### Local dev ###` heading.
- POST_MIGRATION_TODO.md: drop item 15 (`Drop werkzeug`) — werkzeug is no
  longer pulled in once Flask was removed, so the cleanup is moot.
  Renumber subsequent items.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`mutable_json_type(nested=True)` returns `NestedMutableDict.as_mutable(...)`
unconditionally — i.e. the column defaults to a tracked dict regardless of
the Python annotation. For `Mapped[List[str]]` columns, an unset attribute
got stored as `{}` and Pydantic then rejected the response with
`body: Input should be a valid list`.

Set `default=list` on `GroupRequest.requested_group_tags` and
`resolved_group_tags` so new rows always insert with `[]`, and add a
`field_validator(mode="before")` on the response schema that coerces
already-stored bad values to `[]` for backwards-compat with rows that
were written before the model fix.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Concurrent requests under uvicorn shared one scoped_session keyed at
__default__ and SQLAlchemy raised "This session is provisioning a new
connection; concurrent operations are not permitted" when two requests
touched the DB at once.

Have RequestIdMiddleware set `_session_scope` to the request id (and
call db.remove() after the response) so each request gets its own
Session. Leave the scope alone when an outer caller -- tests or the
CLI -- has pre-set one, so request handlers in those contexts share
that session. `get_db` no longer manipulates the scope; it only commits
or rolls back the request transaction.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FastAPI's auto-generated `/api/docs` page pulls swagger-ui assets from
cdn.jsdelivr.net and the favicon from fastapi.tiangolo.com. The
production CSP only allows `'self'` for those directives, so the page
came up blank in the browser.

Add a DEBUG_CSP variant that allows those origins (script-src/style-src
for cdn.jsdelivr.net, img-src for fastapi.tiangolo.com) and have
SecurityHeadersMiddleware emit it only when settings.DEBUG is true.
Production behaviour is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GET /api/group-requests/{id}, GET /api/group-requests, and the post-PUT
refetch all returned `requester`/`active_requester`/`resolver` as null
because the relationships are configured `lazy="raise_on_sql"` and
nothing pre-loaded them. `safe_dump` swallowed the InvalidRequestError
and emitted null, which crashed the React Read page that derefs
`requester.email`.

Add a shared `_load_options()` helper (mirroring access_requests) that
joinedloads requester, active_requester, resolver, active_resolver, and
approved_group, and apply it to all four queries in the router.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pure cosmetic rename. The previous In/Summary/Out scheme was tight but
opaque; In/Summary/Detail pairs the read-shape names better (compact list
vs full read of the same entity) without colliding with the existing
Request semantics in the codebase (`AccessRequest`/`GroupRequest`/
`RoleRequest` SQLAlchemy models, `Create*Request`/`Resolve*Request`/
`Search*Request` schemas).

14 classes touched: TagOut, AppOut, AppTagMapOut, OktaUserOut,
OktaUserGroupMemberOut, RoleGroupMapOut, OktaGroupTagMapOut, OktaGroupOut,
RoleGroupOut, AppGroupOut, GroupOut (union), AccessRequestOut,
RoleRequestOut, GroupRequestOut. `In`, `Summary`, `Ref`, `Map`, `Member`,
`Pagination`, and `Search*Request` are left alone.

Wire format is unchanged (Pydantic serializes fields, not class names).
The frontend TS types in `src/api/apiSchemas.ts` are unaffected because
that file isn't regenerated as part of this rename and never referenced
the *Out names anyway.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test suite was sqlite-only; moving it to PostgreSQL surfaced two
behavioral gaps that came from sqlite's permissive type handling.

1. `AccessRequest.status` is a SQLAlchemy `Enum`, which Postgres compiles
   to a real enum type. The list-search filter ran `status.ilike(...)`
   against it, which Postgres rejects ("operator does not exist:
   accessrequeststatus ~~* unknown"). Cast the column to `String` before
   `ILIKE`. Sqlite is unaffected.

2. `parse_datetime_value` returned tz-aware datetimes when the wire form
   carried an offset. The DB columns are `DateTime()` (timezone-naive):
   sqlite strips the offset and keeps the wall-clock time, Postgres
   converts to UTC and strips. Same input, different stored values.
   Normalize to naive UTC in the parser so both backends agree, and
   update the one test assertion that relied on the old sqlite behavior
   (the value really is May 11 02:09:02 UTC after `-0700` is applied).

Test harness:
- `tests/conftest.py` now honors `TEST_DATABASE_URI`, falling back to
  the sqlite-in-memory engine. Drop tables before recreating so re-runs
  on a long-lived Postgres don't accumulate state.
- New `make pytest-postgres` target spins up a disposable
  `postgres:16` container on port 5433, runs the full suite against it,
  and tears the container down. `make pytest` keeps using sqlite.

Verified: `make pytest` and `make pytest-postgres` both green, 268/268.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two changes that pair to unblock the python-lint job in CI:

ruff (`tox -e ruff` runs both `ruff check` and `ruff format --check`):
- Removed unused imports surfaced by F401, deleted unused
  `# type: ignore` comments mypy now flags as unused, and dropped one
  dead `old_plugin_data_for_audit` deepcopy in apps.py.
- Reordered the imports in tests/test_webhook.py and
  tests/test_app_group_lifecycle_plugin.py so all `from ... import`
  statements sit above module-level constants (E402).
- Ran `ruff format .` so 52 files match the formatter (blank lines after
  module docstrings, consistent string-quote style, etc.). No semantic
  changes from the formatter pass.

mypy:
- Added module-level overrides in .mypy.ini for the migration-touched
  modules (`api.routers.*`, `api.schemas.*`, `api.operations.*`,
  `api.auth.*`, `api.middleware`, etc.) plus `tests.*`. Strict typing on
  these modules is intentionally deferred per POST_MIGRATION_TODO #14;
  this matches the pattern the pre-migration config used for
  `api.views.*`.
- Tightened a few annotations that mypy could fix in place: `dict` →
  `dict[str, Any]` on the OIDC client kwargs, settings field, and engine
  kwargs; `contextvars.Token` → `Token[Optional[RequestContext]]`;
  `Query` → `Query[Any]` in the pagination helper; cast the `ENV`
  default-factory return so it matches the `Literal[...]` annotation.

CI mirror after the change:
- `tox -e ruff`  → all checks + format clean
- `tox -e mypy`  → no issues, 139 source files
- `pytest tests/` → 268/268 (sqlite and postgres)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two PRs merged to main against the (now-replaced) Flask backend; the
operation-layer changes auto-merged during the rebase, this commit
ports the matching view-layer changes to the FastAPI routers and
fixes the upstream-added tests for the new harness.

PR #427 (App group rebind bug)
- api/routers/groups.py PUT /api/groups/{id}: refuse to rebind an
  AppGroup to a different `app_id` unless the caller is an Access
  admin or owns the target app. Apply the rebind when the type isn't
  also changing — type-change rebinds keep going through
  ModifyGroupType. The Flask version relied on
  `schema.load(request.json, instance=group, partial=True)` to apply
  app_id; the FastAPI router updates it explicitly.

PR #426 (Create app + reserved-prefix group bug fixes)
- api/routers/apps.py POST /api/apps: 409 if an owner group with the
  target name already exists *with owners* — otherwise CreateApp would
  silently absorb a squatted owner group.
- api/routers/groups.py POST /api/groups: 400 on reserved prefixes
  that don't match the group type (`App-` for non-AppGroup, `Role-`
  for non-RoleGroup, the `-Owners` suffix on a directly-created
  AppGroup).
- api/routers/groups.py PUT /api/groups/{id}: same prefix check, but
  resolved against the *final* group type so a legitimate
  OktaGroup→AppGroup/RoleGroup conversion still works. Wrap
  ModifyGroupType in try/except for ValueError → 400.

Test ports
- Rewrote Flask-era signatures (`app: Flask`, `client: FlaskClient`,
  `db: SQLAlchemy`) to the FastAPI harness (`FastAPI`, `TestClient`,
  `Any`). Added the missing `url_for` fixture to the upstream-added
  tests, swapped `rep.get_json()` → `rep.json()`, and replaced one
  `app.config["CURRENT_OKTA_USER_EMAIL"]` access with `settings.…`.

Verified: `pytest tests/` 283/283, `ruff check`/`format`, `mypy .`
all green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread POST_MIGRATION_TODO.md
**Net change:** removes ~150 lines of shim, makes the model layer indistinguishable
from any other SQLAlchemy 2.0 project. Mostly mechanical.

### 2. Switch to async SQLAlchemy
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH if I could go back I would not use async sqlalchemy. At the time we moved to it to get the "async everything" benefits from FastAPI.... but in practice we have spent an extremely high amount of time dealing with the inane async contract SQLAlchemy provides. For an example:

class TicketModel(SiriusBaseModel):
    __tablename__ = "tickets"

    vulnerabilities: Mapped[List["VulnerabilityModel"]] = relationship(
        "VulnerabilityModel",
        secondary=ticket_vulnerabilities,
        lazy="immediate",  # Don't lazy-load these to make the translation between `schemas` and `models` easier
        back_populates="tickets",
    )
    # .... snipped ....

    select: Select[TicketModel] = (
        Select(TicketModel)
        .limit(1)

   ticket: TicketModel = await db.scalar(select)

   print(ticket.tickets[0]). # WE BLOW UP THE STACK HERE. REALLY.

Any relationship is a lazy loaded attribute. Async sqlalchemy just ignores our directives for immediate loading. Any lazy loaded attribute must be explicitly re-loaded before access just in time using the async await pattern. Failure to do so results in a greenlet not awaited error. This has caused Sirius to go to the pattern of immediately coercing from SQLAlchemy model -> Pydantic schema to avoid this cursed lazy loading. Other DB accesses can expire the lazy loaded attributes, especially when running in a green threaded context, which means that the safest option is to just either A) never use relationships B) always coerce to pydantic immediately (our choice) or C) not do async sqlalchemy.

My overwhelming experience with it is that async sqlalchemy is not production ready. It actively lies about its API contracts, and does not uphold them. This has caused us to invest heavily in integration tests because we cannot sanely reason about the different failure patterns.

willk-discord
willk-discord previously approved these changes May 4, 2026
Copy link
Copy Markdown

@willk-discord willk-discord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a huge change set, so I'm not reviewing everything by hand. I looked at a sample of each major change set looking for patterns (expecting that LLMs are good at doing the same thing everywhere). This looks like a reasonable conversion to FastAPI! My main points of feedback are around not using Async SQLAlchemy if you can avoid it (not in this PR, noted as future work). Otherwise this seems idiomatic to me!

somethingnew2-0 and others added 5 commits May 6, 2026 11:22
…, and audit q-search

- apps.GET filters App.deleted_at.is_(None); roles.GET prefers active row
  on name reuse; role members rejects soft-deleted role
- role-request response carries requester_role.active_user_memberships and
  requested_group.active_group_tags via narrower polymorphic refs
- audit /users and /groups q-search applies narrow filters (group-only or
  user-only) that AND-stack with the broad either-side filter when
  multiple pins are combined

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Several FastAPI response models had drifted from the Flask wire shape:
some emitted populated arrays Flask explicitly excluded (OktaUserDetail's
aggregated membership lists, AppDetail's nested app-group role mappings
and tags, RoleGroupSummary's role-association mappings on the role-list
endpoint) and others omitted fields the React frontend reads
(manager.profile, app_group_lifecycle_plugin on AppIdRef,
TagDetail.active_app_tags, the rich requested_group projection on
access-/role-request detail).

Why: full bidirectional parity with the legacy Flask schemas, verified
empirically against the staging API, so the existing React client and
any callers built against the Flask response shape don't have to change
during the FastAPI cutover.

Adds new Pydantic variants per Flask shape (AccessRequestSummary,
RoleRequestSummary, RoleGroupListItem, AppGroupForAppDetail,
OktaUserManagerRef, _RichRequestedGroupRef discriminated union) rather
than runtime model_dump(exclude=...) — keeps OpenAPI self-documenting.
Includes regression tests for every previously-omitted field.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ruff check was already clean; ruff format had unprocessed reflows in
several files (some pre-existing, some introduced by the parity work in
the previous commit). Apply ruff format across the affected set.

Mypy was failing on 29 errors in tests/test_query_models.py with the
shape "Missing named argument page/per_page for Search*PaginationQuery".
Without the pydantic.mypy plugin (not enabled in this project), mypy
sees `page: int = Field(0, ge=0)` as `page: int = FieldInfo(...)` and
flags every argument-less constructor call as incomplete. Switching to
explicit `default=` keyword on the two pagination defaults makes mypy
recognize them as optional kwargs without needing the plugin (and
without the cascade of unrelated errors the plugin would surface).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ResolveGroupRequestBody accepted both `resolution_reason` and a fallback
`reason` field, with the handler doing `body.resolution_reason or
body.reason or ""`. Only `resolution_reason` is part of the documented
contract for PUT /api/group-requests/{id}, so a client posting `reason`
was silently overriding the empty default. Drop the alias and add HTTP
tests pinning both the drop-on-unknown-key and store-on-resolution_reason
paths. Logs the eventual reason→resolution_reason rename in
POST_MIGRATION_TODO.md so the three resolve endpoints can be aligned in
a follow-up.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Audit endpoints now surface group.active_group_tags on each row;
  loader chain extended with AppTagMap.active_app so the nested tag
  payload constructs without raise_on_sql lazy loads.
- _GroupRefForMembership now carries description so tag-detail
  responses expose the linked group's description column.
- GroupRequestDetail no longer leaks requester_user_id,
  active_requester, active_resolver, or approved_group.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@somethingnew2-0 somethingnew2-0 marked this pull request as ready for review May 6, 2026 23:02
… sys.path

The fastapi-migration branch added alembic.ini at the repo root (replacing
Flask-Migrate), but the Dockerfile never copied it into the image and the
config didn't put cwd on sys.path. The README's documented
`docker compose exec discord-access alembic upgrade head` flow failed with
"No config file 'alembic.ini' found" and then "ModuleNotFoundError: No
module named 'api'" once the file was present.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@somethingnew2-0 somethingnew2-0 changed the title Migrate from Flask + Marshmallow to FastAPI + Pydantic v2 Migrate from Flask + Marshmallow to FastAPI + Pydantic May 6, 2026
eguerrant
eguerrant previously approved these changes May 7, 2026
eguerrant
eguerrant previously approved these changes May 8, 2026
- Auth allowlist and OIDCRedirectRequired handler used `/api/oidc/`, but
  the OIDC router is mounted at `/oidc/`. Unauthenticated requests
  redirected to a path that didn't exist and 404'd through the SPA
  catch-all. Aligned both with the router and the README.
- `oidc_next` is now stored only when the value is a server-relative
  path; absolute URLs, protocol-relative paths, and `javascript:`-style
  schemes are dropped before reaching the session.
- `SecurityHeadersMiddleware` now emits HSTS outside development.
- `SessionMiddleware` is configured with `Secure` (outside dev) and
  `SameSite=lax`.
- `/oidc/logout` now just clears the local session and redirects home.
  The previous code attempted to call the IdP's `end_session_endpoint`
  but never sent `id_token_hint`, which Okta requires (returns 400
  without it). Wiring up SSO logout properly is out of scope; the
  Flask version cleared the local session only and we match that.
- New `tests/test_oidc.py` covers routing/allowlist, `next` validation,
  full login round-trip, authorize error paths, logout, HSTS, and
  session-cookie flags.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants