fix(legal-documents): suppress 403 for non-admins on /legal scene#59230
Merged
rafaeelaudibert merged 2 commits intoMay 20, 2026
Conversation
Non-admin org members landing on /legal triggered a 403 from the legal-documents list endpoint and surfaced a fresh error tracking issue, even though LegalDocumentsScene already short-circuits the UI for non-admins. The loader had no guard on membership level and no try/catch, so the IsOrganizationAdminOrOwner rejection bubbled up. Gate loadLegalDocuments on organizationLogic's isAdminOrOwner selector (mirroring couponLogic), and as defense in depth swallow 403 ApiError responses inside the loader. Re-run the load on loadCurrentOrganizationSuccess in case afterMount fired before the organization resolved. Generated-By: PostHog Code Task-Id: 8dbd1c51-9b88-45a3-8b6f-485560bd8307
assign-reviewers-posthog
Bot
requested review from
a team,
MattBro,
fercgomes and
rafaeelaudibert
and removed request for
a team
Contributor
|
Size Change: -6.81 kB (-0.01%) Total Size: 118 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
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
products/legal_documents/frontend/scenes/legalDocumentsLogic.ts:149-153
Missing `legalDocumentsLoading` guard in retry listener. The `loadCurrentOrganizationSuccess` listener fires whenever the org is (re-)loaded. If `afterMount` already started a fetch that hasn't finished yet — or if `organizationLogic` refreshes the org while an admin with zero documents is on this scene — the listener will trigger a second concurrent `loadLegalDocuments()` call. Adding `!values.legalDocumentsLoading` prevents duplicate in-flight requests in the timing overlap, and keeps the intent clear: only retry when nothing is already loading.
```suggestion
loadCurrentOrganizationSuccess: () => {
if (values.legalDocuments.length === 0 && values.isAdminOrOwner && !values.legalDocumentsLoading) {
actions.loadLegalDocuments()
}
},
```
Reviews (1): Last reviewed commit: "fix(legal-documents): suppress 403 for n..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Add `!values.legalDocumentsLoading` to the `loadCurrentOrganizationSuccess` listener so it doesn't fire a second `loadLegalDocuments()` call if `afterMount` has a fetch already in flight, or if `organizationLogic` refreshes the org while a request is pending. Generated-By: PostHog Code Task-Id: 8dbd1c51-9b88-45a3-8b6f-485560bd8307
rafaeelaudibert
approved these changes
May 20, 2026
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Problem
Non-admin organization members landing on the
/legalscene triggered a403 Your organization access level is insufficient.fromGET /api/organizations/{id}/legal_documents/, which surfaced as a fresh error tracking issue (fingerprint4045923f…). The scene itself looked fine becauseLegalDocumentsScene.tsxalready short-circuits the UI for non-admins with an "Admin access required" banner — but the loader fired regardless, so the symptom was error-tracking noise and a failed loader state.The mismatch was introduced when the
IsOrganizationAdminOrOwnerpermission was added to the backend list endpoint on top of the original scene, which renders for any org member.Changes
In
products/legal_documents/frontend/scenes/legalDocumentsLogic.ts:loadLegalDocumentsreturns[]early whenisAdminOrOwner === false && currentOrganizationis loaded — same shape used incouponLogic. The extra&& currentOrganizationclause avoids skipping the request during the brief window whereisAdminOrOwnerisfalseonly becausemembership_levelis stillundefined.api.legalDocumentsListin atry/catchand swallowApiErrorwithstatus === 403, returning[]. Future permission tightening can't re-introduce the same error-tracking noise.loadCurrentOrganizationSuccessand re-run the loader once the org resolves, if it found no documents and the user is an admin/owner. Covers the case whereafterMountfires before the organization is loaded.How did you test this code?
I'm an agent (PostHog Code). I did not run the dev server or manually click through the /legal scene. I read
couponLogicto confirm the established pattern, and reviewed the diff for behavior under each membership state. No new unit tests were added — the loader's existing surface is exercised through the scene; happy to add one if a reviewer wants it. Lint/typecheck couldn't be run locally because dependencies aren't installed in the agent workspace; CI will validate.Publish to changelog?
no
🤖 Agent context
Authored by PostHog Code. The signal report pinpointed both the root cause (
loadLegalDocumentsruns unconditionally inafterMount, has no admin-level guard or try/catch) and the backend permission introducing the 403. The fix follows the existingcouponLogicpattern — both the membership-level guard and theloadCurrentOrganizationSuccessre-run hook — so anyone familiar with that scene can recognize the shape immediately. The 403 swallow inside the loader was added because the suggested "defense in depth" prevents this exact kind of regression if backend permissions tighten again later. No changes to the backend permission or to the scene UI; the banner already covers the user-facing affordance.