fix(NavigationManager): Skip invalid default navigation entries

Signed-off-by: provokateurin <kate@provokateurin.de>

apps/theming/lib/Settings/Admin.php

@@ -14,6 +14,7 @@ use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Services\IInitialState;
 use OCP\IConfig;
 use OCP\IL10N;
+use OCP\INavigationManager;
 use OCP\IURLGenerator;
 use OCP\Settings\IDelegatedSettings;
 use OCP\Util;
@@ -28,6 +29,7 @@ class Admin implements IDelegatedSettings {
 		private IInitialState $initialState,
 		private IURLGenerator $urlGenerator,
 		private ImageManager $imageManager,
+		private INavigationManager $navigationManager,
 	) {
 	}
 
@@ -70,7 +72,7 @@ class Admin implements IDelegatedSettings {
 			'docUrlIcons' => $this->urlGenerator->linkToDocs('admin-theming-icons'),
 			'canThemeIcons' => $this->imageManager->shouldReplaceIcons(),
 			'userThemingDisabled' => $this->themingDefaults->isUserThemingDisabled(),
-			'defaultApps' => array_filter(explode(',', $this->config->getSystemValueString('defaultapp', ''))),
+			'defaultApps' => $this->navigationManager->getDefaultEntryIds(),
 		]);
 
 		Util::addScript($this->appName, 'admin-theming');

apps/theming/tests/Settings/AdminTest.php

@@ -13,6 +13,7 @@ use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Services\IInitialState;
 use OCP\IConfig;
 use OCP\IL10N;
+use OCP\INavigationManager;
 use OCP\IURLGenerator;
 use Test\TestCase;
 
