Skip to content

Commit 30f16e7

Browse files
NicoHinderlingclaude
authored andcommitted
fix(snapshots): Fix staff auth blocking initial size comparison selection (#112739)
## Summary The size comparison POST endpoint was applying staff re-auth checks whenever existing comparisons were found, regardless of context. This caused staff users to hit a `403 StaffRequired` error on the build selection page when selecting a pair that already had comparison results. **Fix:** Use a `?rerun=true` query parameter to distinguish the "Rerun Comparison" admin flow from normal comparison triggers. ### Logic paths | Scenario | `?rerun` | Existing state | Behavior | |---|---|---|---| | Build selection page | no | no comparison exists | Creates new comparison | | Build selection page | no | SUCCESS/PENDING exists | Returns `status: "exists"`, navigates to results | | Build selection page | no | all FAILED | Deletes failed, re-creates (retry without staff gate) | | Retry button (failed comparison) | no | all FAILED | Deletes failed, re-creates (retry without staff gate) | | "Rerun Comparison" admin button | yes | active superuser/staff | Deletes and re-runs | | "Rerun Comparison" admin button | yes | `is_staff` but not re-authed | `StaffRequired` (triggers re-auth modal) | | "Rerun Comparison" admin button | yes | non-staff | 403 | ### Files changed - **Backend**: `project_preprod_size_analysis_compare.py` — branching logic based on `?rerun` query param - **Frontend**: `buildComparison.tsx` — rerun mutation now sends `?rerun=true` --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 65b8f0b commit 30f16e7

File tree

3 files changed

+105
-13
lines changed

3 files changed

+105
-13
lines changed

src/sentry/preprod/api/endpoints/size_analysis/project_preprod_size_analysis_compare.py

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -343,13 +343,39 @@ def post(
343343
head_size_analysis__in=head_size_metrics,
344344
base_size_analysis__in=base_size_metrics,
345345
)
346+
is_rerun = request.query_params.get("rerun") == "true"
347+
346348
if existing_comparisons.exists():
347-
if is_active_superuser(request) or is_active_staff(request):
349+
if is_rerun:
350+
if is_active_superuser(request) or is_active_staff(request):
351+
comparisons_deleted, files_deleted = _delete_existing_comparisons(
352+
existing_comparisons
353+
)
354+
logger.info(
355+
"preprod.size_analysis.compare.api.post.rerun_deleted_existing",
356+
extra={
357+
"head_artifact_id": head_artifact.id,
358+
"base_artifact_id": base_artifact.id,
359+
"comparisons_deleted": comparisons_deleted,
360+
"files_deleted": files_deleted,
361+
"user_id": request.user.id,
362+
},
363+
)
364+
elif request.user.is_staff:
365+
raise StaffRequired
366+
else:
367+
return Response({"detail": "Only staff can rerun comparisons."}, status=403)
368+
elif (
369+
existing_comparisons.filter(
370+
state=PreprodArtifactSizeComparison.State.FAILED
371+
).count()
372+
== existing_comparisons.count()
373+
):
348374
comparisons_deleted, files_deleted = _delete_existing_comparisons(
349375
existing_comparisons
350376
)
351377
logger.info(
352-
"preprod.size_analysis.compare.api.post.rerun_deleted_existing",
378+
"preprod.size_analysis.compare.api.post.retry_deleted_failed",
353379
extra={
354380
"head_artifact_id": head_artifact.id,
355381
"base_artifact_id": base_artifact.id,
@@ -358,8 +384,6 @@ def post(
358384
"user_id": request.user.id,
359385
},
360386
)
361-
elif request.user.is_staff:
362-
raise StaffRequired
363387
else:
364388
comparison_models = []
365389
for comparison in existing_comparisons:

static/app/views/preprod/buildComparison/buildComparison.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ export default function BuildComparison() {
6767
RequestError
6868
>({
6969
mutationFn: () => {
70-
return fetchMutation({url: compareUrl, method: 'POST'});
70+
return fetchMutation({url: `${compareUrl}?rerun=true`, method: 'POST'});
7171
},
7272
onSuccess: response => {
7373
if (response?.status === 'exists') {

tests/sentry/preprod/api/endpoints/size_analysis/test_project_preprod_size_analysis_compare.py

Lines changed: 76 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -600,10 +600,12 @@ def test_post_comparison_existing_comparison(self) -> None:
600600
assert data["comparisons"][0]["comparison_id"] == comparison.id
601601

602602
@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
603-
def test_post_comparison_existing_failed_comparison(self) -> None:
604-
"""Test POST endpoint returns existing failed comparison when comparison exists and failed"""
605-
# Create a failed comparison
606-
comparison = self.create_preprod_artifact_size_comparison(
603+
@patch("sentry.preprod.size_analysis.tasks.manual_size_analysis_comparison.apply_async")
604+
def test_post_comparison_existing_failed_comparison_auto_retries(
605+
self, mock_apply_async
606+
) -> None:
607+
"""Test POST endpoint auto-retries when all existing comparisons are failed"""
608+
self.create_preprod_artifact_size_comparison(
607609
head_size_analysis=self.head_size_metric,
608610
base_size_analysis=self.base_size_metric,
609611
organization=self.organization,
@@ -616,12 +618,13 @@ def test_post_comparison_existing_failed_comparison(self) -> None:
616618

617619
assert response.status_code == 200
618620
data = response.json()
619-
assert data["status"] == "exists"
621+
assert data["status"] == "created"
620622
assert len(data["comparisons"]) == 1
621623
comparison_data = data["comparisons"][0]
622-
assert comparison_data["state"] == comparison.state
623-
assert comparison_data["error_code"] == str(comparison.error_code)
624-
assert comparison_data["error_message"] == comparison.error_message
624+
assert comparison_data["state"] == PreprodArtifactSizeComparison.State.PENDING
625+
assert comparison_data["comparison_id"] is None
626+
627+
mock_apply_async.assert_called_once()
625628

626629
@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
627630
def test_post_comparison_cannot_compare_size_metrics(self) -> None:
@@ -804,3 +807,68 @@ def test_get_returns_404_for_expired_base_artifact(self, mock_cutoff):
804807
)
805808
assert response.status_code == 404
806809
assert response.data["detail"] == "This build's size data has expired."
810+
811+
@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
812+
@patch("sentry.preprod.size_analysis.tasks.manual_size_analysis_comparison.apply_async")
813+
def test_post_rerun_as_staff_deletes_and_recreates(self, mock_apply_async):
814+
"""Test POST with rerun=true as active staff deletes existing and creates new comparisons"""
815+
self.create_preprod_artifact_size_comparison(
816+
head_size_analysis=self.head_size_metric,
817+
base_size_analysis=self.base_size_metric,
818+
organization=self.organization,
819+
state=PreprodArtifactSizeComparison.State.SUCCESS,
820+
file_id=12345,
821+
)
822+
823+
staff_user = self.create_user(is_staff=True)
824+
self.organization.member_set.create(user_id=staff_user.id)
825+
self.login_as(user=staff_user)
826+
827+
with patch(
828+
"sentry.preprod.api.endpoints.size_analysis.project_preprod_size_analysis_compare.is_active_staff",
829+
return_value=True,
830+
):
831+
response = self.client.post(f"{self._get_url()}?rerun=true")
832+
833+
assert response.status_code == 200
834+
data = response.json()
835+
assert data["status"] == "created"
836+
assert len(data["comparisons"]) == 1
837+
assert data["comparisons"][0]["state"] == PreprodArtifactSizeComparison.State.PENDING
838+
mock_apply_async.assert_called_once()
839+
840+
@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
841+
def test_post_rerun_as_non_staff_returns_403(self):
842+
"""Test POST with rerun=true as non-staff returns 403"""
843+
self.create_preprod_artifact_size_comparison(
844+
head_size_analysis=self.head_size_metric,
845+
base_size_analysis=self.base_size_metric,
846+
organization=self.organization,
847+
state=PreprodArtifactSizeComparison.State.SUCCESS,
848+
file_id=12345,
849+
)
850+
851+
response = self.client.post(f"{self._get_url()}?rerun=true")
852+
853+
assert response.status_code == 403
854+
assert response.json()["detail"] == "Only staff can rerun comparisons."
855+
856+
@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
857+
def test_post_rerun_as_inactive_staff_raises_staff_required(self):
858+
"""Test POST with rerun=true as is_staff user without active staff session raises StaffRequired"""
859+
self.create_preprod_artifact_size_comparison(
860+
head_size_analysis=self.head_size_metric,
861+
base_size_analysis=self.base_size_metric,
862+
organization=self.organization,
863+
state=PreprodArtifactSizeComparison.State.SUCCESS,
864+
file_id=12345,
865+
)
866+
867+
staff_user = self.create_user(is_staff=True)
868+
self.organization.member_set.create(user_id=staff_user.id)
869+
self.login_as(user=staff_user)
870+
871+
response = self.client.post(f"{self._get_url()}?rerun=true")
872+
873+
assert response.status_code == 403
874+
assert response.json()["detail"]["code"] == "staff-required"

0 commit comments

Comments
 (0)