Skip to content

[stable35] fix(systemtags): properly handle object ids #66229

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.

Sign up for GitHub

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

Merged
AndyScherzinger merged 2 commits into from
Nov 4, 2026
Merged

[stable35] fix(systemtags): properly handle object ids #66229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65c3067
test: add test for assiging object ids to a read-only tag
icewind1991 Oct 30, 2026
06ee19b
fix: forbid assiging objects to a tag that isn't used assignable
icewind1991 Oct 30, 2026
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
2 changes: 1 addition & 1 deletion apps/dav/lib/SystemTag/SystemTagPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,7 @@ public function handleUpdateProperties($path, PropPatch $propPatch) {
}

$user = $this->userSession->getUser();
if (!$user) {
if (!$user || !$this->tagManager->canUserAssignTag($node->getSystemTag(), $user)) {
throw new Forbidden('You don’t have permissions to update tags');
}

Expand Down
70 changes: 35 additions & 35 deletions apps/dav/tests/unit/SystemTag/SystemTagPluginTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@
use OCP\SystemTag\ISystemTagManager;
use OCP\SystemTag\ISystemTagObjectMapper;
use OCP\SystemTag\TagAlreadyExistsException;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\MockObject\MockObject;
use Sabre\DAV\Exception\Forbidden;
use Sabre\DAV\PropPatch;
use Sabre\DAV\Tree;
use Sabre\HTTP\RequestInterface;
Expand Down Expand Up @@ -148,7 +150,7 @@ public static function getPropertiesDataProvider(): array {
];
}

