diff --git a/pkg/integrations/task_comment_test.go b/pkg/integrations/task_comment_test.go index 5aa9bb5f36a..b90007047b6 100644 --- a/pkg/integrations/task_comment_test.go +++ b/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"}`) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`) + _, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "16", "commentid": "4"}, `{"comment":"Lorem Ipsum"}`) + assert.Error(t, err) + 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"}`) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`) + _, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "17", "commentid": "5"}, `{"comment":"Lorem Ipsum"}`) + assert.Error(t, err) + 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"}`) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`) + _, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "19", "commentid": "7"}, `{"comment":"Lorem Ipsum"}`) + assert.Error(t, err) + 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"}`) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`) + _, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "20", "commentid": "8"}, `{"comment":"Lorem Ipsum"}`) + assert.Error(t, err) + 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"}`) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`) + _, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "22", "commentid": "10"}, `{"comment":"Lorem Ipsum"}`) + assert.Error(t, err) + 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"}`) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`) + _, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "23", "commentid": "11"}, `{"comment":"Lorem Ipsum"}`) + assert.Error(t, err) + 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"}`) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`) + _, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "25", "commentid": "13"}, `{"comment":"Lorem Ipsum"}`) + assert.Error(t, err) + 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"}`) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `"comment":"Lorem Ipsum"`) + _, err := testHandler.testUpdateWithUser(nil, map[string]string{"task": "26", "commentid": "14"}, `{"comment":"Lorem Ipsum"}`) + assert.Error(t, err) + 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"}) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `Successfully deleted.`) + _, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "16", "commentid": "4"}) + assert.Error(t, err) + 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"}) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `Successfully deleted.`) + _, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "17", "commentid": "5"}) + assert.Error(t, err) + 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"}) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `Successfully deleted.`) + _, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "19", "commentid": "7"}) + assert.Error(t, err) + 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"}) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `Successfully deleted.`) + _, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "20", "commentid": "8"}) + assert.Error(t, err) + 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"}) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `Successfully deleted.`) + _, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "22", "commentid": "10"}) + assert.Error(t, err) + 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"}) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `Successfully deleted.`) + _, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "23", "commentid": "11"}) + assert.Error(t, err) + 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"}) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `Successfully deleted.`) + _, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "25", "commentid": "13"}) + assert.Error(t, err) + 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"}) - assert.NoError(t, err) - assert.Contains(t, rec.Body.String(), `Successfully deleted.`) + _, err := testHandler.testDeleteWithUser(nil, map[string]string{"task": "26", "commentid": "14"}) + assert.Error(t, err) + assert.Contains(t, err.(*echo.HTTPError).Message, `Forbidden`) }) }) }) diff --git a/pkg/models/task_comment_rights.go b/pkg/models/task_comment_rights.go index e24013f95fc..b8fe5107cbb 100644 --- a/pkg/models/task_comment_rights.go +++ b/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 t.CanWrite(s, a) + return tc.canUserModifyTaskComment(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 t.CanWrite(s, a) + return tc.canUserModifyTaskComment(s, a) } // CanCreate checks if a user can create a new comment diff --git a/pkg/models/task_comments.go b/pkg/models/task_comments.go index 264b8691c16..062313e3d69 100644 --- a/pkg/models/task_comments.go +++ b/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 - } - if !exists { - return ErrTaskCommentDoesNotExist{ - ID: tc.ID, - TaskID: tc.TaskID, - } + return err } // Get the author diff --git a/pkg/models/task_comments_test.go b/pkg/models/task_comments_test.go index ac9590d3cee..8befded20a6 100644 --- a/pkg/models/task_comments_test.go +++ b/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)