Skip to content

Commit d8d3c7e

Browse files
Altahrimnfebe
Altahrim
authored and
nfebe
committed
feat(systemtags): add setting to block non admin to create system tags
Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
1 parent d4ce307 commit d8d3c7e

File tree

8 files changed

+166
-6
lines changed

8 files changed

+166
-6
lines changed

‎apps/dav/lib/SystemTag/SystemTagPlugin.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use OCP\SystemTag\ISystemTagManager;
1919
use OCP\SystemTag\ISystemTagObjectMapper;
2020
use OCP\SystemTag\TagAlreadyExistsException;
21+
use OCP\SystemTag\TagCreationForbiddenException;
2122
use OCP\Util;
2223
use Sabre\DAV\Exception\BadRequest;
2324
use Sabre\DAV\Exception\Conflict;
@@ -189,6 +190,8 @@ private function createTag($data, $contentType = 'application/json') {
189190
return $tag;
190191
} catch (TagAlreadyExistsException $e) {
191192
throw new Conflict('Tag already exists', 0, $e);
193+
} catch (TagCreationForbiddenException $e) {
194+
throw new Forbidden('You don’t have right to create tags', 0, $e);
192195
}
193196
}
194197

@@ -376,7 +379,7 @@ public function handleUpdateProperties($path, PropPatch $propPatch) {
376379
if (!$node instanceof SystemTagNode && !$node instanceof SystemTagObjectType) {
377380
return;
378381
}
379-
382+
380383
$propPatch->handle([self::OBJECTIDS_PROPERTYNAME], function ($props) use ($node) {
381384
if (!$node instanceof SystemTagObjectType) {
382385
return false;
@@ -394,7 +397,7 @@ public function handleUpdateProperties($path, PropPatch $propPatch) {
394397
if (count($objectTypes) !== 1 || $objectTypes[0] !== $node->getName()) {
395398
throw new BadRequest('Invalid object-ids property. All object types must be of the same type: ' . $node->getName());
396399
}
397-
400+
398401
$this->tagMapper->setObjectIdsForTag($node->getSystemTag()->getId(), $node->getName(), array_keys($objects));
399402
}
400403

‎lib/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -790,6 +790,7 @@
790790
'OCP\\SystemTag\\MapperEvent' => $baseDir . '/lib/public/SystemTag/MapperEvent.php',
791791
'OCP\\SystemTag\\SystemTagsEntityEvent' => $baseDir . '/lib/public/SystemTag/SystemTagsEntityEvent.php',
792792
'OCP\\SystemTag\\TagAlreadyExistsException' => $baseDir . '/lib/public/SystemTag/TagAlreadyExistsException.php',
793+
'OCP\\SystemTag\\TagCreationForbiddenException' => $baseDir . '/lib/public/SystemTag/TagCreationForbiddenException.php',
793794
'OCP\\SystemTag\\TagNotFoundException' => $baseDir . '/lib/public/SystemTag/TagNotFoundException.php',
794795
'OCP\\Talk\\Exceptions\\NoBackendException' => $baseDir . '/lib/public/Talk/Exceptions/NoBackendException.php',
795796
'OCP\\Talk\\IBroker' => $baseDir . '/lib/public/Talk/IBroker.php',

‎lib/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -839,6 +839,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
839839
'OCP\\SystemTag\\MapperEvent' => __DIR__ . '/../../..' . '/lib/public/SystemTag/MapperEvent.php',
840840
'OCP\\SystemTag\\SystemTagsEntityEvent' => __DIR__ . '/../../..' . '/lib/public/SystemTag/SystemTagsEntityEvent.php',
841841
'OCP\\SystemTag\\TagAlreadyExistsException' => __DIR__ . '/../../..' . '/lib/public/SystemTag/TagAlreadyExistsException.php',
842+
'OCP\\SystemTag\\TagCreationForbiddenException' => __DIR__ . '/../../..' . '/lib/public/SystemTag/TagCreationForbiddenException.php',
842843
'OCP\\SystemTag\\TagNotFoundException' => __DIR__ . '/../../..' . '/lib/public/SystemTag/TagNotFoundException.php',
843844
'OCP\\Talk\\Exceptions\\NoBackendException' => __DIR__ . '/../../..' . '/lib/public/Talk/Exceptions/NoBackendException.php',
844845
'OCP\\Talk\\IBroker' => __DIR__ . '/../../..' . '/lib/public/Talk/IBroker.php',

‎lib/private/SystemTag/ManagerFactory.php‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,11 @@
99
namespace OC\SystemTag;
1010

1111
use OCP\EventDispatcher\IEventDispatcher;
12+
use OCP\IAppConfig;
13+
use OCP\IDBConnection;
14+
use OCP\IGroupManager;
1215
use OCP\IServerContainer;
16+
use OCP\IUserSession;
1317
use OCP\SystemTag\ISystemTagManager;
1418
use OCP\SystemTag\ISystemTagManagerFactory;
1519
use OCP\SystemTag\ISystemTagObjectMapper;
@@ -36,9 +40,11 @@ public function __construct(
3640
*/
3741
public function getManager(): ISystemTagManager {
3842
return new SystemTagManager(
39-
$this->serverContainer->getDatabaseConnection(),
40-
$this->serverContainer->getGroupManager(),
43+
$this->serverContainer->get(IDBConnection::class),
44+
$this->serverContainer->get(IGroupManager::class),
4145
$this->serverContainer->get(IEventDispatcher::class),
46+
$this->serverContainer->get(IUserSession::class),
47+
$this->serverContainer->get(IAppConfig::class),
4248
);
4349
}
4450

@@ -50,7 +56,7 @@ public function getManager(): ISystemTagManager {
5056
*/
5157
public function getObjectMapper(): ISystemTagObjectMapper {
5258
return new SystemTagObjectMapper(
53-
$this->serverContainer->getDatabaseConnection(),
59+
$this->serverContainer->get(IDBConnection::class),
5460
$this->getManager(),
5561
$this->serverContainer->get(IEventDispatcher::class),
5662
);

‎lib/private/SystemTag/SystemTagManager.php‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,16 @@
1111
use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
1212
use OCP\DB\QueryBuilder\IQueryBuilder;
1313
use OCP\EventDispatcher\IEventDispatcher;
14+
use OCP\IAppConfig;
1415
use OCP\IDBConnection;
1516
use OCP\IGroupManager;
1617
use OCP\IUser;
18+
use OCP\IUserSession;
1719
use OCP\SystemTag\ISystemTag;
1820
use OCP\SystemTag\ISystemTagManager;
1921
use OCP\SystemTag\ManagerEvent;
2022
use OCP\SystemTag\TagAlreadyExistsException;
23+
use OCP\SystemTag\TagCreationForbiddenException;
2124
use OCP\SystemTag\TagNotFoundException;
2225

2326
/**
@@ -36,6 +39,8 @@ public function __construct(
3639
protected IDBConnection $connection,
3740
protected IGroupManager $groupManager,
3841
protected IEventDispatcher $dispatcher,
42+
private IUserSession $userSession,
43+
private IAppConfig $appConfig,
3944
) {
4045
$query = $this->connection->getQueryBuilder();
4146
$this->selectTagQuery = $query->select('*')
@@ -145,6 +150,10 @@ public function getTag(string $tagName, bool $userVisible, bool $userAssignable)
145150
}
146151

147152
public function createTag(string $tagName, bool $userVisible, bool $userAssignable): ISystemTag {
153+
$user = $this->userSession->getUser();
154+
if (!$this->canUserCreateTag($user)) {
155+
throw new TagCreationForbiddenException('Tag creation forbidden');
156+
}
148157
// Length of name column is 64
149158
$truncatedTagName = substr($tagName, 0, 64);
150159
$query = $this->connection->getQueryBuilder();
@@ -319,6 +328,22 @@ public function canUserAssignTag(ISystemTag $tag, ?IUser $user): bool {
319328
return false;
320329
}
321330

331+
public function canUserCreateTag(?IUser $user): bool {
332+
if ($user === null) {
333+
// If no user given, allows only calls from CLI
334+
return \OC::$CLI;
335+
}
336+
337+
if ($this->appConfig->getValueBool('systemtags', 'only_admins_can_create', false) === false) {
338+
return true;
339+
}
340+
341+
return $this->groupManager->isAdmin($user->getUID());
342+
}
343+
344+
/**
345+
* {@inheritdoc}
346+
*/
322347
public function canUserSeeTag(ISystemTag $tag, ?IUser $user): bool {
323348
// If no user, then we only show public tags
324349
if (!$user && $tag->getAccessLevel() === ISystemTag::ACCESS_LEVEL_PUBLIC) {

‎lib/public/SystemTag/ISystemTagManager.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,10 @@ public function getTag(string $tagName, bool $userVisible, bool $userAssignable)
5757
* @return ISystemTag system tag
5858
*
5959
* @throws TagAlreadyExistsException if tag already exists
60+
* @throws TagCreationForbiddenException if user doesn’t have the right to create a new tag
6061
*
6162
* @since 9.0.0
63+
* @since 31.0.0 Can throw TagCreationForbiddenExceptionif user doesn’t have the right to create a new tag
6264
*/
6365
public function createTag(string $tagName, bool $userVisible, bool $userAssignable): ISystemTag;
6466

@@ -117,6 +119,16 @@ public function deleteTags($tagIds);
117119
*/
118120
public function canUserAssignTag(ISystemTag $tag, ?IUser $user): bool;
119121

122+
/**
123+
* Checks whether the given user is allowed to create new tags
124+
*
125+
* @param IUser|null $user user to check permission for
126+
* @return bool true if the user is allowed to create a new tag, false otherwise
127+
*
128+
* @since 31.0.0
129+
*/
130+
public function canUserCreateTag(?IUser $user): bool;
131+
120132
/**
121133
* Checks whether the given user is allowed to see the tag with the given id.
122134
*

‎lib/public/SystemTag/TagCreationForbiddenException.php‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-only
8+
*/
9+
10+
namespace OCP\SystemTag;
11+
12+
/**
13+
* Exception when a user don't have the rigth to create a tag
14+
*
15+
* @since 31.0.0
16+
*/
17+
class TagCreationForbiddenException extends \RuntimeException {
18+
}

‎tests/lib/SystemTag/SystemTagManagerTest.php‎

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,11 @@
1111
use OC\SystemTag\SystemTagManager;
1212
use OC\SystemTag\SystemTagObjectMapper;
1313
use OCP\EventDispatcher\IEventDispatcher;
14+
use OCP\IAppConfig;
1415
use OCP\IDBConnection;
1516
use OCP\IGroupManager;
1617
use OCP\IUser;
18+
use OCP\IUserSession;
1719
use OCP\SystemTag\ISystemTag;
1820
use OCP\SystemTag\ISystemTagManager;
1921
use Test\TestCase;
@@ -40,6 +42,16 @@ class SystemTagManagerTest extends TestCase {
4042
*/
4143
private $groupManager;
4244

45+
/**
46+
* @var IUserSession
47+
*/
48+
private $userSession;
49+
50+
/**
51+
* @var IAppConfig
52+
*/
53+
private $appConfig;
54+
4355
/**
4456
* @var IEventDispatcher
4557
*/
@@ -52,11 +64,15 @@ protected function setUp(): void {
5264

5365
$this->dispatcher = $this->createMock(IEventDispatcher::class);
5466
$this->groupManager = $this->createMock(IGroupManager::class);
67+
$this->userSession = $this->createMock(IUserSession::class);
68+
$this->appConfig = $this->createMock(IAppConfig::class);
5569

5670
$this->tagManager = new SystemTagManager(
5771
$this->connection,
5872
$this->groupManager,
59-
$this->dispatcher
73+
$this->dispatcher,
74+
$this->userSession,
75+
$this->appConfig,
6076
);
6177
$this->pruneTagsTables();
6278
}
@@ -535,6 +551,84 @@ public function testEmptyTagGroup(): void {
535551
$this->assertEquals([], $this->tagManager->getTagGroups($tag1));
536552
}
537553

554+
private function allowedToCreateProvider(): array {
555+
return [
556+
[true, null, true],
557+
[true, null, false],
558+
[false, true, true],
559+
[false, true, false],
560+
[false, false, false],
561+
];
562+
}
563+
564+
/**
565+
* @dataProvider allowedToCreateProvider
566+
*/
567+
public function testAllowedToCreateTag(bool $isCli, ?bool $isAdmin, bool $isRestricted): void {
568+
$oldCli = \OC::$CLI;
569+
\OC::$CLI = $isCli;
570+
571+
$user = $this->getMockBuilder(IUser::class)->getMock();
572+
$user->expects($this->any())
573+
->method('getUID')
574+
->willReturn('test');
575+
$this->userSession->expects($this->any())
576+
->method('getUser')
577+
->willReturn($isAdmin === null ? null : $user);
578+
$this->groupManager->expects($this->any())
579+
->method('isAdmin')
580+
->with('test')
581+
->willReturn($isAdmin);
582+
$this->appConfig->expects($this->any())
583+
->method('getValueBool')
584+
->with('systemtags', 'only_admins_can_create')
585+
->willReturn($isRestricted);
586+
587+
$name = uniqid('tag_', true);
588+
$tag = $this->tagManager->createTag($name, true, true);
589+
$this->assertEquals($tag->getName(), $name);
590+
$this->tagManager->deleteTags($tag->getId());
591+
592+
\OC::$CLI = $oldCli;
593+
}
594+
595+
private function disallowedToCreateProvider(): array {
596+
return [
597+
[false],
598+
[null],
599+
];
600+
}
601+
602+
/**
603+
* @dataProvider disallowedToCreateProvider
604+
*/
605+
public function testDisallowedToCreateTag(?bool $isAdmin): void {
606+
$oldCli = \OC::$CLI;
607+
\OC::$CLI = false;
608+
609+
$user = $this->getMockBuilder(IUser::class)->getMock();
610+
$user->expects($this->any())
611+
->method('getUID')
612+
->willReturn('test');
613+
$this->userSession->expects($this->any())
614+
->method('getUser')
615+
->willReturn($isAdmin === null ? null : $user);
616+
$this->groupManager->expects($this->any())
617+
->method('isAdmin')
618+
->with('test')
619+
->willReturn($isAdmin);
620+
$this->appConfig->expects($this->any())
621+
->method('getValueBool')
622+
->with('systemtags', 'only_admins_can_create')
623+
->willReturn(true);
624+
625+
$this->expectException(\Exception::class);
626+
$tag = $this->tagManager->createTag(uniqid('tag_', true), true, true);
627+
628+
\OC::$CLI = $oldCli;
629+
}
630+
631+
538632
/**
539633
* @param ISystemTag $tag1
540634
* @param ISystemTag $tag2

0 commit comments

Comments
 (0)