Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion api/environments/permissions/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ def has_permission(self, request, view): # type: ignore[no-untyped-def]
project_lookup = Q(id=project_id)
project = Project.objects.get(project_lookup)
return request.user.has_project_permission(CREATE_ENVIRONMENT, project)
except Project.DoesNotExist:
# We catch ValueError and TypeError here to resolve previous issues with invalid project IDs
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

PR commentary left permanently in production code

Low Severity

The comment # We catch ValueError and TypeError here to resolve previous issues with invalid project IDs reads as PR rationale rather than code documentation. It describes the historical motivation for the change rather than explaining the code's behavior. The except clause itself is self-documenting. The project maintainer already flagged this in review but it remains in the diff.

Fix in Cursor Fix in Web

except (Project.DoesNotExist, ValueError, TypeError):
return False

# return true as all users can list and obj permissions will be handled later
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,38 @@ def test_project_user_without_create_environment_permission_cannot_create_enviro
assert result is False


def test_create_environment__invalid_project_id_string__returns_false(
admin_user: FFAdminUser,
) -> None:
# Given
mock_view.action = "create"
mock_view.detail = False
mock_request.user = admin_user
mock_request.data = {"project": "not-a-valid-id", "name": "Test environment"}

# When
result = environment_permissions.has_permission(mock_request, mock_view) # type: ignore[no-untyped-call]

# Then
assert result is False


def test_create_environment__none_project_id__returns_false(
admin_user: FFAdminUser,
) -> None:
# Given
mock_view.action = "create"
mock_view.detail = False
mock_request.user = admin_user
mock_request.data = {"name": "Test environment"}

# When
result = environment_permissions.has_permission(mock_request, mock_view) # type: ignore[no-untyped-call]

# Then
assert result is False


def test_all_users_can_list_environments_for_project(
staff_user: FFAdminUser,
) -> None:
Expand Down