Skip to content

fix: include specific IDs in owner validation errors #7116

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
gagantrivedi wants to merge 1 commit into
base: main
Choose a base branch
from
+22 −10
Open

fix: include specific IDs in owner validation errors #7116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 20 additions & 10 deletions api/features/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,22 +306,32 @@ def validate_multivariate_options(self, multivariate_options): # type: ignore[n

def validate_owners(self, owners: list[FFAdminUser]) -> list[FFAdminUser]:
project: Project = self.context["project"]
for user in owners:
if not user.has_project_permission(VIEW_PROJECT, project):
raise serializers.ValidationError(
"Some users do not have access to this project."
)
invalid_users = [
user
for user in owners
if not user.has_project_permission(VIEW_PROJECT, project)
]
if invalid_users:
invalid_user_ids = [user.id for user in invalid_users]
Copy link
Copy Markdown
Member

@khvn26 khvn26 Apr 3, 2026

Choose a reason for hiding this comment

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

maybe emails or uuids?

Copy link
Copy Markdown
Member Author

@gagantrivedi gagantrivedi Apr 3, 2026

Choose a reason for hiding this comment

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

Returning email would be a data leak and returning uuid when you have supplied id is even more confusing?

raise serializers.ValidationError(
f"Users with ids {invalid_user_ids} do not have access to this project."
)
return owners

def validate_group_owners(
self, group_owners: list[UserPermissionGroup]
) -> list[UserPermissionGroup]:
project: Project = self.context["project"]
for group in group_owners:
if group.organisation_id != project.organisation_id:
raise serializers.ValidationError(
"Some groups do not belong to this project's organisation."
)
invalid_groups = [
group
for group in group_owners
if group.organisation_id != project.organisation_id
]
if invalid_groups:
invalid_group_ids = [group.id for group in invalid_groups]
Copy link
Copy Markdown
Member

@khvn26 khvn26 Apr 3, 2026

Choose a reason for hiding this comment

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

maybe group names?

Copy link
Copy Markdown
Member Author

@gagantrivedi gagantrivedi Apr 3, 2026

Choose a reason for hiding this comment

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

Same as above — I don't think it makes sense to output different params than the input.

raise serializers.ValidationError(
f"Groups with ids {invalid_group_ids} do not belong to this project's organisation."
)
return group_owners

def validate_name(self, name: str): # type: ignore[no-untyped-def]
Expand Down
2 changes: 2 additions & 0 deletions api/tests/unit/features/test_unit_features_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4750,6 +4750,7 @@ def test_create_feature__group_owner_from_different_org__returns_400(
# Then
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "group_owners" in response.json()
assert str(other_group.id) in response.json()["group_owners"][0]


def test_create_feature__owner_without_project_access__returns_400(
Expand All @@ -4773,6 +4774,7 @@ def test_create_feature__owner_without_project_access__returns_400(
# Then
assert response.status_code == status.HTTP_400_BAD_REQUEST
assert "owners" in response.json()
assert str(other_user.id) in response.json()["owners"][0]


def test_update_feature__owners_in_request_body__returns_200_without_changes(
Expand Down
Loading