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

chore: fixes to objects/queryset perm checks + type updates #27330

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 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: 7 additions & 0 deletions frontend/src/exporter/__mocks__/Exporter.mocks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ export const dashboard: DashboardType = {
effective_privilege_level: 37,
timezone: null,
tags: [],
user_access_level: 'editor',
},
} as DashboardTile,
{
Expand Down Expand Up @@ -369,6 +370,7 @@ export const dashboard: DashboardType = {
effective_privilege_level: 37,
timezone: null,
tags: [],
user_access_level: 'editor',
},
} as DashboardTile,
{
Expand Down Expand Up @@ -547,6 +549,7 @@ export const dashboard: DashboardType = {
effective_privilege_level: 37,
timezone: null,
tags: [],
user_access_level: 'editor',
},
} as DashboardTile,
{
Expand Down Expand Up @@ -688,6 +691,7 @@ export const dashboard: DashboardType = {
effective_privilege_level: 37,
timezone: null,
tags: [],
user_access_level: 'editor',
},
} as DashboardTile,
{
Expand Down Expand Up @@ -1107,6 +1111,7 @@ export const dashboard: DashboardType = {
effective_privilege_level: 37,
timezone: null,
tags: [],
user_access_level: 'editor',
},
} as DashboardTile,
{
Expand Down Expand Up @@ -1245,6 +1250,7 @@ export const dashboard: DashboardType = {
effective_privilege_level: 37,
timezone: null,
tags: [],
user_access_level: 'editor',
},
} as DashboardTile,
],
Expand All @@ -1257,5 +1263,6 @@ export const dashboard: DashboardType = {
restriction_level: 37,
effective_restriction_level: 37,
effective_privilege_level: 37,
user_access_level: 'editor',
tags: [],
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function AccessControlObject(props: AccessControlLogicProps): JSX.Element
return (
<BindLogic logic={accessControlLogic} props={props}>
<div className="space-y-6">
{canEditAccessControls === true ? (
{canEditAccessControls === false ? (
<LemonBanner type="warning">
<b>Permission required</b>
<br />
Expand Down
1 change: 1 addition & 0 deletions frontend/src/models/dashboardsModel.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const basicDashboard: DashboardBasicType = {
restriction_level: DashboardRestrictionLevel.EveryoneInProjectCanEdit,
effective_restriction_level: DashboardRestrictionLevel.EveryoneInProjectCanEdit,
effective_privilege_level: DashboardPrivilegeLevel.CanEdit,
user_access_level: 'editor',
}

describe('the dashboards model', () => {
Expand Down
1 change: 1 addition & 0 deletions frontend/src/scenes/insights/insightLogic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ function insightModelWith(properties: Record<string, any>): QueryBasedInsightMod
effective_restriction_level: DashboardRestrictionLevel.EveryoneInProjectCanEdit,
layouts: {},
color: null,
user_access_level: 'editor',
...properties,
} as QueryBasedInsightModel
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ const insightActionsMapping: Record<
disable_baseline: () => null,
dashboard_tiles: () => null,
query_status: () => null,
user_access_level: () => null,
}

function summarizeChanges(filtersAfter: Partial<FilterType>): ChangeMapping | null {
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1818,7 +1818,7 @@ export interface TextModel {
last_modified_at: string
}

export interface InsightModel extends Cacheable {
export interface InsightModel extends Cacheable, WithAccessControl {
/** The unique key we use when communicating with the user, e.g. in URLs */
short_id: InsightShortId
/** The primary key in the database, used as well in API endpoints */
Expand Down Expand Up @@ -1857,7 +1857,7 @@ export interface QueryBasedInsightModel extends Omit<InsightModel, 'filters'> {
query: Node | null
}

export interface DashboardBasicType {
export interface DashboardBasicType extends WithAccessControl {
id: number
name: string
description: string
Expand Down
8 changes: 4 additions & 4 deletions posthog/api/dashboards/dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

import structlog
from django.db.models import Prefetch
from django.shortcuts import get_object_or_404
from django.utils.timezone import now
from rest_framework import exceptions, serializers, viewsets
from rest_framework.permissions import SAFE_METHODS, BasePermission
Expand Down Expand Up @@ -516,13 +515,14 @@ def dangerously_get_queryset(self):
),
)

# Add access level filtering for list actions
queryset = self._filter_queryset_by_access_level(queryset)

return queryset

@monitor(feature=Feature.DASHBOARD, endpoint="dashboard", method="GET")
def retrieve(self, request: Request, *args: Any, **kwargs: Any) -> Response:
pk = kwargs["pk"]
queryset = self.get_queryset()
dashboard = get_object_or_404(queryset, pk=pk)
dashboard = self.get_object()
dashboard.last_accessed_at = now()
dashboard.save(update_fields=["last_accessed_at"])
serializer = DashboardSerializer(dashboard, context=self.get_serializer_context())
Expand Down
10 changes: 7 additions & 3 deletions posthog/api/insight.py
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ class Meta:
fields = ["id", "dashboard_id", "deleted"]


class InsightBasicSerializer(TaggedItemSerializerMixin, serializers.ModelSerializer):
class InsightBasicSerializer(TaggedItemSerializerMixin, serializers.ModelSerializer, UserAccessControlSerializerMixin):
"""
Simplified serializer to speed response times when loading large amounts of objects.
"""
Expand Down Expand Up @@ -272,7 +272,7 @@ def _dashboard_tiles(self, instance):
return [tile.dashboard_id for tile in instance.dashboard_tiles.all()]


class InsightSerializer(InsightBasicSerializer, UserPermissionsSerializerMixin, UserAccessControlSerializerMixin):
class InsightSerializer(InsightBasicSerializer, UserPermissionsSerializerMixin):
result = serializers.SerializerMethodField()
hasMore = serializers.SerializerMethodField()
columns = serializers.SerializerMethodField()
Expand Down Expand Up @@ -800,6 +800,10 @@ def dangerously_get_queryset(self):
),
)

# Add access level filtering for list actions if not sharing access token
if not isinstance(self.request.successful_authenticator, SharingAccessTokenAuthentication):
queryset = self._filter_queryset_by_access_level(queryset)

queryset = queryset.select_related("created_by", "last_modified_by", "team")
if self.action == "list":
queryset = queryset.prefetch_related("tagged_items__tag")
Expand Down Expand Up @@ -1188,7 +1192,7 @@ def calculate_path(self, request: request.Request) -> dict[str, Any]:
# /projects/:id/insights/:short_id/viewed
# Creates or updates an InsightViewed object for the user/insight combo
# ******************************************
@action(methods=["POST"], detail=True)
@action(methods=["POST"], detail=True, required_scopes=["insight:read"])
def viewed(self, request: request.Request, *args: Any, **kwargs: Any) -> Response:
InsightViewed.objects.update_or_create(
team=self.team,
Expand Down
4 changes: 2 additions & 2 deletions posthog/api/notebook.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def log_notebook_activity(
)


class NotebookMinimalSerializer(serializers.ModelSerializer):
class NotebookMinimalSerializer(serializers.ModelSerializer, UserAccessControlSerializerMixin):
created_by = UserBasicSerializer(read_only=True)
last_modified_by = UserBasicSerializer(read_only=True)

Expand All @@ -97,7 +97,7 @@ class Meta:
read_only_fields = fields


class NotebookSerializer(NotebookMinimalSerializer, UserAccessControlSerializerMixin):
class NotebookSerializer(NotebookMinimalSerializer):
class Meta:
model = Notebook
fields = [
Expand Down
36 changes: 19 additions & 17 deletions posthog/api/routing.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,27 +168,29 @@ def get_queryset(self) -> QuerySet:

queryset = self._filter_queryset_by_parents_lookups(queryset)

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

# NOTE: Half implemented - for admins, they may want to include listing of results that are not accessible (like private resources)
include_all_if_admin = self.request.GET.get("admin_include_all") == "true"

# Additionally "projects" is a special one where we always want to include all projects if you're an org admin
if self.scope_object == "project":
include_all_if_admin = True

# Only apply access control filter if we're not already in a recursive call
queryset = self.user_access_control.filter_queryset_by_access_level(
queryset, include_all_if_admin=include_all_if_admin
)
queryset = self._filter_queryset_by_access_level(queryset)

return queryset
finally:
self._in_get_queryset = False

def _filter_queryset_by_access_level(self, queryset: QuerySet) -> QuerySet:
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

# NOTE: Half implemented - for admins, they may want to include listing of results that are not accessible (like private resources)
include_all_if_admin = self.request.GET.get("admin_include_all") == "true"

# Additionally "projects" is a special one where we always want to include all projects if you're an org admin
if self.scope_object == "project":
include_all_if_admin = True

return self.user_access_control.filter_queryset_by_access_level(
queryset, include_all_if_admin=include_all_if_admin
)

def dangerously_get_object(self) -> Any:
"""
WARNING: This should be used very carefully. It bypasses common security access control checks.
Expand Down Expand Up @@ -307,7 +309,7 @@ def organization_id(self) -> str:
current_organization_id = self.team.organization_id
if self._is_project_view:
current_organization_id = self.project.organization_id
else:
elif user:
current_organization_id = user.current_organization_id

if not current_organization_id:
Expand Down
Loading
Loading