Skip to content

ENG-2857 Use get_encryption_key callable in all places#7652

Merged
erosselli merged 2 commits intomainfrom
erosselli/ENG-2857-follow-up
Mar 16, 2026
Merged

ENG-2857 Use get_encryption_key callable in all places#7652
erosselli merged 2 commits intomainfrom
erosselli/ENG-2857-follow-up

Conversation

@erosselli
Copy link
Contributor

@erosselli erosselli commented Mar 13, 2026

Ticket ENG-2857

Description Of Changes

Follow up for ENG-2857 since I missed some places to update.

Code Changes

Steps to Confirm

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

@erosselli erosselli requested a review from a team as a code owner March 13, 2026 19:14
@erosselli erosselli requested review from JadeCara and removed request for a team March 13, 2026 19:14
@vercel
Copy link
Contributor

vercel bot commented Mar 13, 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 13, 2026 7:19pm
fides-privacy-center Ignored Ignored Mar 13, 2026 7:19pm

Request Review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 13, 2026

Greptile Summary

This PR is a follow-up to ENG-2857, replacing all remaining direct accesses to CONFIG.security.app_encryption_key with get_encryption_key() across JWE token creation and extraction paths. The get_encryption_key() function adds a process-lifetime caching layer and a warning when the key is empty, and is designed to support future key manager integrations (e.g., AWS KMS).

  • Updated src/fides/api/oauth/utils.py: 5 call sites across _get_webhook_jwe_or_error, _get_request_task_jwe_or_error, validate_download_token, extract_token_and_load_client, and extract_token_and_load_client_async
  • Updated src/fides/api/v1/endpoints/oauth_endpoints.py: acquire_access_token for JWE creation
  • Updated src/fides/api/v1/endpoints/user_endpoints.py: logout_oauth_client and user_login
  • Updated src/fides/service/user/user_service.py: accept_invite
  • One remaining direct usage of CONFIG.security.app_encryption_key found in src/fides/api/models/registration.py (passed as a static string to StringEncryptedType, evaluated at import time rather than lazily) — this may be a missed update worth addressing

Confidence Score: 4/5

  • Safe to merge; all changes are mechanical substitutions with no behavioral change in the hot path.
  • All modified call sites correctly adopt get_encryption_key(), imports are placed at the top of each file, and there are no contract changes. The small score deduction reflects one remaining direct usage of CONFIG.security.app_encryption_key in registration.py that was not addressed, which is a minor inconsistency with the stated goal of the ticket.
  • src/fides/api/models/registration.py — still passes CONFIG.security.app_encryption_key as a static string to StringEncryptedType, bypassing the callable/lazy-evaluation pattern introduced by this PR.

Important Files Changed

Filename Overview
src/fides/api/oauth/utils.py Replaces 5 direct usages of CONFIG.security.app_encryption_key with get_encryption_key() across JWT extract/validate functions; import added at top of file. Changes are mechanical and correct.
src/fides/api/v1/endpoints/oauth_endpoints.py Updates acquire_access_token to use get_encryption_key() for JWE creation; import added correctly at top.
src/fides/api/v1/endpoints/user_endpoints.py Updates logout_oauth_client and user_login to use get_encryption_key(); import added correctly at top.
src/fides/service/user/user_service.py Updates accept_invite to use get_encryption_key() for JWE creation; import added correctly at top.

Comments Outside Diff (1)

  1. src/fides/api/models/registration.py, line 18-25 (link)

    Missed app_encryption_key usage

    The PR description states this is updating "all places" to use get_encryption_key(), but UserRegistration.user_email in this model still passes CONFIG.security.app_encryption_key as a static string value at class definition time:

    user_email = Column(
        StringEncryptedType(
            String,
            CONFIG.security.app_encryption_key,  # static, evaluated at import time
            AesEngine,
        ),
        ...
    )

    This means the key is captured once when the module is first imported, rather than being resolved lazily through get_encryption_key(). While encrypted_type() in encryption_utils.py passes get_encryption_key as a callable (supporting future key managers like AWS KMS), this model bypasses that pattern entirely.

    Consider updating this to pass get_encryption_key as a callable, consistent with how encrypted_type() works:

    from fides.api.db.encryption_utils import get_encryption_key
    
    user_email = Column(
        StringEncryptedType(
            String,
            get_encryption_key,  # callable, evaluated lazily
            AesEngine,
        ),
        nullable=True,
    )

Last reviewed commit: 36e2397

@erosselli
Copy link
Contributor Author

@greptile

@erosselli erosselli added this pull request to the merge queue Mar 16, 2026
Merged via the queue into main with commit f08b613 Mar 16, 2026
56 of 57 checks passed
@erosselli erosselli deleted the erosselli/ENG-2857-follow-up branch March 16, 2026 13:53
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