-
-
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
Changes from 2 commits
d8dafee
aad7da9
7f5092f
524492a
637f81c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # Generated by Django 5.2.8 on 2025-01-15 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
| from sentry.new_migrations.migrations import CheckedMigration | ||
|
|
||
|
|
||
| class Migration(CheckedMigration): | ||
| # This flag is used to mark that a migration shouldn't be automatically run in production. | ||
| # This should only be used for operations where it's safe to run the migration after your | ||
| # code has deployed. So this should not be used for most operations that alter the schema | ||
| # of a table. | ||
| # Here are some things that make sense to mark as post deployment: | ||
| # - Large data migrations. Typically we want these to be run manually so that they can be | ||
| # monitored and not block the deploy for a long period of time while they run. | ||
| # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to | ||
| # run this outside deployments so that we don't block them. Note that while adding an index | ||
| # is a schema change, it's completely safe to run the operation after the code has deployed. | ||
| # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment | ||
|
|
||
| is_post_deployment = False | ||
|
|
||
| dependencies = [ | ||
| ("sentry", "1018_encrypt_integration_metadata"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="apitoken", | ||
| name="issued_grant_type", | ||
| field=models.CharField(max_length=50, null=True, blank=True), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -675,6 +675,184 @@ def test_inactive_application_rejects_token_refresh(self) -> None: | |
| assert token_after.refresh_token == self.token.refresh_token | ||
| assert token_after.expires_at == self.token.expires_at | ||
|
|
||
| def test_device_flow_token_refresh_without_secret(self) -> None: | ||
| """Device flow tokens can be refreshed without client_secret. | ||
|
|
||
| Per RFC 8628 §5.6: "device clients... should be treated as public clients" | ||
| Per RFC 6749 §6: Public clients don't need to authenticate for refresh. | ||
|
|
||
| Tokens issued via device_code grant can be refreshed with just client_id. | ||
| """ | ||
| self.login_as(self.user) | ||
|
|
||
| # Create a token that was issued via device flow | ||
| 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) | ||
| }, | ||
| ) | ||
|
Comment on lines
+689
to
+705
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 == 200 | ||
|
|
||
| data = json.loads(resp.content) | ||
| assert "access_token" in data | ||
| assert "refresh_token" in data | ||
| assert data["token_type"] == "Bearer" | ||
|
|
||
| # Token should be refreshed | ||
| token2 = ApiToken.objects.get(id=device_flow_token.id) | ||
| assert token2.token != device_flow_token.token | ||
| assert token2.refresh_token != device_flow_token.refresh_token | ||
|
|
||
| def test_authorization_code_token_requires_secret_for_refresh(self) -> None: | ||
| """Authorization code tokens MUST provide client_secret for refresh. | ||
|
|
||
| Per RFC 6749 §6: "If the client type is confidential or the client was | ||
| issued client credentials, the client MUST authenticate." | ||
|
|
||
| Tokens issued via authorization_code are confidential client tokens. | ||
| """ | ||
| self.login_as(self.user) | ||
|
|
||
| # Create a token that was issued via authorization_code (confidential client) | ||
| auth_code_token = ApiToken.objects.create( | ||
| application=self.application, | ||
| user=self.user, | ||
| expires_at=timezone.now(), | ||
| issued_grant_type="authorization_code", | ||
| ) | ||
|
|
||
| # Try to refresh without client_secret | ||
| resp = self.client.post( | ||
| self.path, | ||
| { | ||
| "grant_type": "refresh_token", | ||
| "client_id": self.application.client_id, | ||
| "refresh_token": auth_code_token.refresh_token, | ||
| # No client_secret - should fail for confidential token | ||
| }, | ||
| ) | ||
| assert resp.status_code == 400 | ||
| assert json.loads(resp.content) == {"error": "invalid_client"} | ||
|
|
||
| def test_legacy_token_requires_secret_for_refresh(self) -> None: | ||
| """Legacy tokens (null issued_grant_type) require client_secret for refresh. | ||
|
|
||
| Tokens created before the issued_grant_type field was added are treated | ||
| as confidential client tokens for backward compatibility. | ||
| """ | ||
| self.login_as(self.user) | ||
|
|
||
| # self.token has issued_grant_type=None (legacy token from setUp) | ||
| assert self.token.issued_grant_type is None | ||
|
|
||
| # Try to refresh without client_secret | ||
| resp = self.client.post( | ||
| self.path, | ||
| { | ||
| "grant_type": "refresh_token", | ||
| "client_id": self.application.client_id, | ||
| "refresh_token": self.token.refresh_token, | ||
| # No client_secret - should fail for legacy token | ||
| }, | ||
| ) | ||
| assert resp.status_code == 400 | ||
| assert json.loads(resp.content) == {"error": "invalid_client"} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| def test_public_client_refresh_invalid_client_id(self) -> None: | ||
| """Invalid client_id should return invalid_client.""" | ||
| self.login_as(self.user) | ||
|
|
||
| resp = self.client.post( | ||
| self.path, | ||
| { | ||
| "grant_type": "refresh_token", | ||
| "client_id": "nonexistent_client_id", | ||
| "refresh_token": self.token.refresh_token, | ||
| }, | ||
| ) | ||
| assert resp.status_code == 401 | ||
| assert json.loads(resp.content) == {"error": "invalid_client"} | ||
|
|
||
| def test_refresh_missing_client_id(self) -> None: | ||
| """Refresh without client_id should return invalid_client.""" | ||
| self.login_as(self.user) | ||
|
|
||
| resp = self.client.post( | ||
| self.path, | ||
| { | ||
| "grant_type": "refresh_token", | ||
| "refresh_token": self.token.refresh_token, | ||
| # No client_id | ||
| }, | ||
| ) | ||
| assert resp.status_code == 401 | ||
| assert json.loads(resp.content) == {"error": "invalid_client"} | ||
|
|
||
| def test_device_flow_token_wrong_application(self) -> None: | ||
| """Refresh token from different application should return invalid_grant. | ||
|
|
||
| Per RFC 6749 §6: "the refresh token is bound to the client to which it | ||
| was issued" | ||
| """ | ||
| self.login_as(self.user) | ||
|
|
||
| # Create a device flow token | ||
| device_flow_token = ApiToken.objects.create( | ||
| application=self.application, | ||
| user=self.user, | ||
| expires_at=timezone.now(), | ||
| issued_grant_type="device_code", | ||
| ) | ||
|
|
||
| # Create a different application | ||
| other_app = ApiApplication.objects.create( | ||
| owner=self.user, redirect_uris="https://other.com" | ||
| ) | ||
|
|
||
| # Try to refresh with token from self.application but client_id from other_app | ||
| resp = self.client.post( | ||
| self.path, | ||
| { | ||
| "grant_type": "refresh_token", | ||
| "client_id": other_app.client_id, | ||
| "refresh_token": device_flow_token.refresh_token, | ||
| }, | ||
| ) | ||
| assert resp.status_code == 400 | ||
| assert json.loads(resp.content) == {"error": "invalid_grant"} | ||
|
|
||
| def test_confidential_client_refresh_wrong_secret_rejected(self) -> None: | ||
| """When client_secret is provided, it must be valid. | ||
|
|
||
| Even for device flow tokens, if a client provides client_secret, | ||
| we validate it. | ||
| """ | ||
| self.login_as(self.user) | ||
|
|
||
| resp = self.client.post( | ||
| self.path, | ||
| { | ||
| "grant_type": "refresh_token", | ||
| "client_id": self.application.client_id, | ||
| "refresh_token": self.token.refresh_token, | ||
| "client_secret": "wrong_secret", | ||
| }, | ||
| ) | ||
| assert resp.status_code == 401 | ||
| assert json.loads(resp.content) == {"error": "invalid_client"} | ||
|
|
||
|
|
||
| @control_silo_test | ||
| class OAuthTokenOrganizationScopedTest(TestCase): | ||
|
|
||
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
GrantTypeenum instead of adding magic strings?