Merge pull request #46473 from nextcloud/feat/restrict_admin_to_ips

feat(security): restrict admin actions to IP ranges

3 files changed, 118 insertions, 32 deletions

diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
index bda71c4e8ed..fb457579fac 100644
--- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
+++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php
@@ -32,6 +32,7 @@ use OCP\IRequestId;
 use OCP\ISession;
 use OCP\IURLGenerator;
 use OCP\IUserSession;
+use OCP\Security\Ip\IRemoteAddress;
 use Psr\Log\LoggerInterface;
 use Test\AppFramework\Middleware\Security\Mock\NormalController;
 use Test\AppFramework\Middleware\Security\Mock\OCSController;
@@ -90,6 +91,8 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 		$this->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 @@ class SecurityMiddlewareTest extends \Test\TestCase {
 			$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 81fd77cc78a..4f42e10537d 100644
--- a/tests/lib/Group/ManagerTest.php
+++ b/tests/lib/Group/ManagerTest.php
@@ -16,6 +16,7 @@ use OCP\Group\Backend\ISearchableGroupBackend;
 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 @@ class ManagerTest extends TestCase {
 		$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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 	}
 
 	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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 		$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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 				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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 		$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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 		/** @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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 				}
 			});
 
-		$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 @@ class ManagerTest extends TestCase {
 				}
 			});
 
-		$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 @@ class ManagerTest extends TestCase {
 				}
 			});
 
-		$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 @@ class ManagerTest extends TestCase {
 
 		$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 @@ class ManagerTest extends TestCase {
 
 		$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 @@ class ManagerTest extends TestCase {
 
 		$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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 			->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 @@ class ManagerTest extends TestCase {
 				['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 00000000000..22f38f62356
--- /dev/null
+++ b/tests/lib/Security/Ip/RemoteAddressTest.php
@@ -0,0 +1,77 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace Test\Security\Ip;
+
+use OC\Security\Ip\RemoteAddress;
+use OCP\IConfig;
+use OCP\IRequest;
+
+class RemoteAddressTest extends \Test\TestCase {
+	private IConfig $config;
+	private IRequest $request;
+
+	protected function setUp(): void {
+		parent::setUp();
+		$this->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<string, mixed, bool>
+	 */
+	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],
+		];
+	}
+}