From 172a6214d7c30278017129b950339c78a6ddb7bc Mon Sep 17 00:00:00 2001 From: k2s Date: Sun, 12 Jun 2022 12:50:43 +0000 Subject: [PATCH 1/7] fix: VIKUNJA_SERVICE_JWT_SECRET should be VIKUNJA_SERVICE_JWTSECRET (#1184) Reviewed-on: https://kolaente.dev/vikunja/api/pulls/1184 Reviewed-by: konrad Co-authored-by: k2s Co-committed-by: k2s --- docs/content/doc/setup/config.md | 2 +- magefile.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/content/doc/setup/config.md b/docs/content/doc/setup/config.md index 9124b259bc8..2721d6e827f 100644 --- a/docs/content/doc/setup/config.md +++ b/docs/content/doc/setup/config.md @@ -76,7 +76,7 @@ Default: `` Full path: `service.JWTSecret` -Environment path: `VIKUNJA_SERVICE_JWT_SECRET` +Environment path: `VIKUNJA_SERVICE_JWTSECRET` ### jwtttl diff --git a/magefile.go b/magefile.go index 02642547353..b5a0e17fc62 100644 --- a/magefile.go +++ b/magefile.go @@ -1051,7 +1051,7 @@ func printConfig(config []*configOption, level int, parent string) (rendered str fullPath := parent + "." + option.key rendered += "Full path: `" + fullPath + "`\n\n" - rendered += "Environment path: `VIKUNJA_" + strcase.ToScreamingSnake(fullPath) + "`\n\n" + rendered += "Environment path: `VIKUNJA_" + strcase.ToScreamingSnake(strings.ToUpper(fullPath)) + "`\n\n" } } From 2f25b48869f59256bf7d692c4486c64c30b85e5e Mon Sep 17 00:00:00 2001 From: kolaente Date: Sun, 12 Jun 2022 18:29:12 +0200 Subject: [PATCH 2/7] feat: restrict max avatar size resolves #1171 --- config.yml.sample | 3 +++ docs/content/doc/setup/config.md | 12 ++++++++++++ pkg/config/config.go | 2 ++ pkg/routes/api/v1/avatar.go | 6 +++++- pkg/swagger/docs.go | 2 +- pkg/swagger/swagger.json | 2 +- pkg/swagger/swagger.yaml | 3 ++- 7 files changed, 26 insertions(+), 4 deletions(-) diff --git a/config.yml.sample b/config.yml.sample index 0b368866b38..0df19d8fd61 100644 --- a/config.yml.sample +++ b/config.yml.sample @@ -56,6 +56,9 @@ service: # it may be required to coordinate with them in order to delete the account. This setting will not affect the cli commands # for user deletion. enableuserdeletion: true + # The maximum size clients will be able to request for user avatars. + # If clients request a size bigger than this, it will be changed on the fly. + maxavatarsize: 1024 database: # Database type to use. Supported types are mysql, postgres and sqlite. diff --git a/docs/content/doc/setup/config.md b/docs/content/doc/setup/config.md index 2721d6e827f..5a9035379a5 100644 --- a/docs/content/doc/setup/config.md +++ b/docs/content/doc/setup/config.md @@ -321,6 +321,18 @@ Full path: `service.enableuserdeletion` Environment path: `VIKUNJA_SERVICE_ENABLEUSERDELETION` +### maxavatarsize + +The maximum size clients will be able to request for user avatars. +If clients request a size bigger than this, it will be changed on the fly. + +Default: `1024` + +Full path: `service.maxavatarsize` + +Environment path: `VIKUNJA_SERVICE_MAXAVATARSIZE` + + --- ## database diff --git a/pkg/config/config.go b/pkg/config/config.go index ff40e2cb45e..a8df08a5c4e 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -62,6 +62,7 @@ const ( ServiceTestingtoken Key = `service.testingtoken` ServiceEnableEmailReminders Key = `service.enableemailreminders` ServiceEnableUserDeletion Key = `service.enableuserdeletion` + ServiceMaxAvatarSize Key = `service.maxavatarsize` AuthLocalEnabled Key = `auth.local.enabled` AuthOpenIDEnabled Key = `auth.openid.enabled` @@ -287,6 +288,7 @@ func InitDefaultConfig() { ServiceEnableTotp.setDefault(true) ServiceEnableEmailReminders.setDefault(true) ServiceEnableUserDeletion.setDefault(true) + ServiceMaxAvatarSize.setDefault(1024) // Auth AuthLocalEnabled.setDefault(true) diff --git a/pkg/routes/api/v1/avatar.go b/pkg/routes/api/v1/avatar.go index 7b52254fcc3..2bf540ed17a 100644 --- a/pkg/routes/api/v1/avatar.go +++ b/pkg/routes/api/v1/avatar.go @@ -17,6 +17,7 @@ package v1 import ( + "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/files" "code.vikunja.io/api/pkg/log" @@ -49,7 +50,7 @@ import ( // @tags user // @Produce octet-stream // @Param username path string true "The username of the user who's avatar you want to get" -// @Param size query int false "The size of the avatar you want to get" +// @Param size query int false "The size of the avatar you want to get. If bigger than the max configured size this will be adjusted to the maximum size." // @Success 200 {} blob "The avatar" // @Failure 404 {object} models.Message "The user does not exist." // @Failure 500 {object} models.Message "Internal error" @@ -97,6 +98,9 @@ func GetAvatar(c echo.Context) error { return handler.HandleHTTPError(err, c) } } + if sizeInt > config.ServiceMaxAvatarSize.GetInt64() { + sizeInt = config.ServiceMaxAvatarSize.GetInt64() + } // Get the avatar a, mimeType, err := avatarProvider.GetAvatar(u, sizeInt) diff --git a/pkg/swagger/docs.go b/pkg/swagger/docs.go index 1fbf19e5289..5e8c1494f25 100644 --- a/pkg/swagger/docs.go +++ b/pkg/swagger/docs.go @@ -7537,7 +7537,7 @@ const docTemplate = `{ }, { "type": "integer", - "description": "The size of the avatar you want to get", + "description": "The size of the avatar you want to get. If bigger than the max configured size this will be adjusted to the maximum size.", "name": "size", "in": "query" } diff --git a/pkg/swagger/swagger.json b/pkg/swagger/swagger.json index a2562926127..c4f342378d8 100644 --- a/pkg/swagger/swagger.json +++ b/pkg/swagger/swagger.json @@ -7528,7 +7528,7 @@ }, { "type": "integer", - "description": "The size of the avatar you want to get", + "description": "The size of the avatar you want to get. If bigger than the max configured size this will be adjusted to the maximum size.", "name": "size", "in": "query" } diff --git a/pkg/swagger/swagger.yaml b/pkg/swagger/swagger.yaml index 46d3f79937e..a8ec87d02e2 100644 --- a/pkg/swagger/swagger.yaml +++ b/pkg/swagger/swagger.yaml @@ -1433,7 +1433,8 @@ paths: name: username required: true type: string - - description: The size of the avatar you want to get + - description: The size of the avatar you want to get. If bigger than the max + configured size this will be adjusted to the maximum size. in: query name: size type: integer From 7eb3b96a4465ca6648572b07c506c06f2c28c375 Mon Sep 17 00:00:00 2001 From: kolaente Date: Sun, 12 Jun 2022 21:24:16 +0200 Subject: [PATCH 3/7] feat: send overdue tasks email notification at 9:00 in the user's time zone --- pkg/models/task_overdue_reminder.go | 85 ++++++++++++++++-------- pkg/models/task_overdue_reminder_test.go | 29 +++++--- 2 files changed, 78 insertions(+), 36 deletions(-) diff --git a/pkg/models/task_overdue_reminder.go b/pkg/models/task_overdue_reminder.go index 4faa79930ae..6d1b7b445a8 100644 --- a/pkg/models/task_overdue_reminder.go +++ b/pkg/models/task_overdue_reminder.go @@ -19,35 +19,83 @@ package models import ( "time" - "code.vikunja.io/api/pkg/user" - "code.vikunja.io/api/pkg/config" "code.vikunja.io/api/pkg/cron" "code.vikunja.io/api/pkg/db" "code.vikunja.io/api/pkg/log" "code.vikunja.io/api/pkg/notifications" + "code.vikunja.io/api/pkg/user" "code.vikunja.io/api/pkg/utils" + "xorm.io/builder" "xorm.io/xorm" ) -func getUndoneOverdueTasks(s *xorm.Session, now time.Time) (taskIDs []int64, err error) { +func getUndoneOverdueTasks(s *xorm.Session, now time.Time) (usersWithTasks map[int64]*userWithTasks, err error) { now = utils.GetTimeWithoutNanoSeconds(now) + nextMinute := now.Add(1 * time.Minute) var tasks []*Task err = s. - Where("due_date is not null and due_date < ?", now.Format(dbTimeFormat)). + Where("due_date is not null and due_date < ?", nextMinute.Add(time.Hour*14).Format(dbTimeFormat)). And("done = false"). Find(&tasks) if err != nil { return } + if len(tasks) == 0 { + return + } + + var taskIDs []int64 for _, task := range tasks { taskIDs = append(taskIDs, task.ID) } - return + users, err := getTaskUsersForTasks(s, taskIDs, builder.Eq{"users.overdue_tasks_reminders_enabled": true}) + if err != nil { + return + } + + if len(users) == 0 { + return + } + + uts := make(map[int64]*userWithTasks) + tzs := make(map[string]*time.Location) + for _, t := range users { + if t.User.Timezone == "" { + t.User.Timezone = config.GetTimeZone().String() + } + + tz, exists := tzs[t.User.Timezone] + if !exists { + tz, err = time.LoadLocation(t.User.Timezone) + if err != nil { + return + } + tzs[t.User.Timezone] = tz + } + + // If it is 9:00 for that current user, add the task to their list of overdue tasks + overdueMailTime := time.Date(now.Year(), now.Month(), now.Day(), 9, 0, 0, 0, tz) + isTimeForReminder := overdueMailTime.After(now) || overdueMailTime.Equal(now.In(tz)) + wasTimeForReminder := overdueMailTime.Before(now.Add(time.Minute)) + taskIsOverdueInUserTimezone := overdueMailTime.After(t.Task.DueDate.In(tz)) + if isTimeForReminder && wasTimeForReminder && taskIsOverdueInUserTimezone { + _, exists := uts[t.User.ID] + if !exists { + uts[t.User.ID] = &userWithTasks{ + user: t.User, + tasks: []*Task{}, + } + } + uts[t.User.ID].tasks = append(uts[t.User.ID].tasks, t.Task) + } + } + + return uts, nil } type userWithTasks struct { @@ -66,36 +114,18 @@ func RegisterOverdueReminderCron() { return } - err := cron.Schedule("0 8 * * *", func() { + err := cron.Schedule("* * * * *", func() { s := db.NewSession() defer s.Close() now := time.Now() - taskIDs, err := getUndoneOverdueTasks(s, now) + uts, err := getUndoneOverdueTasks(s, now) if err != nil { - log.Errorf("[Undone Overdue Tasks Reminder] Could not get tasks with reminders in the next minute: %s", err) + log.Errorf("[Undone Overdue Tasks Reminder] Could not get undone overdue tasks in the next minute: %s", err) return } - users, err := getTaskUsersForTasks(s, taskIDs, builder.Eq{"users.overdue_tasks_reminders_enabled": true}) - if err != nil { - log.Errorf("[Undone Overdue Tasks Reminder] Could not get task users to send them reminders: %s", err) - return - } - - uts := make(map[int64]*userWithTasks) - for _, t := range users { - _, exists := uts[t.User.ID] - if !exists { - uts[t.User.ID] = &userWithTasks{ - user: t.User, - tasks: []*Task{}, - } - } - uts[t.User.ID].tasks = append(uts[t.User.ID].tasks, t.Task) - } - - log.Debugf("[Undone Overdue Tasks Reminder] Sending reminders to %d users", len(users)) + log.Debugf("[Undone Overdue Tasks Reminder] Sending reminders to %d users", len(uts)) for _, ut := range uts { var n notifications.Notification = &UndoneTasksOverdueNotification{ @@ -117,7 +147,6 @@ func RegisterOverdueReminderCron() { } log.Debugf("[Undone Overdue Tasks Reminder] Sent reminder email for %d tasks to user %d", len(ut.tasks), ut.user.ID) - continue } }) if err != nil { diff --git a/pkg/models/task_overdue_reminder_test.go b/pkg/models/task_overdue_reminder_test.go index 9845ed6facc..b1ca24eded0 100644 --- a/pkg/models/task_overdue_reminder_test.go +++ b/pkg/models/task_overdue_reminder_test.go @@ -32,21 +32,34 @@ func TestGetUndoneOverDueTasks(t *testing.T) { now, err := time.Parse(time.RFC3339Nano, "2018-01-01T01:13:00Z") assert.NoError(t, err) - taskIDs, err := getUndoneOverdueTasks(s, now) + tasks, err := getUndoneOverdueTasks(s, now) assert.NoError(t, err) - assert.Len(t, taskIDs, 0) + assert.Len(t, tasks, 0) }) t.Run("undone overdue", func(t *testing.T) { db.LoadAndAssertFixtures(t) s := db.NewSession() defer s.Close() - now, err := time.Parse(time.RFC3339Nano, "2018-12-01T01:13:00Z") + now, err := time.Parse(time.RFC3339Nano, "2018-12-01T09:00:00Z") assert.NoError(t, err) - taskIDs, err := getUndoneOverdueTasks(s, now) + uts, err := getUndoneOverdueTasks(s, now) assert.NoError(t, err) - assert.Len(t, taskIDs, 1) - assert.Equal(t, int64(6), taskIDs[0]) + assert.Len(t, uts, 1) + assert.Len(t, uts[1].tasks, 2) + // The tasks don't always have the same order, so we only check their presence, not their position. + var task5Present bool + var task6Present bool + for _, t := range uts[1].tasks { + if t.ID == 5 { + task5Present = true + } + if t.ID == 6 { + task6Present = true + } + } + assert.Truef(t, task5Present, "expected task 5 to be present but was not") + assert.Truef(t, task6Present, "expected task 6 to be present but was not") }) t.Run("done overdue", func(t *testing.T) { db.LoadAndAssertFixtures(t) @@ -55,8 +68,8 @@ func TestGetUndoneOverDueTasks(t *testing.T) { now, err := time.Parse(time.RFC3339Nano, "2018-11-01T01:13:00Z") assert.NoError(t, err) - taskIDs, err := getUndoneOverdueTasks(s, now) + tasks, err := getUndoneOverdueTasks(s, now) assert.NoError(t, err) - assert.Len(t, taskIDs, 0) + assert.Len(t, tasks, 0) }) } From 030bbfa47e1448e3275e97d1e160cd93def0e900 Mon Sep 17 00:00:00 2001 From: renovate Date: Thu, 16 Jun 2022 12:47:17 +0000 Subject: [PATCH 4/7] fix(deps): update module github.com/swaggo/swag to v1.8.3 (#1185) Reviewed-on: https://kolaente.dev/vikunja/api/pulls/1185 Co-authored-by: renovate Co-committed-by: renovate --- go.mod | 2 +- go.sum | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/go.mod b/go.mod index 21c8cdafe1e..b2f1d9ea6bb 100644 --- a/go.mod +++ b/go.mod @@ -57,7 +57,7 @@ require ( github.com/spf13/cobra v1.4.0 github.com/spf13/viper v1.11.0 github.com/stretchr/testify v1.7.2 - github.com/swaggo/swag v1.8.2 + github.com/swaggo/swag v1.8.3 github.com/tkuchiki/go-timezone v0.2.2 github.com/ulule/limiter/v3 v3.10.0 github.com/vectordotdev/go-datemath v0.1.1-0.20211214182920-0a4ac8742b93 diff --git a/go.sum b/go.sum index 58156edafe0..60a7da55a34 100644 --- a/go.sum +++ b/go.sum @@ -756,6 +756,8 @@ github.com/swaggo/swag v1.8.1 h1:JuARzFX1Z1njbCGz+ZytBR15TFJwF2Q7fu8puJHhQYI= github.com/swaggo/swag v1.8.1/go.mod h1:ugemnJsPZm/kRwFUnzBlbHRd0JY9zE1M4F+uy2pAaPQ= github.com/swaggo/swag v1.8.2 h1:D4aBiVS2a65zhyk3WFqOUz7Rz0sOaUcgeErcid5uGL4= github.com/swaggo/swag v1.8.2/go.mod h1:jMLeXOOmYyjk8PvHTsXBdrubsNd9gUJTTCzL5iBnseg= +github.com/swaggo/swag v1.8.3 h1:3pZSSCQ//gAH88lfmxM3Cd1+JCsxV8Md6f36b9hrZ5s= +github.com/swaggo/swag v1.8.3/go.mod h1:jMLeXOOmYyjk8PvHTsXBdrubsNd9gUJTTCzL5iBnseg= github.com/syndtr/goleveldb v1.0.0 h1:fBdIW9lB4Iz0n9khmH8w27SJ3QEJ7+IgjPEwGSZiFdE= github.com/syndtr/goleveldb v1.0.0/go.mod h1:ZVVdQEZoIme9iO1Ch2Jdy24qqXrMMOU6lpPAyBWyWuQ= github.com/tkuchiki/go-timezone v0.2.2 h1:MdHR65KwgVTwWFQrota4SKzc4L5EfuH5SdZZGtk/P2Q= From 8869adfc276f674b686bf68f949d7efbb417e55b Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 16 Jun 2022 16:20:26 +0200 Subject: [PATCH 5/7] feat: add setting to change overdue tasks reminder email time --- pkg/models/label_task_test.go | 1 + pkg/models/label_test.go | 4 +++ pkg/models/list_users_test.go | 2 ++ pkg/models/namespace_users_test.go | 2 ++ pkg/models/task_collection_test.go | 3 ++ pkg/models/task_overdue_reminder.go | 12 ++++--- pkg/models/task_reminder.go | 2 +- pkg/models/users_list_test.go | 13 +++++++ pkg/routes/api/v1/user_settings.go | 18 ++++++++-- pkg/routes/api/v1/user_show.go | 1 + pkg/routes/routes.go | 26 -------------- pkg/routes/validation.go | 55 +++++++++++++++++++++++++++++ pkg/swagger/docs.go | 4 +++ pkg/swagger/swagger.json | 4 +++ pkg/swagger/swagger.yaml | 4 +++ pkg/user/user.go | 2 ++ pkg/utils/time.go | 9 +++++ 17 files changed, 129 insertions(+), 33 deletions(-) create mode 100644 pkg/routes/validation.go diff --git a/pkg/models/label_task_test.go b/pkg/models/label_task_test.go index 925358bc321..451e308e3fe 100644 --- a/pkg/models/label_task_test.go +++ b/pkg/models/label_task_test.go @@ -43,6 +43,7 @@ func TestLabelTask_ReadAll(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, }, diff --git a/pkg/models/label_test.go b/pkg/models/label_test.go index 52151d01fa2..a88cedb7a7e 100644 --- a/pkg/models/label_test.go +++ b/pkg/models/label_test.go @@ -54,6 +54,7 @@ func TestLabel_ReadAll(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -104,6 +105,7 @@ func TestLabel_ReadAll(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, }, @@ -168,6 +170,7 @@ func TestLabel_ReadOne(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -229,6 +232,7 @@ func TestLabel_ReadOne(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, }, diff --git a/pkg/models/list_users_test.go b/pkg/models/list_users_test.go index 3ee8722f036..7ba9d44379b 100644 --- a/pkg/models/list_users_test.go +++ b/pkg/models/list_users_test.go @@ -151,6 +151,7 @@ func TestListUser_ReadAll(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, }, @@ -164,6 +165,7 @@ func TestListUser_ReadAll(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, }, diff --git a/pkg/models/namespace_users_test.go b/pkg/models/namespace_users_test.go index 0cc288bd0fb..32f4d8b9dac 100644 --- a/pkg/models/namespace_users_test.go +++ b/pkg/models/namespace_users_test.go @@ -150,6 +150,7 @@ func TestNamespaceUser_ReadAll(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, }, @@ -163,6 +164,7 @@ func TestNamespaceUser_ReadAll(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, }, diff --git a/pkg/models/task_collection_test.go b/pkg/models/task_collection_test.go index 97f5f1ea82c..b15d77fec3d 100644 --- a/pkg/models/task_collection_test.go +++ b/pkg/models/task_collection_test.go @@ -37,6 +37,7 @@ func TestTaskCollection_ReadAll(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -47,6 +48,7 @@ func TestTaskCollection_ReadAll(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -57,6 +59,7 @@ func TestTaskCollection_ReadAll(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } diff --git a/pkg/models/task_overdue_reminder.go b/pkg/models/task_overdue_reminder.go index 6d1b7b445a8..a96db9585bb 100644 --- a/pkg/models/task_overdue_reminder.go +++ b/pkg/models/task_overdue_reminder.go @@ -32,7 +32,7 @@ import ( ) func getUndoneOverdueTasks(s *xorm.Session, now time.Time) (usersWithTasks map[int64]*userWithTasks, err error) { - now = utils.GetTimeWithoutNanoSeconds(now) + now = utils.GetTimeWithoutSeconds(now) nextMinute := now.Add(1 * time.Minute) var tasks []*Task @@ -78,10 +78,14 @@ func getUndoneOverdueTasks(s *xorm.Session, now time.Time) (usersWithTasks map[i tzs[t.User.Timezone] = tz } - // If it is 9:00 for that current user, add the task to their list of overdue tasks - overdueMailTime := time.Date(now.Year(), now.Month(), now.Day(), 9, 0, 0, 0, tz) + // If it is time for that current user, add the task to their list of overdue tasks + tm, err := time.Parse("15:04", t.User.OverdueTasksRemindersTime) + if err != nil { + return nil, err + } + overdueMailTime := time.Date(now.Year(), now.Month(), now.Day(), tm.Hour(), tm.Minute(), 0, 0, tz) isTimeForReminder := overdueMailTime.After(now) || overdueMailTime.Equal(now.In(tz)) - wasTimeForReminder := overdueMailTime.Before(now.Add(time.Minute)) + wasTimeForReminder := overdueMailTime.Before(nextMinute) taskIsOverdueInUserTimezone := overdueMailTime.After(t.Task.DueDate.In(tz)) if isTimeForReminder && wasTimeForReminder && taskIsOverdueInUserTimezone { _, exists := uts[t.User.ID] diff --git a/pkg/models/task_reminder.go b/pkg/models/task_reminder.go index 620d1ab9ec9..42b337b6cac 100644 --- a/pkg/models/task_reminder.go +++ b/pkg/models/task_reminder.go @@ -61,7 +61,7 @@ func getTaskUsersForTasks(s *xorm.Session, taskIDs []int64, cond builder.Cond) ( // Get all creators of tasks creators := make(map[int64]*user.User, len(taskIDs)) err = s. - Select("users.id, users.username, users.email, users.name, users.timezone"). + Select("users.*"). Join("LEFT", "tasks", "tasks.created_by_id = users.id"). In("tasks.id", taskIDs). Where(cond). diff --git a/pkg/models/users_list_test.go b/pkg/models/users_list_test.go index cf0ed846efc..1a48c9de4e8 100644 --- a/pkg/models/users_list_test.go +++ b/pkg/models/users_list_test.go @@ -32,6 +32,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -42,6 +43,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -52,6 +54,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -63,6 +66,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -74,6 +78,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -84,6 +89,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -95,6 +101,7 @@ func TestListUsersFromList(t *testing.T) { EmailRemindersEnabled: true, DiscoverableByEmail: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -105,6 +112,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -115,6 +123,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -125,6 +134,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -136,6 +146,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -148,6 +159,7 @@ func TestListUsersFromList(t *testing.T) { EmailRemindersEnabled: true, DiscoverableByName: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } @@ -158,6 +170,7 @@ func TestListUsersFromList(t *testing.T) { Issuer: "local", EmailRemindersEnabled: true, OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", Created: testCreatedTime, Updated: testUpdatedTime, } diff --git a/pkg/routes/api/v1/user_settings.go b/pkg/routes/api/v1/user_settings.go index 157160aad7c..536a427a1d3 100644 --- a/pkg/routes/api/v1/user_settings.go +++ b/pkg/routes/api/v1/user_settings.go @@ -17,6 +17,8 @@ package v1 import ( + "errors" + "fmt" "net/http" "github.com/labstack/echo/v4" @@ -46,11 +48,13 @@ type UserSettings struct { DiscoverableByEmail bool `json:"discoverable_by_email"` // If enabled, the user will get an email for their overdue tasks each morning. OverdueTasksRemindersEnabled bool `json:"overdue_tasks_reminders_enabled"` + // The time when the daily summary of overdue tasks will be sent via email. + OverdueTasksRemindersTime string `json:"overdue_tasks_reminders_time" valid:"time,required"` // If a task is created without a specified list this value should be used. Applies // to tasks made directly in API and from clients. DefaultListID int64 `json:"default_list_id"` // The day when the week starts for this user. 0 = sunday, 1 = monday, etc. - WeekStart int `json:"week_start"` + WeekStart int `json:"week_start" valid:"range(0|7)"` // The user's language Language string `json:"language"` // The user's time zone. Used to send task reminders in the time zone of the user. @@ -158,7 +162,16 @@ func UpdateGeneralUserSettings(c echo.Context) error { us := &UserSettings{} err := c.Bind(us) if err != nil { - return echo.NewHTTPError(http.StatusBadRequest, "Bad user name provided.") + var he *echo.HTTPError + if errors.As(err, &he) { + return echo.NewHTTPError(http.StatusBadRequest, fmt.Sprintf("Invalid model provided. Error was: %s", he.Message)) + } + return echo.NewHTTPError(http.StatusBadRequest, "Invalid model provided.") + } + + err = c.Validate(us) + if err != nil { + return echo.NewHTTPError(http.StatusBadRequest, err) } u, err := user2.GetCurrentUser(c) @@ -184,6 +197,7 @@ func UpdateGeneralUserSettings(c echo.Context) error { user.WeekStart = us.WeekStart user.Language = us.Language user.Timezone = us.Timezone + user.OverdueTasksRemindersTime = us.OverdueTasksRemindersTime _, err = user2.UpdateUser(s, user) if err != nil { diff --git a/pkg/routes/api/v1/user_show.go b/pkg/routes/api/v1/user_show.go index 9bcdb348a83..d1e6f1da79b 100644 --- a/pkg/routes/api/v1/user_show.go +++ b/pkg/routes/api/v1/user_show.go @@ -75,6 +75,7 @@ func UserShow(c echo.Context) error { WeekStart: u.WeekStart, Language: u.Language, Timezone: u.Timezone, + OverdueTasksRemindersTime: u.OverdueTasksRemindersTime, }, DeletionScheduledAt: u.DeletionScheduledAt, IsLocalUser: u.Issuer == user.IssuerLocal, diff --git a/pkg/routes/routes.go b/pkg/routes/routes.go index 9c27f9c9692..0e810929b1a 100644 --- a/pkg/routes/routes.go +++ b/pkg/routes/routes.go @@ -79,7 +79,6 @@ import ( "code.vikunja.io/web" "code.vikunja.io/web/handler" - "github.com/asaskevich/govalidator" "github.com/getsentry/sentry-go" sentryecho "github.com/getsentry/sentry-go/echo" "github.com/golang-jwt/jwt/v4" @@ -88,31 +87,6 @@ import ( elog "github.com/labstack/gommon/log" ) -// CustomValidator is a dummy struct to use govalidator with echo -type CustomValidator struct{} - -// Validate validates stuff -func (cv *CustomValidator) Validate(i interface{}) error { - if _, err := govalidator.ValidateStruct(i); err != nil { - - var errs []string - for field, e := range govalidator.ErrorsByField(err) { - errs = append(errs, field+": "+e) - } - - httperr := models.ValidationHTTPError{ - HTTPError: web.HTTPError{ - Code: models.ErrCodeInvalidData, - Message: "Invalid Data", - }, - InvalidFields: errs, - } - - return httperr - } - return nil -} - // NewEcho registers a new Echo instance func NewEcho() *echo.Echo { e := echo.New() diff --git a/pkg/routes/validation.go b/pkg/routes/validation.go new file mode 100644 index 00000000000..38de55db072 --- /dev/null +++ b/pkg/routes/validation.go @@ -0,0 +1,55 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-2021 Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public Licensee as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public Licensee for more details. +// +// You should have received a copy of the GNU Affero General Public Licensee +// along with this program. If not, see . + +package routes + +import ( + "code.vikunja.io/api/pkg/models" + + "code.vikunja.io/web" + "github.com/asaskevich/govalidator" +) + +// CustomValidator is a dummy struct to use govalidator with echo +type CustomValidator struct{} + +func init() { + govalidator.TagMap["time"] = govalidator.Validator(func(str string) bool { + return govalidator.IsTime(str, "15:04") + }) +} + +// Validate validates stuff +func (cv *CustomValidator) Validate(i interface{}) error { + if _, err := govalidator.ValidateStruct(i); err != nil { + + var errs []string + for field, e := range govalidator.ErrorsByField(err) { + errs = append(errs, field+": "+e) + } + + httperr := models.ValidationHTTPError{ + HTTPError: web.HTTPError{ + Code: models.ErrCodeInvalidData, + Message: "Invalid Data", + }, + InvalidFields: errs, + } + + return httperr + } + return nil +} diff --git a/pkg/swagger/docs.go b/pkg/swagger/docs.go index 5e8c1494f25..d7a0e6b32b0 100644 --- a/pkg/swagger/docs.go +++ b/pkg/swagger/docs.go @@ -9184,6 +9184,10 @@ const docTemplate = `{ "description": "If enabled, the user will get an email for their overdue tasks each morning.", "type": "boolean" }, + "overdue_tasks_reminders_time": { + "description": "The time when the daily summary of overdue tasks will be sent via email.", + "type": "string" + }, "timezone": { "description": "The user's time zone. Used to send task reminders in the time zone of the user.", "type": "string" diff --git a/pkg/swagger/swagger.json b/pkg/swagger/swagger.json index c4f342378d8..19a08ceae5a 100644 --- a/pkg/swagger/swagger.json +++ b/pkg/swagger/swagger.json @@ -9175,6 +9175,10 @@ "description": "If enabled, the user will get an email for their overdue tasks each morning.", "type": "boolean" }, + "overdue_tasks_reminders_time": { + "description": "The time when the daily summary of overdue tasks will be sent via email.", + "type": "string" + }, "timezone": { "description": "The user's time zone. Used to send task reminders in the time zone of the user.", "type": "string" diff --git a/pkg/swagger/swagger.yaml b/pkg/swagger/swagger.yaml index a8ec87d02e2..c5cb34ceed4 100644 --- a/pkg/swagger/swagger.yaml +++ b/pkg/swagger/swagger.yaml @@ -1310,6 +1310,10 @@ definitions: description: If enabled, the user will get an email for their overdue tasks each morning. type: boolean + overdue_tasks_reminders_time: + description: The time when the daily summary of overdue tasks will be sent + via email. + type: string timezone: description: The user's time zone. Used to send task reminders in the time zone of the user. diff --git a/pkg/user/user.go b/pkg/user/user.go index 7be296e9acf..802fb27cea1 100644 --- a/pkg/user/user.go +++ b/pkg/user/user.go @@ -94,6 +94,7 @@ type User struct { DiscoverableByName bool `xorm:"bool default false index" json:"-"` DiscoverableByEmail bool `xorm:"bool default false index" json:"-"` OverdueTasksRemindersEnabled bool `xorm:"bool default true index" json:"-"` + OverdueTasksRemindersTime string `xorm:"varchar(5) not null default '09:00'" json:"-"` DefaultListID int64 `xorm:"bigint null index" json:"-"` WeekStart int `xorm:"null" json:"-"` Language string `xorm:"varchar(50) null" json:"-"` @@ -493,6 +494,7 @@ func UpdateUser(s *xorm.Session, user *User) (updatedUser *User, err error) { "week_start", "language", "timezone", + "overdue_tasks_reminders_time", ). Update(user) if err != nil { diff --git a/pkg/utils/time.go b/pkg/utils/time.go index dbd2f3cb2db..00e3fb6ea4e 100644 --- a/pkg/utils/time.go +++ b/pkg/utils/time.go @@ -30,3 +30,12 @@ func GetTimeWithoutNanoSeconds(t time.Time) time.Time { // so we make sure the time we use to get the reminders don't contain nanoseconds. return time.Date(t.Year(), t.Month(), t.Day(), t.Hour(), t.Minute(), t.Second(), 0, t.Location()).In(tz) } + +// GetTimeWithoutSeconds returns a time.Time with the seconds set to 0. +func GetTimeWithoutSeconds(t time.Time) time.Time { + tz := config.GetTimeZone() + + // By default, time.Now() includes nanoseconds which we don't save. That results in getting the wrong dates, + // so we make sure the time we use to get the reminders don't contain nanoseconds. + return time.Date(t.Year(), t.Month(), t.Day(), t.Hour(), t.Minute(), 0, 0, t.Location()).In(tz) +} From d837f8a6248b5ff2700a4bfc300d7f9d466cb918 Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 16 Jun 2022 16:56:35 +0200 Subject: [PATCH 6/7] fix: add missing migration --- pkg/migration/20220616145228.go | 43 +++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 pkg/migration/20220616145228.go diff --git a/pkg/migration/20220616145228.go b/pkg/migration/20220616145228.go new file mode 100644 index 00000000000..3e8ce2a066b --- /dev/null +++ b/pkg/migration/20220616145228.go @@ -0,0 +1,43 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-2021 Vikunja and contributors. All rights reserved. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public Licensee as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public Licensee for more details. +// +// You should have received a copy of the GNU Affero General Public Licensee +// along with this program. If not, see . + +package migration + +import ( + "src.techknowlogick.com/xormigrate" + "xorm.io/xorm" +) + +type users20220616145228 struct { + OverdueTasksRemindersTime string `xorm:"varchar(5) not null default '09:00'" json:"-"` +} + +func (users20220616145228) TableName() string { + return "users" +} + +func init() { + migrations = append(migrations, &xormigrate.Migration{ + ID: "20220616145228", + Description: "Add overdue task summary time field to users", + Migrate: func(tx *xorm.Engine) error { + return tx.Sync2(users20220616145228{}) + }, + Rollback: func(tx *xorm.Engine) error { + return nil + }, + }) +} From 01271c4c0111b3b040dcb9a0d502d31078ad6d4b Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 16 Jun 2022 17:38:27 +0200 Subject: [PATCH 7/7] feat: allow only the authors of task comments to edit them --- pkg/integrations/task_comment_test.go | 98 ++++++++++++++------------- pkg/models/task_comment_rights.go | 28 ++++++-- pkg/models/task_comments.go | 28 +++++--- pkg/models/task_comments_test.go | 22 +++++- 4 files changed, 115 insertions(+), 61 deletions(-) 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)