Dev server#3031
Conversation
fix: update permission checks for viewing and managing Interest Groups
Greptile SummaryThis PR widens access to
|
| Filename | Overview |
|---|---|
| api/dashboard/ig/dash_ig_view.py | Broadens GET and PATCH permissions to IG leads via new helpers, and adds a lead role-sync side-effect in PATCH; the multi-table write is missing transaction.atomic and the caller User lookup is re-queried inside the leads loop. |
Reviews (1): Last reviewed commit: "Merge pull request #3030 from gtech-mule..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if serializer.is_valid(): | ||
| serializer.save() | ||
|
|
||
| # ── Side-effect: sync the per-IG lead role with the leads list ────── | ||
| # Without this the Leads field is cosmetic: the muids are stored on | ||
| # the IG row, but the user never receives "{code} IGLead" and so | ||
| # cannot edit the IG they were just made lead of. | ||
| # | ||
| # When "leads" is present it is treated as the COMPLETE set of leads | ||
| # for this IG — anyone dropped from it loses the role. A payload | ||
| # without the key leaves lead roles untouched, so unrelated partial | ||
| # updates are safe. The frontend only sends "leads" when it changed. | ||
| if "leads" in request.data: | ||
| raw_leads = request.data.get("leads") | ||
| if isinstance(raw_leads, str): | ||
| try: | ||
| raw_leads = json.loads(raw_leads) | ||
| except Exception: | ||
| raw_leads = [] | ||
| if not isinstance(raw_leads, list): | ||
| raw_leads = [] | ||
|
|
||
| lead_role, _ = Role.objects.get_or_create( | ||
| title=RoleType.IG_LEAD_ROLE(ig.code), | ||
| defaults={ | ||
| "id": str(uuid.uuid4()), | ||
| "description": f"{ig.name} Interest Group Lead", | ||
| "created_by_id": user_id, | ||
| "updated_by_id": user_id, | ||
| }, | ||
| ) | ||
|
|
||
| desired_lead_muids = { | ||
| (item.get("muid") if isinstance(item, dict) else item) | ||
| for item in raw_leads | ||
| } | ||
| desired_lead_muids.discard(None) | ||
| desired_lead_muids.discard("") | ||
|
|
||
| # Revoke first, so a lead moved out of the list loses access even | ||
| # if their replacement's muid turns out to be invalid. Deleted | ||
| # rather than deactivated to match the role-transfer logic in | ||
| # dash_campus_helper.assign_ig_campus_lead(). | ||
| UserRoleLink.objects.filter(role=lead_role).exclude( | ||
| user__muid__in=desired_lead_muids | ||
| ).delete() | ||
|
|
||
| for muid in desired_lead_muids: | ||
| target_user = User.objects.filter(muid=muid).first() | ||
| if not target_user: | ||
| continue | ||
|
|
||
| _, lead_link_created = UserRoleLink.objects.get_or_create( | ||
| user=target_user, | ||
| role=lead_role, | ||
| defaults={"verified": True, "created_by_id": user_id}, | ||
| ) | ||
|
|
||
| if lead_link_created: | ||
| caller = User.objects.filter(id=user_id).first() | ||
| caller_name = caller.full_name if caller else "An admin" | ||
| NotificationUtils.insert_notification( | ||
| user=target_user, | ||
| title=f"IG Lead: {ig.name}"[:50], | ||
| description=( | ||
| f"{caller_name} has assigned you as a Lead for " | ||
| f"{ig.name}. You can now edit this interest group." | ||
| )[:200], | ||
| button='View', | ||
| url=f'/interest-groups/{ig.id}/', | ||
| created_by=caller, | ||
| ) |
There was a problem hiding this comment.
transaction.atomic
serializer.save() persists the IG update, then the leads role-sync block writes to Role, UserRoleLink, and Notification — all outside any transaction. If UserRoleLink.objects.get_or_create or NotificationUtils.insert_notification raises after the serializer save has committed, the IG row is updated but the assigned leads never receive the {code} IGLead role, leaving the system in a silently inconsistent state. The established pattern for multi-step writes in this codebase (see InterestGroupAPI.post) is to wrap everything in with transaction.atomic():.
| for muid in desired_lead_muids: | ||
| target_user = User.objects.filter(muid=muid).first() | ||
| if not target_user: | ||
| continue | ||
|
|
||
| _, lead_link_created = UserRoleLink.objects.get_or_create( | ||
| user=target_user, | ||
| role=lead_role, | ||
| defaults={"verified": True, "created_by_id": user_id}, | ||
| ) | ||
|
|
||
| if lead_link_created: | ||
| caller = User.objects.filter(id=user_id).first() | ||
| caller_name = caller.full_name if caller else "An admin" |
There was a problem hiding this comment.
caller = User.objects.filter(id=user_id).first() is inside the for muid in desired_lead_muids loop and is re-executed for every newly assigned lead. If N leads are created in a single request, this fires N extra queries fetching the same row each time. The caller does not change between iterations — fetch it once before the loop.
| for muid in desired_lead_muids: | |
| target_user = User.objects.filter(muid=muid).first() | |
| if not target_user: | |
| continue | |
| _, lead_link_created = UserRoleLink.objects.get_or_create( | |
| user=target_user, | |
| role=lead_role, | |
| defaults={"verified": True, "created_by_id": user_id}, | |
| ) | |
| if lead_link_created: | |
| caller = User.objects.filter(id=user_id).first() | |
| caller_name = caller.full_name if caller else "An admin" | |
| caller = User.objects.filter(id=user_id).first() | |
| caller_name = caller.full_name if caller else "An admin" | |
| for muid in desired_lead_muids: | |
| target_user = User.objects.filter(muid=muid).first() | |
| if not target_user: | |
| continue | |
| _, lead_link_created = UserRoleLink.objects.get_or_create( | |
| user=target_user, | |
| role=lead_role, | |
| defaults={"verified": True, "created_by_id": user_id}, | |
| ) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if not _can_view_ig(roles): | ||
| return CustomResponse( | ||
| general_message="You do not have permission to view this Interest Group" | ||
| ).get_failure_response() |
There was a problem hiding this comment.
Per the codebase conventions, permission denials should return get_unauthorized_response() (HTTP 403), not get_failure_response() (HTTP 400). The same applies to the matching denial in patch at line 413. Using the wrong helper breaks the status-code contract that frontend clients rely on to distinguish "missing resource" from "forbidden".
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
No description provided.