Merge pull request #22124 from owncloud/comments-author-check

Check for authorship on edit attempt

2 files changed, 184 insertions, 1 deletions

diff --git a/apps/dav/lib/comments/commentnode.php b/apps/dav/lib/comments/commentnode.php
index a5d508dd198..d3cd53bceb1 100644
--- a/apps/dav/lib/comments/commentnode.php
+++ b/apps/dav/lib/comments/commentnode.php
@@ -27,6 +27,7 @@ use OCP\Comments\ICommentsManager;
 use OCP\ILogger;
 use OCP\IUserManager;
 use OCP\IUserSession;
+use Sabre\DAV\Exception\Forbidden;
 use Sabre\DAV\Exception\MethodNotAllowed;
 use Sabre\DAV\PropPatch;
 
@@ -112,12 +113,23 @@ class CommentNode implements \Sabre\DAV\INode, \Sabre\DAV\IProperties {
 		];
 	}
 
+	protected function checkWriteAccessOnComment() {
+		$user = $this->userSession->getUser();
+		if(    $this->comment->getActorType() !== 'users'
+			|| is_null($user)
+			|| $this->comment->getActorId() !== $user->getUID()
+		) {
+			throw new Forbidden('Only authors are allowed to edit their comment.');
+		}
+	}
+
 	/**
 	 * Deleted the current node
 	 *
 	 * @return void
 	 */
 	function delete() {
+		$this->checkWriteAccessOnComment();
 		$this->commentsManager->delete($this->comment->getId());
 	}
 
@@ -156,8 +168,10 @@ class CommentNode implements \Sabre\DAV\INode, \Sabre\DAV\IProperties {
 	 *
 	 * @param $propertyValue
 	 * @return bool
+	 * @throws Forbidden
 	 */
 	public function updateComment($propertyValue) {
+		$this->checkWriteAccessOnComment();
 		try {
 			$this->comment->setMessage($propertyValue);
 			$this->commentsManager->save($this->comment);
diff --git a/apps/dav/tests/unit/comments/commentnode.php b/apps/dav/tests/unit/comments/commentnode.php
index 44ac54ae937..8d1bf06ab60 100644
--- a/apps/dav/tests/unit/comments/commentnode.php
+++ b/apps/dav/tests/unit/comments/commentnode.php
@@ -51,10 +51,28 @@ class CommentsNode extends \Test\TestCase {
 	}
 
 	public function testDelete() {
+		$user = $this->getMock('\OCP\IUser');
+
+		$user->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('alice'));
+
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+
 		$this->comment->expects($this->once())
 			->method('getId')
 			->will($this->returnValue('19'));
 
+		$this->comment->expects($this->any())
+			->method('getActorType')
+			->will($this->returnValue('users'));
+
+		$this->comment->expects($this->any())
+			->method('getActorId')
+			->will($this->returnValue('alice'));
+
 		$this->commentsManager->expects($this->once())
 			->method('delete')
 			->with('19');
@@ -62,6 +80,37 @@ class CommentsNode extends \Test\TestCase {
 		$this->node->delete();
 	}
 
+	/**
+	 * @expectedException \Sabre\DAV\Exception\Forbidden
+	 */
+	public function testDeleteForbidden() {
+		$user = $this->getMock('\OCP\IUser');
+
+		$user->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('mallory'));
+
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+
+		$this->comment->expects($this->never())
+			->method('getId');
+
+		$this->comment->expects($this->any())
+			->method('getActorType')
+			->will($this->returnValue('users'));
+
+		$this->comment->expects($this->any())
+			->method('getActorId')
+			->will($this->returnValue('alice'));
+
+		$this->commentsManager->expects($this->never())
+			->method('delete');
+
+		$this->node->delete();
+	}
+
 	public function testGetName() {
 		$id = '19';
 		$this->comment->expects($this->once())
@@ -85,10 +134,28 @@ class CommentsNode extends \Test\TestCase {
 	public function testUpdateComment() {
 		$msg = 'Hello Earth';
 
+		$user = $this->getMock('\OCP\IUser');
+
+		$user->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('alice'));
+
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+
 		$this->comment->expects($this->once())
 			->method('setMessage')
 			->with($msg);
 
+		$this->comment->expects($this->any())
+			->method('getActorType')
+			->will($this->returnValue('users'));
+
+		$this->comment->expects($this->any())
+			->method('getActorId')
+			->will($this->returnValue('alice'));
+
 		$this->commentsManager->expects($this->once())
 			->method('save')
 			->with($this->comment);
@@ -96,14 +163,32 @@ class CommentsNode extends \Test\TestCase {
 		$this->assertTrue($this->node->updateComment($msg));
 	}
 
-	public function testUpdateCommentException() {
+	public function testUpdateCommentLogException() {
 		$msg = null;
 
+		$user = $this->getMock('\OCP\IUser');
+
+		$user->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('alice'));
+
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+
 		$this->comment->expects($this->once())
 			->method('setMessage')
 			->with($msg)
 			->will($this->throwException(new \Exception('buh!')));
 
+		$this->comment->expects($this->any())
+			->method('getActorType')
+			->will($this->returnValue('users'));
+
+		$this->comment->expects($this->any())
+			->method('getActorId')
+			->will($this->returnValue('alice'));
+
 		$this->commentsManager->expects($this->never())
 			->method('save');
 
@@ -113,6 +198,90 @@ class CommentsNode extends \Test\TestCase {
 		$this->assertFalse($this->node->updateComment($msg));
 	}
 
+	/**
+	 * @expectedException \Sabre\DAV\Exception\Forbidden
+	 */
+	public function testUpdateForbiddenByUser() {
+		$msg = 'HaXX0r';
+
+		$user = $this->getMock('\OCP\IUser');
+
+		$user->expects($this->once())
+			->method('getUID')
+			->will($this->returnValue('mallory'));
+
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+
+		$this->comment->expects($this->never())
+			->method('setMessage');
+
+		$this->comment->expects($this->any())
+			->method('getActorType')
+			->will($this->returnValue('users'));
+
+		$this->comment->expects($this->any())
+			->method('getActorId')
+			->will($this->returnValue('alice'));
+
+		$this->commentsManager->expects($this->never())
+			->method('save');
+
+		$this->node->updateComment($msg);
+	}
+
+	/**
+	 * @expectedException \Sabre\DAV\Exception\Forbidden
+	 */
+	public function testUpdateForbiddenByType() {
+		$msg = 'HaXX0r';
+
+		$user = $this->getMock('\OCP\IUser');
+
+		$user->expects($this->never())
+			->method('getUID');
+
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue($user));
+
+		$this->comment->expects($this->never())
+			->method('setMessage');
+
+		$this->comment->expects($this->any())
+			->method('getActorType')
+			->will($this->returnValue('bots'));
+
+		$this->commentsManager->expects($this->never())
+			->method('save');
+
+		$this->node->updateComment($msg);
+	}
+
+	/**
+	 * @expectedException \Sabre\DAV\Exception\Forbidden
+	 */
+	public function testUpdateForbiddenByNotLoggedIn() {
+		$msg = 'HaXX0r';
+
+		$this->userSession->expects($this->once())
+			->method('getUser')
+			->will($this->returnValue(null));
+
+		$this->comment->expects($this->never())
+			->method('setMessage');
+
+		$this->comment->expects($this->any())
+			->method('getActorType')
+			->will($this->returnValue('users'));
+
+		$this->commentsManager->expects($this->never())
+			->method('save');
+
+		$this->node->updateComment($msg);
+	}
+
 	public function testPropPatch() {
 		$propPatch = $this->getMockBuilder('Sabre\DAV\PropPatch')
 			->disableOriginalConstructor()