diff --git a/CHANGELOG.md b/CHANGELOG.md index f58ec5a373..4a35de82a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ All notable changes to this project will be documented in this file. ### Changed +- Segment filters are visible to anyone who can view the dashboard with that segment applied, including personal segments on public dashboards + ### Fixed - To make internal stats API requests for password-protected shared links, shared link auth cookie must be set in the requests diff --git a/assets/js/dashboard/filtering/segments.test.ts b/assets/js/dashboard/filtering/segments.test.ts index a52eac22fd..e896ac3d02 100644 --- a/assets/js/dashboard/filtering/segments.test.ts +++ b/assets/js/dashboard/filtering/segments.test.ts @@ -10,7 +10,7 @@ import { SegmentType, SavedSegment, SegmentData, - canSeeSegmentDetails + canExpandSegment } from './segments' import { Filter } from '../query' import { PlausibleSite } from '../site-context' @@ -183,34 +183,124 @@ describe(`${resolveFilters.name}`, () => { ) }) -describe(`${canSeeSegmentDetails.name}`, () => { - it('should return true if the user is logged in and not a public role', () => { - const user: UserContextValue = { - loggedIn: true, - role: Role.admin, - id: 1, - team: { identifier: null, hasConsolidatedView: false } +describe(`${canExpandSegment.name}`, () => { + it.each([[Role.admin], [Role.editor], [Role.owner]])( + 'allows expanding site segment if the user is logged in and in the role %p', + (role) => { + const site = { siteSegmentsAvailable: true } + const user: UserContextValue = { + loggedIn: true, + role, + id: 1, + team: { identifier: null, hasConsolidatedView: false } + } + expect( + canExpandSegment({ + segment: { id: 1, owner_id: 1, type: SegmentType.site }, + user, + site + }) + ).toBe(true) } - expect(canSeeSegmentDetails({ user })).toBe(true) + ) + + it('allows expanding site segments defined by other users', () => { + expect( + canExpandSegment({ + segment: { id: 1, owner_id: 222, type: SegmentType.site }, + user: { + loggedIn: true, + role: Role.owner, + id: 111, + team: { identifier: null, hasConsolidatedView: false } + }, + site: { siteSegmentsAvailable: true } + }) + ).toBe(true) }) - it('should return false if the user is not logged in', () => { - const user: UserContextValue = { - loggedIn: false, - role: Role.editor, - id: null, - team: { identifier: null, hasConsolidatedView: false } - } - expect(canSeeSegmentDetails({ user })).toBe(false) + it('forbids expanding site segment if site segments are not available', () => { + expect( + canExpandSegment({ + segment: { id: 1, owner_id: 1, type: SegmentType.site }, + user: { + loggedIn: true, + role: Role.owner, + id: 1, + team: { identifier: null, hasConsolidatedView: false } + }, + site: { siteSegmentsAvailable: false } + }) + ).toBe(false) }) - it('should return false if the user has a public role', () => { - const user: UserContextValue = { - loggedIn: true, - role: Role.public, - id: 1, - team: { identifier: null, hasConsolidatedView: false } + it('forbids public role from expanding site segments', () => { + expect( + canExpandSegment({ + segment: { id: 1, owner_id: null, type: SegmentType.site }, + user: { + loggedIn: false, + role: Role.public, + id: null, + team: { identifier: null, hasConsolidatedView: false } + }, + site: { siteSegmentsAvailable: false } + }) + ).toBe(false) + }) + + it.each([ + [Role.viewer], + [Role.billing], + [Role.editor], + [Role.admin], + [Role.owner] + ])( + 'allows expanding personal segment if it belongs to the user and the user is in role %p', + (role) => { + const user: UserContextValue = { + loggedIn: true, + role, + id: 1, + team: { identifier: null, hasConsolidatedView: false } + } + expect( + canExpandSegment({ + segment: { id: 1, owner_id: 1, type: SegmentType.personal }, + user, + site: { siteSegmentsAvailable: false } + }) + ).toBe(true) } - expect(canSeeSegmentDetails({ user })).toBe(false) + ) + + it('forbids expanding personal segment of other users', () => { + expect( + canExpandSegment({ + segment: { id: 2, owner_id: 222, type: SegmentType.personal }, + user: { + loggedIn: true, + role: Role.owner, + id: 111, + team: { identifier: null, hasConsolidatedView: false } + }, + site: { siteSegmentsAvailable: false } + }) + ).toBe(false) + }) + + it('forbids public role from expanding personal segments', () => { + expect( + canExpandSegment({ + segment: { id: 1, owner_id: 1, type: SegmentType.personal }, + user: { + loggedIn: false, + role: Role.public, + id: null, + team: { identifier: null, hasConsolidatedView: false } + }, + site: { siteSegmentsAvailable: false } + }) + ).toBe(false) }) }) diff --git a/assets/js/dashboard/filtering/segments.ts b/assets/js/dashboard/filtering/segments.ts index e3e6aa0cb8..c1516138a8 100644 --- a/assets/js/dashboard/filtering/segments.ts +++ b/assets/js/dashboard/filtering/segments.ts @@ -10,6 +10,16 @@ export enum SegmentType { site = 'site' } +/** keep in sync with Plausible.Segments */ +const ROLES_WITH_MAYBE_SITE_SEGMENTS = [Role.admin, Role.editor, Role.owner] +const ROLES_WITH_PERSONAL_SEGMENTS = [ + Role.billing, + Role.viewer, + Role.admin, + Role.editor, + Role.owner +] + /** This type signifies that the owner can't be shown. */ type SegmentOwnershipHidden = { owner_id: null; owner_name: null } @@ -148,6 +158,36 @@ export function resolveFilters( }) } +export function canExpandSegment({ + segment, + site, + user +}: { + segment: Pick + site: Pick + user: UserContextValue +}) { + if ( + segment.type === SegmentType.site && + site.siteSegmentsAvailable && + user.loggedIn && + ROLES_WITH_MAYBE_SITE_SEGMENTS.includes(user.role) + ) { + return true + } + + if ( + segment.type === SegmentType.personal && + user.loggedIn && + ROLES_WITH_PERSONAL_SEGMENTS.includes(user.role) && + user.id === segment.owner_id + ) { + return true + } + + return false +} + export function isListableSegment({ segment, site, @@ -173,10 +213,6 @@ export function isListableSegment({ return false } -export function canSeeSegmentDetails({ user }: { user: UserContextValue }) { - return user.loggedIn && user.role !== Role.public -} - export function findAppliedSegmentFilter({ filters }: { filters: Filter[] }) { const segmentFilter = filters.find(isSegmentFilter) if (!segmentFilter) { diff --git a/assets/js/dashboard/segments/segment-authorship.tsx b/assets/js/dashboard/segments/segment-authorship.tsx index b3b561022a..1a701e6109 100644 --- a/assets/js/dashboard/segments/segment-authorship.tsx +++ b/assets/js/dashboard/segments/segment-authorship.tsx @@ -3,10 +3,11 @@ import { SavedSegmentPublic, SavedSegment } from '../filtering/segments' import { dateForSite, formatDayShort } from '../util/date' import { useSiteContext } from '../site-context' -type SegmentAuthorshipProps = { className?: string } & ( - | { showOnlyPublicData: true; segment: SavedSegmentPublic } - | { showOnlyPublicData: false; segment: SavedSegment } -) +type SegmentAuthorshipProps = { + className?: string + showOnlyPublicData: boolean + segment: SavedSegmentPublic | SavedSegment +} export function SegmentAuthorship({ className, diff --git a/assets/js/dashboard/segments/segment-modals.test.tsx b/assets/js/dashboard/segments/segment-modals.test.tsx index e9cbca078b..92535aaad7 100644 --- a/assets/js/dashboard/segments/segment-modals.test.tsx +++ b/assets/js/dashboard/segments/segment-modals.test.tsx @@ -58,45 +58,6 @@ describe('Segment details modal - errors', () => { }, message: `Segment not found with with ID "202020"`, siteOptions: { siteSegmentsAvailable: true } - }, - { - case: 'site segment is in list but not listable because site segments are not available', - segments: [anyPersonalSegment, anySiteSegment], - segmentId: anySiteSegment.id, - user: { - loggedIn: true, - id: 1, - role: Role.owner, - team: { identifier: null, hasConsolidatedView: false } - }, - message: `Segment not found with with ID "${anySiteSegment.id}"`, - siteOptions: { siteSegmentsAvailable: false } - }, - { - case: 'personal segment is in list but not listable because it is a public dashboard', - segments: [{ ...anyPersonalSegment, owner_id: null, owner_name: null }], - segmentId: anyPersonalSegment.id, - user: { - loggedIn: false, - id: null, - role: Role.public, - team: { identifier: null, hasConsolidatedView: false } - }, - message: `Segment not found with with ID "${anyPersonalSegment.id}"`, - siteOptions: { siteSegmentsAvailable: true } - }, - { - case: 'segment is in list and listable, but detailed view is not available because user is not logged in', - segments: [{ ...anySiteSegment, owner_id: null, owner_name: null }], - segmentId: anySiteSegment.id, - user: { - loggedIn: false, - id: null, - role: Role.public, - team: { identifier: null, hasConsolidatedView: false } - }, - message: 'Not enough permissions to see segment details', - siteOptions: { siteSegmentsAvailable: true } } ] it.each(cases)( diff --git a/assets/js/dashboard/segments/segment-modals.tsx b/assets/js/dashboard/segments/segment-modals.tsx index 75a03e94b8..1261a24a0c 100644 --- a/assets/js/dashboard/segments/segment-modals.tsx +++ b/assets/js/dashboard/segments/segment-modals.tsx @@ -1,8 +1,7 @@ import React, { ReactNode, useCallback, useState } from 'react' import ModalWithRouting from '../stats/modals/modal' import { - canSeeSegmentDetails, - isListableSegment, + canExpandSegment, isSegmentFilter, SavedSegment, SEGMENT_TYPE_LABELS, @@ -22,9 +21,9 @@ import { MutationStatus } from '@tanstack/react-query' import { ApiError } from '../api' import { ErrorPanel } from '../components/error-panel' import { useSegmentsContext } from '../filtering/segments-context' -import { useSiteContext } from '../site-context' import { Role, UserContextValue, useUserContext } from '../user-context' import { removeFilterButtonClassname } from '../components/remove-filter-button' +import { useSiteContext } from '../site-context' interface ApiRequestProps { status: MutationStatus @@ -501,9 +500,7 @@ export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => { const { query } = useQueryContext() const { segments } = useSegmentsContext() - const segment = segments - .filter((s) => isListableSegment({ segment: s, site, user })) - .find((s) => String(s.id) === String(id)) + const segment = segments.find((s) => String(s.id) === String(id)) let error: ApiError | null = null @@ -511,10 +508,6 @@ export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => { error = new ApiError(`Segment not found with with ID "${id}"`, { error: `Segment not found with with ID "${id}"` }) - } else if (!canSeeSegmentDetails({ user })) { - error = new ApiError('Not enough permissions to see segment details', { - error: `Not enough permissions to see segment details` - }) } const data = !error ? segment : null @@ -542,25 +535,27 @@ export const SegmentModal = ({ id }: { id: SavedSegment['id'] }) => {
- ({ - ...s, - filters: data.segment_data.filters, - labels: data.segment_data.labels - })} - state={{ - expandedSegment: data - }} - > - Edit segment - + {canExpandSegment({ segment: data, site, user }) && ( + ({ + ...s, + filters: data.segment_data.filters, + labels: data.segment_data.labels + })} + state={{ + expandedSegment: data + }} + > + Edit segment + + )}