feat: allow only the authors of task comments to edit them

pkg/integrations/task_comment_test.go

@@ -63,6 +63,7 @@ func TestTaskComments(t *testing.T) {
 			assertHandlerErrorCode(t, err, models.ErrCodeTaskDoesNotExist)
 		})
 		t.Run("Rights check", func(t *testing.T) {
+			// Only the own comments can be updated
 			t.Run("Forbidden", func(t *testing.T) {
 				_, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "14", "commentid": "2"}, `{"comment":"Lorem Ipsum"}`)
 				assert.Error(t, err)
@@ -74,14 +75,14 @@ func TestTaskComments(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via Team write", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "16", "commentid": "4"}, `{"comment":"Lorem Ipsum"}`)
+				_, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "16", "commentid": "4"}, `{"comment":"Lorem Ipsum"}`)
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via Team admin", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "17", "commentid": "5"}, `{"comment":"Lorem Ipsum"}`)
+				_, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "17", "commentid": "5"}, `{"comment":"Lorem Ipsum"}`)
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 
 			t.Run("Shared Via User readonly", func(t *testing.T) {
@@ -90,14 +91,14 @@ func TestTaskComments(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via User write", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "19", "commentid": "7"}, `{"comment":"Lorem Ipsum"}`)
+				_, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "19", "commentid": "7"}, `{"comment":"Lorem Ipsum"}`)
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via User admin", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "20", "commentid": "8"}, `{"comment":"Lorem Ipsum"}`)
+				_, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "20", "commentid": "8"}, `{"comment":"Lorem Ipsum"}`)
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 
 			t.Run("Shared Via NamespaceTeam readonly", func(t *testing.T) {
@@ -106,14 +107,14 @@ func TestTaskComments(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via NamespaceTeam write", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "22", "commentid": "10"}, `{"comment":"Lorem Ipsum"}`)
+				_, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "22", "commentid": "10"}, `{"comment":"Lorem Ipsum"}`)
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via NamespaceTeam admin", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "23", "commentid": "11"}, `{"comment":"Lorem Ipsum"}`)
+				_, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "23", "commentid": "11"}, `{"comment":"Lorem Ipsum"}`)
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 
 			t.Run("Shared Via NamespaceUser readonly", func(t *testing.T) {
@@ -122,14 +123,14 @@ func TestTaskComments(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via NamespaceUser write", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "25", "commentid": "13"}, `{"comment":"Lorem Ipsum"}`)
+				_, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "25", "commentid": "13"}, `{"comment":"Lorem Ipsum"}`)
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via NamespaceUser admin", func(t *testing.T) {
-				rec, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "26", "commentid": "14"}, `{"comment":"Lorem Ipsum"}`)
+				_, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "26", "commentid": "14"}, `{"comment":"Lorem Ipsum"}`)
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 		})
 	})
