Conversation
refresh_token() fell back to access_token when no refresh_token was stored, sending it to the token endpoint as if it were one. Per RFC 6749 4.1.4 a refresh_token is optional and providers reject a request that sends the wrong kind of token, so this always failed with a 401 instead of doing nothing. Removed the fallback. Also added refresh_token to Zoom's and PayPal's EXTRA_DATA, since without it a real refresh_token was never even saved for those two backends. Updated the AzureAD/AzureADB2C test fixtures to include a refresh_token in the mocked login response, matching what those providers actually return. Added tests for the fixed fallback too.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1928 +/- ##
==========================================
+ Coverage 87.32% 89.16% +1.83%
==========================================
Files 352 345 -7
Lines 13713 13898 +185
Branches 675 696 +21
==========================================
+ Hits 11975 12392 +417
+ Misses 1494 1262 -232
Partials 244 244
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:
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes an OAuth2 refresh flow bug where refresh_token() incorrectly fell back to sending the access_token as a refresh_token when no refresh token was stored, which providers reject.
Changes:
- Remove the
access_tokenfallback when selecting a refresh token inUserMixin.refresh_token(). - Ensure Zoom and PayPal persist
refresh_tokeninEXTRA_DATAso refresh can actually work. - Update AzureAD/AzureADB2C test fixtures to include
refresh_token, and add regression tests for the corrected refresh-token behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| social_core/storage.py | Stops falling back to access_token when no refresh_token is present. |
| social_core/backends/zoom.py | Adds refresh_token to stored extra data. |
| social_core/backends/paypal.py | Adds refresh_token to stored extra data. |
| social_core/tests/test_storage.py | Adds regression tests ensuring refresh does not use access_token as a refresh token. |
| social_core/tests/backends/test_azuread.py | Updates mocked token response fixture to include refresh_token. |
| social_core/tests/backends/test_azuread_b2c.py | Updates mocked token response fixture to include refresh_token. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da09e42689
ℹ️ 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".
| token = self.extra_data.get("refresh_token") | ||
| if not token: | ||
| return |
There was a problem hiding this comment.
For Facebook users, FacebookOAuth2.EXTRA_DATA never stores a refresh_token, because its refresh implementation deliberately exchanges the existing access token via fb_exchange_token in FacebookOAuth2.refresh_token_params(). When get_access_token() detects an expired Facebook token, this early return now skips that exchange and returns the expired credential unchanged. Select the token according to the backend's refresh mechanism rather than unconditionally requiring a stored refresh token.
Useful? React with 👍 / 👎.
Fixes #826.
refresh_token() fell back to access_token when no refresh_token was stored, sending it to the token endpoint as if it were one. A refresh_token is optional per RFC 6749 4.1.4, and providers reject a request carrying the wrong kind of token. This always failed with a 401 instead of doing nothing.
Removed the fallback. This also exposed that Zoom and PayPal never actually saved a real refresh_token in the first place, since it wasn't listed in their EXTRA_DATA. Refresh never had a chance to work for those two even with a correct fallback removed. Added it there.
The AzureAD and AzureADB2C tests build their own mocked login response and it didn't include a refresh_token. Their test_refresh_token tests broke once the fallback was gone. Updated those fixtures to match what the providers actually return.
Added tests for the fixed fallback in test_storage.py. Ran the full test suite (1152 passed, 176 skipped, 8 xfailed) and ruff.