diff --git a/apps/settings/composer/composer/autoload_classmap.php b/apps/settings/composer/composer/autoload_classmap.php index 488aaee264d2a..27c1496008e73 100644 --- a/apps/settings/composer/composer/autoload_classmap.php +++ b/apps/settings/composer/composer/autoload_classmap.php @@ -78,6 +78,7 @@ 'OCA\\Settings\\Settings\\Personal\\Security\\TwoFactor' => $baseDir . '/../lib/Settings/Personal/Security/TwoFactor.php', 'OCA\\Settings\\Settings\\Personal\\Security\\WebAuthn' => $baseDir . '/../lib/Settings/Personal/Security/WebAuthn.php', 'OCA\\Settings\\Settings\\Personal\\ServerDevNotice' => $baseDir . '/../lib/Settings/Personal/ServerDevNotice.php', + 'OCA\\Settings\\SetupChecks\\AllowedAdminRanges' => $baseDir . '/../lib/SetupChecks/AllowedAdminRanges.php', 'OCA\\Settings\\SetupChecks\\AppDirsWithDifferentOwner' => $baseDir . '/../lib/SetupChecks/AppDirsWithDifferentOwner.php', 'OCA\\Settings\\SetupChecks\\BruteForceThrottler' => $baseDir . '/../lib/SetupChecks/BruteForceThrottler.php', 'OCA\\Settings\\SetupChecks\\CheckServerResponseTrait' => $baseDir . '/../lib/SetupChecks/CheckServerResponseTrait.php', diff --git a/apps/settings/composer/composer/autoload_static.php b/apps/settings/composer/composer/autoload_static.php index ac2e464523986..14e4c36288767 100644 --- a/apps/settings/composer/composer/autoload_static.php +++ b/apps/settings/composer/composer/autoload_static.php @@ -93,6 +93,7 @@ class ComposerStaticInitSettings 'OCA\\Settings\\Settings\\Personal\\Security\\TwoFactor' => __DIR__ . '/..' . '/../lib/Settings/Personal/Security/TwoFactor.php', 'OCA\\Settings\\Settings\\Personal\\Security\\WebAuthn' => __DIR__ . '/..' . '/../lib/Settings/Personal/Security/WebAuthn.php', 'OCA\\Settings\\Settings\\Personal\\ServerDevNotice' => __DIR__ . '/..' . '/../lib/Settings/Personal/ServerDevNotice.php', + 'OCA\\Settings\\SetupChecks\\AllowedAdminRanges' => __DIR__ . '/..' . '/../lib/SetupChecks/AllowedAdminRanges.php', 'OCA\\Settings\\SetupChecks\\AppDirsWithDifferentOwner' => __DIR__ . '/..' . '/../lib/SetupChecks/AppDirsWithDifferentOwner.php', 'OCA\\Settings\\SetupChecks\\BruteForceThrottler' => __DIR__ . '/..' . '/../lib/SetupChecks/BruteForceThrottler.php', 'OCA\\Settings\\SetupChecks\\CheckServerResponseTrait' => __DIR__ . '/..' . '/../lib/SetupChecks/CheckServerResponseTrait.php', diff --git a/apps/settings/composer/composer/installed.php b/apps/settings/composer/composer/installed.php index d2b87e1bdfdff..651d70adcf8ad 100644 --- a/apps/settings/composer/composer/installed.php +++ b/apps/settings/composer/composer/installed.php @@ -3,7 +3,7 @@ 'name' => '__root__', 'pretty_version' => 'dev-master', 'version' => 'dev-master', - 'reference' => '4ff660ca2e0baa02440ba07296ed7e75fa544c0e', + 'reference' => '071fe73d0a28f44c6e24cc87fbd00e54a3b92b57', 'type' => 'library', 'install_path' => __DIR__ . '/../', 'aliases' => array(), @@ -13,7 +13,7 @@ '__root__' => array( 'pretty_version' => 'dev-master', 'version' => 'dev-master', - 'reference' => '4ff660ca2e0baa02440ba07296ed7e75fa544c0e', + 'reference' => '071fe73d0a28f44c6e24cc87fbd00e54a3b92b57', 'type' => 'library', 'install_path' => __DIR__ . '/../', 'aliases' => array(), diff --git a/apps/settings/lib/AppInfo/Application.php b/apps/settings/lib/AppInfo/Application.php index f62555f0cdb18..80420cb3335cf 100644 --- a/apps/settings/lib/AppInfo/Application.php +++ b/apps/settings/lib/AppInfo/Application.php @@ -22,6 +22,7 @@ use OCA\Settings\Search\AppSearch; use OCA\Settings\Search\SectionSearch; use OCA\Settings\Search\UserSearch; +use OCA\Settings\SetupChecks\AllowedAdminRanges; use OCA\Settings\SetupChecks\AppDirsWithDifferentOwner; use OCA\Settings\SetupChecks\BruteForceThrottler; use OCA\Settings\SetupChecks\CheckUserCertificates; @@ -154,6 +155,7 @@ public function register(IRegistrationContext $context): void { Util::getDefaultEmailAddress('no-reply') ); }); + $context->registerSetupCheck(AllowedAdminRanges::class); $context->registerSetupCheck(AppDirsWithDifferentOwner::class); $context->registerSetupCheck(BruteForceThrottler::class); $context->registerSetupCheck(CheckUserCertificates::class); diff --git a/apps/settings/lib/SetupChecks/AllowedAdminRanges.php b/apps/settings/lib/SetupChecks/AllowedAdminRanges.php new file mode 100644 index 0000000000000..87e11b06be7df --- /dev/null +++ b/apps/settings/lib/SetupChecks/AllowedAdminRanges.php @@ -0,0 +1,63 @@ +l10n->t('Allowed admin IP ranges'); + } + + public function run(): SetupResult { + $allowedAdminRanges = $this->config->getSystemValue(RemoteAddress::SETTING_NAME, false); + if ( + $allowedAdminRanges === false + || (is_array($allowedAdminRanges) && empty($allowedAdminRanges)) + ) { + return SetupResult::success($this->l10n->t('Admin IP filtering isn’t applied.')); + } + + if (!is_array($allowedAdminRanges)) { + return SetupResult::error( + $this->l10n->t( + 'Configuration key "%1$s" expects an array (%2$s found). Admin IP range validation will not be applied.', + [RemoteAddress::SETTING_NAME, gettype($allowedAdminRanges)], + ) + ); + } + + $invalidRanges = array_filter($allowedAdminRanges, static fn (mixed $range): bool => !is_string($range) || !Range::isValid($range)); + if (!empty($invalidRanges)) { + return SetupResult::warning( + $this->l10n->t( + 'Configuration key "%1$s" contains invalid IP range(s): "%2$s"', + [RemoteAddress::SETTING_NAME, implode('", "', $invalidRanges)], + ), + ); + } + + return SetupResult::success($this->l10n->t('Admin IP filtering is correctly configured.')); + } +} diff --git a/config/config.sample.php b/config/config.sample.php index 67110a1844aa6..9840fcfc97caa 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -2207,6 +2207,16 @@ */ 'forwarded_for_headers' => ['HTTP_X_FORWARDED', 'HTTP_FORWARDED_FOR'], +/** + * List of trusted IP ranges for admin actions + * + * If this list is non-empty, all admin actions must be triggered from + * IP addresses inside theses ranges. + * + * Defaults to an empty array. + */ +'allowed_admin_ranges' => ['192.0.2.42/32', '233.252.0.0/24', '2001:db8::13:37/64'], + /** * max file size for animating gifs on public-sharing-site. * If the gif is bigger, it'll show a static preview diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index fbc61bcb1a695..aa6e50eab20ec 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -644,6 +644,10 @@ 'OCP\\Security\\IRemoteHostValidator' => $baseDir . '/lib/public/Security/IRemoteHostValidator.php', 'OCP\\Security\\ISecureRandom' => $baseDir . '/lib/public/Security/ISecureRandom.php', 'OCP\\Security\\ITrustedDomainHelper' => $baseDir . '/lib/public/Security/ITrustedDomainHelper.php', + 'OCP\\Security\\Ip\\IAddress' => $baseDir . '/lib/public/Security/Ip/IAddress.php', + 'OCP\\Security\\Ip\\IFactory' => $baseDir . '/lib/public/Security/Ip/IFactory.php', + 'OCP\\Security\\Ip\\IRange' => $baseDir . '/lib/public/Security/Ip/IRange.php', + 'OCP\\Security\\Ip\\IRemoteAddress' => $baseDir . '/lib/public/Security/Ip/IRemoteAddress.php', 'OCP\\Security\\RateLimiting\\ILimiter' => $baseDir . '/lib/public/Security/RateLimiting/ILimiter.php', 'OCP\\Security\\RateLimiting\\IRateLimitExceededException' => $baseDir . '/lib/public/Security/RateLimiting/IRateLimitExceededException.php', 'OCP\\Security\\VerificationToken\\IVerificationToken' => $baseDir . '/lib/public/Security/VerificationToken/IVerificationToken.php', @@ -896,6 +900,7 @@ 'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CSPMiddleware' => $baseDir . '/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php', + 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AdminIpNotAllowedException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\ExAppRequiredException' => $baseDir . '/lib/private/AppFramework/Middleware/Security/Exceptions/ExAppRequiredException.php', @@ -1807,6 +1812,10 @@ 'OC\\Security\\IdentityProof\\Key' => $baseDir . '/lib/private/Security/IdentityProof/Key.php', 'OC\\Security\\IdentityProof\\Manager' => $baseDir . '/lib/private/Security/IdentityProof/Manager.php', 'OC\\Security\\IdentityProof\\Signer' => $baseDir . '/lib/private/Security/IdentityProof/Signer.php', + 'OC\\Security\\Ip\\Address' => $baseDir . '/lib/private/Security/Ip/Address.php', + 'OC\\Security\\Ip\\Factory' => $baseDir . '/lib/private/Security/Ip/Factory.php', + 'OC\\Security\\Ip\\Range' => $baseDir . '/lib/private/Security/Ip/Range.php', + 'OC\\Security\\Ip\\RemoteAddress' => $baseDir . '/lib/private/Security/Ip/RemoteAddress.php', 'OC\\Security\\Normalizer\\IpAddress' => $baseDir . '/lib/private/Security/Normalizer/IpAddress.php', 'OC\\Security\\RateLimiting\\Backend\\DatabaseBackend' => $baseDir . '/lib/private/Security/RateLimiting/Backend/DatabaseBackend.php', 'OC\\Security\\RateLimiting\\Backend\\IBackend' => $baseDir . '/lib/private/Security/RateLimiting/Backend/IBackend.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index dd25d62f7e366..5c0535b436275 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -677,6 +677,10 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OCP\\Security\\IRemoteHostValidator' => __DIR__ . '/../../..' . '/lib/public/Security/IRemoteHostValidator.php', 'OCP\\Security\\ISecureRandom' => __DIR__ . '/../../..' . '/lib/public/Security/ISecureRandom.php', 'OCP\\Security\\ITrustedDomainHelper' => __DIR__ . '/../../..' . '/lib/public/Security/ITrustedDomainHelper.php', + 'OCP\\Security\\Ip\\IAddress' => __DIR__ . '/../../..' . '/lib/public/Security/Ip/IAddress.php', + 'OCP\\Security\\Ip\\IFactory' => __DIR__ . '/../../..' . '/lib/public/Security/Ip/IFactory.php', + 'OCP\\Security\\Ip\\IRange' => __DIR__ . '/../../..' . '/lib/public/Security/Ip/IRange.php', + 'OCP\\Security\\Ip\\IRemoteAddress' => __DIR__ . '/../../..' . '/lib/public/Security/Ip/IRemoteAddress.php', 'OCP\\Security\\RateLimiting\\ILimiter' => __DIR__ . '/../../..' . '/lib/public/Security/RateLimiting/ILimiter.php', 'OCP\\Security\\RateLimiting\\IRateLimitExceededException' => __DIR__ . '/../../..' . '/lib/public/Security/RateLimiting/IRateLimitExceededException.php', 'OCP\\Security\\VerificationToken\\IVerificationToken' => __DIR__ . '/../../..' . '/lib/public/Security/VerificationToken/IVerificationToken.php', @@ -929,6 +933,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\AppFramework\\Middleware\\Security\\BruteForceMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CORSMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php', 'OC\\AppFramework\\Middleware\\Security\\CSPMiddleware' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/CSPMiddleware.php', + 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AdminIpNotAllowedException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\AppNotEnabledException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/AppNotEnabledException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\CrossSiteRequestForgeryException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/CrossSiteRequestForgeryException.php', 'OC\\AppFramework\\Middleware\\Security\\Exceptions\\ExAppRequiredException' => __DIR__ . '/../../..' . '/lib/private/AppFramework/Middleware/Security/Exceptions/ExAppRequiredException.php', @@ -1840,6 +1845,10 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Security\\IdentityProof\\Key' => __DIR__ . '/../../..' . '/lib/private/Security/IdentityProof/Key.php', 'OC\\Security\\IdentityProof\\Manager' => __DIR__ . '/../../..' . '/lib/private/Security/IdentityProof/Manager.php', 'OC\\Security\\IdentityProof\\Signer' => __DIR__ . '/../../..' . '/lib/private/Security/IdentityProof/Signer.php', + 'OC\\Security\\Ip\\Address' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/Address.php', + 'OC\\Security\\Ip\\Factory' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/Factory.php', + 'OC\\Security\\Ip\\Range' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/Range.php', + 'OC\\Security\\Ip\\RemoteAddress' => __DIR__ . '/../../..' . '/lib/private/Security/Ip/RemoteAddress.php', 'OC\\Security\\Normalizer\\IpAddress' => __DIR__ . '/../../..' . '/lib/private/Security/Normalizer/IpAddress.php', 'OC\\Security\\RateLimiting\\Backend\\DatabaseBackend' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Backend/DatabaseBackend.php', 'OC\\Security\\RateLimiting\\Backend\\IBackend' => __DIR__ . '/../../..' . '/lib/private/Security/RateLimiting/Backend/IBackend.php', diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index c25b6994b4fd6..81365900d9b13 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -46,6 +46,7 @@ use OCP\IURLGenerator; use OCP\IUserSession; use OCP\Security\Bruteforce\IThrottler; +use OCP\Security\Ip\IRemoteAddress; use Psr\Container\ContainerInterface; use Psr\Log\LoggerInterface; @@ -232,7 +233,8 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta $server->getAppManager(), $server->getL10N('lib'), $c->get(AuthorizedGroupMapper::class), - $server->get(IUserSession::class) + $server->get(IUserSession::class), + $c->get(IRemoteAddress::class), ); $dispatcher->registerMiddleware($securityMiddleware); $dispatcher->registerMiddleware( diff --git a/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php b/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php new file mode 100644 index 0000000000000..36eb8f18928dc --- /dev/null +++ b/lib/private/AppFramework/Middleware/Security/Exceptions/AdminIpNotAllowedException.php @@ -0,0 +1,23 @@ +navigationManager = $navigationManager; - $this->request = $request; - $this->reflector = $reflector; - $this->appName = $appName; - $this->urlGenerator = $urlGenerator; - $this->logger = $logger; - $this->isLoggedIn = $isLoggedIn; - $this->isAdminUser = $isAdminUser; - $this->isSubAdmin = $isSubAdmin; - $this->appManager = $appManager; - $this->l10n = $l10n; - $this->groupAuthorizationMapper = $mapper; - $this->userSession = $userSession; } /** @@ -170,6 +134,9 @@ public function beforeController($controller, $methodName) { if (!$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin, a sub admin or gotten special right to access this setting')); } + if (!$this->remoteAddress->allowsAdminActions()) { + throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn’t allow you to perform admin actions')); + } } if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) && !$this->isSubAdmin @@ -183,6 +150,16 @@ public function beforeController($controller, $methodName) { && !$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin')); } + if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) + && !$this->remoteAddress->allowsAdminActions()) { + throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn’t allow you to perform admin actions')); + } + if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) + && !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) + && !$this->remoteAddress->allowsAdminActions()) { + throw new AdminIpNotAllowedException($this->l10n->t('Your current IP address doesn’t allow you to perform admin actions')); + } + } // Check for strict cookie requirement diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 0ab64907c8bb1..092c56fe00a72 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -19,6 +19,7 @@ use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; +use OCP\Security\Ip\IRemoteAddress; use Psr\Log\LoggerInterface; use function is_string; @@ -41,11 +42,6 @@ class Manager extends PublicEmitter implements IGroupManager { /** @var GroupInterface[] */ private $backends = []; - /** @var \OC\User\Manager */ - private $userManager; - private IEventDispatcher $dispatcher; - private LoggerInterface $logger; - /** @var array */ private $cachedGroups = []; @@ -59,13 +55,13 @@ class Manager extends PublicEmitter implements IGroupManager { private const MAX_GROUP_LENGTH = 255; - public function __construct(\OC\User\Manager $userManager, - IEventDispatcher $dispatcher, - LoggerInterface $logger, - ICacheFactory $cacheFactory) { - $this->userManager = $userManager; - $this->dispatcher = $dispatcher; - $this->logger = $logger; + public function __construct( + private \OC\User\Manager $userManager, + private IEventDispatcher $dispatcher, + private LoggerInterface $logger, + ICacheFactory $cacheFactory, + private IRemoteAddress $remoteAddress, + ) { $this->displayNameCache = new DisplayNameCache($cacheFactory, $this); $this->listen('\OC\Group', 'postDelete', function (IGroup $group): void { @@ -325,6 +321,10 @@ public function getUserIdGroups(string $uid): array { * @return bool if admin */ public function isAdmin($userId) { + if (!$this->remoteAddress->allowsAdminActions()) { + return false; + } + foreach ($this->backends as $backend) { if (is_string($userId) && $backend->implementsActions(Backend::IS_ADMIN) && $backend->isAdmin($userId)) { return true; diff --git a/lib/private/Security/Ip/Address.php b/lib/private/Security/Ip/Address.php new file mode 100644 index 0000000000000..b4f12a7e295fe --- /dev/null +++ b/lib/private/Security/Ip/Address.php @@ -0,0 +1,48 @@ +ip = $ip; + } + + public static function isValid(string $ip): bool { + return Factory::parseAddressString($ip) !== null; + } + + public function matches(IRange... $ranges): bool { + foreach($ranges as $range) { + if ($range->contains($this)) { + return true; + } + } + + return false; + } + + public function __toString(): string { + return $this->ip->toString(); + } +} diff --git a/lib/private/Security/Ip/Factory.php b/lib/private/Security/Ip/Factory.php new file mode 100644 index 0000000000000..1eedcf27a0999 --- /dev/null +++ b/lib/private/Security/Ip/Factory.php @@ -0,0 +1,23 @@ +range = $range; + } + + public static function isValid(string $range): bool { + return Factory::parseRangeString($range) !== null; + } + + public function contains(IAddress $address): bool { + return $this->range->contains(Factory::parseAddressString((string) $address)); + } + + public function __toString(): string { + return $this->range->toString(); + } +} diff --git a/lib/private/Security/Ip/RemoteAddress.php b/lib/private/Security/Ip/RemoteAddress.php new file mode 100644 index 0000000000000..54cdb96132ace --- /dev/null +++ b/lib/private/Security/Ip/RemoteAddress.php @@ -0,0 +1,71 @@ +getRemoteAddress(); + $this->ip = $remoteAddress === '' + ? null + : new Address($remoteAddress); + } + + public static function isValid(string $ip): bool { + return Address::isValid($ip); + } + + public function matches(IRange... $ranges): bool { + return $this->ip === null + ? true + : $this->ip->matches(... $ranges); + } + + public function allowsAdminActions(): bool { + if ($this->ip === null) { + return true; + } + + $allowedAdminRanges = $this->config->getSystemValue(self::SETTING_NAME, false); + + // Don't apply restrictions on empty or invalid configuration + if ( + $allowedAdminRanges === false + || !is_array($allowedAdminRanges) + || empty($allowedAdminRanges) + ) { + return true; + } + + foreach ($allowedAdminRanges as $allowedAdminRange) { + if ((new Range($allowedAdminRange))->contains($this->ip)) { + return true; + } + } + + return false; + } + + public function __toString(): string { + return (string) $this->ip; + } +} diff --git a/lib/private/Server.php b/lib/private/Server.php index 4d549918a8fb1..cfbb6dc317fa5 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -100,6 +100,7 @@ use OC\Security\CSRF\CsrfTokenManager; use OC\Security\CSRF\TokenStorage\SessionStorage; use OC\Security\Hasher; +use OC\Security\Ip\RemoteAddress; use OC\Security\RateLimiting\Limiter; use OC\Security\SecureRandom; use OC\Security\TrustedDomainHelper; @@ -213,6 +214,7 @@ use OCP\Security\ICredentialsManager; use OCP\Security\ICrypto; use OCP\Security\IHasher; +use OCP\Security\Ip\IRemoteAddress; use OCP\Security\ISecureRandom; use OCP\Security\ITrustedDomainHelper; use OCP\Security\RateLimiting\ILimiter; @@ -464,7 +466,8 @@ public function __construct($webRoot, \OC\Config $config) { $this->get(IUserManager::class), $this->get(IEventDispatcher::class), $this->get(LoggerInterface::class), - $this->get(ICacheFactory::class) + $this->get(ICacheFactory::class), + $this->get(IRemoteAddress::class), ); return $groupManager; }); @@ -1403,6 +1406,10 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerAlias(\OCP\TaskProcessing\IManager::class, \OC\TaskProcessing\Manager::class); + $this->registerAlias(IRemoteAddress::class, RemoteAddress::class); + + $this->registerAlias(\OCP\Security\Ip\IFactory::class, \OC\Security\Ip\Factory::class); + $this->connectDispatcher(); } diff --git a/lib/public/Security/Ip/IAddress.php b/lib/public/Security/Ip/IAddress.php new file mode 100644 index 0000000000000..242418962fc6c --- /dev/null +++ b/lib/public/Security/Ip/IAddress.php @@ -0,0 +1,35 @@ +appManager->expects($this->any()) ->method('isEnabledForUser') ->willReturn($isAppEnabledForUser); + $remoteIpAddress = $this->createMock(IRemoteAddress::class); + $remoteIpAddress->method('allowsAdminActions')->willReturn(true); return new SecurityMiddleware( $this->request, @@ -104,7 +107,8 @@ private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isSubA $this->appManager, $this->l10n, $this->authorizedGroupMapper, - $this->userSession + $this->userSession, + $remoteIpAddress ); } diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 81fd77cc78ad3..4f42e10537d58 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -16,6 +16,7 @@ use OCP\GroupInterface; use OCP\ICacheFactory; use OCP\IUser; +use OCP\Security\Ip\IRemoteAddress; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; use Test\TestCase; @@ -32,6 +33,8 @@ class ManagerTest extends TestCase { protected $logger; /** @var ICacheFactory|MockObject */ private $cache; + /** @var IRemoteAddress|MockObject */ + private $remoteIpAddress; protected function setUp(): void { parent::setUp(); @@ -40,6 +43,9 @@ protected function setUp(): void { $this->dispatcher = $this->createMock(IEventDispatcher::class); $this->logger = $this->createMock(LoggerInterface::class); $this->cache = $this->createMock(ICacheFactory::class); + + $this->remoteIpAddress = $this->createMock(IRemoteAddress::class); + $this->remoteIpAddress->method('allowsAdminActions')->willReturn(true); } private function getTestUser($userId) { @@ -103,7 +109,7 @@ public function testGet() { ->with('group1') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $group = $manager->get('group1'); @@ -112,7 +118,7 @@ public function testGet() { } public function testGetNoBackend() { - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $this->assertNull($manager->get('group1')); } @@ -127,7 +133,7 @@ public function testGetNotExists() { ->with('group1') ->willReturn(false); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $this->assertNull($manager->get('group1')); @@ -137,7 +143,7 @@ public function testGetDeleted() { $backend = new \Test\Util\Group\Dummy(); $backend->createGroup('group1'); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $group = $manager->get('group1'); @@ -164,7 +170,7 @@ public function testGetMultipleBackends() { ->with('group1') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -190,7 +196,7 @@ public function testCreate() { return true; }); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $group = $manager->createGroup('group1'); @@ -219,7 +225,7 @@ public function testCreateFailure() { ->method('getGroupDetails') ->willReturn([]); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $group = $manager->createGroup('group1'); @@ -243,7 +249,7 @@ public function testCreateTooLong() { ->with($groupName) ->willReturn(false); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $this->expectException(\Exception::class); @@ -260,7 +266,7 @@ public function testCreateExists() { $backend->expects($this->never()) ->method('createGroup'); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $group = $manager->createGroup('group1'); @@ -281,7 +287,7 @@ public function testSearch() { ->with('group1') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $groups = $manager->search('1'); @@ -315,7 +321,7 @@ public function testSearchMultipleBackends() { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -352,7 +358,7 @@ public function testSearchMultipleBackendsLimitAndOffset() { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -381,7 +387,7 @@ public function testSearchResultExistsButGroupDoesNot() { /** @var \OC\User\Manager $userManager */ $userManager = $this->createMock(Manager::class); - $manager = new \OC\Group\Manager($userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $groups = $manager->search('1'); @@ -402,7 +408,7 @@ public function testGetUserGroups() { ->with('group1') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $groups = $manager->getUserGroups($this->getTestUser('user1')); @@ -420,7 +426,7 @@ public function testGetUserGroupIds() { ->with('myUID') ->willReturn(['123', 'abc']); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); /** @var \OC\User\User|\PHPUnit\Framework\MockObject\MockObject $user */ @@ -450,7 +456,7 @@ public function testGetUserGroupsWithDeletedGroup() { ->with('group1') ->willReturn(false); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); /** @var \OC\User\User|\PHPUnit\Framework\MockObject\MockObject $user */ @@ -476,7 +482,7 @@ public function testInGroup() { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $this->assertTrue($manager->isInGroup('user1', 'group1')); @@ -495,7 +501,7 @@ public function testIsAdmin() { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $this->assertTrue($manager->isAdmin('user1')); @@ -514,7 +520,7 @@ public function testNotAdmin() { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $this->assertFalse($manager->isAdmin('user1')); @@ -545,7 +551,7 @@ public function testGetUserGroupsMultipleBackends() { ->method('groupExists') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -604,7 +610,7 @@ public function testDisplayNamesInGroupWithOneUserBackend() { } }); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3'); @@ -664,7 +670,7 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified() { } }); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3', 1); @@ -728,7 +734,7 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpeci } }); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3', 1, 1); @@ -757,7 +763,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { $this->userManager->expects($this->never())->method('get'); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', ''); @@ -785,7 +791,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitS $this->userManager->expects($this->never())->method('get'); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1); @@ -813,7 +819,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitA $this->userManager->expects($this->never())->method('get'); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1, 1); @@ -841,7 +847,7 @@ public function testGetUserGroupsWithAddUser() { ->with('group1') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); // prime cache @@ -884,7 +890,7 @@ public function testGetUserGroupsWithRemoveUser() { ->method('removeFromGroup') ->willReturn(true); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); // prime cache @@ -914,7 +920,7 @@ public function testGetUserIdGroups() { ->with('user1') ->willReturn(null); - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); $groups = $manager->getUserIdGroups('user1'); @@ -939,8 +945,7 @@ public function testGroupDisplayName() { ['group1', ['gid' => 'group1', 'displayName' => 'Group One']], ['group2', ['gid' => 'group2']], ]); - - $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache); + $manager = new \OC\Group\Manager($this->userManager, $this->dispatcher, $this->logger, $this->cache, $this->remoteIpAddress); $manager->addBackend($backend); // group with display name diff --git a/tests/lib/Security/Ip/RemoteAddressTest.php b/tests/lib/Security/Ip/RemoteAddressTest.php new file mode 100644 index 0000000000000..22f38f6235612 --- /dev/null +++ b/tests/lib/Security/Ip/RemoteAddressTest.php @@ -0,0 +1,77 @@ +config = $this->createMock(IConfig::class); + $this->request = $this->createMock(IRequest::class); + } + + /** + * @param mixed $allowedRanges + * @dataProvider dataProvider + */ + public function testAllowedIps(string $remoteIp, $allowedRanges, bool $expected): void { + $this->request + ->method('getRemoteAddress') + ->willReturn($remoteIp); + $this->config + ->method('getSystemValue') + ->with('allowed_admin_ranges', false) + ->willReturn($allowedRanges); + + $remoteAddress = new RemoteAddress($this->config, $this->request); + + $this->assertEquals($expected, $remoteAddress->allowsAdminActions()); + } + + /** + * @return array + */ + public function dataProvider(): array { + return [ + // No IP (ie. CLI) + ['', ['192.168.1.2/24'], true], + ['', ['fe80/8'], true], + // No configuration + ['1.2.3.4', false, true], + ['1234:4567:8910::', false, true], + // Empty configuration + ['1.2.3.4', [], true], + ['1234:4567:8910::', [], true], + // Invalid configuration + ['1.2.3.4', 'hello', true], + ['1234:4567:8910::', 'world', true], + // Mixed configuration + ['192.168.1.5', ['1.2.3.*', '1234::/8'], false], + ['::1', ['127.0.0.1', '1234::/8'], false], + ['192.168.1.5', ['192.168.1.0/24', '1234::/8'], true], + // Allowed IP + ['1.2.3.4', ['1.2.3.*'], true], + ['fc00:1:2:3::1', ['fc00::/7'], true], + ['1.2.3.4', ['192.168.1.2/24', '1.2.3.0/24'], true], + ['1234:4567:8910::1', ['fe80::/8','1234:4567::/16'], true], + // Blocked IP + ['192.168.1.5', ['1.2.3.*'], false], + ['9234:4567:8910::', ['1234:4567::1'], false], + ['192.168.2.1', ['192.168.1.2/24', '1.2.3.0/24'], false], + ['9234:4567:8910::', ['fe80::/8','1234:4567::/16'], false], + ]; + } +}