@@ -145,6 +146,7 @@ func TestTaskComments(t *testing.T) {
 			assertHandlerErrorCode(t, err, models.ErrCodeTaskDoesNotExist)
 		})
 		t.Run("Rights check", func(t *testing.T) {
+			// Only the own comments can be deleted
 			t.Run("Forbidden", func(t *testing.T) {
 				_, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "14", "commentid": "2"})
 				assert.Error(t, err)
@@ -156,14 +158,14 @@ func TestTaskComments(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via Team write", func(t *testing.T) {
-				rec, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "16", "commentid": "4"})
+				_, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "16", "commentid": "4"})
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `Successfully deleted.`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via Team admin", func(t *testing.T) {
-				rec, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "17", "commentid": "5"})
+				_, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "17", "commentid": "5"})
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `Successfully deleted.`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 
 			t.Run("Shared Via User readonly", func(t *testing.T) {
@@ -172,14 +174,14 @@ func TestTaskComments(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via User write", func(t *testing.T) {
-				rec, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "19", "commentid": "7"})
+				_, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "19", "commentid": "7"})
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `Successfully deleted.`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via User admin", func(t *testing.T) {
-				rec, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "20", "commentid": "8"})
+				_, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "20", "commentid": "8"})
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `Successfully deleted.`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 
 			t.Run("Shared Via NamespaceTeam readonly", func(t *testing.T) {
@@ -188,14 +190,14 @@ func TestTaskComments(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via NamespaceTeam write", func(t *testing.T) {
-				rec, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "22", "commentid": "10"})
+				_, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "22", "commentid": "10"})
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `Successfully deleted.`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via NamespaceTeam admin", func(t *testing.T) {
-				rec, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "23", "commentid": "11"})
+				_, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "23", "commentid": "11"})
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `Successfully deleted.`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 
 			t.Run("Shared Via NamespaceUser readonly", func(t *testing.T) {
@@ -204,14 +206,14 @@ func TestTaskComments(t *testing.T) {
 				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via NamespaceUser write", func(t *testing.T) {
-				rec, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "25", "commentid": "13"})
+				_, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "25", "commentid": "13"})
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `Successfully deleted.`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 			t.Run("Shared Via NamespaceUser admin", func(t *testing.T) {
-				rec, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "26", "commentid": "14"})
+				_, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "26", "commentid": "14"})
-				assert.NoError(t, err)
+				assert.Error(t, err)
-				assert.Contains(t, rec.Body.String(), `Successfully deleted.`)
+				assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`)
 			})
 		})
 	})

pkg/models/task_comment_rights.go

@@ -27,16 +27,36 @@ func (tc *TaskComment) CanRead(s *xorm.Session, a web.Auth) (bool, int, error) {
 	return t.CanRead(s, a)
 }
 
+func (tc *TaskComment) canUserModifyTaskComment(s *xorm.Session, a web.Auth) (bool, error) {
+	t := Task{ID: tc.TaskID}
+	canWriteTask, err := t.CanWrite(s, a)
+	if err != nil {
+		return false, err
+	}
+	if !canWriteTask {
+		return false, nil
+	}
+
+	savedComment := &TaskComment{
+		ID:     tc.ID,
+		TaskID: tc.TaskID,
+	}
+	err = getTaskCommentSimple(s, savedComment)
+	if err != nil {
+		return false, err
+	}
+
+	return a.GetID() == savedComment.AuthorID, nil
+}
+
 // CanDelete checks if a user can delete a comment
 func (tc *TaskComment) CanDelete(s *xorm.Session, a web.Auth) (bool, error) {
-	t := Task{ID: tc.TaskID}
+	return tc.canUserModifyTaskComment(s, a)
-	return t.CanWrite(s, a)
 }
 
 // CanUpdate checks if a user can update a comment
 func (tc *TaskComment) CanUpdate(s *xorm.Session, a web.Auth) (bool, error) {
-	t := Task{ID: tc.TaskID}
+	return tc.canUserModifyTaskComment(s, a)
-	return t.CanWrite(s, a)
 }
 
 // CanCreate checks if a user can create a new comment

pkg/models/task_comments.go

@@ -151,6 +151,24 @@ func (tc *TaskComment) Update(s *xorm.Session, a web.Auth) error {
 	})
 }
 
+func getTaskCommentSimple(s *xorm.Session, tc *TaskComment) error {
+	exists, err := s.
+		Where("id = ? and task_id = ?", tc.ID, tc.TaskID).
+		NoAutoCondition().
+		Get(tc)
+	if err != nil {
+		return err
+	}
+	if !exists {
+		return ErrTaskCommentDoesNotExist{
+			ID:     tc.ID,
+			TaskID: tc.TaskID,
+		}
+	}
+
+	return nil
+}
+
 // ReadOne handles getting a single comment
 // @Summary Remove a task comment
 // @Description Remove a task comment. The user doing this need to have at least read access to the task this comment belongs to.
@@ -166,15 +184,9 @@ func (tc *TaskComment) Update(s *xorm.Session, a web.Auth) error {
 // @Failure 500 {object} models.Message "Internal error"
 // @Router /tasks/{taskID}/comments/{commentID} [get]
 func (tc *TaskComment) ReadOne(s *xorm.Session, a web.Auth) (err error) {
-	exists, err := s.Get(tc)
+	err = getTaskCommentSimple(s, tc)
 	if err != nil {
-		return
+		return err
-	}
-	if !exists {
-		return ErrTaskCommentDoesNotExist{
-			ID:     tc.ID,
-			TaskID: tc.TaskID,
-		}
 	}
 
 	// Get the author

pkg/models/task_comments_test.go

@@ -121,6 +121,16 @@ func TestTaskComment_Delete(t *testing.T) {
 		assert.Error(t, err)
 		assert.True(t, IsErrTaskCommentDoesNotExist(err))
 	})
+	t.Run("not the own comment", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		tc := &TaskComment{ID: 1, TaskID: 1}
+		can, err := tc.CanDelete(s, &user.User{ID: 2})
+		assert.NoError(t, err)
+		assert.False(t, can)
+	})
 }
 
 func TestTaskComment_Update(t *testing.T) {
@@ -157,6 +167,16 @@ func TestTaskComment_Update(t *testing.T) {
 		assert.Error(t, err)
 		assert.True(t, IsErrTaskCommentDoesNotExist(err))
 	})
+	t.Run("not the own comment", func(t *testing.T) {
+		db.LoadAndAssertFixtures(t)
+		s := db.NewSession()
+		defer s.Close()
+
+		tc := &TaskComment{ID: 1, TaskID: 1}
+		can, err := tc.CanUpdate(s, &user.User{ID: 2})
+		assert.NoError(t, err)
+		assert.False(t, can)
+	})
 }
 
 func TestTaskComment_ReadOne(t *testing.T) {
@@ -167,7 +187,7 @@ func TestTaskComment_ReadOne(t *testing.T) {
 		s := db.NewSession()
 		defer s.Close()
 
-		tc := &TaskComment{ID: 1}
+		tc := &TaskComment{ID: 1, TaskID: 1}
 		err := tc.ReadOne(s, u)
 		assert.NoError(t, err)
 		assert.Equal(t, "Lorem Ipsum Dolor Sit Amet", tc.Comment)