use perms instead of ffShelterOperatorBaOnlyFields (SDB-277) - #2461
Merged
Merged
Conversation
Contributor
Reviewer's GuideReplaces Sequence diagram for globally authorized shelter contactssequenceDiagram
participant User
participant GraphQL as ShelterGraphQL
participant Service as shelter_update
participant Permissions as can_globally
participant Shelter as Shelter
User->>GraphQL: update_shelter(data)
GraphQL->>Service: shelter_update(user, data)
Service->>Permissions: can_globally(user, ContactInfo.perms.CHANGE)
alt global permission granted
Permissions-->>Service: true
Service->>Shelter: update additional_contacts
Shelter-->>User: updated shelter
else scoped or unauthorized
Permissions-->>Service: false
Service-->>User: PermissionDenied
end
Sequence diagram for globally authorized additional-contact readssequenceDiagram
participant User
participant GraphQL as OperatorShelterType
participant Permissions as can_globally
participant Contacts as Shelter.additional_contacts
User->>GraphQL: additional_contacts
GraphQL->>Permissions: can_globally(user, ContactInfo.perms.VIEW)
alt global permission granted
Permissions-->>GraphQL: true
GraphQL->>Contacts: all()
Contacts-->>User: contact list
else scoped or unauthorized
Permissions-->>GraphQL: false
GraphQL-->>User: empty list
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
vecchp
reviewed
Sep 11, 2026
vecchp
left a comment
vecchp
left a comment
Contributor
There was a problem hiding this comment.
Permissions review — global-tier gate is the right call; two deploy items
Verified the mechanism against the grant model and walked every exposure path on this head:
What's right
holds_globallyis the correct instrument for SDB-277: the requirement is "a scoped grant must never satisfy this", and no scoped predicate can express a negation of its own tier. Worth pinning the near-miss in the docstring, because it's tempting:can_anywhereadmits scoped holders (ALL or s.exists()) and would look equivalent while re-opening exactly the hole this closes — any scoped role carrying the ContactInfo perms would pass. Suggested line: "sibling ofcan_anywhere, which admits scoped holders — use this only when scoped authority must never satisfy the gate."- Exposure audit on this head:
additional_contactsis update-only (shelters/types/inputs.py:206— not on the create input), and the only two GraphQL paths —updateShelter(shelters/schema.py:145) andOperatorShelterType.additional_contacts(shelters/types/outputs.py:289) — are both gated. Admin is staff-side. No bypass found. - Tests: the error-shape class logging in as GSO (so it exercises contact validation, not the gate) is the right adjustment.
Before deploy
- FE still reads the old flag —
libs/react/shelter-operator/src/lib/components/AppSidebar/ShelterProfileLinks.tsx:21gates onFeatureFlags.SO_BA_ONLY_ADMIN_FIELDS. With the BE no longer consulting it, the rollout interleaves: flag ON + scoped operator → FE shows the UI, BE returns[]and denies saves; flag OFF + GSO → FE hides UI the BE would allow. Suggest keeping the flag ON in all environments until the FE moves to permission-based gating (the BaPrivate contacts work, #2449, already carries that TODO) — or landing those together. - Prod membership check — access now keys on a global role carrying the ContactInfo perms (GSO). Worth confirming every current BA user who had access under the flag holds that global role, otherwise it's a silent access loss on deploy.
Nit (non-blocking)
can_globallywould match thecan/can_obj/can_anywheregrammar;holds_globallyis fine if the tier-statement name is deliberate.
LGTM otherwise — nice tightening.
vecchp
self-requested a review
September 11, 2026 00:46
tglaz
marked this pull request as ready for review
September 11, 2026 00:46
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="apps/betterangels-backend/shelters/services/shelter.py" line_range="297-301" />
<code_context>
spas_served_ids = data.pop("spas_served_ids", None)
+ # BA-only field: gate on the global tier only — a scoped Grant must never
+ # pass (ADR 0001 §2.4). Only global Roles carrying the ContactInfo perms
+ # (the Global Shelter Operator) satisfy this. Checked before the shelter
+ # lookup so an unauthorized caller gets the same refusal whether or not
+ # the shelter exists or is visible.
+ if "additional_contacts" in data and not can_globally(user, ContactInfo.perms.CHANGE):
+ raise PermissionDenied("Editing additional contacts is not allowed with this role.")
+
</code_context>
<issue_to_address>
**nitpick:** The comments/docstrings claim that only the Global Shelter Operator role can pass the additional-contact gate, but `can_globally` also authorizes users with a direct `user_permissions` row for the ContactInfo permission. The documentation therefore falsely describes which users can read or edit this field.
**Suggested fix:** Describe the gate as requiring global-tier ContactInfo permission, including direct user permissions, rather than saying it is GSO-only.
</issue_to_address>
### Comment 2
<location path="apps/betterangels-backend/shelters/groups.py" line_range="58" />
<code_context>
# path instead (ADR 0001 §2.4).
ClientProfile.perms.VIEW,
+ # ContactInfo deliberately NOT on the scoped role as it is
+ # currenty used for internal BA users only.
],
invite_html="account/email/shelter_operator_invite.html",
</code_context>
<issue_to_address>
**nitpick:** The newly added comment misspells `currently` as `currenty`, making the permission rationale harder to understand and searchable documentation inconsistent.
**Suggested fix:** Correct `currenty` to `currently`.
```suggestion
# currently used for internal BA users only.
```
</issue_to_address>
vecchp
pushed a commit
that referenced
this pull request
Sep 11, 2026
|
🚀 Expo continuous deployment is ready for betterangels!
iOS Simulator Build: Simulator Build Link |
|
🔍 [betterangels-admin] Preview available at: https://admin.dev.betterangels.la/branches/SDB-277-flag-to-perms Last updated: 2026-09-15T21:33:15.373Z |
|
🔍 [shelter-web] Preview available at: https://shelter.dev.betterangels.la/branches/SDB-277-flag-to-perms Last updated: 2026-09-15T21:33:15.370Z |
shiozakiyuko
self-requested a review
September 14, 2026 16:52
tglaz
enabled auto-merge
September 15, 2026 21:15
shiozakiyuko
approved these changes
Sep 16, 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
3 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.
Private Fields - Additional Contacts - BE perms
SDB-277
Summary by Sourcery
Replace feature-flag gating for BA-only additional contacts with global-tier ContactInfo permission checks.
New Features:
Bug Fixes:
Enhancements:
Tests:
Chores: