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.

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

Jump to bottom

[stable28] Forbid tagging readonly files #44298

Merged
merged 2 commits into from Mar 21, 2024
Merged

[stable28] Forbid tagging readonly files #44298

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
52aa280
fix(systemtags): Forbid tagging of readonly files
come-nc Feb 27, 2024
7f50e5f
chore: Break closure call on two lines to make it readable
come-nc Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 11 additions & 54 deletions apps/dav/lib/SystemTag/SystemTagMappingNode.php
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
17 changes: 14 additions & 3 deletions apps/dav/lib/SystemTag/SystemTagsRelationsCollection.php
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@
*/
namespace OCA\DAV\SystemTag;

use OCP\Constants;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IGroupManager;
use OCP\IUserSession;
Expand All @@ -50,10 +51,19 @@
$tagMapper,
$userSession,
$groupManager,
function ($name) {
function ($name): bool {

Check notice

Code scanning / Psalm

MissingClosureParamType Note

Parameter $name has no provided type
$nodes = \OC::$server->getUserFolder()->getById((int)$name);
return !empty($nodes);
}
},
function ($name): bool {

Check notice

Code scanning / Psalm

MissingClosureParamType Note

Parameter $name has no provided type
$nodes = \OC::$server->getUserFolder()->getById((int)$name);

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\Server::getUserFolder has been marked as deprecated

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method getById on possibly null value
foreach ($nodes as $node) {
if (($node->getPermissions() & Constants::PERMISSION_UPDATE) === Constants::PERMISSION_UPDATE) {
return true;
}
}
return false;
},
),
];

Expand All @@ -68,7 +78,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
View file
Open in desktop
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();
}
}