Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 85 additions & 33 deletions src/sentry/web/frontend/oauth_token.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@
Client authentication
- Either Authorization header (Basic) or form fields `client_id`/`client_secret`
(RFC 6749 §2.3.1). Only one method may be used per request.
- For device_code grant: supports public clients per RFC 8628 §5.6, which only
require `client_id`. If `client_secret` is provided, it will be validated.
Request format
- Requests are `application/x-www-form-urlencoded` as defined in RFC 6749 §3.2.
Expand All @@ -120,6 +122,16 @@
"""
grant_type = request.POST.get("grant_type")

# Validate grant_type first (needed to determine auth requirements)
if not grant_type:
return self.error(request=request, name="invalid_request", reason="missing grant_type")
if grant_type not in [
GrantTypes.AUTHORIZATION,
GrantTypes.REFRESH,
GrantTypes.DEVICE_CODE,
]:
return self.error(request=request, name="unsupported_grant_type")

# Determine client credentials from header or body (mutually exclusive).
(client_id, client_secret), cred_error = self._extract_basic_auth_credentials(request)
if cred_error is not None:
Expand All @@ -131,42 +143,76 @@
tags={
"client_id_exists": bool(client_id),
"client_secret_exists": bool(client_secret),
"grant_type": grant_type,
},
)

if not client_id or not client_secret:
return self.error(
request=request,
name="invalid_client",
reason="missing client credentials",
status=401,
)
# Device flow supports public clients per RFC 8628 §5.6.
# Public clients only provide client_id to identify themselves.
# If client_secret is provided, we still validate it for confidential clients.
if grant_type == GrantTypes.DEVICE_CODE:
if not client_id:
return self.error(
request=request,
name="invalid_client",
reason="missing client_id",
status=401,
)

if not grant_type:
return self.error(request=request, name="invalid_request", reason="missing grant_type")
if grant_type not in [GrantTypes.AUTHORIZATION, GrantTypes.REFRESH, GrantTypes.DEVICE_CODE]:
return self.error(request=request, name="unsupported_grant_type")
# Build query - validate secret only if provided (confidential client)
query = {"client_id": client_id}
if client_secret:
query["client_secret"] = client_secret

try:
# Note: We don't filter by status here to distinguish between invalid
# credentials (unknown client) and inactive applications. This allows
# proper grant cleanup per RFC 6749 §10.5 and clearer metrics.
application = ApiApplication.objects.get(
client_id=client_id,
client_secret=client_secret,
)
except ApiApplication.DoesNotExist:
metrics.incr(
"oauth_token.post.invalid",
sample_rate=1.0,
)
logger.warning("Invalid client_id / secret pair", extra={"client_id": client_id})
return self.error(
request=request,
name="invalid_client",
reason="invalid client_id or client_secret",
status=401,
)
try:
application = ApiApplication.objects.get(**query)
except ApiApplication.DoesNotExist:
metrics.incr("oauth_token.post.invalid", sample_rate=1.0)
if client_secret:
logger.warning(
"Invalid client_id / secret pair",
extra={"client_id": client_id},
)
reason = "invalid client_id or client_secret"
else:
logger.warning("Invalid client_id", extra={"client_id": client_id})
reason = "invalid client_id"
return self.error(
request=request,
name="invalid_client",
reason=reason,
status=401,
)
else:
# Other grant types require confidential client authentication
if not client_id or not client_secret:
return self.error(
request=request,
name="invalid_client",
reason="missing client credentials",
status=401,
)

try:
# Note: We don't filter by status here to distinguish between invalid
# credentials (unknown client) and inactive applications. This allows
# proper grant cleanup per RFC 6749 §10.5 and clearer metrics.
application = ApiApplication.objects.get(
client_id=client_id,
client_secret=client_secret,
)
except ApiApplication.DoesNotExist:
metrics.incr(
"oauth_token.post.invalid",
sample_rate=1.0,
)
logger.warning("Invalid client_id / secret pair", extra={"client_id": client_id})
return self.error(
request=request,
name="invalid_client",
reason="invalid client_id or client_secret",
status=401,
)

# Check application status separately from credential validation.
# This preserves metric clarity and provides consistent error handling.
Expand Down Expand Up @@ -340,9 +386,15 @@
code_verifier=request.POST.get("code_verifier"),
)
except InvalidGrantError as e:
return {"error": "invalid_grant", "reason": str(e) if str(e) else "invalid grant"}
return {
"error": "invalid_grant",
"reason": str(e) if str(e) else "invalid grant",
}
except ExpiredGrantError as e:
return {"error": "invalid_grant", "reason": str(e) if str(e) else "grant expired"}
return {
"error": "invalid_grant",
"reason": str(e) if str(e) else "grant expired",
}

token_data = {"token": api_token}

Expand Down
121 changes: 118 additions & 3 deletions tests/sentry/web/frontend/test_oauth_token.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ def test_invalid_grant_type(self) -> None:
self.login_as(self.user)

resp = self.client.post(
self.path, {"grant_type": "foo", "client_id": "abcd", "client_secret": "abcd"}
self.path,
{"grant_type": "foo", "client_id": "abcd", "client_secret": "abcd"},
)

assert resp.status_code == 400
Expand All @@ -57,7 +58,9 @@ def setUp(self) -> None:
)
self.client_secret = self.application.client_secret
self.grant = ApiGrant.objects.create(
user=self.user, application=self.application, redirect_uri="https://example.com"
user=self.user,
application=self.application,
redirect_uri="https://example.com",
)

def _basic_auth_value(self) -> str:
Expand Down Expand Up @@ -537,7 +540,9 @@ def setUp(self) -> None:
self.client_secret = self.application.client_secret

self.grant = ApiGrant.objects.create(
user=self.user, application=self.application, redirect_uri="https://example.com"
user=self.user,
application=self.application,
redirect_uri="https://example.com",
)
self.token = ApiToken.objects.create(
application=self.application, user=self.user, expires_at=timezone.now()
Expand Down Expand Up @@ -1359,3 +1364,113 @@ def test_inactive_application_rejects_device_code_grant(self) -> None:

# No token should be created
assert not ApiToken.objects.filter(application=self.application, user=self.user).exists()

def test_public_client_success(self) -> None:
"""Public clients (without client_secret) should be able to exchange device codes.

