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

Forbid tagging readonly files #44297

Merged
merged 2 commits into from
Mar 21, 2024
Merged
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
65 changes: 11 additions & 54 deletions apps/dav/lib/SystemTag/SystemTagMappingNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,62 +37,15 @@
* Mapping node for system tag to object id
*/
class SystemTagMappingNode implements \Sabre\DAV\INode {
/**
* @var ISystemTag
*/
protected $tag;

/**
* @var string
*/
private $objectId;

/**
* @var string
*/
private $objectType;

/**
* User
*
* @var IUser
*/
protected $user;

/**
* @var ISystemTagManager
*/
protected $tagManager;

/**
* @var ISystemTagObjectMapper
*/
private $tagMapper;

/**
* Sets up the node, expects a full path name
*
* @param ISystemTag $tag system tag
* @param string $objectId
* @param string $objectType
* @param IUser $user user
* @param ISystemTagManager $tagManager
* @param ISystemTagObjectMapper $tagMapper
*/
public function __construct(
ISystemTag $tag,
$objectId,
$objectType,
IUser $user,
ISystemTagManager $tagManager,
ISystemTagObjectMapper $tagMapper
private ISystemTag $tag,
private string $objectId,
private string $objectType,
private IUser $user,
private ISystemTagManager $tagManager,
private ISystemTagObjectMapper $tagMapper,
private \Closure $childWriteAccessFunction,
) {
$this->tag = $tag;
$this->objectId = $objectId;
$this->objectType = $objectType;
$this->user = $user;
$this->tagManager = $tagManager;
$this->tagMapper = $tagMapper;
}

/**
Expand Down Expand Up @@ -166,6 +119,10 @@ public function delete() {
if (!$this->tagManager->canUserAssignTag($this->tag, $this->user)) {
throw new Forbidden('No permission to unassign tag ' . $this->tag->getId());
}
$writeAccessFunction = $this->childWriteAccessFunction;
if (!$writeAccessFunction($this->objectId)) {
throw new Forbidden('No permission to unassign tag to ' . $this->objectId);
}
$this->tagMapper->unassignTags($this->objectId, $this->objectType, $this->tag->getId());
} catch (TagNotFoundException $e) {
// can happen if concurrent deletion occurred
Expand Down
62 changes: 12 additions & 50 deletions apps/dav/lib/SystemTag/SystemTagsObjectMappingCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,56 +40,14 @@
* Collection containing tags by object id
*/
class SystemTagsObjectMappingCollection implements ICollection {

/**
* @var string
*/
private $objectId;

/**
* @var string
*/
private $objectType;

/**
* @var ISystemTagManager
*/
private $tagManager;

/**
* @var ISystemTagObjectMapper
*/
private $tagMapper;

/**
* User
*
* @var IUser
*/
private $user;


/**
* Constructor
*
* @param string $objectId object id
* @param string $objectType object type
* @param IUser $user user
* @param ISystemTagManager $tagManager tag manager
* @param ISystemTagObjectMapper $tagMapper tag mapper
*/
public function __construct(
$objectId,
$objectType,
IUser $user,
ISystemTagManager $tagManager,
ISystemTagObjectMapper $tagMapper
private string $objectId,
private string $objectType,
private IUser $user,
private ISystemTagManager $tagManager,
private ISystemTagObjectMapper $tagMapper,
protected \Closure $childWriteAccessFunction,
) {
$this->tagManager = $tagManager;
$this->tagMapper = $tagMapper;
$this->objectId = $objectId;
$this->objectType = $objectType;
$this->user = $user;
}

/**
Expand All @@ -106,7 +64,10 @@ public function createFile($name, $data = null) {
if (!$this->tagManager->canUserAssignTag($tag, $this->user)) {
throw new Forbidden('No permission to assign tag ' . $tagId);
}

$writeAccessFunction = $this->childWriteAccessFunction;
if (!$writeAccessFunction($this->objectId)) {
throw new Forbidden('No permission to assign tag to ' . $this->objectId);
}
$this->tagMapper->assignTags($this->objectId, $this->objectType, $tagId);
} catch (TagNotFoundException $e) {
throw new PreconditionFailed('Tag with id ' . $tagId . ' does not exist, cannot assign');
Expand Down Expand Up @@ -224,7 +185,8 @@ private function makeNode(ISystemTag $tag) {
$this->objectType,
$this->user,
$this->tagManager,
$this->tagMapper
$this->tagMapper,
$this->childWriteAccessFunction,
);
}
}
63 changes: 9 additions & 54 deletions apps/dav/lib/SystemTag/SystemTagsObjectTypeCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,61 +38,15 @@
* Collection containing object ids by object type
*/
class SystemTagsObjectTypeCollection implements ICollection {

/**
* @var string
*/
private $objectType;

/**
* @var ISystemTagManager
*/
private $tagManager;

/**
* @var ISystemTagObjectMapper
*/
private $tagMapper;

/**
* @var IGroupManager
*/
private $groupManager;

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

/**
* @var \Closure
**/
protected $childExistsFunction;

/**
* Constructor
*
* @param string $objectType object type
* @param ISystemTagManager $tagManager
* @param ISystemTagObjectMapper $tagMapper
* @param IUserSession $userSession
* @param IGroupManager $groupManager
* @param \Closure $childExistsFunction
*/
public function __construct(
$objectType,
ISystemTagManager $tagManager,
ISystemTagObjectMapper $tagMapper,
IUserSession $userSession,
IGroupManager $groupManager,
\Closure $childExistsFunction
private string $objectType,
private ISystemTagManager $tagManager,
private ISystemTagObjectMapper $tagMapper,
private IUserSession $userSession,
private IGroupManager $groupManager,
protected \Closure $childExistsFunction,
protected \Closure $childWriteAccessFunction,
) {
$this->tagManager = $tagManager;
$this->tagMapper = $tagMapper;
$this->objectType = $objectType;
$this->userSession = $userSession;
$this->groupManager = $groupManager;
$this->childExistsFunction = $childExistsFunction;
}

/**
Expand Down Expand Up @@ -134,7 +88,8 @@ public function getChild($objectName) {
$this->objectType,
$this->userSession->getUser(),
$this->tagManager,
$this->tagMapper
$this->tagMapper,
$this->childWriteAccessFunction,
);
}

Expand Down
20 changes: 18 additions & 2 deletions apps/dav/lib/SystemTag/SystemTagsRelationsCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
*/
namespace OCA\DAV\SystemTag;

use OCP\Constants;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\IRootFolder;
use OCP\IGroupManager;
Expand Down Expand Up @@ -60,7 +61,21 @@
} else {
return false;
}
}
},
function (string $name) use ($rootFolder, $userSession): bool {
$user = $userSession->getUser();
if ($user) {
$nodes = $rootFolder->getUserFolder($user->getUID())->getById((int)$name);
foreach ($nodes as $node) {
if (($node->getPermissions() & Constants::PERMISSION_UPDATE) === Constants::PERMISSION_UPDATE) {
return true;
}
}
return false;
} else {
return false;
}
},
),
];

