-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(oauth): Support public clients for token refresh per RFC 6749 §6 #106356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…lient refresh Per RFC 6749 §6, public clients (like CLI apps using device flow) should not require client_secret for token refresh. This adds issued_grant_type field to ApiToken to track how each token was issued: - device_code: Can refresh without client_secret (public client per RFC 8628) - authorization_code: Must provide client_secret (confidential client) - null (legacy): Treated as confidential for backward compatibility This ensures strict RFC compliance while maintaining security for confidential clients.
|
This PR has a migration; here is the generated SQL for for --
-- Add field issued_grant_type to apitoken
--
ALTER TABLE "sentry_apitoken" ADD COLUMN "issued_grant_type" varchar(50) NULL; |
… violation Extract duplicated public client authentication logic into a reusable helper method. The DEVICE_CODE and REFRESH grant handlers had ~35 lines of nearly identical code for: - Validating client_id presence - Building query with optional client_secret - Looking up ApiApplication - Error handling with appropriate logging and metrics This improves: - DRY: Single source of truth for public client auth - Maintainability: Easier to add new public client grant types - Readability: post() method is now more concise
markstory
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Schema change looks ok.
| user=grant.user, | ||
| scope_list=grant.get_scopes(), | ||
| scoping_organization_id=grant.organization_id, | ||
| issued_grant_type="authorization_code", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use/expand the existing GrantType enum instead of adding magic strings?
| device_flow_token = ApiToken.objects.create( | ||
| application=self.application, | ||
| user=self.user, | ||
| expires_at=timezone.now(), | ||
| issued_grant_type="device_code", | ||
| ) | ||
|
|
||
| # Request without client_secret (public client) | ||
| resp = self.client.post( | ||
| self.path, | ||
| { | ||
| "grant_type": "refresh_token", | ||
| "client_id": self.application.client_id, | ||
| "refresh_token": device_flow_token.refresh_token, | ||
| # No client_secret - this is a public client (device flow) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there also be a test covering device_code + client_secret submit?
| }, | ||
| ) | ||
| assert resp.status_code == 400 | ||
| assert json.loads(resp.content) == {"error": "invalid_client"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't know that I need to send my client secret as well by reading this error message.
|
thanks for the review @markstory . I'm putting this in draft because we're not sure we really need it. we'll do another pass on the RFC and I'll add your suggestions / close the PR. |
|
closing this in favour of #106451 |
Summary
Adds RFC-compliant public client support for
refresh_tokengrant by tracking how each token was originally issued.This is a follow-up to #106169 which added public client support for
device_codegrant.Problem
After #106169, device flow clients can obtain tokens without
client_secret. However, when these tokens expire and the client attempts to refresh them, it fails becauserefresh_tokengrant requiredclient_secretfor ALL tokens - even those issued to public clients.What the RFCs Say
RFC 6749 §6 - Refreshing an Access Token:
The inverse: public clients (without credentials) can refresh with just
client_id.RFC 8628 §5.6 - Non-Confidential Clients:
Solution
Track
issued_grant_typeon each token to determine if it was issued to a public or confidential client:issued_grant_typeclient_secret?"device_code""authorization_code"client_secretnull(legacy)This ensures strict RFC 6749 §6 compliance: confidential client tokens still require
client_secretfor refresh.Changes
src/sentry/models/apitoken.pyissued_grant_typefield; set to"authorization_code"infrom_grant()src/sentry/migrations/1019_add_issued_grant_type_to_apitoken.pysrc/sentry/web/frontend/oauth_token.pyissued_grant_type="device_code"for device flow tokens; checkissued_grant_typein refresh logictests/sentry/web/frontend/test_oauth_token.pyNew Tests
test_device_flow_token_refresh_without_secret- Device code tokens CAN refresh withoutclient_secrettest_authorization_code_token_requires_secret_for_refresh- Auth code tokens MUST provideclient_secrettest_legacy_token_requires_secret_for_refresh- Legacy tokens (null) requireclient_secretfor backward compatibilitySecurity
Per RFC 6749 §6, the refresh token is bound to the client. This is enforced by:
application(client binding)issued_grant_typeto determine authentication requirementsclient_secretfor confidential client tokensThis prevents a public client from refreshing tokens that were issued to a confidential client.
Related