Per RFC 8628 §5.6, device clients are generally public clients that cannot
securely store credentials. They should be able to authenticate with just
client_id.
"""
self.device_code.status = self.DeviceCodeStatus.APPROVED
self.device_code.user = self.user
self.device_code.save()

# Request without client_secret (public client)
resp = self.client.post(
self.path,
{
"grant_type": "urn:ietf:params:oauth:grant-type:device_code",
"device_code": self.device_code.device_code,
"client_id": self.application.client_id,
# No client_secret - this is a public client
},
)
assert resp.status_code == 200

data = json.loads(resp.content)
assert "access_token" in data
assert data["token_type"] == "Bearer"
assert data["scope"] == "project:read"
assert data["user"]["id"] == str(self.user.id)

# Device code should be deleted
assert not ApiDeviceCode.objects.filter(id=self.device_code.id).exists()

# Token should be created
token = ApiToken.objects.get(token=data["access_token"])
assert token.user == self.user
assert token.application == self.application

def test_public_client_invalid_client_id(self) -> None:
"""Public clients with invalid client_id should return invalid_client."""
self.device_code.status = self.DeviceCodeStatus.APPROVED
self.device_code.user = self.user
self.device_code.save()

resp = self.client.post(
self.path,
{
"grant_type": "urn:ietf:params:oauth:grant-type:device_code",
"device_code": self.device_code.device_code,
"client_id": "nonexistent_client_id",
# No client_secret - public client
},
)
assert resp.status_code == 401
assert json.loads(resp.content) == {"error": "invalid_client"}

def test_public_client_missing_client_id(self) -> None:
"""Device flow without client_id should return invalid_client."""
resp = self.client.post(
self.path,
{
"grant_type": "urn:ietf:params:oauth:grant-type:device_code",
"device_code": self.device_code.device_code,
# No client_id or client_secret
},
)
assert resp.status_code == 401
assert json.loads(resp.content) == {"error": "invalid_client"}

def test_public_client_authorization_pending(self) -> None:
"""Public client polling pending device code should return authorization_pending."""
assert self.device_code.status == self.DeviceCodeStatus.PENDING

resp = self.client.post(
self.path,
{
"grant_type": "urn:ietf:params:oauth:grant-type:device_code",
"device_code": self.device_code.device_code,
"client_id": self.application.client_id,
# No client_secret - public client
},
)
assert resp.status_code == 400
assert json.loads(resp.content) == {"error": "authorization_pending"}

# Device code should still exist
assert ApiDeviceCode.objects.filter(id=self.device_code.id).exists()

def test_confidential_client_wrong_secret_rejected(self) -> None:
"""Device flow with wrong client_secret should be rejected.

When a client provides client_secret, we should validate it even
though device flow supports public clients. This allows confidential
clients to use device flow with full credential validation.
"""
self.device_code.status = self.DeviceCodeStatus.APPROVED
self.device_code.user = self.user
self.device_code.save()

resp = self.client.post(
self.path,
{
"grant_type": "urn:ietf:params:oauth:grant-type:device_code",
"device_code": self.device_code.device_code,
"client_id": self.application.client_id,
"client_secret": "wrong_secret",
},
)
assert resp.status_code == 401
assert json.loads(resp.content) == {"error": "invalid_client"}
Loading