Skip to content

Dev server #3031

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
awindsr merged 2 commits into from
Jul 19, 2026
Merged

Dev server #3031

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6799846
fix: update permission checks for viewing and managing Interest Groups
awindsr Jul 19, 2026
24af8c4
Merge pull request #3030 from gtech-mulearn/dev
awindsr Jul 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 116 additions & 5 deletions api/dashboard/ig/dash_ig_view.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,43 @@ def _validate_muids(request_data, fields=("leads", "mentors")):
return True, None


# " IGLead" — derived rather than hardcoded so it tracks IG_LEAD_ROLE().
_IG_LEAD_ROLE_SUFFIX = RoleType.IG_LEAD_ROLE("")


def _can_view_ig(roles):
"""
Who may load a single Interest Group.

Deliberately broader than _can_manage_ig: any IG lead can read any IG,
so leads can reference how other groups are set up. Mirrors the frontend
middleware, which admits any role ending in " IGLead".
"""
return (
RoleType.ADMIN.value in roles
or RoleType.IG_LEAD.value in roles
or any(role.endswith(_IG_LEAD_ROLE_SUFFIX) for role in roles)
)


def _can_manage_ig(roles, ig):
"""
Who may edit a single Interest Group.

Admins and the platform-wide "IG Lead" role cover every IG; the dynamic
"{code} IGLead" role covers only the IG it belongs to — an AI lead must
not be able to edit the Web Development group.

This cannot be expressed with @role_required, because the per-IG role
title is only known once the IG has been loaded.
"""
return (
RoleType.ADMIN.value in roles
or RoleType.IG_LEAD.value in roles
or RoleType.IG_LEAD_ROLE(ig.code) in roles
)


class InterestGroupAPI(APIView):
authentication_classes = [CustomizePermission]

Expand Down Expand Up @@ -332,27 +369,33 @@ def get(self, request):

class InterestGroupGetAPI(APIView):
authentication_classes = [CustomizePermission]
@role_required([RoleType.ADMIN.value])

@extend_schema(
tags=['Dashboard - Ig'],
description="Retrieve Interest Group Get.",
responses={200: InterestGroupSerializer},
)
def get(self, request, pk):
"""Allow Admin or any IG lead to load the IG."""
roles = JWTUtils.fetch_role(request)
ig_data = InterestGroup.objects.filter(id=pk).first()

if not ig_data:
return CustomResponse(
general_message="Interest Group Does Not Exist"
).get_failure_response()

if not _can_view_ig(roles):
return CustomResponse(
general_message="You do not have permission to view this Interest Group"
).get_failure_response()
Comment on lines +388 to +391

@greptile-apps greptile-apps Bot Jul 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Auth denial uses wrong response helper

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!


serializer = InterestGroupSerializer(ig_data, many=False)

return CustomResponse(
response={"interestGroup": serializer.data}
).get_success_response()

@role_required([RoleType.ADMIN.value])
@extend_schema(
tags=['Dashboard - Ig'],
description="Partially update Interest Group Get.",
Expand All @@ -367,9 +410,7 @@ def patch(self, request, pk):
if not ig:
return CustomResponse(general_message="Interest Group Does Not Exist").get_failure_response()

# Permission: Admins or IG Lead role for this IG code
ig_lead_role_title = RoleType.IG_LEAD_ROLE(ig.code)
if (RoleType.ADMIN.value not in roles) and (ig_lead_role_title not in roles):
if not _can_manage_ig(roles, ig):
return CustomResponse(general_message="You do not have permission to update this Interest Group").get_failure_response()

request_data = request.data
Expand Down Expand Up @@ -399,6 +440,76 @@ def patch(self, request, pk):
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"
Comment on lines +487 to +500

@greptile-apps greptile-apps Bot Jul 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Suggested change
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!

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,
)
Comment on lines 440 to +511

@greptile-apps greptile-apps Bot Jul 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Multi-table write missing 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():.


# ── Side-effect: assign Mentor role + UserIgLink for new mentors ────
raw_mentors = request.data.get("mentors")
if raw_mentors:
Expand Down
Loading