fix: add validation for updating the team PoC role #283
Conversation
…ly authorized users can update the point of contact for a team
…on to ensure proper authorization checks
Summary by CodeRabbit
WalkthroughAdds a permission check for updating a team’s POC in TeamService.update_team. Introduces a new classmethod to validate updater permissions, imports ApiErrors, and updates unit tests to cover admin and non-admin scenarios for the new authorization gate. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant TS as TeamService
participant URS as UserRoleService
C->>TS: update_team(team_id, dto, updated_by_user_id)
alt dto includes poc_id
TS->>TS: _validate_poc_update_permissions(team_id, updated_by_user_id, team)
alt Updater is team creator
Note right of TS: Allow
else Check ADMIN role in TEAM scope
TS->>URS: has_role(user_id, ADMIN, TEAM, team_id)
URS-->>TS: boolean
alt is_admin == false
TS-->>C: PermissionError(ApiErrors.UNAUTHORIZED_TITLE)
else Continue
end
end
TS-->>C: proceed with update flow
else No POC change
TS-->>C: proceed with update flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Missing context in PermissionError for PoC update validation ▹ view | |
 |
Unnecessary PoC permission validation on unchanged values ▹ view | |
 |
Unclear Permission Logic Flow ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| todo/services/team_service.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| if UserRoleService.has_role(updated_by_user_id, RoleName.ADMIN.value, RoleScope.TEAM.value, team_id): | ||
| return | ||
|
|
||
| raise PermissionError(ApiErrors.UNAUTHORIZED_TITLE) |
There was a problem hiding this comment.
Missing context in PermissionError for PoC update validation 
Tell me more
What is the issue?
Generic PermissionError is raised without contextual information about what specific permission check failed.
Why this matters
When this error occurs, developers and users won't know whether the failure was due to not being the team creator, not being an admin, or which specific PoC update permission was violated, making debugging and user experience poor.
Suggested change ∙ Feature Preview
Include specific context about the failed permission check:
raise PermissionError(f"Unauthorized to update team PoC. Only team creator or admin can update PoC for team {team_id}. User {updated_by_user_id} lacks required permissions.")Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if dto.poc_id is not None: | ||
| cls._validate_poc_update_permissions(team_id, updated_by_user_id, existing_team) |
There was a problem hiding this comment.
Unnecessary PoC permission validation on unchanged values 
Tell me more
What is the issue?
The PoC permission validation is called for every team update when poc_id is not None, even when the PoC is not actually being changed.
Why this matters
This results in unnecessary permission checks and database queries when the PoC field is included in the update but remains the same value, wasting computational resources.
Suggested change ∙ Feature Preview
Only validate PoC update permissions when the PoC is actually being changed by comparing the new value with the existing value:
if dto.poc_id is not None and str(existing_team.poc_id) != dto.poc_id:
cls._validate_poc_update_permissions(team_id, updated_by_user_id, existing_team)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| if str(team.created_by) == updated_by_user_id: | ||
| return | ||
|
|
||
| if UserRoleService.has_role(updated_by_user_id, RoleName.ADMIN.value, RoleScope.TEAM.value, team_id): | ||
| return | ||
|
|
||
| raise PermissionError(ApiErrors.UNAUTHORIZED_TITLE) |
There was a problem hiding this comment.
Unclear Permission Logic Flow 
Tell me more
What is the issue?
The _validate_poc_update_permissions method uses early returns which makes the authorization logic harder to follow. The conditions for authorization are split across multiple returns.
Why this matters
Early returns in permission validation can make it difficult to understand all the conditions that grant access. A more explicit approach would make the authorization rules immediately clear.
Suggested change ∙ Feature Preview
@classmethod
def _validate_poc_update_permissions(cls, team_id: str, updated_by_user_id: str, team):
is_team_creator = str(team.created_by) == updated_by_user_id
is_team_admin = UserRoleService.has_role(updated_by_user_id, RoleName.ADMIN.value, RoleScope.TEAM.value, team_id)
if not (is_team_creator or is_team_admin):
raise PermissionError(ApiErrors.UNAUTHORIZED_TITLE)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
todo/services/team_service.py(3 hunks)todo/tests/unit/services/test_team_service.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-22T15:34:24.054Z
Learnt from: Hariom01010
PR: Real-Dev-Squad/todo-backend#279
File: todo/management/commands/migrate_add_roles_to_teams.py:3-3
Timestamp: 2025-09-22T15:34:24.054Z
Learning: In the todo-backend repository, UserTeamDetailsRepository is defined in todo/repositories/team_repository.py and should be imported from there, not from a separate module.
Applied to files:
todo/services/team_service.py
🧬 Code graph analysis (2)
todo/services/team_service.py (3)
todo/constants/messages.py (1)
ApiErrors(25-58)todo/services/user_role_service.py (1)
has_role(82-87)todo/constants/role.py (2)
RoleName(9-13)RoleScope(4-6)
todo/tests/unit/services/test_team_service.py (3)
todo/constants/messages.py (1)
ApiErrors(25-58)todo/constants/role.py (2)
RoleName(9-13)RoleScope(4-6)todo/services/team_service.py (2)
TeamService(29-545)_validate_poc_update_permissions(514-521)
🔇 Additional comments (3)
todo/tests/unit/services/test_team_service.py (1)
5-5: LGTM!The new imports are correctly added and necessary for the test methods that validate POC update permissions.
Also applies to: 17-17
todo/services/team_service.py (2)
9-9: LGTM!The
ApiErrorsimport is correctly added and used in the new permission validation method.
513-521: LGTM!The permission validation logic is correctly implemented:
- Team creators can always update the POC
- Team admins (with ADMIN role in TEAM scope) can update the POC
- Other users are denied with an appropriate error message
The string comparison on line 515 correctly converts the
PyObjectIdto string for comparison.
| if dto.poc_id is not None: | ||
| cls._validate_poc_update_permissions(team_id, updated_by_user_id, existing_team) |
There was a problem hiding this comment.
Add validation to prevent POC removal without reassignment.
The current implementation allows admins to set the POC to None (by passing an empty string), but ApiErrors.CANNOT_REMOVE_POC indicates that "POC cannot be removed from a team. Reassign the POC first." This suggests a business rule that's not being enforced.
Additionally, there's no validation ensuring the new POC is actually a member of the team (unlike create_team which automatically adds the POC as a member).
Consider these validations:
if dto.poc_id is not None:
cls._validate_poc_update_permissions(team_id, updated_by_user_id, existing_team)
+
+ # Prevent removal of POC without reassignment
+ if existing_team.poc_id and (not dto.poc_id or not dto.poc_id.strip()):
+ raise ValueError(ApiErrors.CANNOT_REMOVE_POC)
+
+ # Validate new POC is a team member
+ if dto.poc_id and dto.poc_id.strip():
+ team_members = UserService.get_users_by_team_id(team_id)
+ team_member_ids = [user.id for user in team_members]
+ if dto.poc_id not in team_member_ids:
+ raise ValueError(f"User {dto.poc_id} is not a member of the team")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if dto.poc_id is not None: | |
| cls._validate_poc_update_permissions(team_id, updated_by_user_id, existing_team) | |
| if dto.poc_id is not None: | |
| cls._validate_poc_update_permissions(team_id, updated_by_user_id, existing_team) | |
| # Prevent removal of POC without reassignment | |
| if existing_team.poc_id and (not dto.poc_id or not dto.poc_id.strip()): | |
| raise ValueError(ApiErrors.CANNOT_REMOVE_POC) | |
| # Validate new POC is a team member | |
| if dto.poc_id and dto.poc_id.strip(): | |
| team_members = UserService.get_users_by_team_id(team_id) | |
| team_member_ids = [user.id for user in team_members] | |
| if dto.poc_id not in team_member_ids: | |
| raise ValueError(f"User {dto.poc_id} is not a member of the team") |
🤖 Prompt for AI Agents
In todo/services/team_service.py around lines 348-349, the update path currently
allows clearing the POC by passing an empty string and does not validate that a
newly assigned POC is a team member; update the logic to (1) treat
empty-string/blank values as an attempt to remove the POC and reject that by
raising ApiErrors.CANNOT_REMOVE_POC (i.e., only allow POC changes when a
non-blank ID is provided), and (2) when a non-blank new poc_id is provided,
ensure that user is a team member—if they are not, either add them to the team’s
members list (consistent with create_team) or raise a validation error;
implement the chosen behavior and add tests for both the removal attempt and
assignment-of-non-member cases.
| @patch("todo.services.team_service.UserRoleService.has_role") | ||
| def test_poc_update_permissions_team_admin_success(self, mock_has_role): | ||
| mock_has_role.return_value = True | ||
|
|
||
| result = TeamService._validate_poc_update_permissions(self.team_id, self.admin_id, self.team_model) | ||
| self.assertIsNone(result) | ||
| mock_has_role.assert_called_once_with(self.admin_id, RoleName.ADMIN.value, RoleScope.TEAM.value, self.team_id) | ||
|
|
||
| @patch("todo.services.team_service.UserRoleService.has_role") | ||
| def test_poc_update_permissions_regular_member_fails(self, mock_has_role): | ||
| mock_has_role.return_value = False | ||
|
|
||
| with self.assertRaises(PermissionError) as context: | ||
| TeamService._validate_poc_update_permissions(self.team_id, self.member_id, self.team_model) | ||
|
|
||
| self.assertIn(ApiErrors.UNAUTHORIZED_TITLE, str(context.exception)) | ||
| mock_has_role.assert_called_once_with(self.member_id, RoleName.ADMIN.value, RoleScope.TEAM.value, self.team_id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test coverage for the team creator updating POC.
The tests validate the admin and non-admin scenarios, but don't cover the case where the team creator (not necessarily an admin) updates the POC. According to the implementation in team_service.py (line 515-516), the creator can update the POC without needing the admin role.
Add a test case to verify the creator bypass path:
@patch("todo.services.team_service.UserRoleService.has_role")
def test_poc_update_permissions_team_creator_success(self, mock_has_role):
"""Test that team creator can update POC without admin role"""
# Creator updates POC - should bypass role check
result = TeamService._validate_poc_update_permissions(self.team_id, self.user_id, self.team_model)
self.assertIsNone(result)
# Verify has_role was NOT called since creator bypass should occur first
mock_has_role.assert_not_called()🤖 Prompt for AI Agents
In todo/tests/unit/services/test_team_service.py around lines 306 to 322, tests
cover admin and non-admin flows for _validate_poc_update_permissions but miss
the team-creator bypass case; add a new unit test where the caller is the team
creator (self.user_id) so the method returns None and UserRoleService.has_role
is not invoked. Implement the test using
@patch("todo.services.team_service.UserRoleService.has_role") with
mock_has_role, call TeamService._validate_poc_update_permissions(self.team_id,
self.user_id, self.team_model), assert the result is None, and assert
mock_has_role.assert_not_called() to confirm the creator path short-circuits the
role check.
Date: 10 Oct 2025
Developer Name: @AnujChhikara
Issue Ticket Number
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes
Description by Korbit AI
What change is being made?
Enforce PoC update permissions by adding _validate_poc_update_permissions and invoking it during team updates when PoC changes, and extend tests to cover authorized/unauthorized scenarios.
Why are these changes being made?
To ensure only the team creator or an admin can update the PoC, preventing unauthorized updates and surfacing a consistent unauthorized error. This strengthens access control around PoC changes.