#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'getPropertiesDataProvider')]
#[DataProvider(methodName: 'getPropertiesDataProvider')]
public function testGetProperties(ISystemTag $systemTag, array $groups, array $requestedProperties, array $expectedProperties): void {
$this->user->expects($this->any())
->method('getUID')
Expand Down Expand Up @@ -197,7 +199,7 @@ public function testGetProperties(ISystemTag $systemTag, array $groups, array $r
}

public function testGetPropertiesForbidden(): void {
$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
$this->expectException(Forbidden::class);

$systemTag = new SystemTag('1', 'Test', true, false);
$requestedProperties = [
Expand Down Expand Up @@ -293,7 +295,7 @@ public function testUpdatePropertiesAdmin(): void {
}

public function testUpdatePropertiesForbidden(): void {
$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
$this->expectException(Forbidden::class);

$systemTag = new SystemTag('1', 'Test', true, false);
$this->user->expects($this->any())
Expand Down Expand Up @@ -338,56 +340,54 @@ public function testUpdatePropertiesForbidden(): void {

public static function updateObjectIdsProvider(): array {
return [
'add a visible file' => [['2', '3'], ['2', '3', '1']],
'remove a visible file' => [['3'], ['3', '1']],
'remove all visible files' => [null, ['1']],
'add a visible file' => [['2', '3'], Constants::PERMISSION_ALL, true, ['2', '3', '1']],
'remove a visible file' => [['3'], Constants::PERMISSION_ALL, true, ['3', '1']],
'remove all visible files' => [null, Constants::PERMISSION_ALL, true, ['1']],
'add a hidden file' => [['1', '2', '4'], Constants::PERMISSION_ALL, true, null],
'remove a read-only file' => [null, Constants::PERMISSION_READ, true, null],
'add a visible file to non-assignable tag' => [['2', '3'], Constants::PERMISSION_ALL, false, null],
];
}

#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'updateObjectIdsProvider')]
public function testUpdateObjectIdsKeepsHiddenObjects(?array $requestedIds, array $expectedIds): void {
$this->mockObjectIdsUpdate(Constants::PERMISSION_ALL);

$this->tagMapper->expects($this->once())
->method('setObjectIdsForTag')
->with('5', 'files', $expectedIds);

$this->updateObjectIds($requestedIds);
}

public static function updateObjectIdsForbiddenProvider(): array {
return [
'add a hidden file' => [['1', '2', '4'], Constants::PERMISSION_ALL],
'remove a read-only file' => [null, Constants::PERMISSION_READ],
];
}
/**
* @param list<string>|null $expectedIds expected file ids for the tag after the update, or `null` if the update should be blocked
*/
#[DataProvider(methodName: 'updateObjectIdsProvider')]
public function testUpdateObjectIds(?array $requestedIds, int $filePermissions, bool $tagIsAssignable, ?array $expectedIds): void {
if ($expectedIds === null) {
$this->expectException(Forbidden::class);
}
$this->mockObjectIdsUpdate($filePermissions, $tagIsAssignable);

#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'updateObjectIdsForbiddenProvider')]
public function testUpdateObjectIdsForbidden(?array $requestedIds, int $permissions): void {
$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
$this->mockObjectIdsUpdate($permissions);
if ($expectedIds) {
$this->tagMapper->expects($this->once())
->method('setObjectIdsForTag')
->with('5', 'files', $expectedIds);
} else {
$this->tagMapper->expects($this->never())
->method('setObjectIdsForTag');

$this->tagMapper->expects($this->never())
->method('setObjectIdsForTag');
}

$this->updateObjectIds($requestedIds);
}

/**
* Files 1 and 2 are tagged, only files 2 and 3 are visible to the user
*/
private function mockObjectIdsUpdate(int $permissions): void {
private function mockObjectIdsUpdate(int $filePermissions, bool $tagIsAssignable): void {
$this->user->method('getUID')->willReturn('user');

$node = $this->createMock(SystemTagObjectType::class);
$node->method('getName')->willReturn('files');
$node->method('getSystemTag')->willReturn(new SystemTag('5', 'Test', true, true));
$node->method('getSystemTag')->willReturn(new SystemTag('5', 'Test', true, $tagIsAssignable));
$this->tree->method('getNodeForPath')->willReturn($node);

$this->tagManager->method('canUserAssignTag')->willReturn($tagIsAssignable);
$this->tagMapper->method('getObjectIdsForTags')->willReturn(['1', '2']);

$fileNode = $this->createMock(\OCP\Files\Node::class);
$fileNode->method('getPermissions')->willReturn($permissions);
$fileNode->method('getPermissions')->willReturn($filePermissions);
$userFolder = $this->createMock(Folder::class);
$userFolder->method('getFirstNodeById')
->willReturnCallback(fn (int $id): ?\OCP\Files\Node => in_array($id, [2, 3], true) ? $fileNode : null);
Expand All @@ -411,7 +411,7 @@ public static function createTagInsufficientPermissionsProvider(): array {
[true, true, 'group1|group2'],
];
}
#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'createTagInsufficientPermissionsProvider')]
#[DataProvider(methodName: 'createTagInsufficientPermissionsProvider')]
public function testCreateNotAssignableTagAsRegularUser(bool $userVisible, bool $userAssignable, string $groups): void {
$this->expectException(\Sabre\DAV\Exception\BadRequest::class);
$this->expectExceptionMessage('Not sufficient permissions');
Expand Down Expand Up @@ -520,7 +520,7 @@ public static function createTagProvider(): array {
];
}

#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'createTagProvider')]
#[DataProvider(methodName: 'createTagProvider')]
public function testCreateTagInByIdCollection(bool $userVisible, bool $userAssignable, string $groups): void {
$this->user->expects($this->once())
->method('getUID')
Expand Down Expand Up @@ -684,7 +684,7 @@ public function testCreateTagToUnknownNode(): void {
$this->plugin->httpPost($request, $response);
}

#[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'nodeClassProvider')]
#[DataProvider(methodName: 'nodeClassProvider')]
public function testCreateTagConflict(string $nodeClass): void {
$this->expectException(\Sabre\DAV\Exception\Conflict::class);

Expand Down
Loading