diff --git a/README.md b/README.md index a19fb0b8..a7488793 100644 --- a/README.md +++ b/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 diff --git a/lib/AppWhitelist.php b/lib/AppWhitelist.php index 0b5d812f..05302e90 100644 --- a/lib/AppWhitelist.php +++ b/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 { diff --git a/tests/unit/AppWhitelistTest.php b/tests/unit/AppWhitelistTest.php index d84d03bd..a18bddc4 100644 --- a/tests/unit/AppWhitelistTest.php +++ b/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')); + } }