fix(api): tighten published mutation scopes#113120
Closed
dcramer wants to merge 12 commits into
Closed
Conversation
Tighten published mutation endpoints that already have an obvious existing write-capable scope. Previously, several write methods accepted readonly scopes like org:read, project:read, or event:read even though they mutate server-side state. Require the existing write scope for those surfaces instead. Keep session behavior intact, including team-scoped alert access, but stop exposing readonly token writes through these endpoints. Co-Authored-By: OpenAI Codex <noreply@openai.com>
github-actions
Bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as low quality.
This comment was marked as low quality.
Drop the member-session surfaces that were not actually uncontroversial from this PR. Dashboards, code mappings, repo path parsing, key transactions, and org auth token updates still rely on broader session behavior today, so tightening them here broke existing member flows. Revert those paths in this branch and keep the safe mutation tightening for incidents, alerts, user issue creation, data export, and replay summary. Also keep replay delete in the project scope domain so delete remains consistent with the rest of the replay permission class. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Add an explicit owner for static/app/components/markdownTextArea.tsx so the branch clears the CODEOWNERS coverage check. The file was missing ownership coverage entirely, which caused the fresh PR run to fail even though the API changes on this branch are backend-only. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Keep the replay permission maps aligned with the current repo style of listing implied scopes explicitly. This does not change the effective permissions on these endpoints, but it removes a local inconsistency in this PR where replay endpoints relied on hierarchy expansion while nearby scope maps did not. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Contributor
Backend Test FailuresFailures on
|
wedamija
reviewed
Apr 15, 2026
Replay delete is a public mutation, so it should not depend on a project read-style contract. Move the DELETE permission to event write/admin so member sessions keep working while token auth requires a real write scope. Add direct endpoint tests covering the token contract: event:read is denied and event:write is allowed for replay delete. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Remove the incident permission change from the existing-write-scope pass. Incident mutation has an older member-access contract tied to project access, so tightening it cleanly needs a dedicated scope decision instead of being folded into the uncontroversial cleanup. Also drop an unrelated CODEOWNERS hunk so this branch stays focused on backend mutation scope changes. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Keep replay summary POST on read-level replay scopes and mark it as an explicit readonly-mutation exception, since it derives summary data from existing replay/event data. Document the preview endpoints that intentionally keep write-aligned permissions because they are part of alert and monitor authoring flows, even though they use POST helper endpoints. Add replay summary token tests covering the read-scope contract. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Add endpoint-level regression tests for the scope changes on this branch so each permission update is covered directly. Cover the write-scope transitions for data export, alert rule creation, project user issues, replay delete, detector updates, and the alert helper endpoints. This keeps the branch from relying on indirect role coverage alone and makes the replay summary readonly exception explicit next to direct token tests. Refs getsentry/getsentry#19897 Co-Authored-By: OpenAI Codex <noreply@openai.com>
Contributor
Backend Test FailuresFailures on
|
Use supported token auth for data export, give alert-rule token tests a valid member project setup, and return a real anomaly payload in the Seer regression test. These tests were asserting the new permission contracts, but two of them were exercising invalid success paths and one was using an auth shape that the endpoint does not serialize correctly. Co-Authored-By: Codex <noreply@openai.com>
Contributor
Backend Test FailuresFailures on
|
Alert and monitor mutations now accept alerts:write at the endpoint level, but the nested project validators still required project:read. That let the request through permission checks and then failed token-based create and update flows with a 400 during validation. Allow those serializers to validate project selection against the same write-capable scopes the endpoints already advertise, and add direct token regression tests for alert rule updates and monitor creation so the contract stays enforced. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Limit org-scoped alert and detector mutations to either the target project's alerts:write access or an explicit endpoint opt-in for the older team-scoped fallback. This closes the cross-team mutation path on alert rule and detector details, and adds project-scoped checks for the other shared consumers that still relied on the broad organization permission. Align replay deletion with the rest of the event delete surface by requiring event:admin instead of event:write. Add regression coverage for the scope changes and the cross-project authorization cases. Co-Authored-By: Codex <noreply@openai.com>
| request.access.has_any_project_scope(monitor.project, ALERT_MUTATION_SCOPES) | ||
| for monitor in monitors | ||
| ): | ||
| return self.respond(status=403) |
Contributor
There was a problem hiding this comment.
monitor.project attribute does not exist on Monitor model
High Severity
The Monitor model defines project_id as a BoundedBigIntegerField, not a ForeignKey. This means monitor.project does not exist as an attribute—accessing it will raise an AttributeError at runtime, causing a 500 error on every bulk-edit PUT request to this endpoint. The check needs to look up the Project by monitor.project_id first, or use select_related with a proper FK relationship.
Reviewed by Cursor Bugbot for commit 2a711cc. Configure here.
Contributor
Backend Test FailuresFailures on
|
dcramer
changed the title
Normalize RpcUserOrganizationContext to the underlying organization before resolving target projects for alert-rule and detector mutation permissions. Without that, the new project-scoped alerts:write path breaks during organization object-permission checks because those hooks run before convert_args swaps in the concrete organization. Add team-admin session regressions that force the project-scoped alerts:write authorization path for alert rule and detector updates and deletes, including the workflow-engine fake-detector alert-rule route. Co-Authored-By: Codex <noreply@openai.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 34ef4d0. Configure here.
| if isinstance(organization, RpcUserOrganizationContext): | ||
| return organization.organization.id | ||
|
|
||
| return organization.id |
Contributor
There was a problem hiding this comment.
Duplicate _get_organization_id helper across two files
Low Severity
The _get_organization_id function is identically defined in both bases.py and organization_detector_details.py. This duplication increases the risk that a future fix to one copy won't be applied to the other. It could be extracted to a shared utility.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 34ef4d0. Configure here.
Contributor
Backend Test FailuresFailures on
|
Member
Author
|
my local env is having some issues that i need to fix so cant run tests fully locall yet did a ton of cleanup and audit here, but ill flip it to review as soon as i feel confident that it needs more eyes |
Member
Author
|
im rebasing this on main in #113194 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Align published mutation scopes with the write-scope family each feature already belongs to, and document the published mutation endpoints that intentionally remain readonly or member-capable.
The main alternative was to remove all legacy org-scoped alert access immediately. I did not do that for the oldest public metric alert and cron monitor mutation APIs because they are the most likely to have external token clients that still depend on
org:write. Everything newer, or clearly part of internal authoring helpers, now moves to the alert-specific write scope directly.Behavioral Scope Changes
Alerts
alerts:writeonly viaALERT_MUTATION_SCOPES.OrganizationAlertRulePermissionmutation methods conceptually move fromorg:write/org:admin/alerts:writetoalerts:write.org:writecompatibility, tagged withTODO(api-write-scope-compat), onOrganizationAlertRuleIndexEndpoint.POST,OrganizationAlertRuleDetailsEndpoint.PUT, andOrganizationAlertRuleDetailsEndpoint.DELETE.project:read,alerts:write, and the temporary legacyorg:writecompatibility path.org:adminis no longer accepted there.Cron Monitors
org:write/org:admin/alerts:writetoalerts:write.org:writecompatibility, tagged withTODO(api-write-scope-compat), onOrganizationMonitorIndexEndpoint.POST.project:read,alerts:write, and the temporary legacyorg:writecompatibility path.org:adminis no longer accepted there.alerts:writefor every target monitor project.Workflow Engine Alerts
OrganizationWorkflowPermissionmutation methods now requirealerts:write.org:writeandorg:adminare removed.Workflow Engine Detectors
OrganizationDetectorPermissionmutation methods now requirealerts:write.org:writeandorg:adminare removed.alerts:writerequirement. System-created detectors no longer keep a separateorg:writepath.Alert Authoring Helpers
alerts:writeon the requested project.org:writeno longer authorizes it, and the existing403contract for inaccessible or unknown project ids is preserved.alerts:write.org:writeno longer authorizes it.alerts:write.org:writeno longer authorizes it.Published Mutation Note Coverage
This PR also adds explicit
readonly_mutation_scope_exceptionsnotes for published mutation endpoints that intentionally still accept readonly or member scopes. These notes are documentation only and are enforced by the published mutation scope note test.Search And Member Preference Surfaces
OrganizationPinnedSearchPermission.PUT/DELETEOrganizationSearchPermission.POST/PUT/DELETEOrganizationRecentSearchPermission.POSTDiscoverSavedQueryPermission.POST/PUT/DELETEExploreSavedQueryPermission.POST/PUT/DELETEOnboarding And Project Setup Helpers
Integrations And Misc Published Helpers
Future Legacy Compat To Remove
TODO(api-write-scope-compat)marks the temporaryorg:writepaths that should be removed after client migration:There is no temporary
org:admincompatibility in this PR. That scope is removed from the changed alert mutation paths instead of being grandfathered.Every changed behavior in this PR has direct regression coverage for either the allow path, the deny path, or both.
Refs getsentry/getsentry#19897