Expand All @@ -75,7 +90,8 @@
$tagMapper,
$userSession,
$groupManager,
$entityExistsFunction
$entityExistsFunction,
fn ($name) => true,

Check notice

Code scanning / Psalm

MissingClosureParamType Note

Parameter $name has no provided type
);
}

Expand Down
48 changes: 28 additions & 20 deletions apps/dav/tests/unit/SystemTag/SystemTagMappingNodeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,21 +33,9 @@
use OCP\SystemTag\TagNotFoundException;

class SystemTagMappingNodeTest extends \Test\TestCase {

/**
* @var \OCP\SystemTag\ISystemTagManager
*/
private $tagManager;

/**
* @var \OCP\SystemTag\ISystemTagObjectMapper
*/
private $tagMapper;

/**
* @var \OCP\IUser
*/
private $user;
private ISystemTagManager $tagManager;
private ISystemTagObjectMapper $tagMapper;
private IUser $user;

protected function setUp(): void {
parent::setUp();
Expand All @@ -60,7 +48,7 @@ protected function setUp(): void {
->getMock();
}

public function getMappingNode($tag = null) {
public function getMappingNode($tag = null, array $writableNodeIds = []) {
if ($tag === null) {
$tag = new SystemTag(1, 'Test', true, true);
}
Expand All @@ -70,7 +58,8 @@ public function getMappingNode($tag = null) {
'files',
$this->user,
$this->tagManager,
$this->tagMapper
$this->tagMapper,
fn ($id): bool => in_array($id, $writableNodeIds),
);
}

Expand All @@ -84,7 +73,7 @@ public function testGetters(): void {
}

public function testDeleteTag(): void {
$node = $this->getMappingNode();
$node = $this->getMappingNode(null, [123]);
$this->tagManager->expects($this->once())
->method('canUserSeeTag')
->with($node->getSystemTag())
Expand All @@ -102,6 +91,25 @@ public function testDeleteTag(): void {
$node->delete();
}

public function testDeleteTagForbidden(): void {
$node = $this->getMappingNode();
$this->tagManager->expects($this->once())
->method('canUserSeeTag')
->with($node->getSystemTag())
->willReturn(true);
$this->tagManager->expects($this->once())
->method('canUserAssignTag')
->with($node->getSystemTag())
->willReturn(true);
$this->tagManager->expects($this->never())
->method('deleteTags');
$this->tagMapper->expects($this->never())
->method('unassignTags');

$this->expectException(\Sabre\DAV\Exception\Forbidden::class);
$node->delete();
}

public function tagNodeDeleteProviderPermissionException() {
return [
[
Expand Down Expand Up @@ -144,7 +152,7 @@ public function testDeleteTagExpectedException(ISystemTag $tag, $expectedExcepti
$this->assertInstanceOf($expectedException, $thrown);
}


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

Expand All @@ -164,6 +172,6 @@ public function testDeleteTagNotFound(): void {
->with(123, 'files', 1)
->will($this->throwException(new TagNotFoundException()));

$this->getMappingNode($tag)->delete();
$this->getMappingNode($tag, [123])->delete();
}
}
Loading
Loading