Skip to content

ENG-3001: OAuth client management#7658

Merged
tvandort merged 24 commits intomainfrom
ENG-3001
Mar 19, 2026
Merged

ENG-3001: OAuth client management#7658
tvandort merged 24 commits intomainfrom
ENG-3001

Conversation

@tvandort
Copy link
Contributor

@tvandort tvandort commented Mar 16, 2026

Ticket ENG-3001

Description Of Changes

Extends the OAuth client model with name and description fields, and adds the missing CRUD endpoints for managing clients via the API. Previously, clients could be created and deleted but not listed, fetched, or updated. This PR fills that gap and also adds a dedicated secret rotation endpoint so callers can cycle credentials without deleting and recreating the client.

A shared _generate_and_hash_secret helper consolidates the secret-hashing logic that was previously duplicated between initial creation and any future rotation path.

Code Changes

  • Added name and description columns to ClientDetail (nullable, migration included)
  • Added ClientCreateRequest, ClientUpdateRequest, ClientResponse, and ClientSecretRotateResponse Pydantic schemas
  • Added GET /oauth/client — paginated list of all clients (excludes root client)
  • Added GET /oauth/client/{client_id} — fetch a single client by ID
  • Added PUT /oauth/client/{client_id} — update name, description, and/or scopes
  • Added POST /oauth/client/{client_id}/secret — rotate a client's secret, returning it exactly once
  • Updated POST /oauth/client to accept a ClientCreateRequest body (name, description, scopes) instead of a raw scopes list
  • Extracted _generate_and_hash_secret helper to deduplicate secret generation logic between creation and rotation
  • Added rotate_secret method to ClientDetail
  • Added client_id property alias on ClientDetail for clean API serialization

Steps to Confirm

  1. Create a client via POST /oauth/client with a name and description — confirm name/description are persisted and returned
  2. GET /oauth/client — confirm the list is paginated and the root client is excluded
  3. GET /oauth/client/{client_id} — confirm the correct client is returned; confirm 404 for unknown ID
  4. PUT /oauth/client/{client_id} — update scopes, name, and description; confirm changes are reflected
  5. POST /oauth/client/{client_id}/secret — confirm a new secret is returned and the old one no longer authenticates
  6. Confirm the Alembic migration applies cleanly on a fresh DB and on an existing DB with clients already present

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link
Contributor

vercel bot commented Mar 16, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Mar 19, 2026 5:41pm
fides-privacy-center Ignored Ignored Mar 19, 2026 5:41pm

Request Review

@tvandort tvandort force-pushed the ENG-3001 branch 3 times, most recently from d38f227 to f3ed393 Compare March 18, 2026 22:13
@tvandort tvandort marked this pull request as ready for review March 18, 2026 23:45
@tvandort tvandort requested a review from a team as a code owner March 18, 2026 23:45
@tvandort tvandort requested review from thabofletcher and removed request for a team March 18, 2026 23:45
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 18, 2026

Greptile Summary

This PR fills the CRUD gap for OAuth clients by adding GET (list + by ID), PUT (update), and POST secret-rotation endpoints, and extends the ClientDetail model with nullable name and description columns backed by an Alembic migration. The implementation is well-structured: the _generate_and_hash_secret helper correctly deduplicates secret-hashing logic, the _get_client_or_error / _get_client_or_none helpers consistently guard against root-client access across all new and updated endpoints, and the asymmetric name/description update logic in update_client correctly reflects the stated product intent (names cannot be cleared, descriptions can).

Key observations:

  • Three new test methods include explicit client.delete(db) calls (tests/ops/models/test_client.py:21, tests/ops/models/test_client.py:32, tests/ops/api/v1/endpoints/test_oauth_endpoints.py:619), which is against the project convention that the database is auto-cleared between test runs.
  • The test_can_clear_description unit test verifies req.description is None but does not assert "description" in req.model_fields_set, which is the actual predicate the update_client endpoint uses to decide whether to clear the field.
  • The migration filename uses a non-standard xx_2026_03_13_1658_ prefix; all other migrations in the project use the bare revision ID as the filename prefix (e.g. 38071fffda39_add_name_to_client.py).

Confidence Score: 4/5

  • This PR is safe to merge with minor test-quality fixes recommended before landing.
  • The core logic (new endpoints, model changes, migration, root-client guards) is correct and well-tested at the integration level. The issues found are all in the test layer: unnecessary manual teardown calls and a weak schema-unit-test assertion. No logic bugs or security issues were identified.
  • tests/ops/models/test_client.py and tests/lib/test_client_schemas.py for test-quality fixes; src/fides/api/alembic/migrations/versions/xx_2026_03_13_1658_38071fffda39_add_name_to_client.py for filename convention.

Important Files Changed

Filename Overview
src/fides/api/models/client.py Adds name and description columns, extracts _generate_and_hash_secret helper to deduplicate secret generation, adds rotate_secret method and client_id property alias. Changes are clean and well-structured.
src/fides/api/schemas/client.py Adds ClientCreateRequest, ClientUpdateRequest, ClientResponse, and ClientSecretRotateResponse schemas with a shared _validate_name_not_empty helper to avoid validator duplication. Schemas look correct.
src/fides/api/v1/endpoints/oauth_endpoints.py Adds GET list, GET by ID, PUT update, and POST secret-rotation endpoints. Introduces _get_client_or_error and _get_client_or_none helpers to correctly guard against root-client access. Asymmetric name/description update logic (is not None vs model_fields_set) correctly reflects stated requirements.
src/fides/api/alembic/migrations/versions/xx_2026_03_13_1658_38071fffda39_add_name_to_client.py Adds nullable name and description columns to the client table. Migration is functionally correct but the filename uses a non-standard xx_ prefix that's inconsistent with every other migration in the project.
tests/ops/models/test_client.py Adds unit tests for new name/description fields, but two new tests (test_create_client_with_name_and_description and test_create_client_name_defaults_to_none) include explicit client.delete(db) calls, violating the project convention that the DB is auto-cleared between test runs.
tests/ops/api/v1/endpoints/test_oauth_endpoints.py Extensive new test classes for list, get, update, and secret-rotation endpoints. Good coverage of auth, error, root-client guard, and happy-path cases. One test (test_create_client_with_name_and_description) has an unnecessary client.delete(db) call.
tests/lib/test_client_schemas.py Unit tests for new Pydantic schemas. The test_can_clear_description test verifies the value but not the model_fields_set membership that the endpoint relies on to distinguish "omitted" from "explicitly null".

Last reviewed commit: "Updates."

@tvandort
Copy link
Contributor Author

@greptile rereview

2 similar comments
@tvandort
Copy link
Contributor Author

@greptile rereview

@tvandort
Copy link
Contributor Author

@greptile rereview

@tvandort
Copy link
Contributor Author

@greptile rereview

@tvandort
Copy link
Contributor Author

@greptile rereview

Copy link
Contributor

@thabofletcher thabofletcher left a comment

Choose a reason for hiding this comment

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

👍 - I'm sending over a doc with some considerations, but no blockers so approving.

@tvandort tvandort added this pull request to the merge queue Mar 19, 2026
Merged via the queue into main with commit 84c6de8 Mar 19, 2026
56 of 57 checks passed
@tvandort tvandort deleted the ENG-3001 branch March 19, 2026 21:46
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.

2 participants