From ffd1a4fd0f098938385ddfadf013696a6c50a488 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Fri, 21 Aug 2026 16:01:56 +0000 Subject: [PATCH] fix(notifications): enforce settings access permission Keep notification preferences hidden and inaccessible unless the user has notification access, while preserving the demo restriction. Fixes #1057 --- client/env.d.ts | 4 ++ client/src/components/AppHeader.vue | 4 +- .../settings/__tests__/SettingsNav.spec.ts | 17 ++++-- .../__tests__/SettingsSidebar.spec.ts | 18 +++++- .../settings/__tests__/settings-nav.spec.ts | 25 +++++++- .../src/features/settings/lib/settings-nav.ts | 2 +- .../router/__tests__/route-meta-title.spec.ts | 11 ++++ client/src/router/guards/auth.guard.spec.ts | 58 +++++++++++++++++++ client/src/router/guards/auth.guard.ts | 13 +++++ client/src/router/index.ts | 8 ++- 10 files changed, 148 insertions(+), 12 deletions(-) diff --git a/client/env.d.ts b/client/env.d.ts index 7b008bc84..f0a941900 100644 --- a/client/env.d.ts +++ b/client/env.d.ts @@ -1,11 +1,15 @@ /// import 'vue-router' +import type { Permission } from '@bookorbit/types' import type { RouteLocationNormalizedLoaded } from 'vue-router' declare module 'vue-router' { interface RouteMeta { public?: boolean + requiredPermission?: Permission + forbiddenPermission?: Permission + permissionFallback?: string maxWidth?: string title?: string | ((to: RouteLocationNormalizedLoaded) => string) } diff --git a/client/src/components/AppHeader.vue b/client/src/components/AppHeader.vue index b7062507b..3c6351233 100644 --- a/client/src/components/AppHeader.vue +++ b/client/src/components/AppHeader.vue @@ -53,7 +53,7 @@ import NotificationSheet from '@/features/notifications/components/NotificationS import { useNotifications } from '@/features/notifications/composables/useNotifications' import { useWhatsNew } from '@/features/whats-new/composables/useWhatsNew' import UserAvatar from '@/components/UserAvatar.vue' -import { DEFAULT_FORMAT_PRIORITY, LOCALE_LABELS, type Locale } from '@bookorbit/types' +import { DEFAULT_FORMAT_PRIORITY, LOCALE_LABELS, Permission, type Locale } from '@bookorbit/types' import { useThemeStore } from '@/stores/theme' import { useLocaleStore } from '@/stores/locale' import { getFormatColor } from '@/features/book/lib/format-colors' @@ -108,7 +108,7 @@ function navigateToAchievements() { const canChangePassword = computed( () => !isDemoRestrictedAccount.value && user.value?.provisioningMethod !== 'oidc' && user.value?.provisioningMethod !== 'shared', ) -const canAccessNotifications = computed(() => hasPermission('notification_access') && !isDemoRestrictedAccount.value) +const canAccessNotifications = computed(() => hasPermission(Permission.NotificationAccess) && !isDemoRestrictedAccount.value) const GLOBAL_SEARCH_ROW_HEIGHT = 84 const GLOBAL_SEARCH_OVERSCAN = 4 const GLOBAL_SEARCH_VIEWPORT_HEIGHT = 512 diff --git a/client/src/features/settings/__tests__/SettingsNav.spec.ts b/client/src/features/settings/__tests__/SettingsNav.spec.ts index 0d34bbacd..4d5db8d0e 100644 --- a/client/src/features/settings/__tests__/SettingsNav.spec.ts +++ b/client/src/features/settings/__tests__/SettingsNav.spec.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi, beforeEach } from 'vitest' import { mount, RouterLinkStub } from '@vue/test-utils' import { computed } from 'vue' +import { Permission } from '@bookorbit/types' import SettingsNav from '../components/SettingsNav.vue' const permState = { @@ -76,12 +77,18 @@ describe('SettingsNav', () => { expect(labels).toContain('Restrictions') }) - it('shows notifications for a normal account', () => { - expect(itemLabels(mountNav())).toContain('Notifications') + it('hides notifications without notification access', () => { + expect(itemLabels(mountNav())).not.toContain('Notifications') }) - it('hides notifications for a demo restricted account', () => { - expect(itemLabels(mountNav({ demo: true }))).not.toContain('Notifications') + it('shows notifications with notification access or superuser status', () => { + expect(itemLabels(mountNav({ perms: [Permission.NotificationAccess] }))).toContain('Notifications') + expect(itemLabels(mountNav({ su: true }))).toContain('Notifications') + }) + + it('hides notifications for a demo-restricted account even when otherwise allowed', () => { + expect(itemLabels(mountNav({ perms: [Permission.NotificationAccess], demo: true }))).not.toContain('Notifications') + expect(itemLabels(mountNav({ su: true, demo: true }))).not.toContain('Notifications') }) }) @@ -242,6 +249,8 @@ describe('SettingsNav', () => { it('never returns a page the user cannot open', async () => { expect(await search(mountNav(), 'users')).toEqual([]) + expect(await search(mountNav(), 'notifications')).toEqual([]) + expect(await search(mountNav({ perms: [Permission.NotificationAccess] }), 'notifications')).toContain('Notifications') }) it('shows an empty state when nothing matches', async () => { diff --git a/client/src/features/settings/__tests__/SettingsSidebar.spec.ts b/client/src/features/settings/__tests__/SettingsSidebar.spec.ts index 220e789c4..9f4e4a39f 100644 --- a/client/src/features/settings/__tests__/SettingsSidebar.spec.ts +++ b/client/src/features/settings/__tests__/SettingsSidebar.spec.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import { mount, RouterLinkStub } from '@vue/test-utils' import { computed } from 'vue' +import { Permission } from '@bookorbit/types' import SettingsSidebar from '../components/SettingsSidebar.vue' const permState = { @@ -21,10 +22,10 @@ vi.mock('@/features/auth/composables/usePermissions', () => ({ }), })) -function mountSidebar(opts?: { rail?: boolean; su?: boolean; perms?: string[] }) { +function mountSidebar(opts?: { rail?: boolean; su?: boolean; perms?: string[]; demo?: boolean }) { permState.isSuperuser = opts?.su ?? false permState.permissions = opts?.perms ?? [] - permState.demoRestricted = false + permState.demoRestricted = opts?.demo ?? false return mount(SettingsSidebar, { props: { isRail: opts?.rail ?? false }, global: { @@ -83,4 +84,17 @@ describe('SettingsSidebar', () => { expect(mountSidebar({ rail: true }).findAll('[data-testid="rail-group"]')).toHaveLength(1) expect(mountSidebar({ rail: true, perms: ['manage_libraries'] }).findAll('[data-testid="rail-group"]')).toHaveLength(2) }) + + it('applies notification access rules in collapsed rail mode', () => { + const labels = (opts?: Parameters[0]) => + mountSidebar({ ...opts, rail: true }) + .findAll('[data-testid="settings-sidebar-rail-item"]') + .map((node) => node.text()) + + expect(labels()).not.toContain('Notifications') + expect(labels({ perms: [Permission.NotificationAccess] })).toContain('Notifications') + expect(labels({ su: true })).toContain('Notifications') + expect(labels({ perms: [Permission.NotificationAccess], demo: true })).not.toContain('Notifications') + expect(labels({ su: true, demo: true })).not.toContain('Notifications') + }) }) diff --git a/client/src/features/settings/__tests__/settings-nav.spec.ts b/client/src/features/settings/__tests__/settings-nav.spec.ts index 162a422cf..cc616e84f 100644 --- a/client/src/features/settings/__tests__/settings-nav.spec.ts +++ b/client/src/features/settings/__tests__/settings-nav.spec.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest' import type { RouteRecordRaw } from 'vue-router' import { BookMarked, Highlighter, LibraryBig } from '@lucide/vue' +import { Permission } from '@bookorbit/types' import { routes } from '@/router' import { SETTINGS_NAV, findSettingsNavItem, firstVisibleSettingsRoute, visibleSettingsNav, type SettingsNavContext } from '../lib/settings-nav' @@ -56,11 +57,31 @@ describe('visibleSettingsNav', () => { expect(visibleSettingsNav(SUPERUSER).map((group) => group.id)).toEqual(['you', 'library', 'devices', 'server']) }) - it('filters children as well as top level items', () => { - const you = visibleSettingsNav({ ...NOBODY, isDemoRestricted: true })[0] + it('hides notifications without notification access', () => { + const you = visibleSettingsNav(NOBODY)[0] expect(you?.items.map((item) => item.id)).not.toContain('notifications') }) + it('shows notifications with notification access or superuser status', () => { + const permitted = visibleSettingsNav({ ...NOBODY, permissions: [Permission.NotificationAccess] })[0] + const superuser = visibleSettingsNav(SUPERUSER)[0] + + expect(permitted?.items.map((item) => item.id)).toContain('notifications') + expect(superuser?.items.map((item) => item.id)).toContain('notifications') + }) + + it('hides notifications from demo-restricted accounts even when otherwise allowed', () => { + const permittedDemo = visibleSettingsNav({ + ...NOBODY, + permissions: [Permission.NotificationAccess], + isDemoRestricted: true, + })[0] + const superuserDemo = visibleSettingsNav({ ...SUPERUSER, isDemoRestricted: true })[0] + + expect(permittedDemo?.items.map((item) => item.id)).not.toContain('notifications') + expect(superuserDemo?.items.map((item) => item.id)).not.toContain('notifications') + }) + it('only exposes the metadata pages the permission allows', () => { const library = visibleSettingsNav({ ...NOBODY, permissions: ['manage_libraries'] }).find((group) => group.id === 'library') const ids = library?.items.map((item) => item.id) ?? [] diff --git a/client/src/features/settings/lib/settings-nav.ts b/client/src/features/settings/lib/settings-nav.ts index 36a691b82..5648ef54b 100644 --- a/client/src/features/settings/lib/settings-nav.ts +++ b/client/src/features/settings/lib/settings-nav.ts @@ -207,7 +207,7 @@ export const SETTINGS_NAV: readonly SettingsNavGroup[] = [ descriptionKey: 'settings.nav.descriptions.notifications', icon: Bell, keywords: 'notify email digest alerts events', - isVisible: (context) => !context.isDemoRestricted, + isVisible: (context) => !context.isDemoRestricted && (context.isSuperuser || context.permissions.includes(Permission.NotificationAccess)), }, { id: 'privacy', diff --git a/client/src/router/__tests__/route-meta-title.spec.ts b/client/src/router/__tests__/route-meta-title.spec.ts index 9fc48fc9e..2b72eee8a 100644 --- a/client/src/router/__tests__/route-meta-title.spec.ts +++ b/client/src/router/__tests__/route-meta-title.spec.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest' import type { RouteRecordRaw } from 'vue-router' +import { Permission } from '@bookorbit/types' import { routes } from '@/router' type MissingRoute = { @@ -45,6 +46,16 @@ describe('router title metadata', () => { }) describe('router redirects', () => { + it('protects the notification settings route from direct navigation', () => { + const route = findRoute(routes, 'settings-notifications') + + expect(route?.meta).toMatchObject({ + requiredPermission: Permission.NotificationAccess, + forbiddenPermission: Permission.DemoRestricted, + permissionFallback: 'settings-account-profile', + }) + }) + it('redirects the legacy Integrations tab URL to the Readwise page', () => { const route = findRoute(routes, 'settings-integrations') expect(route?.redirect).toBeTypeOf('function') diff --git a/client/src/router/guards/auth.guard.spec.ts b/client/src/router/guards/auth.guard.spec.ts index c2ef1998f..eb2927b79 100644 --- a/client/src/router/guards/auth.guard.spec.ts +++ b/client/src/router/guards/auth.guard.spec.ts @@ -1,5 +1,6 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import { ref, type Ref } from 'vue' +import { Permission } from '@bookorbit/types' import { registerAuthGuard } from './auth.guard' @@ -8,6 +9,8 @@ const mocks = vi.hoisted(() => ({ openChangePassword: vi.fn<(required: boolean) => void>(), user: null as unknown as Ref<{ isDefaultPassword: boolean + isSuperuser: boolean + permissions: string[] provisioningMethod: string settings: { achievementPreferences: { enabled: boolean } } }>, @@ -40,6 +43,8 @@ describe('registerAuthGuard', () => { mocks.openChangePassword.mockReset() mocks.user = ref({ isDefaultPassword: false, + isSuperuser: false, + permissions: [], provisioningMethod: 'local', settings: { achievementPreferences: { enabled: false } }, }) @@ -64,4 +69,57 @@ describe('registerAuthGuard', () => { await expect(guard({ name: 'achievements', path: '/achievements', fullPath: '/achievements', meta: {} })).resolves.toBe(true) }) + + it('redirects a direct route visit when the required permission is missing', async () => { + if (!guard) throw new Error('Expected guard to be registered') + + await expect( + guard({ + name: 'settings-notifications', + path: '/settings/account/notifications', + fullPath: '/settings/account/notifications', + meta: { + requiredPermission: Permission.NotificationAccess, + forbiddenPermission: Permission.DemoRestricted, + permissionFallback: 'settings-account-profile', + }, + }), + ).resolves.toEqual({ name: 'settings-account-profile' }) + }) + + it('allows a direct route visit with notification access or superuser status', async () => { + if (!guard) throw new Error('Expected guard to be registered') + const route = { + name: 'settings-notifications', + path: '/settings/account/notifications', + fullPath: '/settings/account/notifications', + meta: { requiredPermission: Permission.NotificationAccess }, + } + + mocks.user.value.permissions = [Permission.NotificationAccess] + await expect(guard(route)).resolves.toBe(true) + + mocks.user.value.permissions = [] + mocks.user.value.isSuperuser = true + await expect(guard(route)).resolves.toBe(true) + }) + + it('redirects demo-restricted users even when notification access would otherwise allow the route', async () => { + if (!guard) throw new Error('Expected guard to be registered') + mocks.user.value.isSuperuser = true + mocks.user.value.permissions = [Permission.NotificationAccess, Permission.DemoRestricted] + + await expect( + guard({ + name: 'settings-notifications', + path: '/settings/account/notifications', + fullPath: '/settings/account/notifications', + meta: { + requiredPermission: Permission.NotificationAccess, + forbiddenPermission: Permission.DemoRestricted, + permissionFallback: 'settings-account-profile', + }, + }), + ).resolves.toEqual({ name: 'settings-account-profile' }) + }) }) diff --git a/client/src/router/guards/auth.guard.ts b/client/src/router/guards/auth.guard.ts index 4bf20b597..5c2f47072 100644 --- a/client/src/router/guards/auth.guard.ts +++ b/client/src/router/guards/auth.guard.ts @@ -3,6 +3,10 @@ import { useAuth } from '@/features/auth/composables/useAuth' import { useChangePasswordDialog } from '@/composables/useChangePasswordDialog' import { useSetupStatus } from '@/features/auth/composables/useSetupStatus' +function permissionFallback(name: string | undefined) { + return name ? { name } : { path: '/' } +} + export function registerAuthGuard(router: Router): void { router.beforeEach(async (to) => { const { fetchSetupStatus, allowRegistration } = useSetupStatus() @@ -41,6 +45,15 @@ export function registerAuthGuard(router: Router): void { if (to.path !== '/') return { path: '/' } } + const permissions = user.value.permissions ?? [] + if (to.meta.forbiddenPermission && permissions.includes(to.meta.forbiddenPermission)) { + return permissionFallback(to.meta.permissionFallback) + } + + if (to.meta.requiredPermission && !user.value.isSuperuser && !permissions.includes(to.meta.requiredPermission)) { + return permissionFallback(to.meta.permissionFallback) + } + if (to.name === 'achievements' && user.value.settings.achievementPreferences?.enabled === false) { return { name: 'settings-account-profile' } } diff --git a/client/src/router/index.ts b/client/src/router/index.ts index 49594d2de..7d52104e6 100644 --- a/client/src/router/index.ts +++ b/client/src/router/index.ts @@ -6,6 +6,7 @@ import { type RouteLocationNormalizedLoaded, type RouteRecordRaw, } from 'vue-router' +import { Permission } from '@bookorbit/types' import { normalizeEmailTab } from '@/features/email/lib/email-tabs' import { normalizeMetadataTab, type MetadataTab } from '@/features/settings/lib/metadata-tabs' import { normalizeReaderTab, type ReaderTab } from '@/features/settings/lib/reader-tabs' @@ -173,7 +174,12 @@ export const routes: RouteRecordRaw[] = [ name: 'settings-notifications', component: () => import('@/features/notifications/components/NotificationPreferences.vue'), props: { embedded: true }, - meta: { title: () => t('titles.account.notifications') }, + meta: { + requiredPermission: Permission.NotificationAccess, + forbiddenPermission: Permission.DemoRestricted, + permissionFallback: 'settings-account-profile', + title: () => t('titles.account.notifications'), + }, }, { path: 'account/restrictions',