Skip to content

Plumb expiration data through to OAuth tokens#7306

Merged
johnewart merged 3 commits intomainfrom
johnewart/ENG-2140
Feb 4, 2026
Merged

Plumb expiration data through to OAuth tokens#7306
johnewart merged 3 commits intomainfrom
johnewart/ENG-2140

Conversation

@johnewart
Copy link
Copy Markdown
Collaborator

@johnewart johnewart commented Feb 4, 2026

Ticket ENG-2140

Description Of Changes

Tracks expiration data and calculates TTE for oauth tokens and adds that information to the response - the seconds until expiry and the expires at data

Code Changes

  • Uses already known expiration data to calculate the token's remaining TTE and includes it in the response

Steps to Confirm

None should be needed

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

@johnewart johnewart requested a review from a team as a code owner February 4, 2026 06:54
@johnewart johnewart requested review from JadeCara and removed request for a team February 4, 2026 06:54
@vercel
Copy link
Copy Markdown
Contributor

vercel bot commented Feb 4, 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 Feb 4, 2026 10:11pm
fides-privacy-center Ignored Ignored Feb 4, 2026 10:11pm

Request Review

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR adds OAuth token expiration data (expires_in and expires_at) to API responses, tracking time-to-expiry for access tokens. The implementation adds a standard JWT exp claim (Unix timestamp) to tokens and updates the AccessToken schema to include expiration information.

Key changes:

  • Added expires_in (seconds until expiry) and expires_at (ISO 8601 timestamp) fields to AccessToken schema
  • Modified create_access_code_jwe to include exp claim in JWT payload with Unix timestamp
  • Added is_token_expired_by_payload function that checks exp claim with fallback to iat + duration for backward compatibility
  • Updated all token-generating endpoints (acquire_access_token, user_login, accept_user_invite) to populate expiration data

Critical issue found:

  • src/fides/api/db/ctl_session.py:122 has a critical indentation bug that moves session creation outside the lock context, introducing a race condition in the readonly database pool initialization

Confidence Score: 0/5

  • This PR is not safe to merge due to a critical race condition bug
  • The indentation bug in ctl_session.py creates a race condition by moving session creation outside lock protection, which could cause production issues with the readonly database pool. While the OAuth token expiration feature appears well-implemented, this critical bug must be fixed before merging
  • Pay close attention to src/fides/api/db/ctl_session.py - the indentation change at line 122 introduces a race condition

Important Files Changed

Filename Overview
src/fides/api/db/ctl_session.py Added logging for readonly async pool, but critical indentation bug moves session creation outside lock protection causing race condition
src/fides/api/schemas/oauth.py Added expires_in (seconds) and expires_at (ISO 8601) fields to AccessToken schema for OAuth2 standard compliance
src/fides/api/models/client.py Modified create_access_code_jwe to accept token_expire_minutes parameter and include exp claim in JWT payload
src/fides/api/oauth/utils.py Added is_token_expired_by_payload function to check expiration using exp claim with fallback to iat + duration for backward compatibility

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

4 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Copy Markdown
Contributor

@JadeCara JadeCara left a comment

Choose a reason for hiding this comment

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

Approving with a comment that is not a blocker.

There will probably need to be a Fidesplus PR to accompany this one. There are a few places using create_access_code_jwe without passing in the token_expire_minutes.

)

return AccessToken(access_token=access_code)
expires_at = datetime.now() + timedelta(minutes=expire_minutes)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure if this is worth updating now (especially because I think its all .now() in this file), but datetime.now() makes a naive date time. Its probably fine, but adding a timezone to it might be a good safeguard.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It does, and I agree that we should; unfortunately I think we are a bit inconsistent about time zones - I am not sure if adding one in this case could make the calculations wrong because we use now() elsewhere in this file 😄

Undo moving session creation out of the lock

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@johnewart johnewart enabled auto-merge February 4, 2026 22:18
@johnewart johnewart added this pull request to the merge queue Feb 4, 2026
Merged via the queue into main with commit 7bf9777 Feb 4, 2026
54 of 55 checks passed
@johnewart johnewart deleted the johnewart/ENG-2140 branch February 4, 2026 23:17
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