fix(oidc): simplify refresh identity validation - #1905
Conversation
Require core claims before login and validate refreshed ID tokens without relying on the authentication-request nonce. Persist an immutable identity context while retaining the latest ID token. This removes UID and backend-specific migration logic, accepts valid authorized-party claim changes and audience reordering, and avoids false mismatches for custom user IDs and repeated refreshes. Legacy associations establish their baseline on the first fully validated refresh; malformed stored contexts require reauthentication.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1905 +/- ##
==========================================
+ Coverage 87.31% 87.36% +0.04%
==========================================
Files 352 352
Lines 13700 13713 +13
Branches 674 674
==========================================
+ Hits 11962 11980 +18
+ Misses 1494 1490 -4
+ Partials 244 243 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f77f0acb92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if previous_context is None: | ||
| # Legacy associations have no original claim context. Their refreshes | ||
| # were historically not continuity-checked, so establish the baseline | ||
| # from this fully validated refresh token and enforce it thereafter. | ||
| previous_context = self._id_token_context(self.id_token) |
There was a problem hiding this comment.
For any association created before 5.1, _oidc_id_token_context is absent, so if its first refresh returns a correctly signed token for a different sub, this branch accepts that subject and makes it the permanent baseline while retaining the original local association and UID. That defeats the refresh-continuity protection precisely during the upgrade window and can attach another provider identity's tokens to the existing user; retain the UID comparison for standard sub-based backends and require reauthentication when the stored UID cannot establish continuity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The protection was non-existing before. This starts to protect new refreshes. To keep the code limited, the legacy context is not migrated.
| ) | ||
| from social_core.utils import cache | ||
|
|
||
| _ID_TOKEN_CONTEXT_KEY = "_oidc_id_token_context" |
There was a problem hiding this comment.
Replacing OpenIdConnectAuth.ID_TOKEN_CONTEXT_KEY, which shipped in 5.1.0 and was accessed through backend instances, with a private module constant breaks integrations that reference the attribute and silently ignores subclasses that override it to control the storage key. Preserve the class attribute and have the implementation use it so upgrades do not raise AttributeError or leave previously customized contexts unread.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Having it on class gave false impression that this is something to override, what it is not.
| if "azp" in claims and claims["azp"] != client_id: | ||
| raise AuthTokenError(self, "Incorrect id_token: azp") |
There was a problem hiding this comment.
When aud contains multiple client IDs and azp is absent, this now accepts the token merely because PyJWT finds this backend's client ID among the audiences. The authorized party is therefore ambiguous, contrary to the OIDC multi-audience validation check that the previous implementation enforced, so a token issued under another client's authorization can be accepted here; retain the missing-azp rejection whenever aud has more than one entry.
Useful? React with 👍 / 👎.
| self.validate_required_id_token_claims(claims) | ||
| self.validate_temporal_claims(claims) |
There was a problem hiding this comment.
The 5.1.0 base class exposed validate_refresh_claims() specifically for validating claims independently of the authentication request, but this path now bypasses that method and calls validate_temporal_claims() directly. Any third-party OIDC backend overriding the hook to enforce provider-specific refresh claims will silently stop running those checks after upgrading; keep invoking the hook and move the new common required-claim checks around its default implementation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I believe that the intermediate validate_refresh_claims is not that useful.
1 participant
Require core claims before login and validate refreshed ID tokens without relying on the authentication-request nonce.
Persist an immutable identity context while retaining the latest ID token. This removes UID and backend-specific migration logic, accepts valid authorized-party claim changes and audience reordering, and avoids false mismatches for custom user IDs and repeated refreshes.
Legacy associations establish their baseline on the first fully validated refresh; malformed stored contexts require reauthentication.