Skip to content
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.

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

Fix _perm_cache processing #498

Merged
merged 10 commits into from
Mar 16, 2024
Merged
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
7 changes: 3 additions & 4 deletions strawberry_django/utils/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,10 @@ def filter_for_user_q(
raise ValueError(f"Cannot mix app_labels ({app_labels!r})")

# Small optimization if the user's permissions are cached
perm_cache = getattr(user, "_perm_cache", None)
if perm_cache is not None: # pragma:nocover
perm_cache: Optional[Set[str]] = getattr(user, "_perm_cache", None)
if perm_cache is not None:
f = any if any_perm else all
user_perms: Set[str] = {p.codename for p in perm_cache}
Copy link
Contributor

Choose a reason for hiding this comment

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

issue (performance): The removal of the explicit set creation and type hint for user_perms simplifies the code. However, ensure that perm_cache directly supports efficient membership testing as a set does. If perm_cache is a list or another data structure with O(n) lookup time, this change could negatively impact performance, especially with a large number of permissions.

Suggested change
user_perms: Set[str] = {p.codename for p in perm_cache}
if isinstance(perm_cache, set) or isinstance(perm_cache, frozenset):
if f(p in perm_cache for p in perms_list):
pass # Your existing logic here
else:
perm_cache_set = set(perm_cache) # Convert to set for efficient lookup
if f(p in perm_cache_set for p in perms_list):
pass # Your existing logic here

Copy link
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): This change assumes that perm_cache directly contains permission codenames or objects that can be compared to perms_list. It's crucial to verify that the structure and content of perm_cache are compatible with this direct iteration and comparison, to avoid introducing bugs related to permission checks.

Suggested change
user_perms: Set[str] = {p.codename for p in perm_cache}
if f(perm_cache.get(p, False) for p in perms_list):

if f(p in user_perms for p in perms_list):
if f(p in perm_cache for p in perms_list):
return qs

q = Q()
Expand Down
18 changes: 18 additions & 0 deletions tests/projects/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
OuterRef,
Prefetch,
Q,
Subquery,
)
from django.db.models.functions import Now
from django.db.models.query import QuerySet
Expand All @@ -34,6 +35,7 @@
IsAuthenticated,
IsStaff,
IsSuperuser,
filter_for_user,
)
from strawberry_django.relay import ListConnectionWithTotalCount

Expand Down Expand Up @@ -205,6 +207,22 @@ def milestone_name(self) -> str:
def milestone_name_without_only_optimization(self) -> str:
return self.milestone.name

@strawberry_django.field(
annotate={
"_private_name": lambda info: Subquery(
filter_for_user(
Issue.objects.all(),
info.context.request.user,
["projects.view_issue"],
)
.filter(id=OuterRef("pk"))
.values("name")[:1],
),
},
)
def private_name(self, root: Issue) -> Optional[str]:
return root._private_name # type: ignore


@strawberry_django.type(Tag)
class TagType(relay.Node):
Expand Down
1 change: 1 addition & 0 deletions tests/projects/snapshots/schema.gql
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ type IssueType implements Node {
): FavoriteTypeConnection!
milestoneName: String!
milestoneNameWithoutOnlyOptimization: String!
privateName: String
}

"""A connection to a list of items."""
Expand Down
45 changes: 45 additions & 0 deletions tests/test_permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
from strawberry.relay import to_base64
from typing_extensions import Literal, TypeAlias

from strawberry_django.optimizer import DjangoOptimizerExtension

from .projects.faker import (
GroupFactory,
IssueFactory,
Expand Down Expand Up @@ -282,6 +284,49 @@ def test_superuser_required_optional(db, gql_client: GraphQLTestClient):
}


@pytest.mark.django_db(transaction=True)
def test_perm_cached(db, gql_client: GraphQLTestClient):
"""Validates that the permission caching mechanism correctly stores permissions as a set of strings.

The test targets the `_perm_cache` attribute used in `utils/query.py`. It verifies that
the attribute behaves as expected, holding a `Set[str]` that represents permission
codenames, rather than direct Permission objects.

This test addresses a regression captured by the following error:

```
user_perms: Set[str] = {p.codename for p in perm_cache}
^^^^^^^^^^
AttributeError: 'str' object has no attribute 'codename'
```
"""
query = """
query Issue ($id: GlobalID!) {
issuePermRequired (id: $id) {
id
privateName
}
}
"""
issue = IssueFactory.create(name="Test")

# User with permission
user_with_perm = UserFactory.create()
user_with_perm.user_permissions.add(
Permission.objects.get(codename="view_issue"),
)
assign_perm("view_issue", user_with_perm, issue)
with gql_client.login(user_with_perm):
if DjangoOptimizerExtension.enabled.get():
res = gql_client.query(query, {"id": to_base64("IssueType", issue.pk)})
assert res.data == {
"issuePermRequired": {
"id": to_base64("IssueType", issue.pk),
"privateName": issue.name,
},
}


@pytest.mark.django_db(transaction=True)
@pytest.mark.parametrize("kind", perm_kinds)
def test_perm_required(db, gql_client: GraphQLTestClient, kind: PermKind):
Expand Down
Loading