Display segment filters to anyone that can see the dashboard being filtered by the segment (#5935)

* Remove segment filters secrecy

* Update changelog

* Update CHANGELOG.md

Co-authored-by: Adam Rutkowski <hq@mtod.org>

---------

Co-authored-by: Adam Rutkowski <hq@mtod.org>

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

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}`, () => {
+describe(`${canExpandSegment.name}`, () => {
-  it('should return true if the user is logged in and not a public role', () => {
+  it.each([[Role.admin], [Role.editor], [Role.owner]])(
-    const user: UserContextValue = {
+    'allows expanding site segment if the user is logged in and in the role %p',
-      loggedIn: true,
+    (role) => {
-      role: Role.admin,
+      const site = { siteSegmentsAvailable: true }
-      id: 1,
+      const user: UserContextValue = {
-      team: { identifier: null, hasConsolidatedView: false }
+        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', () => {
+  it('forbids expanding site segment if site segments are not available', () => {
-    const user: UserContextValue = {
+    expect(
-      loggedIn: false,
+      canExpandSegment({
-      role: Role.editor,
+        segment: { id: 1, owner_id: 1, type: SegmentType.site },
-      id: null,
+        user: {
-      team: { identifier: null, hasConsolidatedView: false }
+          loggedIn: true,
-    }
+          role: Role.owner,
-    expect(canSeeSegmentDetails({ user })).toBe(false)
+          id: 1,
+          team: { identifier: null, hasConsolidatedView: false }
+        },
+        site: { siteSegmentsAvailable: false }
+      })
+    ).toBe(false)
   })
 
-  it('should return false if the user has a public role', () => {
+  it('forbids public role from expanding site segments', () => {
-    const user: UserContextValue = {
+    expect(
-      loggedIn: true,
+      canExpandSegment({
-      role: Role.public,
+        segment: { id: 1, owner_id: null, type: SegmentType.site },
-      id: 1,
+        user: {
-      team: { identifier: null, hasConsolidatedView: false }
+          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)
   })
 })

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<SavedSegment, 'id' | 'owner_id' | 'type'>
+  site: Pick<PlausibleSite, 'siteSegmentsAvailable'>
+  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) {

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 } & (
+type SegmentAuthorshipProps = {
-  | { showOnlyPublicData: true; segment: SavedSegmentPublic }
+  className?: string
-  | { showOnlyPublicData: false; segment: SavedSegment }
+  showOnlyPublicData: boolean
-)
+  segment: SavedSegmentPublic | SavedSegment
+}
 
 export function SegmentAuthorship({
   className,

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)(

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,
+  canExpandSegment,
-  isListableSegment,
   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
+  const segment = segments.find((s) => String(s.id) === String(id))
-    .filter((s) => isListableSegment({ segment: s, site, user }))
-    .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'] }) => {
 
             <SegmentAuthorship
               segment={data}
-              showOnlyPublicData={false}
+              showOnlyPublicData={!user.loggedIn || user.role === Role.public}
               className="mt-4 text-sm"
             />
             <div className="mt-4">
               <ButtonsRow>
-                <AppNavigationLink
+                {canExpandSegment({ segment: data, site, user }) && (
-                  className={primaryNeutralButtonClassName}
+                  <AppNavigationLink
-                  path={rootRoute.path}
+                    className={primaryNeutralButtonClassName}
-                  search={(s) => ({
+                    path={rootRoute.path}
-                    ...s,
+                    search={(s) => ({
-                    filters: data.segment_data.filters,
+                      ...s,
-                    labels: data.segment_data.labels
+                      filters: data.segment_data.filters,
-                  })}
+                      labels: data.segment_data.labels
-                  state={{
+                    })}
-                    expandedSegment: data
+                    state={{
-                  }}
+                      expandedSegment: data
-                >
+                    }}
-                  Edit segment
+                  >
-                </AppNavigationLink>
+                    Edit segment
+                  </AppNavigationLink>
+                )}
 
                 <AppNavigationLink
                   className={removeFilterButtonClassname}