From 74dbbbc104822eb0904cb38369aa162c5b7f2444 Mon Sep 17 00:00:00 2001 From: Peter Ringelmann Date: Mon, 31 Aug 2026 15:43:12 +0200 Subject: [PATCH] fix(core): make the app store link in the app menu configurable Signed-off-by: Peter Ringelmann --- core/AppInfo/ConfigLexicon.php | 12 ++ core/src/components/AppMenu.vue | 16 ++- core/src/tests/components/AppMenu.spec.ts | 87 +++++++------ lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + .../App/AppStore/AppStoreLinkVisibility.php | 45 +++++++ lib/private/TemplateLayout.php | 2 + .../AppStore/AppStoreLinkVisibilityTest.php | 123 ++++++++++++++++++ .../core/admin-settings-appstore-link.spec.ts | 43 ++++++ 9 files changed, 283 insertions(+), 47 deletions(-) create mode 100644 lib/private/App/AppStore/AppStoreLinkVisibility.php create mode 100644 tests/lib/App/AppStore/AppStoreLinkVisibilityTest.php create mode 100644 tests/playwright/e2e/core/admin-settings-appstore-link.spec.ts diff --git a/core/AppInfo/ConfigLexicon.php b/core/AppInfo/ConfigLexicon.php index dccacc4a06616..4a9f15321169d 100644 --- a/core/AppInfo/ConfigLexicon.php +++ b/core/AppInfo/ConfigLexicon.php @@ -40,6 +40,8 @@ class ConfigLexicon implements ILexicon { public const ON_DEMAND_PREVIEW_MIGRATION = 'on_demand_preview_migration'; + public const APPSTORE_LINK_SHOWN = 'appstore_link_shown'; + #[\Override] public function getStrictness(): Strictness { return Strictness::IGNORE; @@ -102,6 +104,16 @@ public function getAppConfigs(): array { defaultRaw: true, definition: 'Whether on demand preview migration is enabled.' ), + new Entry( + key: self::APPSTORE_LINK_SHOWN, + type: ValueType::BOOL, + defaultRaw: fn (Preset $p): bool => match ($p) { + Preset::NONE, Preset::PRIVATE, Preset::FAMILY, Preset::CLUB => true, + default => false, + }, + definition: 'Show the app store link in the app menu to accounts without admin rights', + note: 'When this key is not set, the link is also hidden while a valid subscription is available or while "appstoreenabled" is disabled. Setting this key explicitly takes precedence over both.', + ), new Entry( key: self::DAV_REPAIR_REMOVED_BROKEN_PROPERTIES, type: ValueType::BOOL, diff --git a/core/src/components/AppMenu.vue b/core/src/components/AppMenu.vue index 405169d588dcd..3271901622b02 100644 --- a/core/src/components/AppMenu.vue +++ b/core/src/components/AppMenu.vue @@ -152,6 +152,8 @@ export default defineComponent({ appList, navigationActions, settingsList, + // Fail closed: a missing state must not leak the link. + appStoreLinkShown: loadState('core', 'appStoreLinkShown', false), isAdmin: getCurrentUser()?.isAdmin ?? false, // Roving tabindex: only this tile has tabindex=0; arrow keys move it. focusedIndex: 0, @@ -252,12 +254,16 @@ export default defineComponent({ // Stable-ordered list that focusedIndex indexes into. The trailing // utility tile is "More apps" (local app management) for admins and - // "App store" (apps.nextcloud.com) for everyone else. + // "App store" (apps.nextcloud.com) for everyone else when + // appstore_link_shown allows it. gridItems(): INavigationEntry[] { - const tail = this.isAdmin - ? { ...this.moreAppsEntry, active: this.currentApp?.id === APP_MANAGEMENT_ID } - : this.appStoreEntry - return [...this.appList, tail] + const tail: INavigationEntry[] = [] + if (this.isAdmin) { + tail.push({ ...this.moreAppsEntry, active: this.currentApp?.id === APP_MANAGEMENT_ID }) + } else if (this.appStoreLinkShown) { + tail.push(this.appStoreEntry) + } + return [...this.appList, ...tail] }, }, diff --git a/core/src/tests/components/AppMenu.spec.ts b/core/src/tests/components/AppMenu.spec.ts index efe0e2e80f091..f9c0f66fd167a 100644 --- a/core/src/tests/components/AppMenu.spec.ts +++ b/core/src/tests/components/AppMenu.spec.ts @@ -70,15 +70,10 @@ function fakeApps(): INavigationEntry[] { // ships getAll('settings') keyed by entry id. function mockActiveSettingsEntry(overrides: Partial): void { const entry = makeApp({ type: 'settings', active: true, ...overrides }) - initialState.loadState.mockImplementation((_a: string, key: string, fallback: unknown) => { - if (key === 'apps') { - return [makeApp({ id: 'files', name: 'Files', active: false })] - } - if (key === 'settingsNavEntries') { - return { [entry.id]: entry } - } - return fallback - }) + initialState.loadState.mockImplementation(stateFor({ + apps: [makeApp({ id: 'files', name: 'Files', active: false })], + settingsNavEntries: { [entry.id]: entry }, + })) } // Navigation actions (INavigationManager::TYPE_ACTION). Without an `href` the @@ -98,17 +93,16 @@ function fakeActions(count: number): INavigationEntry[] { return ids.slice(0, count).map((id) => makeAction(id)) } +// AppMenu hides the app store tile when the state is absent, so a default +// instance has to supply it. +function stateFor(states: Record) { + const all: Record = { appStoreLinkShown: true, ...states } + return (_app: string, key: string, fallback: unknown) => key in all ? all[key] : fallback +} + // loadState implementation serving both apps and navigation actions. function stateWith(apps: INavigationEntry[], actions: INavigationEntry[]) { - return (_app: string, key: string, fallback: unknown) => { - if (key === 'apps') { - return apps - } - if (key === 'navigationActions') { - return actions - } - return fallback - } + return stateFor({ apps, navigationActions: actions }) } function eightApps(activeIndex: number = -1): INavigationEntry[] { @@ -132,7 +126,7 @@ beforeEach(async () => { for (const k of Object.keys(eventBus.__handlers)) { delete eventBus.__handlers[k] } - initialState.loadState.mockImplementation((_app: string, key: string, fallback: unknown) => key === 'apps' ? fakeApps() : fallback) + initialState.loadState.mockImplementation(stateFor({ apps: fakeApps() })) auth.getCurrentUser.mockReturnValue({ isAdmin: false }) AppMenu = (await import('../../components/AppMenu.vue')).default }) @@ -144,6 +138,11 @@ afterEach(() => { } }) +function gridLabels(): string[] { + return Array.from(document.querySelectorAll('.app-menu__grid [role="menuitem"]')) + .map((el) => el.querySelector('.app-item__label')?.textContent?.trim() ?? '') +} + // Click the waffle trigger and poll until the teleported menuitems are in the // DOM. NcPopover teleports to so wrapper.find() can't see them; vi.waitFor // retries the DOM query rather than relying on flaky nextTick/setTimeout flushes. @@ -166,10 +165,24 @@ describe('core: AppMenu', () => { const wrapper = mount(AppMenu, { attachTo: document.body }) await openPopover(wrapper) - const items = document.querySelectorAll('.app-menu__grid [role="menuitem"]') - expect(items).toHaveLength(4) - const labels = Array.from(items).map((el) => el.querySelector('.app-item__label')?.textContent?.trim() ?? '') - expect(labels).toEqual(['Files', 'Mail', 'Calendar', 'App store']) + expect(gridLabels()).toEqual(['Files', 'Mail', 'Calendar', 'App store']) + }) + + it('omits the "App store" tile when the instance does not offer it', async () => { + initialState.loadState.mockImplementation(stateFor({ apps: fakeApps(), appStoreLinkShown: false })) + const wrapper = mount(AppMenu, { attachTo: document.body }) + await openPopover(wrapper) + + expect(gridLabels()).toEqual(['Files', 'Mail', 'Calendar']) + }) + + it('keeps the "More apps" tile for admins when the app store link is hidden', async () => { + initialState.loadState.mockImplementation(stateFor({ apps: fakeApps(), appStoreLinkShown: false })) + auth.getCurrentUser.mockReturnValue({ isAdmin: true }) + const wrapper = mount(AppMenu, { attachTo: document.body }) + await openPopover(wrapper) + + expect(gridLabels()).toEqual(['Files', 'Mail', 'Calendar', 'More apps']) }) it('renders the "More apps" tile when the current user is an admin', async () => { @@ -196,7 +209,7 @@ describe('core: AppMenu', () => { }) it('ArrowRight moves the roving stop from index 0 to index 1 and focuses it', async () => { - initialState.loadState.mockImplementation((_a: string, key: string, fallback: unknown) => key === 'apps' ? eightApps() : fallback) + initialState.loadState.mockImplementation(stateFor({ apps: eightApps() })) const wrapper = mount(AppMenu, { attachTo: document.body }) await openPopover(wrapper) @@ -274,15 +287,10 @@ describe('core: AppMenu', () => { }) it('prefers the active app over a settings entry when both are marked active', () => { - initialState.loadState.mockImplementation((_a: string, key: string, fallback: unknown) => { - if (key === 'apps') { - return [makeApp({ id: 'files', name: 'Files', active: true })] - } - if (key === 'settingsNavEntries') { - return { settings_administration: makeApp({ id: 'settings_administration', name: 'Administration settings', type: 'settings', active: true }) } - } - return fallback - }) + initialState.loadState.mockImplementation(stateFor({ + apps: [makeApp({ id: 'files', name: 'Files', active: true })], + settingsNavEntries: { settings_administration: makeApp({ id: 'settings_administration', name: 'Administration settings', type: 'settings', active: true }) }, + })) const wrapper = mount(AppMenu, { attachTo: document.body }) expect(wrapper.find('.app-menu__current-app-name').text()).toBe('Files') }) @@ -292,15 +300,10 @@ describe('core: AppMenu', () => { // "current section" even though it carries type=settings. NavigationManager // today never marks it active, but a future regression shouldn't leak a // "Log out" label into the header. - initialState.loadState.mockImplementation((_a: string, key: string, fallback: unknown) => { - if (key === 'apps') { - return [makeApp({ id: 'files', name: 'Files', active: false })] - } - if (key === 'settingsNavEntries') { - return { logout: makeApp({ id: 'logout', name: 'Log out', type: 'settings', href: '/logout', active: true }) } - } - return fallback - }) + initialState.loadState.mockImplementation(stateFor({ + apps: [makeApp({ id: 'files', name: 'Files', active: false })], + settingsNavEntries: { logout: makeApp({ id: 'logout', name: 'Log out', type: 'settings', href: '/logout', active: true }) }, + })) const wrapper = mount(AppMenu, { attachTo: document.body }) expect(wrapper.find('.app-menu__current-app').exists()).toBe(false) }) diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 5c01b0c2221ed..360363197addf 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1256,6 +1256,7 @@ 'OC\\AppScriptSort' => $baseDir . '/lib/private/AppScriptSort.php', 'OC\\App\\AppManager' => $baseDir . '/lib/private/App/AppManager.php', 'OC\\App\\AppStore\\AppNotFoundException' => $baseDir . '/lib/private/App/AppStore/AppNotFoundException.php', + 'OC\\App\\AppStore\\AppStoreLinkVisibility' => $baseDir . '/lib/private/App/AppStore/AppStoreLinkVisibility.php', 'OC\\App\\AppStore\\Bundles\\Bundle' => $baseDir . '/lib/private/App/AppStore/Bundles/Bundle.php', 'OC\\App\\AppStore\\Bundles\\BundleFetcher' => $baseDir . '/lib/private/App/AppStore/Bundles/BundleFetcher.php', 'OC\\App\\AppStore\\Bundles\\EducationBundle' => $baseDir . '/lib/private/App/AppStore/Bundles/EducationBundle.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 669c234843b58..b4629e15288ba 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1297,6 +1297,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\AppScriptSort' => __DIR__ . '/../../..' . '/lib/private/AppScriptSort.php', 'OC\\App\\AppManager' => __DIR__ . '/../../..' . '/lib/private/App/AppManager.php', 'OC\\App\\AppStore\\AppNotFoundException' => __DIR__ . '/../../..' . '/lib/private/App/AppStore/AppNotFoundException.php', + 'OC\\App\\AppStore\\AppStoreLinkVisibility' => __DIR__ . '/../../..' . '/lib/private/App/AppStore/AppStoreLinkVisibility.php', 'OC\\App\\AppStore\\Bundles\\Bundle' => __DIR__ . '/../../..' . '/lib/private/App/AppStore/Bundles/Bundle.php', 'OC\\App\\AppStore\\Bundles\\BundleFetcher' => __DIR__ . '/../../..' . '/lib/private/App/AppStore/Bundles/BundleFetcher.php', 'OC\\App\\AppStore\\Bundles\\EducationBundle' => __DIR__ . '/../../..' . '/lib/private/App/AppStore/Bundles/EducationBundle.php', diff --git a/lib/private/App/AppStore/AppStoreLinkVisibility.php b/lib/private/App/AppStore/AppStoreLinkVisibility.php new file mode 100644 index 0000000000000..b4ad569087a72 --- /dev/null +++ b/lib/private/App/AppStore/AppStoreLinkVisibility.php @@ -0,0 +1,45 @@ +appConfig->hasKey('core', ConfigLexicon::APPSTORE_LINK_SHOWN)) { + if (!$this->config->getSystemValueBool('appstoreenabled', true)) { + return false; + } + + if ($this->registry->delegateHasValidSubscription()) { + return false; + } + } + + return $this->appConfig->getValueBool('core', ConfigLexicon::APPSTORE_LINK_SHOWN); + } +} diff --git a/lib/private/TemplateLayout.php b/lib/private/TemplateLayout.php index 4836a33560a3b..bddf34638d719 100644 --- a/lib/private/TemplateLayout.php +++ b/lib/private/TemplateLayout.php @@ -11,6 +11,7 @@ namespace OC; use bantu\IniGetWrapper\IniGetWrapper; +use OC\App\AppStore\AppStoreLinkVisibility; use OC\AppFramework\Http\Request; use OC\Authentication\Token\IProvider; use OC\Core\AppInfo\Application; @@ -83,6 +84,7 @@ public function getPageTemplate(string $renderAs, string $appId): ITemplate { $this->initialState->provideInitialState('core', 'active-app', $this->navigationManager->getActiveEntry()); $this->initialState->provideInitialState('core', 'apps', array_values($this->navigationManager->getAll())); $this->initialState->provideInitialState('core', 'navigationActions', array_values($this->navigationManager->getAll(INavigationManager::TYPE_ACTION))); + $this->initialState->provideInitialState('core', 'appStoreLinkShown', Server::get(AppStoreLinkVisibility::class)->isShownToUsers()); $this->initialState->provideInitialState('unified-search', 'min-search-length', $this->appConfig->getValueInt(Application::APP_ID, ConfigLexicon::UNIFIED_SEARCH_MIN_SEARCH_LENGTH)); if ($this->config->getSystemValueBool('unified_search.enabled', false) || !$this->config->getSystemValueBool('enable_non-accessible_features', true)) { diff --git a/tests/lib/App/AppStore/AppStoreLinkVisibilityTest.php b/tests/lib/App/AppStore/AppStoreLinkVisibilityTest.php new file mode 100644 index 0000000000000..2bed8c32f4e33 --- /dev/null +++ b/tests/lib/App/AppStore/AppStoreLinkVisibilityTest.php @@ -0,0 +1,123 @@ +config = $this->createMock(IConfig::class); + $this->appConfig = $this->createMock(IAppConfig::class); + $this->registry = $this->createMock(IRegistry::class); + + $this->visibility = new AppStoreLinkVisibility( + $this->config, + $this->appConfig, + $this->registry, + ); + } + + /** + * @param bool $stored whether an admin set the config key + * @param bool $value the stored value, or the lexicon default when $stored is false + */ + private function arrange(bool $stored, bool $value, bool $appStoreEnabled, bool $subscription): void { + $this->appConfig->method('hasKey') + ->with('core', ConfigLexicon::APPSTORE_LINK_SHOWN) + ->willReturn($stored); + $this->appConfig->method('getValueBool') + ->with('core', ConfigLexicon::APPSTORE_LINK_SHOWN) + ->willReturn($value); + $this->config->method('getSystemValueBool') + ->with('appstoreenabled', true) + ->willReturn($appStoreEnabled); + $this->registry->method('delegateHasValidSubscription') + ->willReturn($subscription); + } + + public static function dataBool(): array { + return [ + 'shown' => [true], + 'hidden' => [false], + ]; + } + + #[DataProvider('dataBool')] + public function testStoredValueWinsOverSubscriptionAndDisabledAppStore(bool $stored): void { + $this->arrange(stored: true, value: $stored, appStoreEnabled: false, subscription: true); + + self::assertSame($stored, $this->visibility->isShownToUsers()); + } + + public function testHiddenWhenAppStoreIsDisabled(): void { + $this->arrange(stored: false, value: true, appStoreEnabled: false, subscription: false); + + self::assertFalse($this->visibility->isShownToUsers()); + } + + public function testHiddenWhenSubscriptionIsAvailable(): void { + $this->arrange(stored: false, value: true, appStoreEnabled: true, subscription: true); + + self::assertFalse($this->visibility->isShownToUsers()); + } + + #[DataProvider('dataBool')] + public function testUnstoredKeyReturnsTheLexiconDefault(bool $default): void { + $this->arrange(stored: false, value: $default, appStoreEnabled: true, subscription: false); + + self::assertSame($default, $this->visibility->isShownToUsers()); + } + + public static function dataLexiconPreset(): array { + return [ + // Existing instances run without a preset and must keep the link. + [Preset::NONE, '1'], + [Preset::FAMILY, '1'], + [Preset::UNIVERSITY, '0'], + ]; + } + + /** + * The lexicon default {@see AppStoreLinkVisibility} falls back to is preset + * dependent. A fresh entry is built per case because {@see Entry::getDefault()} + * memoizes the first default it is asked for. + */ + #[DataProvider('dataLexiconPreset')] + public function testLexiconPresetDefault(Preset $preset, string $expected): void { + self::assertSame($expected, $this->lexiconEntry()->getDefault($preset)); + } + + private function lexiconEntry(): Entry { + foreach ((new ConfigLexicon())->getAppConfigs() as $entry) { + if ($entry->getKey() === ConfigLexicon::APPSTORE_LINK_SHOWN) { + return $entry; + } + } + + self::fail('No lexicon entry for ' . ConfigLexicon::APPSTORE_LINK_SHOWN); + } +} diff --git a/tests/playwright/e2e/core/admin-settings-appstore-link.spec.ts b/tests/playwright/e2e/core/admin-settings-appstore-link.spec.ts new file mode 100644 index 0000000000000..b9845459788df --- /dev/null +++ b/tests/playwright/e2e/core/admin-settings-appstore-link.spec.ts @@ -0,0 +1,43 @@ +/* + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { runOcc } from '@nextcloud/e2e-test-server' +import { expect } from '@playwright/test' +import { test as userTest } from '../../support/fixtures/random-user-session.ts' +import { NavigationHeaderPage } from '../../support/sections/NavigationHeaderPage.ts' + +/** + * Set the app config key that decides whether the app store tile is offered. + * + * @param shown - Whether the tile should be offered + */ +async function setAppStoreLinkShown(shown: boolean): Promise { + await runOcc(['config:app:set', 'core', 'appstore_link_shown', '--value', String(shown), '--type', 'boolean']) +} + +// The `admin-settings-` prefix puts this in the serial project: it changes +// instance-wide config. Both states are set explicitly because the test server +// runs with `appstoreenabled` disabled, which hides the tile when the key is unset. +userTest.describe('core: app store link visibility', () => { + userTest.afterAll(async () => { + await runOcc(['config:app:delete', 'core', 'appstore_link_shown']) + }) + + userTest('offers the "App store" tile only while an admin allows it', async ({ page }) => { + const navigationHeader = new NavigationHeaderPage(page) + const appStoreTile = () => navigationHeader.navigationEntries().filter({ hasText: 'App store' }) + + await setAppStoreLinkShown(true) + await page.goto('/') + await navigationHeader.openMenu() + await expect(appStoreTile()).toBeVisible() + + await setAppStoreLinkShown(false) + await page.goto('/') + await navigationHeader.openMenu() + await expect(navigationHeader.navigationEntries()).not.toHaveCount(0) + await expect(appStoreTile()).toHaveCount(0) + }) +})