Skip to content

fix: add validation for updating the team PoC role #283

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
+34 −1
Open

fix: add validation for updating the team PoC role #283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4e8e19
feat(team_service): add POC update permission validation to ensure on…
AnujChhikara Oct 8, 2025
cfbfb38
test(team_service): add unit tests for POC update permission validati…
AnujChhikara Oct 8, 2025
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
15 changes: 14 additions & 1 deletion todo/services/team_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from todo.models.common.pyobjectid import PyObjectId
from todo.repositories.team_creation_invite_code_repository import TeamCreationInviteCodeRepository
from todo.repositories.team_repository import TeamRepository, UserTeamDetailsRepository
from todo.constants.messages import AppMessages
from todo.constants.messages import AppMessages, ApiErrors
from todo.constants.role import RoleName
from todo.utils.invite_code_utils import generate_invite_code
from typing import List
Expand Down Expand Up @@ -345,6 +345,9 @@ def update_team(cls, team_id: str, dto: UpdateTeamDTO, updated_by_user_id: str)
if not existing_team:
raise ValueError(f"Team with id {team_id} not found")

if dto.poc_id is not None:
cls._validate_poc_update_permissions(team_id, updated_by_user_id, existing_team)
Comment on lines +348 to +349
Copy link

Choose a reason for hiding this comment

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

Unnecessary PoC permission validation on unchanged values category Performance

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

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +348 to +349
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

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


# Prepare update data
update_data = {}
if dto.name is not None:
Expand Down Expand Up @@ -507,6 +510,16 @@ def _validate_remove_member_permissions(cls, user_id: str, team_id: str, removed
if not UserRoleService.has_role(removed_by_user_id, RoleName.ADMIN.value, RoleScope.TEAM.value, team_id):
raise NotTeamAdminException()

@classmethod
def _validate_poc_update_permissions(cls, team_id: str, updated_by_user_id: str, team):
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)
Copy link

Choose a reason for hiding this comment

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

Missing context in PermissionError for PoC update validation category Error Handling

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

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

Comment on lines +515 to +521
Copy link

Choose a reason for hiding this comment

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

Unclear Permission Logic Flow category Readability

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

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.


@classmethod
def remove_member_from_team(cls, user_id: str, team_id: str, removed_by_user_id: str):
cls._validate_remove_member_permissions(user_id, team_id, removed_by_user_id)
Expand Down
20 changes: 20 additions & 0 deletions todo/tests/unit/services/test_team_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from unittest.mock import patch
from datetime import datetime, timezone

from todo.constants.messages import ApiErrors
from todo.exceptions.team_exceptions import (
CannotRemoveOwnerException,
CannotRemoveTeamPOCException,
Expand All @@ -13,6 +14,7 @@
from todo.models.common.pyobjectid import PyObjectId
from todo.dto.user_dto import UserDTO
from todo.dto.team_dto import TeamDTO
from todo.constants.role import RoleName, RoleScope


class TeamServiceTests(TestCase):
Expand Down Expand Up @@ -300,3 +302,21 @@ def test_user_can_remove_themselves(
self.assertEqual(log_entry.action, "member_left_team")
self.assertEqual(str(log_entry.team_id), self.team_id)
self.assertEqual(str(log_entry.performed_by), self.member_id)

@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)
Comment on lines +306 to +322
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.