fix(whitelist): Recognise every two-factor provider app for guests

README.md

@@ -150,6 +150,13 @@ In addition, the following apps are always whitelisted to ensure minimal functio
 * `weather_status`
 * `user_status`
 * `apporder`
+* `twofactor_totp`
+* `twofactor_webauthn`
+* `twofactor_backupcodes`
+* `twofactor_nextcloud_notification`
+* `twofactor_gateway`
+
+Two-factor authentication has to keep working for guests as well, otherwise an instance that enforces it locks them out. So any enabled app that declares a two-factor provider in its `appinfo/info.xml` is allowed too, even when it is not on the list above and the administrator did not whitelist it. Such an app is still offered in the whitelist picker, but ticking or unticking it there makes no difference. Apps that register their provider from the app bootstrap instead cannot be recognised that way, which is why `twofactor_backupcodes` and `twofactor_gateway` are listed by name.
 
 ### Hide other users
 

lib/AppWhitelist.php

@@ -28,7 +28,16 @@ class AppWhitelist {
 
 	private readonly int $baseUrlLength;
 
-	public const WHITELIST_ALWAYS = 'core,theming,settings,avatar,files,heartbeat,dav,guests,impersonate,accessibility,terms_of_service,dashboard,weather_status,user_status,apporder,twofactor_totp,twofactor_webauthn,twofactor_backupcodes,twofactor_nextcloud_notification';
+	/**
+	 * Apps a guest may always reach, no matter what the administrator
+	 * configured.
+	 *
+	 * The twofactor_* entries are a safety net. Provider apps are also picked
+	 * up at runtime by isTwoFactorProviderApp(), but that only sees providers
+	 * declared in info.xml, and only while info.xml is readable, so the ones
+	 * that were listed here before stay listed.
+	 */
+	public const WHITELIST_ALWAYS = 'core,theming,settings,avatar,files,heartbeat,dav,guests,impersonate,accessibility,terms_of_service,dashboard,weather_status,user_status,apporder,twofactor_totp,twofactor_webauthn,twofactor_backupcodes,twofactor_nextcloud_notification,twofactor_gateway';
 
 	public const DEFAULT_WHITELIST = 'files_trashbin,files_versions,files_sharing,files_texteditor,text,activity,firstrunwizard,photos,notifications,dashboard,user_status,weather_status';
 
@@ -48,7 +57,19 @@ public function isAppWhitelisted(string $appId): bool {
 		$whitelist = $this->config->getAppWhitelist();
 		$alwaysEnabled = explode(',', self::WHITELIST_ALWAYS);
 
-		return in_array($appId, array_merge($whitelist, $alwaysEnabled), true);
+		if (in_array($appId, array_merge($whitelist, $alwaysEnabled), true)) {
+			return true;
+		}
+
+		return $this->isTwoFactorProviderApp($appId);
+	}
+
+	private function isTwoFactorProviderApp(string $appId): bool {
+		if (!in_array($appId, $this->appManager->getEnabledApps(), true)) {
+			return false;
+		}
+
+		return !empty($this->appManager->getAppInfo($appId)['two-factor-providers']);
 	}
 
 	public function isWhitelistEnabled(): bool {

tests/unit/AppWhitelistTest.php

@@ -93,4 +93,72 @@ public function testEmptyAppIdIsNotWhitelisted(): void {
 
 		$this->assertFalse($this->appWhitelist->isAppWhitelisted(''));
 	}
+
+	/**
+	 * Two-factor authentication has to keep working for guests, so an app that
+	 * declares a provider in its info.xml is allowed without the administrator
+	 * having to whitelist it by hand.
+	 */
+	public function testTwoFactorProviderAppIsWhitelisted(): void {
+		$this->config->method('getAppWhitelist')
+			->willReturn(['foo', 'bar']);
+		$this->appManager->method('getEnabledApps')
+			->willReturn(['twofactor_email']);
+		$this->appManager->method('getAppInfo')
+			->willReturn(['two-factor-providers' => ['OCA\TwoFactorEmail\Provider\EmailProvider']]);
+
+		$this->assertTrue($this->appWhitelist->isAppWhitelisted('twofactor_email'));
+	}
+
+	/**
+	 * A provider app the administrator disabled must not be whitelisted, and an
+	 * unknown app id must not reach the info.xml lookup at all, so that guesses
+	 * cannot make every request resolve an app path on disk.
+	 */
+	public function testDisabledTwoFactorProviderAppIsNotWhitelisted(): void {
+		$this->config->method('getAppWhitelist')
+			->willReturn([]);
+		$this->appManager->method('getEnabledApps')
+			->willReturn([]);
+		$this->appManager->expects($this->never())
+			->method('getAppInfo');
+
+		$this->assertFalse($this->appWhitelist->isAppWhitelisted('twofactor_email'));
+	}
+
+	/**
+	 * The runtime lookup only sees providers declared in info.xml, and only
+	 * while info.xml is readable, so the provider apps that are whitelisted by
+	 * name have to stay whitelisted by name. Both stubs are deliberately empty
+	 * here so that only WHITELIST_ALWAYS can satisfy the assertions.
+	 */
+	public function testTwoFactorAppsAreWhitelistedWithoutTheInfoXmlLookup(): void {
+		$this->config->method('getAppWhitelist')
+			->willReturn([]);
+		$this->appManager->method('getEnabledApps')
+			->willReturn([]);
+		$this->appManager->method('getAppInfo')
+			->willReturn(null);
+
+		foreach ([
+			'twofactor_totp',
+			'twofactor_webauthn',
+			'twofactor_backupcodes',
+			'twofactor_nextcloud_notification',
+			'twofactor_gateway',
+		] as $appId) {
+			$this->assertTrue($this->appWhitelist->isAppWhitelisted($appId), $appId);
+		}
+	}
+
+	public function testAppWithoutTwoFactorProviderIsNotWhitelisted(): void {
+		$this->config->method('getAppWhitelist')
+			->willReturn([]);
+		$this->appManager->method('getEnabledApps')
+			->willReturn(['news']);
+		$this->appManager->method('getAppInfo')
+			->willReturn(['two-factor-providers' => []]);
+
+		$this->assertFalse($this->appWhitelist->isAppWhitelisted('news'));
+	}
 }