@@ -24,6 +25,7 @@ class AdminTest extends TestCase {
 	private IURLGenerator $urlGenerator;
 	private ImageManager $imageManager;
 	private IL10N $l10n;
+	private INavigationManager $navigationManager;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -33,6 +35,7 @@ class AdminTest extends TestCase {
 		$this->initialState = $this->createMock(IInitialState::class);
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->imageManager = $this->createMock(ImageManager::class);
+		$this->navigationManager = $this->createMock(INavigationManager::class);
 
 		$this->admin = new Admin(
 			Application::APP_ID,
@@ -41,7 +44,8 @@ class AdminTest extends TestCase {
 			$this->themingDefaults,
 			$this->initialState,
 			$this->urlGenerator,
-			$this->imageManager
+			$this->imageManager,
+			$this->navigationManager,
 		);
 	}
 

lib/private/NavigationManager.php

@@ -44,8 +44,6 @@ class NavigationManager implements INavigationManager {
 	private $groupManager;
 	/** @var IConfig */
 	private $config;
-	/** The default entry for the current user (cached for the `add` function) */
-	private ?string $defaultEntry;
 	/** User defined app order (cached for the `add` function) */
 	private array $customAppOrder;
 	private LoggerInterface $logger;
@@ -66,8 +64,6 @@ class NavigationManager implements INavigationManager {
 		$this->groupManager = $groupManager;
 		$this->config = $config;
 		$this->logger = $logger;
-
-		$this->defaultEntry = null;
 	}
 
 	/**
@@ -100,13 +96,22 @@ class NavigationManager implements INavigationManager {
 				$entry['app'] = $id;
 			}
 
-			// This is the default app that will always be shown first
-			$entry['default'] = ($entry['id'] ?? false) === $this->defaultEntry;
 			// Set order from user defined app order
 			$entry['order'] = (int)($this->customAppOrder[$id]['order'] ?? $entry['order'] ?? 100);
 		}
 
 		$this->entries[$id] = $entry;
+
+		// Needs to be done after adding the new entry to account for the default entries containing this new entry.
+		$this->updateDefaultEntries();
+	}
+
+	private function updateDefaultEntries() {
+		foreach ($this->entries as $id => $entry) {
+			if ($entry['type'] === 'link') {
+				$this->entries[$id]['default'] = $id === $this->getDefaultEntryIdForUser($this->userSession->getUser(), false);
+			}
+		}
 	}
 
 	/**
@@ -221,8 +226,6 @@ class NavigationManager implements INavigationManager {
 			]);
 		}
 
-		$this->defaultEntry = $this->getDefaultEntryIdForUser($this->userSession->getUser(), false);
-
 		if ($this->userSession->isLoggedIn()) {
 			// Profile
 			$this->add([
@@ -422,8 +425,8 @@ class NavigationManager implements INavigationManager {
 
 	public function getDefaultEntryIdForUser(?IUser $user = null, bool $withFallbacks = true): string {
 		$this->init();
-		$defaultEntryIds = explode(',', $this->config->getSystemValueString('defaultapp', ''));
-		$defaultEntryIds = array_filter($defaultEntryIds);
+		// Disable fallbacks here, as we need to override them with the user defaults if none are configured.
+		$defaultEntryIds = $this->getDefaultEntryIds(false);
 
 		$user ??= $this->userSession->getUser();
 
@@ -461,8 +464,18 @@ class NavigationManager implements INavigationManager {
 		return $withFallbacks ? 'files' : '';
 	}
 
-	public function getDefaultEntryIds(): array {
-		return explode(',', $this->config->getSystemValueString('defaultapp', 'dashboard,files'));
+	public function getDefaultEntryIds(bool $withFallbacks = true): array {
+		$this->init();
+		$storedIds = explode(',', $this->config->getSystemValueString('defaultapp', $withFallbacks ? 'dashboard,files' : ''));
+		$ids = [];
+		$entryIds = array_keys($this->entries);
+		foreach ($storedIds as $id) {
+			if (in_array($id, $entryIds, true)) {
+				$ids[] = $id;
+				break;
+			}
+		}
+		return array_filter($ids);
 	}
 
 	public function setDefaultEntryIds(array $ids): void {

tests/lib/NavigationManagerTest.php

@@ -724,4 +724,61 @@ class NavigationManagerTest extends TestCase {
 
 		$this->assertEquals($expectedApp, $this->navigationManager->getDefaultEntryIdForUser(null, $withFallbacks));
 	}
+
+	public function testDefaultEntryUpdated() {
+		$this->appManager->method('getInstalledApps')->willReturn([]);
+
+		$user = $this->createMock(IUser::class);
+		$user->method('getUID')->willReturn('user1');
+
+		$this->userSession
+			->method('getUser')
+			->willReturn($user);
+
+		$this->config
+			->method('getSystemValueString')
+			->with('defaultapp', $this->anything())
+			->willReturn('app4,app3,app2,app1');
+
+		$this->config
+			->method('getUserValue')
+			->willReturnMap([
+				['user1', 'core', 'defaultapp', '', ''],
+				['user1', 'core', 'apporder', '[]', ''],
+			]);
+
+		$this->navigationManager->add([
+			'id' => 'app1',
+		]);
+
+		$this->assertEquals('app1', $this->navigationManager->getDefaultEntryIdForUser(null, false));
+		$this->assertEquals(true, $this->navigationManager->get('app1')['default']);
+
+		$this->navigationManager->add([
+			'id' => 'app3',
+		]);
+
+		$this->assertEquals('app3', $this->navigationManager->getDefaultEntryIdForUser(null, false));
+		$this->assertEquals(false, $this->navigationManager->get('app1')['default']);
+		$this->assertEquals(true, $this->navigationManager->get('app3')['default']);
+
+		$this->navigationManager->add([
+			'id' => 'app2',
+		]);
+
+		$this->assertEquals('app3', $this->navigationManager->getDefaultEntryIdForUser(null, false));
+		$this->assertEquals(false, $this->navigationManager->get('app1')['default']);
+		$this->assertEquals(false, $this->navigationManager->get('app2')['default']);
+		$this->assertEquals(true, $this->navigationManager->get('app3')['default']);
+
+		$this->navigationManager->add([
+			'id' => 'app4',
+		]);
+
+		$this->assertEquals('app4', $this->navigationManager->getDefaultEntryIdForUser(null, false));
+		$this->assertEquals(false, $this->navigationManager->get('app1')['default']);
+		$this->assertEquals(false, $this->navigationManager->get('app2')['default']);
+		$this->assertEquals(false, $this->navigationManager->get('app3')['default']);
+		$this->assertEquals(true, $this->navigationManager->get('app4')['default']);
+	}
 }