Check for authorship on edit and delete attempts

diff --git a/apps/dav/lib/comments/commentnode.php b/apps/dav/lib/comments/commentnode.php
index a5d508dd198a75fa2886ef335991a7500aab636f..d3cd53bceb12fd35413be40cf34329bd99072e61 100644 (file)
--- 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 44ac54ae937ed9c7e4ad9e2ebcddd5e2c1436f42..8d1bf06ab60781b097ec19f4b79992f037c0a22f 100644 (file)
--- 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()