add managing org to org perms#3586
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR extends Key observations:
Confidence Score: 2/5
Important Files Changed
Reviews (1): Last reviewed commit: "add managing org to org perms" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if organization.org_type == OrganizationTypeChoices.role.value: | ||
| managing_organization_ids = [] | ||
| for managing_organization in organization.managing_organizations: | ||
| managing_organization_ids.append(managing_organization) | ||
| organization_parents = [*organization_parents, *managing_organization_ids] |
There was a problem hiding this comment.
The manual loop to build managing_organization_ids is redundant since managing_organizations is already a list of integer IDs (ArrayField(models.IntegerField())). Every other usage of this field in the same file unpacks it directly (lines 65, 81, 97). This adds unnecessary indirection without benefit.
| if organization.org_type == OrganizationTypeChoices.role.value: | |
| managing_organization_ids = [] | |
| for managing_organization in organization.managing_organizations: | |
| managing_organization_ids.append(managing_organization) | |
| organization_parents = [*organization_parents, *managing_organization_ids] | |
| if organization.org_type == OrganizationTypeChoices.role.value: | |
| organization_parents = [*organization_parents, *organization.managing_organizations] |
| def get_permission_on_organization(self, organization, user): | ||
| organization_parents = [*organization.parent_cache, organization.id] | ||
| if organization.org_type == OrganizationTypeChoices.role.value: | ||
| managing_organization_ids = [] | ||
| for managing_organization in organization.managing_organizations: | ||
| managing_organization_ids.append(managing_organization) | ||
| organization_parents = [*organization_parents, *managing_organization_ids] |
There was a problem hiding this comment.
parent_cache included for role-type orgs — inconsistent with sibling methods
The initial assignment unconditionally sets organization_parents = [*organization.parent_cache, organization.id], so for role-type orgs the final lookup becomes [parent_cache..., organization.id, ...managing_organizations].
By contrast, both can_manage_organization_users_obj (line 63) and can_list_organization_users_obj (line 96) explicitly exclude parent_cache for role-type organizations and only use [organization.id, *managing_organizations]. This suggests that the parent hierarchy is intentionally not checked for role-type orgs in permission decisions.
Keeping parent_cache here means a user who holds a role in an ancestor of the role-type org (but has no connection through managing_organizations) would have those ancestor permissions surfaced via the permissions field in the retrieve API — potentially widening the apparent permission set beyond what can actually be exercised through the other authorization checks.
If the intent is to mirror the behaviour of the other methods, parent_cache should be excluded for role types:
def get_permission_on_organization(self, organization, user):
if organization.org_type == OrganizationTypeChoices.role.value:
organization_parents = [organization.id, *organization.managing_organizations]
else:
organization_parents = [*organization.parent_cache, organization.id]![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
care/security/authorization/organization.py (1)
141-145: The intermediate loop is unnecessary and inconsistent with existing patterns.The loop at lines 142-144 simply copies
organization.managing_organizationsinto a new list before spreading it. Other methods in this same file (lines 65, 81, 97) spreadorganization.managing_organizationsdirectly. It would be lovely if this followed the same pattern.♻️ Suggested simplification
organization_parents = [*organization.parent_cache, organization.id] if organization.org_type == OrganizationTypeChoices.role.value: - managing_organization_ids = [] - for managing_organization in organization.managing_organizations: - managing_organization_ids.append(managing_organization) - organization_parents = [*organization_parents, *managing_organization_ids] + organization_parents = [*organization_parents, *organization.managing_organizations]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@care/security/authorization/organization.py` around lines 141 - 145, Replace the unnecessary intermediate loop that copies organization.managing_organizations into managing_organization_ids with the same direct spreading used elsewhere: when organization.org_type == OrganizationTypeChoices.role.value, append organization.managing_organizations directly into organization_parents (remove managing_organization_ids and the for loop that populates it), keeping the conditional and resulting assignment to organization_parents unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@care/security/authorization/organization.py`:
- Around line 141-145: Replace the unnecessary intermediate loop that copies
organization.managing_organizations into managing_organization_ids with the same
direct spreading used elsewhere: when organization.org_type ==
OrganizationTypeChoices.role.value, append organization.managing_organizations
directly into organization_parents (remove managing_organization_ids and the for
loop that populates it), keeping the conditional and resulting assignment to
organization_parents unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6e056053-f44d-4eb7-a4bf-c38d89822151
📒 Files selected for processing (1)
care/security/authorization/organization.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3586 +/- ##
===========================================
+ Coverage 77.07% 77.20% +0.13%
===========================================
Files 474 474
Lines 22408 22424 +16
Branches 2346 2349 +3
===========================================
+ Hits 17270 17313 +43
+ Misses 4579 4531 -48
- Partials 559 580 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Proposed Changes
Merge Checklist
/docsOnly PR's with test cases included and passing lint and test pipelines will be reviewed
@ohcnetwork/care-backend-maintainers @ohcnetwork/care-backend-admins
Summary by CodeRabbit