feat(oauth): log scope-ceiling rejections at /authorize#61216
Conversation
The per-app scope ceiling rejects out-of-ceiling requests by returning False, which oauthlib turns into a 302 with error=invalid_scope. That path emitted no log or capture, so a misconfigured first-party app (empty/partial ceiling) failed silently. Emit a structured warning carrying client_id, is_first_party, and the requested vs allowed sets so a log alert can page on first-party rejections. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
posthog/api/oauth/test_views.py:2290-2305
**Prefer a parameterised test to cover both rejection paths**
The new test only exercises the `has_ceiling=True` branch. The `has_ceiling=False` branch (no app ceiling, client requests a privileged or wildcard scope) also reaches the `logger.warning` call and is worth asserting in the same spirit. Following the team's preference for parameterised tests, both paths could live under a single `@pytest.mark.parametrize` (or `subTest`): one case with a ceiling set where the requested scope falls outside it, and one with no ceiling where a privileged scope is requested.
Reviews (1): Last reviewed commit: "chore: ruff format the ceiling-rejection..." | Re-trigger Greptile |
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
| def test_authorize_rejection_emits_ceiling_log(self): | ||
| self._set_ceiling("experiment:read") | ||
| with patch("posthog.api.oauth.views.logger") as mock_logger: | ||
| response = self.client.get(f"{self.base_authorization_url}&scope=experiment:write") | ||
| self.assertEqual(response.status_code, status.HTTP_302_FOUND) | ||
| rejection_calls = [ | ||
| call | ||
| for call in mock_logger.warning.call_args_list | ||
| if call.args and call.args[0] == "oauth_scope_ceiling_rejected" | ||
| ] | ||
| self.assertEqual(len(rejection_calls), 1) | ||
| kwargs = rejection_calls[0].kwargs | ||
| self.assertEqual(kwargs["client_id"], "test_confidential_client_id") | ||
| self.assertEqual(kwargs["is_first_party"], self.confidential_application.is_first_party) | ||
| self.assertEqual(kwargs["requested"], ["experiment:write"]) | ||
| self.assertEqual(kwargs["ceiling"], ["experiment:read"]) |
There was a problem hiding this comment.
Prefer a parameterised test to cover both rejection paths
The new test only exercises the has_ceiling=True branch. The has_ceiling=False branch (no app ceiling, client requests a privileged or wildcard scope) also reaches the logger.warning call and is worth asserting in the same spirit. Following the team's preference for parameterised tests, both paths could live under a single @pytest.mark.parametrize (or subTest): one case with a ceiling set where the requested scope falls outside it, and one with no ceiling where a privileged scope is requested.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/api/oauth/test_views.py
Line: 2290-2305
Comment:
**Prefer a parameterised test to cover both rejection paths**
The new test only exercises the `has_ceiling=True` branch. The `has_ceiling=False` branch (no app ceiling, client requests a privileged or wildcard scope) also reaches the `logger.warning` call and is worth asserting in the same spirit. Following the team's preference for parameterised tests, both paths could live under a single `@pytest.mark.parametrize` (or `subTest`): one case with a ceiling set where the requested scope falls outside it, and one with no ceiling where a privileged scope is requested.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Parameterized it. Covers the empty-ceiling reject path now too: 7d74e35
| def test_authorize_rejection_emits_ceiling_log(self): | ||
| self._set_ceiling("experiment:read") | ||
| with patch("posthog.api.oauth.views.logger") as mock_logger: | ||
| response = self.client.get(f"{self.base_authorization_url}&scope=experiment:write") | ||
| self.assertEqual(response.status_code, status.HTTP_302_FOUND) | ||
| rejection_calls = [ | ||
| call | ||
| for call in mock_logger.warning.call_args_list | ||
| if call.args and call.args[0] == "oauth_scope_ceiling_rejected" | ||
| ] | ||
| self.assertEqual(len(rejection_calls), 1) | ||
| kwargs = rejection_calls[0].kwargs | ||
| self.assertEqual(kwargs["client_id"], "test_confidential_client_id") | ||
| self.assertEqual(kwargs["is_first_party"], self.confidential_application.is_first_party) | ||
| self.assertEqual(kwargs["requested"], ["experiment:write"]) | ||
| self.assertEqual(kwargs["ceiling"], ["experiment:read"]) |
Parameterize the test so it asserts the oauth_scope_ceiling_rejected log fires on both branches: a set ceiling with an out-of-ceiling scope, and an empty ceiling with a privileged scope excluded by the broad default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ClickHouse migration SQL per cloud environment
…truncated. See the full SQL in the workflow logs. |
Problem
The per-app OAuth scope ceiling (#60477) enforces, at
/authorize, that a client may only be granted scopes within itsOAuthApplication.scopesset. When a request asks for a scope outside that ceiling,OAuthValidator.validate_scopesreturnsFalse, and oauthlib turns that into a 302 redirect carryingerror=invalid_scope.That reject path is silent: no
loggercall, nocapture_exception, and a 302 is not a 4xx/5xx, so error-rate and APM dashboards miss it too. The failure mode this hides is a first-party app whosescopesceiling is empty or only partially seeded. Its OAuth clients (for example the setup wizard, which requestsllm_gateway:readamong others) begin failing/authorizewithinvalid_scope, users can't complete login, and nothing server-side records which client requested which scope.This is the observability gap behind a real regression: after the ceiling enforcement shipped, a first-party client's logins broke with
invalid_scopeand there was no server-side trace of the rejection. The only signals were the client's own exception telemetry (fragmented across install paths, unalerted) and users noticing.Changes
posthog/api/oauth/views.py: invalidate_scopes, emit a structuredlogger.warning("oauth_scope_ceiling_rejected", ...)on the reject branch, carryingclient_id,is_first_party, the requested out-of-ceiling scopes, and the app's effective ceiling. The boolean resolution is byte-for-byte unchanged; this only adds a log side-effect on the existingFalsepath.posthog/api/oauth/test_views.py: asserts the event fires once with the expected fields when a scope is rejected.is_first_partyis included so a downstream alert can page only on first-party rejections, which are near-zero baseline and almost always a misconfiguration (an unseeded or partially-seeded ceiling). Third-party clients over-asking is expected and logs atis_first_party=false.Follow-ups
oauth_scope_ceiling_rejectedfiltered tois_first_party=true, created once this deploys (the event has to exist before there's anything to alert on). That alert is what turns this from "users report it" into "we're paged at deploy time."How did you test this code?
I'm an agent (Claude). Automated only:
test_authorize_rejection_emits_ceiling_logpatches the module logger and asserts a singleoauth_scope_ceiling_rejectedwarning with the expectedclient_id/is_first_party/requested/ceiling. It ran green in the Django CI matrix.ruff checkandruff formatclean on both files.Automatic notifications
Docs update
skip-inkeep-docs — no user-facing docs change.
🤖 Agent context
Authored by Claude Code (Opus 4.8). Follow-up observability on the per-app scope ceiling (#60477): the reject path in
validate_scopeswas silent, so a misconfigured first-party ceiling producedinvalid_scopewith no server-side signal. Chose a singlelogger.warningon the existingFalsebranch (structlog, matching the otherlogger.warningcalls in this file) overcapture_exception, since the rejection is a normal 302 redirect rather than a 4xx/5xx error path. Boolean resolution is unchanged; only the log was added.