fix: use locks to handle apigrants safely #82052
Conversation
github-actions
bot
added
the
Scope: Backend
mdtro
force-pushed
the
mdtro/handle-api-grant-auth-correctly
branch
from
cb93a9d to
edd9b69
Compare
| @@ -86,6 +87,8 @@ def post(self, request: Request, installation) -> Response: | |||
| }, | |||
| ) | |||
| return Response({"error": e.msg or "Unauthorized"}, status=403) | |||
| except UnableToAcquireLock: | |||
There was a problem hiding this comment.
Would this happen frequently? I wonder if we need to do a frontend change to improve user experience if double requests to this endpoint can easily happen.
There was a problem hiding this comment.
This should never happen in practice.
From the user's perspective, they will not see this as it is the request from the backend system of the app to exchange the authorization code for an access token.
We do make some assumptions here though, if they are receiving a 409, they may be sending two requests too quickly. This is a bug in their implementation, but at least one of their request flows will receive the access token. If not, they can retry the request after the lock has expired (we only hold a lock for 10 seconds).
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #82052 +/- ##
=======================================
Coverage 87.89% 87.89%
=======================================
Files 9657 9657
Lines 546420 546466 +46
Branches 21267 21267
=======================================
+ Hits 480272 480314 +42
- Misses 65842 65846 +4
Partials 306 306 |
mdtro
force-pushed
the
mdtro/handle-api-grant-auth-correctly
branch
from
edd9b69 to
c9445b0
Compare
mdtro
force-pushed
the
mdtro/handle-api-grant-auth-correctly
branch
from
af1e62a to
2280c66
Compare
Pulled from #82052 to break out these changes into smaller PRs. - Delete associated grant when revoking API authorizations - Use proper transaction management with outbox_context
Pulled from #82052 to break out these changes into smaller PRs. - Delete associated grant when revoking API authorizations - Use proper transaction management with outbox_context
ApiGrantcannot be used twice during a race condition that would result in multiple access/refresh token pairs.ApiAuthorizationis deleted, remove allApiGrants as well that may not have been exchanged yet.ApiGrants that are past their expiration time, 10 minutes, rather than 30 days.ApiTokens will still be deleted after 30 days, aligned with the current behavior.