diff --git a/pkg/db/fixtures/webhooks.yml b/pkg/db/fixtures/webhooks.yml index 4ec5687c7..983a03aff 100644 --- a/pkg/db/fixtures/webhooks.yml +++ b/pkg/db/fixtures/webhooks.yml @@ -41,3 +41,41 @@ created_by_id: 3 created: 2024-01-01 00:00:00 updated: 2024-01-01 00:00:00 +# Webhooks 6-8 are user-level (project_id null, user_id set) and back the v2 +# user-webhook tests. #6/#7 belong to user6; #6 carries credentials so masking +# can be asserted. #8 belongs to user1 so the owner-isolation check (user6 must +# not see or mutate another user's webhook) has a target. +# +# Event choice matters because the pkg/e2etests user-webhook suite shares these +# fixtures and dispatches real events. The WebhookListener fans a fired event out +# to ALL of the event-user's webhooks, asynchronously; a user-level fixture +# subscribed to a user-directed event the suite dispatches for its owner fires a +# real (failing) delivery to example.com, and that in-flight write then races the +# next test's fixture reload ("database table is locked: webhooks"). The suite +# dispatches user-directed events only for user1, so #6/#7 are owned by user6, and +# #8 (owned by user1) subscribes to task.updated — a project-only event the +# listener never matches for user webhooks. None of the three can fire there. +- id: 6 + target_url: "https://example.com/user-webhook-fixture" + events: '["task.reminder.fired"]' + user_id: 6 + secret: "uwh-secret-fixture" + basic_auth_user: "uwh-basicauth-user" + basic_auth_password: "uwh-basicauth-pass" + created_by_id: 6 + created: 2024-01-01 00:00:00 + updated: 2024-01-01 00:00:00 +- id: 7 + target_url: "https://example.com/user-webhook-second" + events: '["task.reminder.fired"]' + user_id: 6 + created_by_id: 6 + created: 2024-01-01 00:00:00 + updated: 2024-01-01 00:00:00 +- id: 8 + target_url: "https://example.com/user-webhook-other" + events: '["task.updated"]' + user_id: 1 + created_by_id: 1 + created: 2024-01-01 00:00:00 + updated: 2024-01-01 00:00:00 diff --git a/pkg/models/webhooks.go b/pkg/models/webhooks.go index fd7ee8e81..b4038bf6c 100644 --- a/pkg/models/webhooks.go +++ b/pkg/models/webhooks.go @@ -40,6 +40,7 @@ import ( "code.vikunja.io/api/pkg/version" "code.vikunja.io/api/pkg/web" + "xorm.io/builder" "xorm.io/xorm" ) @@ -216,24 +217,36 @@ func (w *Webhook) Create(s *xorm.Session, a web.Auth) (err error) { // @Failure 500 {object} models.Message "Internal server error" // @Router /projects/{id}/webhooks [get] func (w *Webhook) ReadAll(s *xorm.Session, a web.Auth, _ string, page int, perPage int) (result interface{}, resultCount int, numberOfTotalItems int64, err error) { - p := &Project{ID: w.ProjectID} - can, _, err := p.CanRead(s, a) - if err != nil { - return nil, 0, 0, err - } - if !can { - return nil, 0, 0, ErrGenericForbidden{} + // w.UserID set selects the user-level list: a user may only see their own + // webhooks. The project list (w.UserID == 0) delegates to the project's read + // permission instead. + var listCond builder.Cond + if w.UserID > 0 { + if _, isShareAuth := a.(*LinkSharing); isShareAuth || w.UserID != a.GetID() { + return nil, 0, 0, ErrGenericForbidden{} + } + listCond = builder.Eq{"user_id": w.UserID} + } else { + p := &Project{ID: w.ProjectID} + can, _, cerr := p.CanRead(s, a) + if cerr != nil { + return nil, 0, 0, cerr + } + if !can { + return nil, 0, 0, ErrGenericForbidden{} + } + listCond = builder.Eq{"project_id": w.ProjectID} } ws := []*Webhook{} - err = s.Where("project_id = ?", w.ProjectID). + err = s.Where(listCond). Limit(getLimitFromPageIndex(page, perPage)). Find(&ws) if err != nil { return } - total, err := s.Where("project_id = ?", w.ProjectID). + total, err := s.Where(listCond). Count(&Webhook{}) if err != nil { return diff --git a/pkg/routes/api/v2/user_webhooks.go b/pkg/routes/api/v2/user_webhooks.go new file mode 100644 index 000000000..b35407c79 --- /dev/null +++ b/pkg/routes/api/v2/user_webhooks.go @@ -0,0 +1,167 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present 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 License 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 License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package apiv2 + +import ( + "context" + "fmt" + "net/http" + + "code.vikunja.io/api/pkg/config" + "code.vikunja.io/api/pkg/models" + "code.vikunja.io/api/pkg/web/handler" + + "github.com/danielgtaylor/huma/v2" +) + +// models.Webhook.ReadAll returns []*models.Webhook, so that's the element type. +type userWebhookListBody struct { + Body Paginated[*models.Webhook] +} + +type userWebhookEventsBody struct { + Body []string +} + +// RegisterUserWebhookRoutes wires the per-user webhook CRUD onto the Huma API. +// User webhooks are the project-less sibling of the project webhooks (see +// webhooks.go): they fire across all of a user's projects and are owned by the +// user, not a project. Both resources share the webhooks.enabled gate; the check +// runs here (not at init()) because RegisterAll fires after config is loaded. +// Like project webhooks there is deliberately no ReadOne — webhooks carry +// credentials — so AutoPatch synthesises no PATCH and update is PUT only. +func RegisterUserWebhookRoutes(api huma.API) { + if !config.WebhooksEnabled.GetBool() { + return + } + + tags := []string{"webhooks"} + + Register(api, huma.Operation{ + OperationID: "user-webhooks-list", + Summary: "List the current user's webhooks", + Description: "Returns the webhook targets the authenticated user has configured for themselves (not project webhooks), paginated. Secret and basic-auth credentials are never included.", + Method: http.MethodGet, + Path: "/user/settings/webhooks", + Tags: tags, + }, userWebhooksList) + + Register(api, huma.Operation{ + OperationID: "user-webhooks-events", + Summary: "List available user-directed webhook events", + Description: "Returns the webhook event names a user-level webhook may subscribe to. This is a subset of the project webhook events — only events that target a single user.", + Method: http.MethodGet, + Path: "/user/settings/webhooks/events", + Tags: tags, + }, userWebhooksEvents) + + Register(api, huma.Operation{ + OperationID: "user-webhooks-create", + Summary: "Create a webhook for the current user", + Description: "Creates a webhook target owned by the authenticated user that receives POST requests across all of their projects. The owning user is taken from the token, not the body. May only subscribe to user-directed events (see the events route). The secret and basic-auth credentials are write-only and not returned in the response.", + Method: http.MethodPost, + Path: "/user/settings/webhooks", + Tags: tags, + }, userWebhooksCreate) + + Register(api, huma.Operation{ + OperationID: "user-webhooks-update", + Summary: "Update a user webhook's events", + Description: "Changes the events a user webhook subscribes to. Only the events list can be changed; target_url, secret and auth are immutable after creation. Only the owning user may update it.", + Method: http.MethodPut, + Path: "/user/settings/webhooks/{webhook}", + Tags: tags, + }, userWebhooksUpdate) + + Register(api, huma.Operation{ + OperationID: "user-webhooks-delete", + Summary: "Delete a user webhook", + Description: "Deletes a webhook owned by the authenticated user. Only the owning user may delete it.", + Method: http.MethodDelete, + Path: "/user/settings/webhooks/{webhook}", + Tags: tags, + }, userWebhooksDelete) +} + +func init() { AddRouteRegistrar(RegisterUserWebhookRoutes) } + +func userWebhooksList(ctx context.Context, in *ListParams) (*userWebhookListBody, error) { + a, err := authFromCtx(ctx) + if err != nil { + return nil, err + } + result, _, total, err := handler.DoReadAll(ctx, &models.Webhook{UserID: a.GetID()}, a, in.Q, in.Page, in.PerPage) + if err != nil { + return nil, translateDomainError(err) + } + items, ok := result.([]*models.Webhook) + if !ok { + return nil, fmt.Errorf("webhooks.ReadAll returned unexpected type %T (expected []*models.Webhook)", result) + } + return &userWebhookListBody{Body: NewPaginated(items, total, in.Page, in.PerPage)}, nil +} + +func userWebhooksEvents(_ context.Context, _ *struct{}) (*userWebhookEventsBody, error) { + return &userWebhookEventsBody{Body: models.GetUserDirectedWebhookEvents()}, nil +} + +func userWebhooksCreate(ctx context.Context, in *struct { + Body models.Webhook +}) (*singleBody[models.Webhook], error) { + a, err := authFromCtx(ctx) + if err != nil { + return nil, err + } + // Force user ownership: a user webhook is keyed on the user, never a project. + in.Body.UserID = a.GetID() + in.Body.ProjectID = 0 + if err := handler.DoCreate(ctx, &in.Body, a); err != nil { + return nil, translateDomainError(err) + } + return &singleBody[models.Webhook]{Body: &in.Body}, nil +} + +func userWebhooksUpdate(ctx context.Context, in *struct { + ID int64 `path:"webhook"` + Body models.Webhook +}) (*singleBody[models.Webhook], error) { + a, err := authFromCtx(ctx) + if err != nil { + return nil, err + } + // canDoWebhook resolves the owner from the stored row, so only the id is + // needed to gate the update; the rest of the body's ownership fields are + // ignored. Update persists only the events list. + in.Body.ID = in.ID + if err := handler.DoUpdate(ctx, &in.Body, a); err != nil { + return nil, translateDomainError(err) + } + return &singleBody[models.Webhook]{Body: &in.Body}, nil +} + +func userWebhooksDelete(ctx context.Context, in *struct { + ID int64 `path:"webhook"` +}) (*emptyBody, error) { + a, err := authFromCtx(ctx) + if err != nil { + return nil, err + } + if err := handler.DoDelete(ctx, &models.Webhook{ID: in.ID}, a); err != nil { + return nil, translateDomainError(err) + } + return &emptyBody{}, nil +} diff --git a/pkg/webtests/huma_user_webhook_test.go b/pkg/webtests/huma_user_webhook_test.go new file mode 100644 index 000000000..8c061a6ff --- /dev/null +++ b/pkg/webtests/huma_user_webhook_test.go @@ -0,0 +1,189 @@ +// Vikunja is a to-do list application to facilitate your life. +// Copyright 2018-present 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 License 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 License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package webtests + +import ( + "encoding/json" + "net/http" + "testing" + + "code.vikunja.io/api/pkg/config" + "code.vikunja.io/api/pkg/models" + "code.vikunja.io/api/pkg/routes" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestHumaUserWebhook ports the v1 user-webhook coverage (the per-user sibling of +// the project webhooks tested in TestHumaWebhook) to /api/v2. User webhooks live +// at /user/settings/webhooks{,/{webhook}} — list, events, create, update, delete; +// there is deliberately no ReadOne (webhooks carry credentials). +// +// Ownership gradient — a user webhook is owned by its UserID, and every Can* boils +// down to "are you that user". Fixtures: webhooks #6/#7 belong to user6, #8 to +// user1. The actor is user6 (not user1): the user-webhook e2e tests dispatch +// user-directed events only for users 1 and 2, so user6-owned fixtures never fire +// there. The point of these cases is that user6 sees and mutates only their own +// webhooks and is forbidden on user1's. +func TestHumaUserWebhook(t *testing.T) { + // availableWebhookEvents / userDirectedWebhookEvents are populated by + // RegisterListeners(), which the webtests harness does not call. Register the + // one user-directed event the fixtures and these cases use so Create/Update + // validation accepts it. + models.RegisterUserDirectedEventForWebhook(&models.TaskReminderFiredEvent{}) + + owner := webHandlerTestV2{ + user: &testuser6, + basePath: "/api/v2/user/settings/webhooks", + idParam: "webhook", + t: t, + } + require.NoError(t, owner.ensureEnv()) + + t.Run("ReadAll", func(t *testing.T) { + t.Run("Normal - sees only own webhooks", func(t *testing.T) { + rec, err := owner.testReadAllWithUser(nil, nil) + require.NoError(t, err) + ids := webhookIDsFromReadAll(t, rec.Body.Bytes()) + // user6 owns #6 and #7; #8 belongs to user1 and must not appear. + assert.ElementsMatch(t, []int64{6, 7}, ids, "body: %s", rec.Body.String()) + assert.Contains(t, rec.Body.String(), `"target_url"`) + }) + t.Run("Secret and basic auth credentials are never exposed", func(t *testing.T) { + rec, err := owner.testReadAllWithUser(nil, nil) + require.NoError(t, err) + assert.NotContains(t, rec.Body.String(), `uwh-secret-fixture`) + assert.NotContains(t, rec.Body.String(), `uwh-basicauth-user`) + assert.NotContains(t, rec.Body.String(), `uwh-basicauth-pass`) + }) + }) + + t.Run("Events", func(t *testing.T) { + // The events route reports only user-directed events. task.reminder.fired + // is registered above; task.updated (project-only) must not be listed. + token := humaTokenFor(t, &testuser6) + rec := humaRequest(t, owner.e, http.MethodGet, "/api/v2/user/settings/webhooks/events", "", token, "") + require.Equal(t, http.StatusOK, rec.Code, "body: %s", rec.Body.String()) + var events []string + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &events), "body: %s", rec.Body.String()) + assert.Contains(t, events, "task.reminder.fired") + assert.NotContains(t, events, "task.updated") + }) + + t.Run("Create", func(t *testing.T) { + t.Run("Normal", func(t *testing.T) { + rec, err := owner.testCreateWithUser(nil, nil, + `{"target_url":"https://example.com/new","events":["task.reminder.fired"]}`) + require.NoError(t, err) + assert.Equal(t, http.StatusCreated, rec.Code) + assert.Contains(t, rec.Body.String(), `"target_url":"https://example.com/new"`) + // Ownership comes from the token, not the body. + assert.Contains(t, rec.Body.String(), `"user_id":6`) + }) + t.Run("Secret and basic auth are not echoed back", func(t *testing.T) { + rec, err := owner.testCreateWithUser(nil, nil, + `{"target_url":"https://example.com/secret","events":["task.reminder.fired"],"secret":"top-secret","basic_auth_user":"u","basic_auth_password":"p"}`) + require.NoError(t, err) + assert.Equal(t, http.StatusCreated, rec.Code) + assert.NotContains(t, rec.Body.String(), `top-secret`) + assert.NotContains(t, rec.Body.String(), `"basic_auth_user":"u"`) + assert.NotContains(t, rec.Body.String(), `"basic_auth_password":"p"`) + }) + t.Run("Non user-directed event rejected", func(t *testing.T) { + // task.updated is a project event, not user-directed; Create rejects it + // → InvalidFieldError, surfaced as 422 on v2. + _, err := owner.testCreateWithUser(nil, nil, + `{"target_url":"https://example.com/x","events":["task.updated"]}`) + require.Error(t, err) + assert.Equal(t, http.StatusUnprocessableEntity, getHTTPErrorCode(err)) + }) + t.Run("Missing target url", func(t *testing.T) { + _, err := owner.testCreateWithUser(nil, nil, `{"events":["task.reminder.fired"]}`) + require.Error(t, err) + assert.Equal(t, http.StatusUnprocessableEntity, getHTTPErrorCode(err)) + }) + }) + + t.Run("Update", func(t *testing.T) { + t.Run("Normal - only events change", func(t *testing.T) { + rec, err := owner.testUpdateWithUser(nil, map[string]string{"webhook": "6"}, + `{"events":["task.reminder.fired"],"target_url":"https://example.com/ignored"}`) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Contains(t, rec.Body.String(), `"id":6`) + + rec, err = owner.testReadAllWithUser(nil, nil) + require.NoError(t, err) + assert.Contains(t, rec.Body.String(), `https://example.com/user-webhook-fixture`, + "target_url must stay the fixture value; only events are mutable") + assert.NotContains(t, rec.Body.String(), `https://example.com/ignored`) + }) + t.Run("Cannot update another user's webhook", func(t *testing.T) { + // webhook #8 belongs to user1; canDoWebhook resolves ownership from the + // stored row, so user6 is forbidden regardless of the URL. + _, err := owner.testUpdateWithUser(nil, map[string]string{"webhook": "8"}, + `{"target_url":"https://example.com/wh","events":["task.reminder.fired"]}`) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("Nonexisting", func(t *testing.T) { + // canDoWebhook returns false for a missing webhook → 403, not 404. + _, err := owner.testUpdateWithUser(nil, map[string]string{"webhook": "9999"}, + `{"target_url":"https://example.com/wh","events":["task.reminder.fired"]}`) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + }) + + t.Run("Delete", func(t *testing.T) { + t.Run("Cannot delete another user's webhook", func(t *testing.T) { + _, err := owner.testDeleteWithUser(nil, map[string]string{"webhook": "8"}) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("Nonexisting", func(t *testing.T) { + _, err := owner.testDeleteWithUser(nil, map[string]string{"webhook": "9999"}) + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, getHTTPErrorCode(err)) + }) + t.Run("Normal", func(t *testing.T) { + rec, err := owner.testDeleteWithUser(nil, map[string]string{"webhook": "7"}) + require.NoError(t, err) + assert.Equal(t, http.StatusNoContent, rec.Code) + assert.Empty(t, rec.Body.String()) + }) + }) +} + +// TestHumaUserWebhook_DisabledByConfig confirms RegisterUserWebhookRoutes skips +// the resource when webhooks.enabled is false, so the v2 user-webhook routes 404 +// rather than running with the feature toggled off. +func TestHumaUserWebhook_DisabledByConfig(t *testing.T) { + _, err := setupTestEnv() + require.NoError(t, err) + + config.WebhooksEnabled.Set(false) + defer config.WebhooksEnabled.Set(true) + + e := routes.NewEcho() + routes.RegisterRoutes(e) + + token := humaTokenFor(t, &testuser1) + rec := humaRequest(t, e, http.MethodGet, "/api/v2/user/settings/webhooks", "", token, "") + assert.Equal(t, http.StatusNotFound, rec.Code, "route must be absent when webhooks are disabled; body: %s", rec.Body.String()) +}