Skip to content

Commit

Permalink
Fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
benjackwhite committed Mar 25, 2024
1 parent d9fc0c5 commit 1101b53
Show file tree
Hide file tree
Showing 6 changed files with 23 additions and 34 deletions.
31 changes: 11 additions & 20 deletions ee/api/rbac/access_control.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,25 +79,6 @@ def validate(self, data):
return data


class AccessControlFilterBackend(BaseFilterBackend):
"""
Filters the queryset automatically based on access controls
"""

def filter_queryset(
self,
request: Request,
queryset: Union[QuerySet, RawQuerySet],
view: APIView,
):
if isinstance(queryset, RawQuerySet):
return queryset

queryset = view.user_access_control.filter_queryset_by_access_level(queryset)

return queryset


class AccessControlViewSetMixin:
"""
Adds an "access_controls" action to the viewset that handles access control for the given resource
Expand All @@ -110,7 +91,17 @@ class AccessControlViewSetMixin:
# 2. Get the actual object which we can pass to the serializer to check if the user created it
# 3. We can also use the serializer to check the access level for the object

filter_backends = [AccessControlFilterBackend]
def filter_queryset(self, queryset):
queryset = super().filter_queryset(queryset)
# TODO: Detect GET param to include hidden resources (for admins)

if self.action != "list":
# NOTE: If we are getting an individual object then we don't filter it out here - this is handled by the permission logic
# The reason being, that if we filter out here already, we can't load the object which is required for checking access controls for it
return queryset
queryset = self.user_access_control.filter_queryset_by_access_level(queryset)

return queryset

def _get_access_control_serializer(self, *args, **kwargs):
kwargs.setdefault("context", self.get_serializer_context())
Expand Down
2 changes: 1 addition & 1 deletion posthog/api/feature_flag.py
Original file line number Diff line number Diff line change
Expand Up @@ -363,9 +363,9 @@ class Meta:

class FeatureFlagViewSet(
TeamAndOrgViewSetMixin,
AccessControlViewSetMixin,
TaggedItemViewSetMixin,
ForbidDestroyModel,
AccessControlViewSetMixin,
viewsets.ModelViewSet,
):
"""
Expand Down
2 changes: 1 addition & 1 deletion posthog/api/insight.py
Original file line number Diff line number Diff line change
Expand Up @@ -562,9 +562,9 @@ def dashboard_tile_from_context(self, insight: Insight, dashboard: Optional[Dash

class InsightViewSet(
TeamAndOrgViewSetMixin,
AccessControlViewSetMixin,
TaggedItemViewSetMixin,
ForbidDestroyModel,
AccessControlViewSetMixin,
viewsets.ModelViewSet,
):
scope_object = "insight"
Expand Down
2 changes: 1 addition & 1 deletion posthog/api/notebook.py
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ def update(self, instance: Notebook, validated_data: Dict, **kwargs) -> Notebook
],
)
)
class NotebookViewSet(TeamAndOrgViewSetMixin, ForbidDestroyModel, viewsets.ModelViewSet, AccessControlViewSetMixin):
class NotebookViewSet(TeamAndOrgViewSetMixin, AccessControlViewSetMixin, ForbidDestroyModel, viewsets.ModelViewSet):
scope_object = "notebook"
queryset = Notebook.objects.all()
filter_backends = [DjangoFilterBackend]
Expand Down
6 changes: 1 addition & 5 deletions posthog/api/routing.py
Original file line number Diff line number Diff line change
Expand Up @@ -244,11 +244,7 @@ def user_permissions(self) -> "UserPermissions":
def user_access_control(self) -> "UserAccessControl":
team: Optional[Team] = None
try:
# TRICKY: We don't want to load the team if that is the object we are validating (because we get into a recursive loop)
if not self.request.resolver_match.url_name.startswith("team-"):
# TODO: This calls get_object for team.py which triggers a recursive loop via get_queryset (which uses this method)
# It's mostly a problem for team view because the object we are filtering for _is_ this object :see_no_evil:
team = self.team
team = self.team
except (Team.DoesNotExist, KeyError):
pass

Expand Down
14 changes: 8 additions & 6 deletions posthog/rbac/user_access_control.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,9 @@ def filter_queryset_by_access_level(self, queryset: QuerySet) -> QuerySet:
# Find all items related to the queryset model that have access controls such that the effective level for the user is "none"
# and exclude them from the queryset

resource = model_to_resource(queryset.model)
model = queryset.model
resource = model_to_resource(model)
model_has_creator = hasattr(model, "created_by")

# TODO: Make this more efficient
role_memberships = self._user.role_memberships.select_related("role").all()
Expand Down Expand Up @@ -242,13 +244,13 @@ def filter_queryset_by_access_level(self, queryset: QuerySet) -> QuerySet:
if all(access_level == NO_ACCESS_LEVEL for access_level in access_levels):
blocked_resource_ids.add(resource_id)

# TODO: Don't filter the queryset if the user is an admin
# TODO: Allow the creator of the resource to be able to see it - could either do this by saving
# a special access control when adding one if no other exists, or by saving a link to the foreign resource

# Filter the queryset based on the access controls
if blocked_resource_ids:
queryset = queryset.exclude(id__in=blocked_resource_ids)
# Filter out any IDs where the user is not the creator and the id is blocked
if model_has_creator:
queryset = queryset.exclude(Q(id__in=blocked_resource_ids) & ~Q(created_by=self._user))
else:
queryset = queryset.exclude(id__in=blocked_resource_ids)

return queryset

Expand Down

0 comments on commit 1101b53

Please sign in to comment.