Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(systemtags): add setting to block non admin to create system tags #49514

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions apps/dav/lib/SystemTag/SystemTagPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use OCP\SystemTag\ISystemTagManager;
use OCP\SystemTag\ISystemTagObjectMapper;
use OCP\SystemTag\TagAlreadyExistsException;
use OCP\SystemTag\TagCreationForbiddenException;
use OCP\Util;
use Sabre\DAV\Exception\BadRequest;
use Sabre\DAV\Exception\Conflict;
Expand Down Expand Up @@ -187,6 +188,8 @@ private function createTag($data, $contentType = 'application/json') {
return $tag;
} catch (TagAlreadyExistsException $e) {
throw new Conflict('Tag already exists', 0, $e);
} catch (TagCreationForbiddenException $e) {
throw new Forbidden('You don’t have right to create tags', 0, $e);
}
}

Expand Down Expand Up @@ -370,7 +373,7 @@ public function handleUpdateProperties($path, PropPatch $propPatch) {
if (!($node instanceof SystemTagNode) && !($node instanceof SystemTagObjectType)) {
return;
}

$propPatch->handle([self::OBJECTIDS_PROPERTYNAME], function ($props) use ($node) {
if (!($node instanceof SystemTagObjectType)) {
return false;
Expand All @@ -388,7 +391,7 @@ public function handleUpdateProperties($path, PropPatch $propPatch) {
if (count($objectTypes) !== 1 || $objectTypes[0] !== $node->getName()) {
throw new BadRequest('Invalid object-ids property. All object types must be of the same type: ' . $node->getName());
}

$this->tagMapper->setObjectIdsForTag($node->getSystemTag()->getId(), $node->getName(), array_keys($objects));
}

Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,7 @@
'OCP\\SystemTag\\MapperEvent' => $baseDir . '/lib/public/SystemTag/MapperEvent.php',
'OCP\\SystemTag\\SystemTagsEntityEvent' => $baseDir . '/lib/public/SystemTag/SystemTagsEntityEvent.php',
'OCP\\SystemTag\\TagAlreadyExistsException' => $baseDir . '/lib/public/SystemTag/TagAlreadyExistsException.php',
'OCP\\SystemTag\\TagCreationForbiddenException' => $baseDir . '/lib/public/SystemTag/TagCreationForbiddenException.php',
'OCP\\SystemTag\\TagNotFoundException' => $baseDir . '/lib/public/SystemTag/TagNotFoundException.php',
'OCP\\Talk\\Exceptions\\NoBackendException' => $baseDir . '/lib/public/Talk/Exceptions/NoBackendException.php',
'OCP\\Talk\\IBroker' => $baseDir . '/lib/public/Talk/IBroker.php',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OCP\\SystemTag\\MapperEvent' => __DIR__ . '/../../..' . '/lib/public/SystemTag/MapperEvent.php',
'OCP\\SystemTag\\SystemTagsEntityEvent' => __DIR__ . '/../../..' . '/lib/public/SystemTag/SystemTagsEntityEvent.php',
'OCP\\SystemTag\\TagAlreadyExistsException' => __DIR__ . '/../../..' . '/lib/public/SystemTag/TagAlreadyExistsException.php',
'OCP\\SystemTag\\TagCreationForbiddenException' => __DIR__ . '/../../..' . '/lib/public/SystemTag/TagCreationForbiddenException.php',
'OCP\\SystemTag\\TagNotFoundException' => __DIR__ . '/../../..' . '/lib/public/SystemTag/TagNotFoundException.php',
'OCP\\Talk\\Exceptions\\NoBackendException' => __DIR__ . '/../../..' . '/lib/public/Talk/Exceptions/NoBackendException.php',
'OCP\\Talk\\IBroker' => __DIR__ . '/../../..' . '/lib/public/Talk/IBroker.php',
Expand Down
12 changes: 9 additions & 3 deletions lib/private/SystemTag/ManagerFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@
namespace OC\SystemTag;

use OCP\EventDispatcher\IEventDispatcher;
use OCP\IAppConfig;
use OCP\IDBConnection;
use OCP\IGroupManager;
use OCP\IServerContainer;
use OCP\IUserSession;
use OCP\SystemTag\ISystemTagManager;
use OCP\SystemTag\ISystemTagManagerFactory;
use OCP\SystemTag\ISystemTagObjectMapper;
Expand All @@ -36,9 +40,11 @@ public function __construct(
*/
public function getManager(): ISystemTagManager {
return new SystemTagManager(
$this->serverContainer->getDatabaseConnection(),
$this->serverContainer->getGroupManager(),
$this->serverContainer->get(IDBConnection::class),
$this->serverContainer->get(IGroupManager::class),
$this->serverContainer->get(IEventDispatcher::class),
$this->serverContainer->get(IUserSession::class),
$this->serverContainer->get(IAppConfig::class),
);
}

Expand All @@ -50,7 +56,7 @@ public function getManager(): ISystemTagManager {
*/
public function getObjectMapper(): ISystemTagObjectMapper {
return new SystemTagObjectMapper(
$this->serverContainer->getDatabaseConnection(),
$this->serverContainer->get(IDBConnection::class),
$this->getManager(),
$this->serverContainer->get(IEventDispatcher::class),
);
Expand Down
22 changes: 22 additions & 0 deletions lib/private/SystemTag/SystemTagManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,16 @@
use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IAppConfig;
use OCP\IDBConnection;
use OCP\IGroupManager;
use OCP\IUser;
use OCP\IUserSession;
use OCP\SystemTag\ISystemTag;
use OCP\SystemTag\ISystemTagManager;
use OCP\SystemTag\ManagerEvent;
use OCP\SystemTag\TagAlreadyExistsException;
use OCP\SystemTag\TagCreationForbiddenException;
use OCP\SystemTag\TagNotFoundException;

/**
Expand All @@ -36,6 +39,8 @@ public function __construct(
protected IDBConnection $connection,
protected IGroupManager $groupManager,
protected IEventDispatcher $dispatcher,
private IUserSession $userSession,
private IAppConfig $appConfig,
) {
$query = $this->connection->getQueryBuilder();
$this->selectTagQuery = $query->select('*')
Expand Down Expand Up @@ -157,6 +162,10 @@ public function getTag(string $tagName, bool $userVisible, bool $userAssignable)
* {@inheritdoc}
*/
public function createTag(string $tagName, bool $userVisible, bool $userAssignable): ISystemTag {
$user = $this->userSession->getUser();
if (!$this->canUserCreateTag($user)) {
throw new TagCreationForbiddenException('Tag creation forbidden');
}
// Length of name column is 64
$truncatedTagName = substr($tagName, 0, 64);
$query = $this->connection->getQueryBuilder();
Expand Down Expand Up @@ -335,6 +344,19 @@ public function canUserAssignTag(ISystemTag $tag, ?IUser $user): bool {
return false;
}

public function canUserCreateTag(?IUser $user): bool {
if ($user === null) {
// If no user given, allows only calls from CLI
return \OC::$CLI;
}

if ($this->appConfig->getValueBool('systemtags', 'only_admins_can_create', false) === false) {
return true;
}

return $this->groupManager->isAdmin($user->getUID());
}

/**
* {@inheritdoc}
*/
Expand Down
12 changes: 12 additions & 0 deletions lib/public/SystemTag/ISystemTagManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,10 @@ public function getTag(string $tagName, bool $userVisible, bool $userAssignable)
* @return ISystemTag system tag
*
* @throws TagAlreadyExistsException if tag already exists
* @throws TagCreationForbiddenException if user doesn’t have the right to create a new tag
*
* @since 9.0.0
* @since 31.0.0 Can throw TagCreationForbiddenExceptionif user doesn’t have the right to create a new tag
*/
public function createTag(string $tagName, bool $userVisible, bool $userAssignable): ISystemTag;

Expand Down Expand Up @@ -115,6 +117,16 @@ public function deleteTags($tagIds);
*/
public function canUserAssignTag(ISystemTag $tag, ?IUser $user): bool;

/**
* Checks whether the given user is allowed to create new tags
*
* @param IUser|null $user user to check permission for
* @return bool true if the user is allowed to create a new tag, false otherwise
*
* @since 31.0.0
*/
public function canUserCreateTag(?IUser $user): bool;

/**
* Checks whether the given user is allowed to see the tag with the given id.
*
Expand Down
18 changes: 18 additions & 0 deletions lib/public/SystemTag/TagCreationForbiddenException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-only
*/

namespace OCP\SystemTag;

/**
* Exception when a user don't have the rigth to create a tag
*
* @since 31.0.0
*/
class TagCreationForbiddenException extends \RuntimeException {
}
96 changes: 95 additions & 1 deletion tests/lib/SystemTag/SystemTagManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
use OC\SystemTag\SystemTagManager;
use OC\SystemTag\SystemTagObjectMapper;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IAppConfig;
use OCP\IDBConnection;
use OCP\IGroupManager;
use OCP\IUser;
use OCP\IUserSession;
use OCP\SystemTag\ISystemTag;
use OCP\SystemTag\ISystemTagManager;
use Test\TestCase;
Expand All @@ -40,6 +42,16 @@ class SystemTagManagerTest extends TestCase {
*/
private $groupManager;

/**
* @var IUserSession
*/
private $userSession;

/**
* @var IAppConfig
*/
private $appConfig;

/**
* @var IEventDispatcher
*/
Expand All @@ -52,11 +64,15 @@ protected function setUp(): void {

$this->dispatcher = $this->createMock(IEventDispatcher::class);
$this->groupManager = $this->createMock(IGroupManager::class);
$this->userSession = $this->createMock(IUserSession::class);
$this->appConfig = $this->createMock(IAppConfig::class);

$this->tagManager = new SystemTagManager(
$this->connection,
$this->groupManager,
$this->dispatcher
$this->dispatcher,
$this->userSession,
$this->appConfig,
);
$this->pruneTagsTables();
}
Expand Down Expand Up @@ -527,6 +543,84 @@ public function testEmptyTagGroup(): void {
$this->assertEquals([], $this->tagManager->getTagGroups($tag1));
}

private function allowedToCreateProvider(): array {
return [
[true, null, true],
[true, null, false],
[false, true, true],
[false, true, false],
[false, false, false],
];
}

/**
* @dataProvider allowedToCreateProvider
*/
public function testAllowedToCreateTag(bool $isCli, ?bool $isAdmin, bool $isRestricted): void {
$oldCli = \OC::$CLI;
\OC::$CLI = $isCli;

$user = $this->getMockBuilder(IUser::class)->getMock();
$user->expects($this->any())
->method('getUID')
->willReturn('test');
$this->userSession->expects($this->any())
->method('getUser')
->willReturn($isAdmin === null ? null : $user);
$this->groupManager->expects($this->any())
->method('isAdmin')
->with('test')
->willReturn($isAdmin);
$this->appConfig->expects($this->any())
->method('getValueBool')
->with('systemtags', 'only_admins_can_create')
->willReturn($isRestricted);

$name = uniqid('tag_', true);
$tag = $this->tagManager->createTag($name, true, true);
$this->assertEquals($tag->getName(), $name);
$this->tagManager->deleteTags($tag->getId());

\OC::$CLI = $oldCli;
}

private function disallowedToCreateProvider(): array {
return [
[false],
[null],
];
}

/**
* @dataProvider disallowedToCreateProvider
*/
public function testDisallowedToCreateTag(?bool $isAdmin): void {
$oldCli = \OC::$CLI;
\OC::$CLI = false;

$user = $this->getMockBuilder(IUser::class)->getMock();
$user->expects($this->any())
->method('getUID')
->willReturn('test');
$this->userSession->expects($this->any())
->method('getUser')
->willReturn($isAdmin === null ? null : $user);
$this->groupManager->expects($this->any())
->method('isAdmin')
->with('test')
->willReturn($isAdmin);
$this->appConfig->expects($this->any())
->method('getValueBool')
->with('systemtags', 'only_admins_can_create')
->willReturn(true);

$this->expectException(\Exception::class);
$tag = $this->tagManager->createTag(uniqid('tag_', true), true, true);

\OC::$CLI = $oldCli;
}


/**
* @param ISystemTag $tag1
* @param ISystemTag $tag2
Expand Down
Loading