diff --git a/pkg/db/fixtures/labels.yml b/pkg/db/fixtures/labels.yml index d685d6392..c84663d34 100644 --- a/pkg/db/fixtures/labels.yml +++ b/pkg/db/fixtures/labels.yml @@ -43,3 +43,11 @@ created_by_id: 1 updated: 2018-12-02 15:13:12 created: 2018-12-01 15:13:12 +# Covers the bot-owner branch: created by bot 23, whose owner is user 21. +# User 21 should be able to read/update/delete it; user 22 (who owns bot 24) +# should not. +- id: 9 + title: 'Label #9 - created by bot 23 owned by user 21' + created_by_id: 23 + updated: 2018-12-02 15:13:12 + created: 2018-12-01 15:13:12 diff --git a/pkg/models/label_permissions.go b/pkg/models/label_permissions.go index 75468d1bb..2854cbf7b 100644 --- a/pkg/models/label_permissions.go +++ b/pkg/models/label_permissions.go @@ -17,6 +17,7 @@ package models import ( + "code.vikunja.io/api/pkg/user" "code.vikunja.io/api/pkg/web" "xorm.io/builder" "xorm.io/xorm" @@ -57,7 +58,19 @@ func (l *Label) isLabelOwner(s *xorm.Session, a web.Auth) (bool, error) { if err != nil { return false, err } - return lorig.CreatedByID == a.GetID(), nil + if lorig.CreatedByID == a.GetID() { + return true, nil + } + + // A bot owner inherits write/delete access to labels their bots created. + creator, err := user.GetUserByID(s, lorig.CreatedByID) + if err != nil { + if user.IsErrUserDoesNotExist(err) { + return false, nil + } + return false, err + } + return creator.IsBot() && creator.BotOwnerID == a.GetID(), nil } // hasAccessToLabel reports whether the caller can read a label and, if so, @@ -91,7 +104,12 @@ func (l *Label) hasAccessToLabel(s *xorm.Session, a web.Auth) (has bool, maxPerm accessBranches := []builder.Cond{labelAttachedToAccessibleTask} if !isLinkShare { - accessBranches = append(accessBranches, builder.Eq{"labels.created_by_id": a.GetID()}) + accessBranches = append(accessBranches, + builder.Eq{"labels.created_by_id": a.GetID()}, + builder.In("labels.created_by_id", + builder.Select("id").From("users").Where(builder.Eq{"bot_owner_id": a.GetID()}), + ), + ) } cond := builder.And( diff --git a/pkg/models/label_task.go b/pkg/models/label_task.go index ad8b46c2c..312f5d027 100644 --- a/pkg/models/label_task.go +++ b/pkg/models/label_task.go @@ -212,7 +212,12 @@ func GetLabelsByTaskIDs(s *xorm.Session, opts *LabelByTaskIDsOptions) (ls []*Lab ), cond) } if opts.GetUnusedLabels && !isLinkShareAuth { - cond = builder.Or(cond, builder.Eq{"labels.created_by_id": opts.User.GetID()}) + cond = builder.Or(cond, + builder.Eq{"labels.created_by_id": opts.User.GetID()}, + builder.In("labels.created_by_id", + builder.Select("id").From("users").Where(builder.Eq{"bot_owner_id": opts.User.GetID()}), + ), + ) } ids := []int64{} diff --git a/pkg/models/label_test.go b/pkg/models/label_test.go index 5320678b3..02bff9f06 100644 --- a/pkg/models/label_test.go +++ b/pkg/models/label_test.go @@ -336,6 +336,46 @@ func TestLabel_ReadOne(t *testing.T) { wantForbidden: true, auth: &user.User{ID: 1}, }, + { + // Label 9 was created by bot 23, whose owner is user 21. The + // bot owner inherits admin-level access. + name: "bot owner can read label created by their bot", + fields: fields{ + ID: 9, + }, + want: &Label{ + ID: 9, + Title: "Label #9 - created by bot 23 owned by user 21", + CreatedByID: 23, + CreatedBy: &user.User{ + ID: 23, + Name: "Owner A Assistant", + Username: "bot-owner-a-assistant", + Issuer: "local", + BotOwnerID: 21, + EmailRemindersEnabled: true, + OverdueTasksRemindersEnabled: true, + OverdueTasksRemindersTime: "09:00", + Created: testCreatedTime, + Updated: testUpdatedTime, + }, + Created: testCreatedTime, + Updated: testUpdatedTime, + }, + auth: &user.User{ID: 21}, + assertMaxPermission: true, + wantMaxPermission: int(PermissionAdmin), + }, + { + // User 22 owns a different bot and must not see another owner's + // bot's label. + name: "non-owner cannot read label created by someone else's bot", + fields: fields{ + ID: 9, + }, + wantForbidden: true, + auth: &user.User{ID: 22}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -507,6 +547,27 @@ func TestLabel_Update(t *testing.T) { auth: &user.User{ID: 1}, wantForbidden: true, }, + { + // Label 9 was created by bot 23 (owned by user 21). The bot's + // owner inherits update permission. + name: "bot owner can update label created by their bot", + fields: fields{ + ID: 9, + Title: "new and better", + }, + auth: &user.User{ID: 21}, + }, + { + // User 22 owns a different bot and must not be able to update + // another owner's bot's label. + name: "non-owner cannot update label created by someone else's bot", + fields: fields{ + ID: 9, + Title: "new and better", + }, + auth: &user.User{ID: 22}, + wantForbidden: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -594,6 +655,25 @@ func TestLabel_Delete(t *testing.T) { auth: &user.User{ID: 1}, wantForbidden: true, }, + { + // Label 9 was created by bot 23 (owned by user 21). The bot's + // owner inherits delete permission. + name: "bot owner can delete label created by their bot", + fields: fields{ + ID: 9, + }, + auth: &user.User{ID: 21}, + }, + { + // User 22 owns a different bot and must not be able to delete + // another owner's bot's label. + name: "non-owner cannot delete label created by someone else's bot", + fields: fields{ + ID: 9, + }, + auth: &user.User{ID: 22}, + wantForbidden: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/webtests/huma_label_test.go b/pkg/webtests/huma_label_test.go index 02b314dc5..eb59ea675 100644 --- a/pkg/webtests/huma_label_test.go +++ b/pkg/webtests/huma_label_test.go @@ -24,10 +24,17 @@ import ( "strings" "testing" + "code.vikunja.io/api/pkg/user" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// testuser22 is the second bot owner from pkg/db/fixtures/users.yml; user22 +// owns bot 24. Paired with testuser21 to assert bot-owner isolation: each +// owner sees and acts on their own bots' resources, never the other's. +var testuser22 = user.User{ID: 22, Username: "user_bot_owner_b", Issuer: "local"} + // TestHumaLabel mirrors v1's TestProject shape so v2 contract parity is // readable side-by-side. Labels has no v1 webtest; coverage is ported 1:1 // from the model-level matrix in pkg/models/label_test.go so the v2 HTTP @@ -228,6 +235,65 @@ func TestHumaLabel(t *testing.T) { }) } +// TestHumaLabel_BotOwner asserts that bot owners can read, update, and delete +// labels that were created by bots they own. Fixture label #9 is owned by +// bot 23, whose owner is user 21 (testuser21); user 22 owns a different bot +// and must not see or touch it. +func TestHumaLabel_BotOwner(t *testing.T) { + botOwner := webHandlerTestV2{ + user: &testuser21, + basePath: "/api/v2/labels", + idParam: "label", + t: t, + } + require.NoError(t, botOwner.ensureEnv()) + otherOwner := webHandlerTestV2{ + user: &testuser22, + basePath: "/api/v2/labels", + idParam: "label", + t: t, + e: botOwner.e, + } + + t.Run("ReadOne - bot owner can read label created by their bot", func(t *testing.T) { + rec, err := botOwner.testReadOneWithUser(nil, map[string]string{"label": "9"}) + require.NoError(t, err) + assert.Contains(t, rec.Body.String(), `"title":"Label #9 - created by bot 23 owned by user 21"`) + }) + t.Run("ReadOne - non-owner cannot read another owner's bot's label", func(t *testing.T) { + _, err := otherOwner.testReadOneWithUser(nil, map[string]string{"label": "9"}) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("ReadAll - bot owner's listing surfaces their bot's labels", func(t *testing.T) { + rec, err := botOwner.testReadAllWithUser(nil, nil) + require.NoError(t, err) + ids := labelIDsFromReadAll(t, rec.Body.Bytes()) + assert.Contains(t, ids, int64(9), "label #9 (created by user 21's bot) must be listed") + }) + t.Run("Update - bot owner can update label created by their bot", func(t *testing.T) { + rec, err := botOwner.testUpdateWithUser(nil, map[string]string{"label": "9"}, `{"title":"renamed by owner"}`) + require.NoError(t, err) + assert.Contains(t, rec.Body.String(), `"title":"renamed by owner"`) + }) + t.Run("Update - non-owner cannot update another owner's bot's label", func(t *testing.T) { + _, err := otherOwner.testUpdateWithUser(nil, map[string]string{"label": "9"}, `{"title":"hijack"}`) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("Delete - non-owner cannot delete another owner's bot's label", func(t *testing.T) { + _, err := otherOwner.testDeleteWithUser(nil, map[string]string{"label": "9"}) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("Delete - bot owner can delete label created by their bot", func(t *testing.T) { + // Run last so the earlier subtests still have label #9 to operate on. + rec, err := botOwner.testDeleteWithUser(nil, map[string]string{"label": "9"}) + require.NoError(t, err) + assert.Equal(t, http.StatusNoContent, rec.Code) + }) +} + // labelIDsFromReadAll extracts the label IDs from a v2 paginated list body so // the visible set can be asserted exactly rather than via substring matching. func labelIDsFromReadAll(t *testing.T, body []byte) []int64 {