From ecfa00efe1714939792ba4677a72b6b3cf5c781b Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Fri, 20 Dec 2024 16:09:23 -0500 Subject: [PATCH 01/15] Add access denied middleware --- frontend/src/layout/navigation-3000/Navigation.tsx | 6 +++++- frontend/src/lib/components/AccessDenied/index.tsx | 10 ++++++++++ frontend/src/lib/logic/apiStatusLogic.ts | 11 +++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 frontend/src/lib/components/AccessDenied/index.tsx diff --git a/frontend/src/layout/navigation-3000/Navigation.tsx b/frontend/src/layout/navigation-3000/Navigation.tsx index 11b0e928d4f23..3cbd466defb7d 100644 --- a/frontend/src/layout/navigation-3000/Navigation.tsx +++ b/frontend/src/layout/navigation-3000/Navigation.tsx @@ -2,10 +2,12 @@ import './Navigation.scss' import clsx from 'clsx' import { useValues } from 'kea' +import { AccessDenied } from 'lib/components/AccessDenied' import { BillingAlertsV2 } from 'lib/components/BillingAlertsV2' import { CommandBar } from 'lib/components/CommandBar/CommandBar' import { FlaggedFeature } from 'lib/components/FlaggedFeature' import { FEATURE_FLAGS } from 'lib/constants' +import { apiStatusLogic } from 'lib/logic/apiStatusLogic' import { ReactNode } from 'react' import { SceneConfig } from 'scenes/sceneTypes' @@ -30,6 +32,8 @@ export function Navigation({ const { mobileLayout } = useValues(navigationLogic) const { activeNavbarItem, mode } = useValues(navigation3000Logic) + const { resourceAccessDenied } = useValues(apiStatusLogic) + if (mode !== 'full') { return ( // eslint-disable-next-line react/forbid-dom-props @@ -60,7 +64,7 @@ export function Navigation({ > {!sceneConfig?.hideBillingNotice && } {!sceneConfig?.hideProjectNotice && } - {children} + {resourceAccessDenied ? : children} diff --git a/frontend/src/lib/components/AccessDenied/index.tsx b/frontend/src/lib/components/AccessDenied/index.tsx new file mode 100644 index 0000000000000..06541fc4736b9 --- /dev/null +++ b/frontend/src/lib/components/AccessDenied/index.tsx @@ -0,0 +1,10 @@ +export function AccessDenied(): JSX.Element { + return ( +
+

Access denied

+

+ You don't have access to this resource. Please contact support if you think this is a mistake. +

+
+ ) +} diff --git a/frontend/src/lib/logic/apiStatusLogic.ts b/frontend/src/lib/logic/apiStatusLogic.ts index a71a2e11518ad..55e51b42780fc 100644 --- a/frontend/src/lib/logic/apiStatusLogic.ts +++ b/frontend/src/lib/logic/apiStatusLogic.ts @@ -10,6 +10,8 @@ export const apiStatusLogic = kea([ onApiResponse: (response?: Response, error?: any) => ({ response, error }), setInternetConnectionIssue: (issue: boolean) => ({ issue }), setTimeSensitiveAuthenticationRequired: (required: boolean) => ({ required }), + setResourceAccessDenied: (resource: string) => ({ resource }), + clearResourceAccessDenied: true, }), reducers({ @@ -26,6 +28,13 @@ export const apiStatusLogic = kea([ setTimeSensitiveAuthenticationRequired: (_, { required }) => required, }, ], + resourceAccessDenied: [ + null as string | null, + { + setResourceAccessDenied: (_, { resource }) => resource, + clearResourceAccessDenied: () => null, + }, + ], }), listeners(({ cache, actions, values }) => ({ onApiResponse: async ({ response, error }, breakpoint) => { @@ -46,6 +55,8 @@ export const apiStatusLogic = kea([ const data = await response?.json() if (data.detail === 'This action requires you to be recently authenticated.') { actions.setTimeSensitiveAuthenticationRequired(true) + } else if (data.code === 'permission_denied') { + actions.setResourceAccessDenied(data.resource || 'resource') } } } catch (e) { From f98f20eecc0b4c01afdea8762429e9fe9cb92e8e Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Fri, 27 Dec 2024 10:08:21 -0500 Subject: [PATCH 02/15] Update some more user access control types --- frontend/src/exporter/__mocks__/Exporter.mocks.tsx | 7 +++++++ frontend/src/models/dashboardsModel.test.ts | 1 + frontend/src/scenes/insights/insightLogic.test.ts | 1 + .../src/scenes/saved-insights/activityDescriptions.tsx | 1 + frontend/src/types.ts | 4 ++-- 5 files changed, 12 insertions(+), 2 deletions(-) diff --git a/frontend/src/exporter/__mocks__/Exporter.mocks.tsx b/frontend/src/exporter/__mocks__/Exporter.mocks.tsx index 6d2b7e87d1fad..3da3f05856d88 100644 --- a/frontend/src/exporter/__mocks__/Exporter.mocks.tsx +++ b/frontend/src/exporter/__mocks__/Exporter.mocks.tsx @@ -222,6 +222,7 @@ export const dashboard: DashboardType = { effective_privilege_level: 37, timezone: null, tags: [], + user_access_level: 'editor', }, } as DashboardTile, { @@ -369,6 +370,7 @@ export const dashboard: DashboardType = { effective_privilege_level: 37, timezone: null, tags: [], + user_access_level: 'editor', }, } as DashboardTile, { @@ -547,6 +549,7 @@ export const dashboard: DashboardType = { effective_privilege_level: 37, timezone: null, tags: [], + user_access_level: 'editor', }, } as DashboardTile, { @@ -688,6 +691,7 @@ export const dashboard: DashboardType = { effective_privilege_level: 37, timezone: null, tags: [], + user_access_level: 'editor', }, } as DashboardTile, { @@ -1107,6 +1111,7 @@ export const dashboard: DashboardType = { effective_privilege_level: 37, timezone: null, tags: [], + user_access_level: 'editor', }, } as DashboardTile, { @@ -1245,6 +1250,7 @@ export const dashboard: DashboardType = { effective_privilege_level: 37, timezone: null, tags: [], + user_access_level: 'editor', }, } as DashboardTile, ], @@ -1258,4 +1264,5 @@ export const dashboard: DashboardType = { effective_restriction_level: 37, effective_privilege_level: 37, tags: [], + user_access_level: 'editor', } diff --git a/frontend/src/models/dashboardsModel.test.ts b/frontend/src/models/dashboardsModel.test.ts index c3d42a78b83b7..d0e6517278ef5 100644 --- a/frontend/src/models/dashboardsModel.test.ts +++ b/frontend/src/models/dashboardsModel.test.ts @@ -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', () => { diff --git a/frontend/src/scenes/insights/insightLogic.test.ts b/frontend/src/scenes/insights/insightLogic.test.ts index cdf3b53a54784..af97f9689a0fd 100644 --- a/frontend/src/scenes/insights/insightLogic.test.ts +++ b/frontend/src/scenes/insights/insightLogic.test.ts @@ -99,6 +99,7 @@ function insightModelWith(properties: Record): QueryBasedInsightMod effective_restriction_level: DashboardRestrictionLevel.EveryoneInProjectCanEdit, layouts: {}, color: null, + user_access_level: 'editor', ...properties, } as QueryBasedInsightModel } diff --git a/frontend/src/scenes/saved-insights/activityDescriptions.tsx b/frontend/src/scenes/saved-insights/activityDescriptions.tsx index cd7668905e5ad..8f6f9f10dc2a6 100644 --- a/frontend/src/scenes/saved-insights/activityDescriptions.tsx +++ b/frontend/src/scenes/saved-insights/activityDescriptions.tsx @@ -231,6 +231,7 @@ const insightActionsMapping: Record< disable_baseline: () => null, dashboard_tiles: () => null, query_status: () => null, + user_access_level: () => null, } function summarizeChanges(filtersAfter: Partial): ChangeMapping | null { diff --git a/frontend/src/types.ts b/frontend/src/types.ts index d8e85d4cdc027..d5d3d08524f92 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -1809,7 +1809,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 */ @@ -1848,7 +1848,7 @@ export interface QueryBasedInsightModel extends Omit { query: Node | null } -export interface DashboardBasicType { +export interface DashboardBasicType extends WithAccessControl { id: number name: string description: string From 2e9ca06846ed26f3e80ed582a35860939a3fbf72 Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Fri, 27 Dec 2024 10:09:47 -0500 Subject: [PATCH 03/15] add api middleware for 403s --- frontend/src/lib/api.ts | 2 +- frontend/src/lib/logic/apiStatusLogic.ts | 24 ++++++++++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index f1497f937c334..27daf36b5fdf4 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -2677,7 +2677,7 @@ async function handleFetch(url: string, method: string, fetcher: () => Promise([ path(['lib', 'apiStatusLogic']), + connect({ + actions: [router, ['locationChanged']], + }), actions({ - onApiResponse: (response?: Response, error?: any) => ({ response, error }), + onApiResponse: (response?: Response, error?: any, extra?: { method: string }) => ({ response, error, extra }), setInternetConnectionIssue: (issue: boolean) => ({ issue }), setTimeSensitiveAuthenticationRequired: (required: boolean) => ({ required }), setResourceAccessDenied: (resource: string) => ({ resource }), @@ -37,7 +42,8 @@ export const apiStatusLogic = kea([ ], }), listeners(({ cache, actions, values }) => ({ - onApiResponse: async ({ response, error }, breakpoint) => { + onApiResponse: async ({ response, error, extra }, breakpoint) => { + const { method } = extra || {} if (error || !response?.status) { await breakpoint(50) // Likely CORS headers errors (i.e. request failing without reaching Django)) @@ -56,7 +62,12 @@ export const apiStatusLogic = kea([ if (data.detail === 'This action requires you to be recently authenticated.') { actions.setTimeSensitiveAuthenticationRequired(true) } else if (data.code === 'permission_denied') { - actions.setResourceAccessDenied(data.resource || 'resource') + // TODO - only do if the RBAC feature flag is enabled + if (method === 'GET') { + actions.setResourceAccessDenied(data.resource || 'resource') + } else { + toast.error('You are not authorized to perform this action') + } } } } catch (e) { @@ -84,4 +95,9 @@ export const apiStatusLogic = kea([ } }, })), + listeners(({ actions }) => ({ + locationChanged: () => { + actions.clearResourceAccessDenied() + }, + })), ]) From bfbfa2211e29d909a96935574ddc2762335b4d8b Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Fri, 27 Dec 2024 10:11:24 -0500 Subject: [PATCH 04/15] Create AccessControlAction.tsx --- .../lib/components/AccessControlAction.tsx | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 frontend/src/lib/components/AccessControlAction.tsx diff --git a/frontend/src/lib/components/AccessControlAction.tsx b/frontend/src/lib/components/AccessControlAction.tsx new file mode 100644 index 0000000000000..978e0c8955676 --- /dev/null +++ b/frontend/src/lib/components/AccessControlAction.tsx @@ -0,0 +1,28 @@ +import { WithAccessControl } from '../../types' + +interface AccessControlActionProps { + children: (props: { disabled: boolean; disabledReason: string | null }) => React.ReactElement + userAccessLevel?: WithAccessControl['user_access_level'] + requiredLevels: WithAccessControl['user_access_level'][] + resourceType?: string +} + +export const AccessControlAction = ({ + children, + userAccessLevel, + requiredLevels, + resourceType = 'resource', +}: AccessControlActionProps): JSX.Element => { + // Fallback to true if userAccessLevel is not set + const hasAccess = userAccessLevel ? requiredLevels.includes(userAccessLevel) : true + const disabledReason = !hasAccess + ? `You don't have sufficient permissions for this ${resourceType}. Your access level (${userAccessLevel}) doesn't meet the required level (${requiredLevels.join( + ' or ' + )}).` + : null + + return children({ + disabled: !hasAccess, + disabledReason, + }) +} From e0be46d7a09de494b4b4d5d033ce77491432f55d Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Fri, 27 Dec 2024 10:12:18 -0500 Subject: [PATCH 05/15] hookup access control action wrapper --- .../src/scenes/dashboard/DashboardHeader.tsx | 148 +++++++++------ .../src/scenes/feature-flags/FeatureFlag.tsx | 173 ++++++++++-------- .../src/scenes/feature-flags/FeatureFlags.tsx | 151 ++++++++------- .../src/scenes/insights/InsightPageHeader.tsx | 86 ++++++--- 4 files changed, 340 insertions(+), 218 deletions(-) diff --git a/frontend/src/scenes/dashboard/DashboardHeader.tsx b/frontend/src/scenes/dashboard/DashboardHeader.tsx index 403827fde6668..798eff3ffb2dd 100644 --- a/frontend/src/scenes/dashboard/DashboardHeader.tsx +++ b/frontend/src/scenes/dashboard/DashboardHeader.tsx @@ -1,5 +1,6 @@ import { useActions, useValues } from 'kea' import { router } from 'kea-router' +import { AccessControlAction } from 'lib/components/AccessControlAction' import { TextCardModal } from 'lib/components/Cards/TextCard/TextCardModal' import { EditableField } from 'lib/components/EditableField/EditableField' import { ExportButton, ExportButtonItem } from 'lib/components/ExportButton/ExportButton' @@ -178,17 +179,26 @@ export function DashboardHeader(): JSX.Element | null { )} {canEditDashboard && ( - - setDashboardMode( - DashboardMode.Edit, - DashboardEventSource.MoreDropdown - ) - } - fullWidth + - Edit layout (E) - + {({ disabledReason: accessControlDisabledReason }) => ( + + setDashboardMode( + DashboardMode.Edit, + DashboardEventSource.MoreDropdown + ) + } + fullWidth + disabledReason={accessControlDisabledReason} + > + Edit layout (E) + + )} + )} @@ -243,14 +253,23 @@ export function DashboardHeader(): JSX.Element | null { )} - { - showDuplicateDashboardModal(dashboard.id, dashboard.name) - }} - fullWidth + - Duplicate dashboard - + {({ disabledReason: accessControlDisabledReason }) => ( + { + showDuplicateDashboardModal(dashboard.id, dashboard.name) + }} + fullWidth + disabledReason={accessControlDisabledReason} + > + Duplicate dashboard + + )} + createNotebookFromDashboard(dashboard)} fullWidth @@ -258,15 +277,24 @@ export function DashboardHeader(): JSX.Element | null { Create notebook from dashboard {canEditDashboard && ( - { - showDeleteDashboardModal(dashboard.id) - }} - status="danger" - fullWidth + - Delete dashboard - + {({ disabledReason: accessControlDisabledReason }) => ( + { + showDeleteDashboardModal(dashboard.id) + }} + status="danger" + fullWidth + disabledReason={accessControlDisabledReason} + > + Delete dashboard + + )} + )} ) : undefined @@ -288,36 +316,48 @@ export function DashboardHeader(): JSX.Element | null { )} - {dashboard ? ( - - { - push(urls.dashboardTextTile(dashboard.id, 'new')) - }} - data-attr="add-text-tile-to-dashboard" - > - Add text card - - - ), - }, - disabled: false, - 'data-attr': 'dashboard-add-dropdown', - }} + {dashboard && ( + - Add insight - - ) : null} + {({ disabledReason: accessControlDisabledReason }) => ( + + { + push(urls.dashboardTextTile(dashboard.id, 'new')) + }} + data-attr="add-text-tile-to-dashboard" + disabledReason={accessControlDisabledReason} + > + Add text card + + + ), + }, + disabled: false, + 'data-attr': 'dashboard-add-dropdown', + }} + > + Add insight + + )} + + )} ) } diff --git a/frontend/src/scenes/feature-flags/FeatureFlag.tsx b/frontend/src/scenes/feature-flags/FeatureFlag.tsx index c9d7b50b6ab78..d5db478215e6c 100644 --- a/frontend/src/scenes/feature-flags/FeatureFlag.tsx +++ b/frontend/src/scenes/feature-flags/FeatureFlag.tsx @@ -5,6 +5,7 @@ import { LemonDialog, LemonSegmentedButton, LemonSkeleton, LemonSwitch } from '@ import { useActions, useValues } from 'kea' import { Form, Group } from 'kea-forms' import { router } from 'kea-router' +import { AccessControlAction } from 'lib/components/AccessControlAction' import { ActivityLog } from 'lib/components/ActivityLog/ActivityLog' import { CopyToClipboardInline } from 'lib/components/CopyToClipboard' import { NotFound } from 'lib/components/NotFound' @@ -508,31 +509,39 @@ export function FeatureFlag({ id }: { id?: string } = {}): JSX.Element { )} - { - featureFlag.deleted - ? restoreFeatureFlag(featureFlag) - : deleteFeatureFlag(featureFlag) - }} - disabledReason={ - !featureFlag.can_edit - ? "You have only 'View' access for this feature flag. To make changes, please contact the flag's creator." - : (featureFlag.features?.length || 0) > 0 - ? 'This feature flag is in use with an early access feature. Delete the early access feature to delete this flag' - : (featureFlag.experiment_set?.length || 0) > 0 - ? 'This feature flag is linked to an experiment. Delete the experiment to delete this flag' - : null - } + - {featureFlag.deleted ? 'Restore' : 'Delete'} feature flag - + {({ disabledReason: accessControlDisabledReason }) => ( + { + featureFlag.deleted + ? restoreFeatureFlag(featureFlag) + : deleteFeatureFlag(featureFlag) + }} + disabledReason={ + accessControlDisabledReason || + ((featureFlag.features?.length || 0) > 0 + ? 'This feature flag is in use with an early access feature. Delete the early access feature to delete this flag' + : (featureFlag.experiment_set?.length || 0) > 0 + ? 'This feature flag is linked to an experiment. Delete the experiment to delete this flag' + : null) + } + > + {featureFlag.deleted ? 'Restore' : 'Delete'} feature + flag + + )} + } /> @@ -544,22 +553,29 @@ export function FeatureFlag({ id }: { id?: string } = {}): JSX.Element { }} type="secondary" /> - { - editFeatureFlag(true) - }} + - Edit - + {({ disabledReason: accessControlDisabledReason }) => ( + { + editFeatureFlag(true) + }} + > + Edit + + )} + } @@ -754,39 +770,52 @@ function FeatureFlagRollout({ readOnly }: { readOnly?: boolean }): JSX.Element { ) : (
- { - LemonDialog.open({ - title: `${newValue === true ? 'Enable' : 'Disable'} this flag?`, - description: `This flag will be immediately ${ - newValue === true ? 'rolled out to' : 'rolled back from' - } the users matching the release conditions.`, - primaryButton: { - children: 'Confirm', - type: 'primary', - onClick: () => { - const updatedFlag = { ...featureFlag, active: newValue } - setFeatureFlag(updatedFlag) - saveFeatureFlag(updatedFlag) - }, - size: 'small', - }, - secondaryButton: { - children: 'Cancel', - type: 'tertiary', - size: 'small', - }, - }) - }} - label="Enabled" - disabledReason={ - !featureFlag.can_edit - ? "You only have view access to this feature flag. To make changes, contact the flag's creator." - : null - } - checked={featureFlag.active} - /> - + + {({ disabledReason: accessControlDisabledReason }) => ( + <> + { + LemonDialog.open({ + title: `${ + newValue === true ? 'Enable' : 'Disable' + } this flag?`, + description: `This flag will be immediately ${ + newValue === true + ? 'rolled out to' + : 'rolled back from' + } the users matching the release conditions.`, + primaryButton: { + children: 'Confirm', + type: 'primary', + onClick: () => { + const updatedFlag = { + ...featureFlag, + active: newValue, + } + setFeatureFlag(updatedFlag) + saveFeatureFlag(updatedFlag) + }, + size: 'small', + }, + secondaryButton: { + children: 'Cancel', + type: 'tertiary', + size: 'small', + }, + }) + }} + label="Enabled" + disabledReason={accessControlDisabledReason} + checked={featureFlag.active} + /> + + + )} +
)} diff --git a/frontend/src/scenes/feature-flags/FeatureFlags.tsx b/frontend/src/scenes/feature-flags/FeatureFlags.tsx index a0cc42afd600f..7998ace0133c6 100644 --- a/frontend/src/scenes/feature-flags/FeatureFlags.tsx +++ b/frontend/src/scenes/feature-flags/FeatureFlags.tsx @@ -2,6 +2,7 @@ import { IconLock } from '@posthog/icons' import { LemonDialog, LemonInput, LemonSelect, LemonTag } from '@posthog/lemon-ui' import { useActions, useValues } from 'kea' import { router } from 'kea-router' +import { AccessControlAction } from 'lib/components/AccessControlAction' import { ActivityLog } from 'lib/components/ActivityLog/ActivityLog' import { FeatureFlagHog } from 'lib/components/hedgehogs' import { MemberSelect } from 'lib/components/MemberSelect' @@ -185,79 +186,103 @@ export function OverViewTab({ > Copy feature flag key - { - const newValue = !featureFlag.active - LemonDialog.open({ - title: `${newValue === true ? 'Enable' : 'Disable'} this flag?`, - description: `This flag will be immediately ${ - newValue === true ? 'rolled out to' : 'rolled back from' - } the users matching the release conditions.`, - primaryButton: { - children: 'Confirm', - type: 'primary', - onClick: () => { - featureFlag.id - ? updateFeatureFlag({ - id: featureFlag.id, - payload: { active: newValue }, - }) - : null - }, - size: 'small', - }, - secondaryButton: { - children: 'Cancel', - type: 'tertiary', - size: 'small', - }, - }) - }} - id={`feature-flag-${featureFlag.id}-switch`} - disabled={!featureFlag.can_edit} - fullWidth + - {featureFlag.active ? 'Disable' : 'Enable'} feature flag - + {({ disabledReason: accessControlDisabledReason }) => ( + { + const newValue = !featureFlag.active + LemonDialog.open({ + title: `${newValue === true ? 'Enable' : 'Disable'} this flag?`, + description: `This flag will be immediately ${ + newValue === true ? 'rolled out to' : 'rolled back from' + } the users matching the release conditions.`, + primaryButton: { + children: 'Confirm', + type: 'primary', + onClick: () => { + featureFlag.id + ? updateFeatureFlag({ + id: featureFlag.id, + payload: { active: newValue }, + }) + : null + }, + size: 'small', + }, + secondaryButton: { + children: 'Cancel', + type: 'tertiary', + size: 'small', + }, + }) + }} + id={`feature-flag-${featureFlag.id}-switch`} + disabledReason={accessControlDisabledReason} + fullWidth + > + {featureFlag.active ? 'Disable' : 'Enable'} feature flag + + )} + {featureFlag.id && ( - - featureFlag.id && router.actions.push(urls.featureFlag(featureFlag.id)) - } + - Edit - + {({ disabledReason: accessControlDisabledReason }) => ( + + featureFlag.id && + router.actions.push(urls.featureFlag(featureFlag.id)) + } + > + Edit + + )} + )} Try out in Insights {featureFlag.id && ( - { - void deleteWithUndo({ - endpoint: `projects/${currentProjectId}/feature_flags`, - object: { name: featureFlag.key, id: featureFlag.id }, - callback: loadFeatureFlags, - }) - }} - disabledReason={ - !featureFlag.can_edit - ? "You have only 'View' access for this feature flag. To make changes, please contact the flag's creator." - : (featureFlag.features?.length || 0) > 0 - ? 'This feature flag is in use with an early access feature. Delete the early access feature to delete this flag' - : (featureFlag.experiment_set?.length || 0) > 0 - ? 'This feature flag is linked to an experiment. Delete the experiment to delete this flag' - : null - } - fullWidth + - Delete feature flag - + {({ disabledReason: accessControlDisabledReason }) => ( + { + void deleteWithUndo({ + endpoint: `projects/${currentProjectId}/feature_flags`, + object: { name: featureFlag.key, id: featureFlag.id }, + callback: loadFeatureFlags, + }) + }} + disabledReason={ + accessControlDisabledReason || + ((featureFlag.features?.length || 0) > 0 + ? 'This feature flag is in use with an early access feature. Delete the early access feature to delete this flag' + : (featureFlag.experiment_set?.length || 0) > 0 + ? 'This feature flag is linked to an experiment. Delete the experiment to delete this flag' + : null) + } + fullWidth + > + Delete feature flag + + )} + )} } diff --git a/frontend/src/scenes/insights/InsightPageHeader.tsx b/frontend/src/scenes/insights/InsightPageHeader.tsx index 9ff53430145ad..16fc079492b3e 100644 --- a/frontend/src/scenes/insights/InsightPageHeader.tsx +++ b/frontend/src/scenes/insights/InsightPageHeader.tsx @@ -1,5 +1,6 @@ import { useActions, useMountedLogic, useValues } from 'kea' import { router } from 'kea-router' +import { AccessControlAction } from 'lib/components/AccessControlAction' import { AddToDashboard } from 'lib/components/AddToDashboard/AddToDashboard' import { AddToDashboardModal } from 'lib/components/AddToDashboard/AddToDashboardModal' import { AlertsButton } from 'lib/components/Alerts/AlertsButton' @@ -149,15 +150,24 @@ export function InsightPageHeader({ insightLogicProps }: { insightLogicProps: In <> {hasDashboardItemId && ( <> - - duplicateInsight(insight as QueryBasedInsightModel, true) - } - fullWidth - data-attr="duplicate-insight-from-insight-view" + - Duplicate - + {({ disabledReason: accessControlDisabledReason }) => ( + + duplicateInsight(insight as QueryBasedInsightModel, true) + } + fullWidth + data-attr="duplicate-insight-from-insight-view" + disabledReason={accessControlDisabledReason} + > + Duplicate + + )} + setInsightMetadata({ @@ -311,22 +321,31 @@ export function InsightPageHeader({ insightLogicProps }: { insightLogicProps: In {hasDashboardItemId && ( <> - - void deleteInsightWithUndo({ - object: insight as QueryBasedInsightModel, - endpoint: `projects/${currentProjectId}/insights`, - callback: () => { - loadInsights() - push(urls.savedInsights()) - }, - }) - } - fullWidth + - Delete insight - + {({ disabledReason: accessControlDisabledReason }) => ( + + void deleteInsightWithUndo({ + object: insight as QueryBasedInsightModel, + endpoint: `projects/${currentProjectId}/insights`, + callback: () => { + loadInsights() + push(urls.savedInsights()) + }, + }) + } + fullWidth + disabledReason={accessControlDisabledReason} + > + Delete insight + + )} + )} @@ -369,13 +388,22 @@ export function InsightPageHeader({ insightLogicProps }: { insightLogicProps: In {insightMode !== ItemMode.Edit ? ( canEditInsight && ( - setInsightMode(ItemMode.Edit, null)} - data-attr="insight-edit-button" + - Edit - + {({ disabledReason: accessControlDisabledReason }) => ( + setInsightMode(ItemMode.Edit, null)} + data-attr="insight-edit-button" + disabledReason={accessControlDisabledReason} + > + Edit + + )} + ) ) : ( Date: Fri, 27 Dec 2024 10:15:23 -0500 Subject: [PATCH 06/15] Add insight read permissions --- posthog/api/insight.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/posthog/api/insight.py b/posthog/api/insight.py index 22b53f07e73af..ba99e59d77a0f 100644 --- a/posthog/api/insight.py +++ b/posthog/api/insight.py @@ -1160,7 +1160,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, From d7eff084f65e0c65c57671030f9f683c6eac7b9b Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Fri, 27 Dec 2024 11:45:45 -0500 Subject: [PATCH 07/15] add user access control to more serializers --- posthog/api/feature_flag.py | 3 ++- posthog/api/insight.py | 5 +++-- posthog/api/notebook.py | 5 +++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/posthog/api/feature_flag.py b/posthog/api/feature_flag.py index f20c1a4a6105a..92764327eccaa 100644 --- a/posthog/api/feature_flag.py +++ b/posthog/api/feature_flag.py @@ -446,7 +446,7 @@ def _create_usage_dashboard(feature_flag: FeatureFlag, user): return usage_dashboard -class MinimalFeatureFlagSerializer(serializers.ModelSerializer): +class MinimalFeatureFlagSerializer(serializers.ModelSerializer, UserAccessControlSerializerMixin): filters = serializers.DictField(source="get_filters", required=False) class Meta: @@ -460,6 +460,7 @@ class Meta: "deleted", "active", "ensure_experience_continuity", + "user_access_level", ] diff --git a/posthog/api/insight.py b/posthog/api/insight.py index ba99e59d77a0f..bfe8208bf8ce9 100644 --- a/posthog/api/insight.py +++ b/posthog/api/insight.py @@ -196,7 +196,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. """ @@ -225,6 +225,7 @@ class Meta: "created_at", "last_modified_at", "favorited", + "user_access_level", ] read_only_fields = ("short_id", "updated_at", "last_refresh", "refreshing") @@ -252,7 +253,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() diff --git a/posthog/api/notebook.py b/posthog/api/notebook.py index f68d2ddb0b26a..09eb8b1c769d1 100644 --- a/posthog/api/notebook.py +++ b/posthog/api/notebook.py @@ -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) @@ -93,11 +93,12 @@ class Meta: "created_by", "last_modified_at", "last_modified_by", + "user_access_level", ] read_only_fields = fields -class NotebookSerializer(NotebookMinimalSerializer, UserAccessControlSerializerMixin): +class NotebookSerializer(NotebookMinimalSerializer): class Meta: model = Notebook fields = [ From 70cea73d69376cf5dea90b9f51b234c32367cc0f Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Fri, 27 Dec 2024 11:46:22 -0500 Subject: [PATCH 08/15] more access control wrappers --- .../dashboard/dashboards/DashboardsTable.tsx | 84 +++++++++++------ .../src/scenes/insights/insightSceneLogic.tsx | 8 +- .../scenes/saved-insights/SavedInsights.tsx | 93 +++++++++++++------ 3 files changed, 125 insertions(+), 60 deletions(-) diff --git a/frontend/src/scenes/dashboard/dashboards/DashboardsTable.tsx b/frontend/src/scenes/dashboard/dashboards/DashboardsTable.tsx index 9ff1afc89a79f..dbea9c78c9c2c 100644 --- a/frontend/src/scenes/dashboard/dashboards/DashboardsTable.tsx +++ b/frontend/src/scenes/dashboard/dashboards/DashboardsTable.tsx @@ -1,6 +1,7 @@ import { IconHome, IconLock, IconPin, IconPinFilled, IconShare } from '@posthog/icons' import { LemonInput } from '@posthog/lemon-ui' import { useActions, useValues } from 'kea' +import { AccessControlAction } from 'lib/components/AccessControlAction' import { MemberSelect } from 'lib/components/MemberSelect' import { ObjectTags } from 'lib/components/ObjectTags/ObjectTags' import { DashboardPrivilegeLevel } from 'lib/constants' @@ -131,7 +132,7 @@ export function DashboardsTable({ ? {} : { width: 0, - render: function RenderActions(_, { id, name }: DashboardType) { + render: function RenderActions(_, { id, name, user_access_level }: DashboardType) { return ( View - { - dashboardLogic({ id }).mount() - dashboardLogic({ id }).actions.setDashboardMode( - DashboardMode.Edit, - DashboardEventSource.DashboardsList - ) - }} - fullWidth + - Edit - - { - showDuplicateDashboardModal(id, name) - }} - fullWidth + {({ disabledReason: accessControlDisabledReason }) => ( + { + dashboardLogic({ id }).mount() + dashboardLogic({ id }).actions.setDashboardMode( + DashboardMode.Edit, + DashboardEventSource.DashboardsList + ) + }} + fullWidth + disabledReason={accessControlDisabledReason} + > + Edit + + )} + + - Duplicate - + {({ disabledReason: accessControlDisabledReason }) => ( + { + showDuplicateDashboardModal(id, name) + }} + fullWidth + disabledReason={accessControlDisabledReason} + > + Duplicate + + )} + + + } fullWidth status="warning"> Change the default dashboard @@ -180,15 +199,24 @@ export function DashboardsTable({ - { - showDeleteDashboardModal(id) - }} - fullWidth - status="danger" + + - Delete dashboard - + {({ disabledReason: accessControlDisabledReason }) => ( + { + showDeleteDashboardModal(id) + }} + fullWidth + status="danger" + disabledReason={accessControlDisabledReason} + > + Delete dashboard + + )} + } /> diff --git a/frontend/src/scenes/insights/insightSceneLogic.tsx b/frontend/src/scenes/insights/insightSceneLogic.tsx index d9b89d13ff25c..199d3bb0acee8 100644 --- a/frontend/src/scenes/insights/insightSceneLogic.tsx +++ b/frontend/src/scenes/insights/insightSceneLogic.tsx @@ -209,9 +209,11 @@ export const insightSceneLogic = kea([ cohortsById, mathDefinitions, }), - onRename: async (name: string) => { - await insightLogicRef?.logic.asyncActions.setInsightMetadata({ name }) - }, + onRename: insightLogicRef?.logic.values.canEditInsight + ? async (name: string) => { + await insightLogicRef?.logic.asyncActions.setInsightMetadata({ name }) + } + : undefined, }, ] }, diff --git a/frontend/src/scenes/saved-insights/SavedInsights.tsx b/frontend/src/scenes/saved-insights/SavedInsights.tsx index bd155048b0490..48a287d231e20 100644 --- a/frontend/src/scenes/saved-insights/SavedInsights.tsx +++ b/frontend/src/scenes/saved-insights/SavedInsights.tsx @@ -23,6 +23,7 @@ import { } from '@posthog/icons' import { LemonSelectOptions, LemonTag } from '@posthog/lemon-ui' import { useActions, useValues } from 'kea' +import { AccessControlAction } from 'lib/components/AccessControlAction' import { ActivityLog } from 'lib/components/ActivityLog/ActivityLog' import { Alerts } from 'lib/components/Alerts/views/Alerts' import { InsightCard } from 'lib/components/Cards/InsightCard' @@ -566,38 +567,72 @@ export function SavedInsights(): JSX.Element { View - - Edit - - renameInsight(insight)} - data-attr={`insight-item-${insight.short_id}-dropdown-rename`} - fullWidth + - Rename - - duplicateInsight(insight)} - data-attr="duplicate-insight-from-list-view" - fullWidth + {({ disabledReason: accessControlDisabledReason }) => ( + + Edit + + )} + + - Duplicate - - - - void deleteInsightWithUndo({ - object: insight, - endpoint: `projects/${currentProjectId}/insights`, - callback: loadInsights, - }) - } - data-attr={`insight-item-${insight.short_id}-dropdown-remove`} - fullWidth + {({ disabledReason: accessControlDisabledReason }) => ( + renameInsight(insight)} + data-attr={`insight-item-${insight.short_id}-dropdown-rename`} + fullWidth + disabledReason={accessControlDisabledReason} + > + Rename + + )} + + - Delete insight - + {({ disabledReason: accessControlDisabledReason }) => ( + duplicateInsight(insight)} + data-attr="duplicate-insight-from-list-view" + fullWidth + disabledReason={accessControlDisabledReason} + > + Duplicate + + )} + + + {({ disabledReason: accessControlDisabledReason }) => ( + + void deleteInsightWithUndo({ + object: insight, + endpoint: `projects/${currentProjectId}/insights`, + callback: loadInsights, + }) + } + data-attr={`insight-item-${insight.short_id}-dropdown-remove`} + fullWidth + disabledReason={accessControlDisabledReason} + > + Delete insight + + )} + } /> From f7f70702068d7764cce4e5ac21e02cd7772d0b85 Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Fri, 27 Dec 2024 12:02:28 -0500 Subject: [PATCH 09/15] More access limitations --- .../access_control/accessControlLogic.ts | 16 ++++++++ .../lib/components/AccessControlAction.tsx | 9 ++++- .../src/scenes/dashboard/dashboardLogic.tsx | 37 ++++++++++++------- frontend/src/scenes/insights/insightLogic.tsx | 21 ++++++++--- 4 files changed, 62 insertions(+), 21 deletions(-) diff --git a/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/accessControlLogic.ts b/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/accessControlLogic.ts index 8182b41c2b602..f3b59cf5e07a3 100644 --- a/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/accessControlLogic.ts +++ b/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/accessControlLogic.ts @@ -17,6 +17,7 @@ import { APIScopeObject, OrganizationMemberType, RoleType, + WithAccessControl, } from '~/types' import type { accessControlLogicType } from './accessControlLogicType' @@ -242,6 +243,21 @@ export const accessControlLogic = kea([ : [] }, ], + + hasResourceAccess: [ + () => [], + () => + ({ + userAccessLevel, + requiredLevels, + }: { + userAccessLevel?: WithAccessControl['user_access_level'] + requiredLevels: WithAccessControl['user_access_level'][] + }) => { + // Fallback to true if userAccessLevel is not set + return userAccessLevel ? requiredLevels.includes(userAccessLevel) : true + }, + ], }), afterMount(({ actions }) => { actions.loadAccessControls() diff --git a/frontend/src/lib/components/AccessControlAction.tsx b/frontend/src/lib/components/AccessControlAction.tsx index 978e0c8955676..625abce067460 100644 --- a/frontend/src/lib/components/AccessControlAction.tsx +++ b/frontend/src/lib/components/AccessControlAction.tsx @@ -1,3 +1,7 @@ +import { useValues } from 'kea' + +import { accessControlLogic } from '~/layout/navigation-3000/sidepanel/panels/access_control/accessControlLogic' + import { WithAccessControl } from '../../types' interface AccessControlActionProps { @@ -13,8 +17,9 @@ export const AccessControlAction = ({ requiredLevels, resourceType = 'resource', }: AccessControlActionProps): JSX.Element => { - // Fallback to true if userAccessLevel is not set - const hasAccess = userAccessLevel ? requiredLevels.includes(userAccessLevel) : true + const { hasResourceAccess } = useValues(accessControlLogic) + + const hasAccess = hasResourceAccess({ userAccessLevel, requiredLevels }) const disabledReason = !hasAccess ? `You don't have sufficient permissions for this ${resourceType}. Your access level (${userAccessLevel}) doesn't meet the required level (${requiredLevels.join( ' or ' diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index 0b6236931cf2b..0e3ac459eae2f 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -15,7 +15,7 @@ import { import { loaders } from 'kea-loaders' import { router, urlToAction } from 'kea-router' import api, { ApiMethodOptions, getJSONOrNull } from 'lib/api' -import { DashboardPrivilegeLevel, OrganizationMembershipLevel } from 'lib/constants' +import { DashboardPrivilegeLevel, FEATURE_FLAGS, OrganizationMembershipLevel } from 'lib/constants' import { Dayjs, dayjs, now } from 'lib/dayjs' import { currentSessionId, TimeToSeeDataPayload } from 'lib/internalMetrics' import { lemonToast } from 'lib/lemon-ui/LemonToast/LemonToast' @@ -919,8 +919,15 @@ export const dashboardLogic = kea([ }, ], canEditDashboard: [ - (s) => [s.dashboard], - (dashboard) => !!dashboard && dashboard.effective_privilege_level >= DashboardPrivilegeLevel.CanEdit, + (s) => [s.dashboard, s.featureFlags], + (dashboard, featureFlags) => { + if (featureFlags[FEATURE_FLAGS.ROLE_BASED_ACCESS_CONTROL]) { + return true + // TODO: figure out how to access this method + // return hasResourceAccess({ userAccessLevel: dashboard.user_access_level, requiredLevels: ['admin', 'editor'] }) + } + return !!dashboard && dashboard.effective_privilege_level >= DashboardPrivilegeLevel.CanEdit + }, ], canRestrictDashboard: [ // Sync conditions with backend can_user_restrict @@ -965,8 +972,8 @@ export const dashboardLogic = kea([ }, ], breadcrumbs: [ - (s) => [s.dashboard, s._dashboardLoading, s.dashboardFailedToLoad], - (dashboard, dashboardLoading, dashboardFailedToLoad): Breadcrumb[] => [ + (s) => [s.dashboard, s._dashboardLoading, s.dashboardFailedToLoad, s.canEditDashboard], + (dashboard, dashboardLoading, dashboardFailedToLoad, canEditDashboard): Breadcrumb[] => [ { key: Scene.Dashboards, name: 'Dashboards', @@ -981,15 +988,17 @@ export const dashboardLogic = kea([ : !dashboardLoading ? 'Not found' : null, - onRename: async (name) => { - if (dashboard) { - await dashboardsModel.asyncActions.updateDashboard({ - id: dashboard.id, - name, - allowUndo: true, - }) - } - }, + onRename: canEditDashboard + ? async (name) => { + if (dashboard) { + await dashboardsModel.asyncActions.updateDashboard({ + id: dashboard.id, + name, + allowUndo: true, + }) + } + } + : undefined, }, ], ], diff --git a/frontend/src/scenes/insights/insightLogic.tsx b/frontend/src/scenes/insights/insightLogic.tsx index 1ca1548f30047..be196aac42dd3 100644 --- a/frontend/src/scenes/insights/insightLogic.tsx +++ b/frontend/src/scenes/insights/insightLogic.tsx @@ -2,9 +2,10 @@ import { LemonDialog, LemonInput } from '@posthog/lemon-ui' import { actions, connect, events, kea, key, listeners, LogicWrapper, path, props, reducers, selectors } from 'kea' import { loaders } from 'kea-loaders' import { router } from 'kea-router' -import { DashboardPrivilegeLevel } from 'lib/constants' +import { DashboardPrivilegeLevel, FEATURE_FLAGS } from 'lib/constants' import { LemonField } from 'lib/lemon-ui/LemonField' import { lemonToast } from 'lib/lemon-ui/LemonToast/LemonToast' +import { featureFlagLogic } from 'lib/logic/featureFlagLogic' import { objectsEqual } from 'lib/utils' import { eventUsageLogic, InsightEventSource } from 'lib/utils/eventUsageLogic' import { dashboardLogic } from 'scenes/dashboard/dashboardLogic' @@ -58,6 +59,8 @@ export const insightLogic: LogicWrapper = kea = kea [s.insight, s.derivedName], (insight, derivedName) => insight.name || derivedName], insightId: [(s) => [s.insight], (insight) => insight?.id || null], canEditInsight: [ - (s) => [s.insight], - (insight) => - insight.effective_privilege_level == undefined || - insight.effective_privilege_level >= DashboardPrivilegeLevel.CanEdit, + (s) => [s.insight, s.featureFlags], + (insight, featureFlags) => { + if (featureFlags[FEATURE_FLAGS.ROLE_BASED_ACCESS_CONTROL]) { + return true + // TODO: figure out how to access this method + // return hasResourceAccess({ userAccessLevel: insight.user_access_level, requiredLevels: ['admin', 'editor'] }) + } + return ( + insight.effective_privilege_level == undefined || + insight.effective_privilege_level >= DashboardPrivilegeLevel.CanEdit + ) + }, ], insightChanged: [ (s) => [s.insight, s.savedInsight], From a80e80768d5ec7c433711105a0aa3b50d6aa4516 Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Mon, 6 Jan 2025 17:11:25 -0500 Subject: [PATCH 10/15] Update AccessControlObject.tsx --- .../sidepanel/panels/access_control/AccessControlObject.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/AccessControlObject.tsx b/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/AccessControlObject.tsx index dd8f202933a2b..a5a00f0b50908 100644 --- a/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/AccessControlObject.tsx +++ b/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/AccessControlObject.tsx @@ -39,7 +39,7 @@ export function AccessControlObject(props: AccessControlLogicProps): JSX.Element return (
- {canEditAccessControls === true ? ( + {canEditAccessControls === false ? ( Permission required
From 07bc3d586c4a2431a1e708dd28ad1a49e3bf0dba Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Mon, 6 Jan 2025 17:11:37 -0500 Subject: [PATCH 11/15] Update user_access_control.py --- posthog/rbac/user_access_control.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/posthog/rbac/user_access_control.py b/posthog/rbac/user_access_control.py index f3b5f5b2b0d9c..bc397a20dc553 100644 --- a/posthog/rbac/user_access_control.py +++ b/posthog/rbac/user_access_control.py @@ -474,7 +474,7 @@ def user_access_control(self) -> Optional[UserAccessControl]: elif hasattr(self.context.get("view", None), "user_access_control"): # Otherwise from the view (the default case) return self.context["view"].user_access_control - else: + elif "request" in self.context: user = cast(User | AnonymousUser, self.context["request"].user) # The user could be anonymous - if so there is no access control to be used @@ -484,6 +484,7 @@ def user_access_control(self) -> Optional[UserAccessControl]: user = cast(User, user) return UserAccessControl(user, organization_id=str(user.current_organization_id)) + return None def get_user_access_level(self, obj: Model) -> Optional[str]: if not self.user_access_control: From 36ef1748a51009b3b5f3d13116699e1b379cdd42 Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Mon, 6 Jan 2025 18:36:28 -0500 Subject: [PATCH 12/15] Pull out access control queryset checks so they can be reused in insights and dashboards --- posthog/api/dashboards/dashboard.py | 3 +++ posthog/api/insight.py | 3 +++ posthog/api/routing.py | 34 +++++++++++++++-------------- posthog/rbac/user_access_control.py | 1 - 4 files changed, 24 insertions(+), 17 deletions(-) diff --git a/posthog/api/dashboards/dashboard.py b/posthog/api/dashboards/dashboard.py index 393df01a2d554..8ce4056893d1e 100644 --- a/posthog/api/dashboards/dashboard.py +++ b/posthog/api/dashboards/dashboard.py @@ -516,6 +516,9 @@ 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") diff --git a/posthog/api/insight.py b/posthog/api/insight.py index d979f614e8b8f..30c13d847ebc3 100644 --- a/posthog/api/insight.py +++ b/posthog/api/insight.py @@ -806,6 +806,9 @@ def dangerously_get_queryset(self): queryset = queryset.prefetch_related("tagged_items__tag") queryset = self._filter_request(self.request, queryset) + # Add access level filtering for list actions + queryset = self._filter_queryset_by_access_level(queryset) + order = self.request.GET.get("order", None) if order: queryset = queryset.order_by(order) diff --git a/posthog/api/routing.py b/posthog/api/routing.py index 7c59d73de662a..e538c8322bf60 100644 --- a/posthog/api/routing.py +++ b/posthog/api/routing.py @@ -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. diff --git a/posthog/rbac/user_access_control.py b/posthog/rbac/user_access_control.py index bc397a20dc553..abc0ac609dfca 100644 --- a/posthog/rbac/user_access_control.py +++ b/posthog/rbac/user_access_control.py @@ -411,7 +411,6 @@ def check_access_level_for_resource(self, resource: APIScopeObject, required_lev def filter_queryset_by_access_level(self, queryset: QuerySet, include_all_if_admin=False) -> 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 - model = cast(Model, queryset.model) resource = model_to_resource(model) From 5a4ccb04206e3902e97e9fb38e73c850b90b48af Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Mon, 6 Jan 2025 18:52:28 -0500 Subject: [PATCH 13/15] Update can edit logic for insights and dashboards --- frontend/src/scenes/dashboard/dashboardLogic.tsx | 5 ++--- frontend/src/scenes/insights/insightLogic.tsx | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/frontend/src/scenes/dashboard/dashboardLogic.tsx b/frontend/src/scenes/dashboard/dashboardLogic.tsx index 0e3ac459eae2f..18c759660a698 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.tsx +++ b/frontend/src/scenes/dashboard/dashboardLogic.tsx @@ -922,9 +922,8 @@ export const dashboardLogic = kea([ (s) => [s.dashboard, s.featureFlags], (dashboard, featureFlags) => { if (featureFlags[FEATURE_FLAGS.ROLE_BASED_ACCESS_CONTROL]) { - return true - // TODO: figure out how to access this method - // return hasResourceAccess({ userAccessLevel: dashboard.user_access_level, requiredLevels: ['admin', 'editor'] }) + const requiredLevels = ['admin', 'editor'] + return dashboard?.user_access_level ? requiredLevels.includes(dashboard.user_access_level) : true } return !!dashboard && dashboard.effective_privilege_level >= DashboardPrivilegeLevel.CanEdit }, diff --git a/frontend/src/scenes/insights/insightLogic.tsx b/frontend/src/scenes/insights/insightLogic.tsx index be196aac42dd3..a1c3dccb34321 100644 --- a/frontend/src/scenes/insights/insightLogic.tsx +++ b/frontend/src/scenes/insights/insightLogic.tsx @@ -299,9 +299,8 @@ export const insightLogic: LogicWrapper = kea [s.insight, s.featureFlags], (insight, featureFlags) => { if (featureFlags[FEATURE_FLAGS.ROLE_BASED_ACCESS_CONTROL]) { - return true - // TODO: figure out how to access this method - // return hasResourceAccess({ userAccessLevel: insight.user_access_level, requiredLevels: ['admin', 'editor'] }) + const requiredLevels = ['admin', 'editor'] + return insight.user_access_level ? requiredLevels.includes(insight.user_access_level) : true } return ( insight.effective_privilege_level == undefined || From 540e7a3822f0e229d19b883860199ea630a98eff Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Mon, 6 Jan 2025 19:10:26 -0500 Subject: [PATCH 14/15] Moving sharing cta to access control to be from one component --- .../access_control/AccessControlPopoutCTA.tsx | 29 +++++++++++++++++++ .../lib/components/Sharing/SharingModal.tsx | 19 ++++++++++++ .../src/scenes/FeatureFlagPermissions.tsx | 27 ++++------------- .../dashboard/DashboardCollaborators.tsx | 29 +++++-------------- .../notebooks/Notebook/NotebookShareModal.tsx | 28 +++++------------- 5 files changed, 68 insertions(+), 64 deletions(-) create mode 100644 frontend/src/layout/navigation-3000/sidepanel/panels/access_control/AccessControlPopoutCTA.tsx diff --git a/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/AccessControlPopoutCTA.tsx b/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/AccessControlPopoutCTA.tsx new file mode 100644 index 0000000000000..f24477f74287d --- /dev/null +++ b/frontend/src/layout/navigation-3000/sidepanel/panels/access_control/AccessControlPopoutCTA.tsx @@ -0,0 +1,29 @@ +import { IconOpenSidebar } from '@posthog/icons' +import { LemonBanner, LemonButton } from '@posthog/lemon-ui' +import { useActions } from 'kea' + +import { sidePanelStateLogic } from '~/layout/navigation-3000/sidepanel/sidePanelStateLogic' +import { SidePanelTab } from '~/types' + +export const AccessControlPopoutCTA = ({ callback }: { callback?: () => void }): JSX.Element => { + const { openSidePanel } = useActions(sidePanelStateLogic) + + return ( +
+

Access control

+ + Permissions are moving. We're rolling out our new access control system. Click below to open it. + + } + onClick={() => { + openSidePanel(SidePanelTab.AccessControl) + callback?.() + }} + > + Open access control + +
+ ) +} diff --git a/frontend/src/lib/components/Sharing/SharingModal.tsx b/frontend/src/lib/components/Sharing/SharingModal.tsx index 7ef76f6fc54d6..2dee89e5708fc 100644 --- a/frontend/src/lib/components/Sharing/SharingModal.tsx +++ b/frontend/src/lib/components/Sharing/SharingModal.tsx @@ -5,8 +5,10 @@ import { LemonButton, LemonDivider, LemonModal, LemonSkeleton, LemonSwitch } fro import { captureException } from '@sentry/react' import { useActions, useValues } from 'kea' import { Form } from 'kea-forms' +import { router } from 'kea-router' import { CodeSnippet, Language } from 'lib/components/CodeSnippet' import { TitleWithIcon } from 'lib/components/TitleWithIcon' +import { useFeatureFlag } from 'lib/hooks/useFeatureFlag' import { IconLink } from 'lib/lemon-ui/icons' import { LemonDialog } from 'lib/lemon-ui/LemonDialog' import { LemonField } from 'lib/lemon-ui/LemonField' @@ -16,7 +18,9 @@ import { copyToClipboard } from 'lib/utils/copyToClipboard' import { useEffect, useState } from 'react' import { DashboardCollaboration } from 'scenes/dashboard/DashboardCollaborators' import { insightVizDataLogic } from 'scenes/insights/insightVizDataLogic' +import { urls } from 'scenes/urls' +import { AccessControlPopoutCTA } from '~/layout/navigation-3000/sidepanel/panels/access_control/AccessControlPopoutCTA' import { isInsightVizNode } from '~/queries/utils' import { AvailableFeature, InsightShortId, QueryBasedInsightModel } from '~/types' @@ -68,6 +72,10 @@ export function SharingModalContent({ const { setIsEnabled, togglePreview } = useActions(sharingLogic(logicProps)) const { guardAvailableFeature } = useValues(upgradeModalLogic) + const { push } = useActions(router) + + const newAccessControl = useFeatureFlag('ROLE_BASED_ACCESS_CONTROL') + const [iframeLoaded, setIframeLoaded] = useState(false) const resource = dashboardId ? 'dashboard' : insightShortId ? 'insight' : recordingId ? 'recording' : 'this' @@ -85,6 +93,17 @@ export function SharingModalContent({ ) : undefined} + {insightShortId && newAccessControl ? ( + <> + { + push(urls.insightView(insightShortId)) + }} + /> + + + ) : undefined} +
{!sharingConfiguration && sharingConfigurationLoading ? ( diff --git a/frontend/src/scenes/FeatureFlagPermissions.tsx b/frontend/src/scenes/FeatureFlagPermissions.tsx index 24d4ebbe458d8..5b783e9b0daa4 100644 --- a/frontend/src/scenes/FeatureFlagPermissions.tsx +++ b/frontend/src/scenes/FeatureFlagPermissions.tsx @@ -1,5 +1,5 @@ -import { IconGear, IconOpenSidebar, IconTrash } from '@posthog/icons' -import { LemonBanner, LemonButton, LemonTable } from '@posthog/lemon-ui' +import { IconGear, IconTrash } from '@posthog/icons' +import { LemonButton, LemonTable } from '@posthog/lemon-ui' import { useActions, useValues } from 'kea' import { PayGateMini } from 'lib/components/PayGateMini/PayGateMini' import { TitleWithIcon } from 'lib/components/TitleWithIcon' @@ -7,8 +7,8 @@ import { useFeatureFlag } from 'lib/hooks/useFeatureFlag' import { LemonInputSelect, LemonInputSelectOption } from 'lib/lemon-ui/LemonInputSelect/LemonInputSelect' import { LemonTableColumns } from 'lib/lemon-ui/LemonTable' -import { sidePanelStateLogic } from '~/layout/navigation-3000/sidepanel/sidePanelStateLogic' -import { AccessLevel, AvailableFeature, FeatureFlagType, Resource, RoleType, SidePanelTab } from '~/types' +import { AccessControlPopoutCTA } from '~/layout/navigation-3000/sidepanel/panels/access_control/AccessControlPopoutCTA' +import { AccessLevel, AvailableFeature, FeatureFlagType, Resource, RoleType } from '~/types' import { featureFlagPermissionsLogic } from './feature-flags/featureFlagPermissionsLogic' import { permissionsLogic } from './settings/organization/Permissions/permissionsLogic' @@ -46,29 +46,12 @@ export function FeatureFlagPermissions({ featureFlag }: { featureFlag: FeatureFl const { setRolesToAdd, addAssociatedRoles, deleteAssociatedRole } = useActions( featureFlagPermissionsLogic({ flagId: featureFlag.id }) ) - const { openSidePanel } = useActions(sidePanelStateLogic) - const newAccessControls = useFeatureFlag('ROLE_BASED_ACCESS_CONTROL') if (newAccessControls) { if (!featureFlag.id) { return

Please save the feature flag before changing the access controls.

} - return ( -
- - Permissions have moved! We're rolling out our new access control system. Click below to open it. - - } - onClick={() => { - openSidePanel(SidePanelTab.AccessControl) - }} - > - Open access control - -
- ) + return } return ( diff --git a/frontend/src/scenes/dashboard/DashboardCollaborators.tsx b/frontend/src/scenes/dashboard/DashboardCollaborators.tsx index 75b83719330d4..d4fa144d743b3 100644 --- a/frontend/src/scenes/dashboard/DashboardCollaborators.tsx +++ b/frontend/src/scenes/dashboard/DashboardCollaborators.tsx @@ -1,4 +1,4 @@ -import { IconLock, IconOpenSidebar, IconTrash, IconUnlock } from '@posthog/icons' +import { IconLock, IconTrash, IconUnlock } from '@posthog/icons' import { useActions, useValues } from 'kea' import { router } from 'kea-router' import { PayGateMini } from 'lib/components/PayGateMini/PayGateMini' @@ -14,8 +14,8 @@ import { Tooltip } from 'lib/lemon-ui/Tooltip' import { dashboardLogic } from 'scenes/dashboard/dashboardLogic' import { urls } from 'scenes/urls' -import { sidePanelStateLogic } from '~/layout/navigation-3000/sidepanel/sidePanelStateLogic' -import { AvailableFeature, DashboardType, FusedDashboardCollaboratorType, SidePanelTab, UserType } from '~/types' +import { AccessControlPopoutCTA } from '~/layout/navigation-3000/sidepanel/panels/access_control/AccessControlPopoutCTA' +import { AvailableFeature, DashboardType, FusedDashboardCollaboratorType, UserType } from '~/types' import { dashboardCollaboratorsLogic } from './dashboardCollaboratorsLogic' @@ -41,8 +41,6 @@ export function DashboardCollaboration({ dashboardId }: { dashboardId: Dashboard dashboardCollaboratorsLogic({ dashboardId }) ) const { push } = useActions(router) - const { openSidePanel } = useActions(sidePanelStateLogic) - const newAccessControl = useFeatureFlag('ROLE_BASED_ACCESS_CONTROL') if (!dashboard) { @@ -51,22 +49,11 @@ export function DashboardCollaboration({ dashboardId }: { dashboardId: Dashboard if (newAccessControl) { return ( -
-

Access control

- - Permissions have moved! We're rolling out our new access control system. Click below to open it. - - } - onClick={() => { - openSidePanel(SidePanelTab.AccessControl) - push(urls.dashboard(dashboard.id)) - }} - > - Open access control - -
+ { + push(urls.dashboard(dashboard.id)) + }} + /> ) } diff --git a/frontend/src/scenes/notebooks/Notebook/NotebookShareModal.tsx b/frontend/src/scenes/notebooks/Notebook/NotebookShareModal.tsx index 534599664149f..69fa1283e9b19 100644 --- a/frontend/src/scenes/notebooks/Notebook/NotebookShareModal.tsx +++ b/frontend/src/scenes/notebooks/Notebook/NotebookShareModal.tsx @@ -1,4 +1,4 @@ -import { IconCopy, IconOpenSidebar } from '@posthog/icons' +import { IconCopy } from '@posthog/icons' import { LemonBanner, LemonButton, LemonDivider, LemonModal } from '@posthog/lemon-ui' import { useActions, useValues } from 'kea' import { FlaggedFeature } from 'lib/components/FlaggedFeature' @@ -9,8 +9,7 @@ import posthog from 'posthog-js' import { useState } from 'react' import { urls } from 'scenes/urls' -import { sidePanelStateLogic } from '~/layout/navigation-3000/sidepanel/sidePanelStateLogic' -import { SidePanelTab } from '~/types' +import { AccessControlPopoutCTA } from '~/layout/navigation-3000/sidepanel/panels/access_control/AccessControlPopoutCTA' import { notebookLogic } from './notebookLogic' @@ -21,7 +20,6 @@ export type NotebookShareModalProps = { export function NotebookShareModal({ shortId }: NotebookShareModalProps): JSX.Element { const { content, isLocalOnly, isShareModalOpen } = useValues(notebookLogic({ shortId })) const { closeShareModal } = useActions(notebookLogic({ shortId })) - const { openSidePanel } = useActions(sidePanelStateLogic) const notebookUrl = urls.absolute(urls.currentProject(urls.notebook(shortId))) const canvasUrl = urls.absolute(urls.canvas()) + `#🦔=${base64Encode(JSON.stringify(content))}` @@ -47,23 +45,11 @@ export function NotebookShareModal({ shortId }: NotebookShareModalProps): JSX.El
<> -
-

Access control

- - Permissions have moved! We're rolling out our new access control system. Click below to - open it. - - } - onClick={() => { - openSidePanel(SidePanelTab.AccessControl) - closeShareModal() - }} - > - Open access control - -
+ { + closeShareModal() + }} + /> From 05231fd7ec128da7022a3237df145015cf610f76 Mon Sep 17 00:00:00 2001 From: Zach Waterfield Date: Tue, 7 Jan 2025 12:58:23 -0500 Subject: [PATCH 15/15] use get object so permissions are checked --- posthog/api/dashboards/dashboard.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/posthog/api/dashboards/dashboard.py b/posthog/api/dashboards/dashboard.py index 8ce4056893d1e..6c8046c3743cd 100644 --- a/posthog/api/dashboards/dashboard.py +++ b/posthog/api/dashboards/dashboard.py @@ -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 @@ -523,9 +522,7 @@ def dangerously_get_queryset(self): @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())