From 79a13448d3228372d7015ba000fb5e0b1ce94c8a Mon Sep 17 00:00:00 2001 From: Axenos-dev Date: Wed, 8 Jul 2026 13:23:16 -0700 Subject: [PATCH 1/3] Return 409 if username already exists on creation --- README.md | 2 +- internal/db/gen/users.sql.go | 3 +- internal/db/queries/users.sql | 3 +- internal/server/control/users/create.go | 7 +- .../server/control/users/handlers_test.go | 81 +++++++++++++++++++ internal/server/response/codes.go | 1 + internal/services/users/errors.go | 5 +- internal/services/users/service.go | 5 ++ internal/services/users/service_test.go | 53 ++++++++++++ 9 files changed, 155 insertions(+), 5 deletions(-) create mode 100644 internal/server/control/users/handlers_test.go create mode 100644 internal/services/users/service_test.go diff --git a/README.md b/README.md index d08f077..966f761 100644 --- a/README.md +++ b/README.md @@ -118,7 +118,7 @@ Every request requires `Authorization: Bearer `. Responses are enve | Method | Path | Body | Description | | -------- | ------------------------------ | -------------------- | ------------------------------------------------------------ | -| `POST` | `/users` | `{username, kind}` | Create a user/service account (`kind`: `user` \| `service`). | +| `POST` | `/users` | `{username, kind}` | Create a user/service account (`kind`: `user` \| `service`); `409 user_exists` if the username is taken. | | `GET` | `/users/{id}` | — | Get an account. | | `GET` | `/users/{id}/repositories` | — | List repositories owned by the account. | | `POST` | `/users/{id}/ssh-keys` | `{title, publicKey}` | Register an SSH public key. | diff --git a/internal/db/gen/users.sql.go b/internal/db/gen/users.sql.go index 88f7fec..fc15c6d 100644 --- a/internal/db/gen/users.sql.go +++ b/internal/db/gen/users.sql.go @@ -14,7 +14,8 @@ insert into users ( username, kind ) values ( ?, ? -) returning id, username, kind, is_admin, created_at_unix_ms, updated_at_unix_ms +) on conflict(username) do nothing +returning id, username, kind, is_admin, created_at_unix_ms, updated_at_unix_ms ` type CreateUserParams struct { diff --git a/internal/db/queries/users.sql b/internal/db/queries/users.sql index 3d25b23..c6959cb 100644 --- a/internal/db/queries/users.sql +++ b/internal/db/queries/users.sql @@ -7,7 +7,8 @@ insert into users ( username, kind ) values ( ?, ? -) returning *; +) on conflict(username) do nothing +returning *; -- name: EnsureAdminUser :one insert into users ( diff --git a/internal/server/control/users/create.go b/internal/server/control/users/create.go index a174aad..1eb3f54 100644 --- a/internal/server/control/users/create.go +++ b/internal/server/control/users/create.go @@ -2,10 +2,12 @@ package users import ( "encoding/json" + "errors" "net/http" "github.com/Axenos-dev/HeadlessGit/internal/domain" "github.com/Axenos-dev/HeadlessGit/internal/server/response" + usersservice "github.com/Axenos-dev/HeadlessGit/internal/services/users" "go.uber.org/zap" ) @@ -22,7 +24,10 @@ func (h *handlers) createUser(w http.ResponseWriter, r *http.Request) error { Username: req.Username, Kind: domain.UserKind(req.Kind), }) - if err != nil { + switch { + case errors.Is(err, usersservice.ErrUserExists): + return response.NewError(http.StatusConflict, response.CodeUserExists, "user already exists") + case err != nil: h.logger.Error("failed to create user", zap.Error(err)) return response.NewError(http.StatusInternalServerError, response.CodeInternalError, "failed to create user") } diff --git a/internal/server/control/users/handlers_test.go b/internal/server/control/users/handlers_test.go new file mode 100644 index 0000000..9ed47cf --- /dev/null +++ b/internal/server/control/users/handlers_test.go @@ -0,0 +1,81 @@ +package users + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/Axenos-dev/HeadlessGit/internal/domain" + usersservice "github.com/Axenos-dev/HeadlessGit/internal/services/users" + "github.com/go-chi/chi/v5" + "go.uber.org/zap" +) + +// fakeUserManager stubs UserManager for handler tests: embed the interface +// and override only what the endpoint under test touches +type fakeUserManager struct { + UserManager + account domain.Account + err error +} + +func (f fakeUserManager) Create(ctx context.Context, info domain.UserInfo) (domain.Account, error) { + return f.account, f.err +} + +// newTestRouter mounts the handlers the same way the control server does +func newTestRouter(users UserManager) http.Handler { + r := chi.NewRouter() + NewHandlers(zap.NewNop(), users, nil).RegisterRoutes(r) + return r +} + +func TestCreateUser(t *testing.T) { + tests := []struct { + name string + body string + svcErr error + wantStatus int + wantCode string + }{ + {"created", `{"username":"alice","kind":"user"}`, nil, http.StatusCreated, ""}, + {"duplicate", `{"username":"alice","kind":"user"}`, usersservice.ErrUserExists, http.StatusConflict, "user_exists"}, + {"service error", `{"username":"alice","kind":"user"}`, context.DeadlineExceeded, http.StatusInternalServerError, "internal_error"}, + {"invalid body", `not json`, nil, http.StatusBadRequest, "invalid_request"}, + {"missing username", `{"kind":"user"}`, nil, http.StatusBadRequest, "invalid_request"}, + {"bad kind", `{"username":"alice","kind":"robot"}`, nil, http.StatusBadRequest, "invalid_request"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + router := newTestRouter(fakeUserManager{ + account: domain.Account{UserID: 7, Username: "alice", Kind: domain.UserKindUser}, + err: tt.svcErr, + }) + + req := httptest.NewRequest(http.MethodPost, "/users", strings.NewReader(tt.body)) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != tt.wantStatus { + t.Fatalf("status = %d, want %d (body: %s)", rec.Code, tt.wantStatus, rec.Body.String()) + } + if tt.wantCode != "" { + var envelope struct { + Error struct { + Code string `json:"code"` + } `json:"error"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &envelope); err != nil { + t.Fatalf("decode error envelope: %v", err) + } + if envelope.Error.Code != tt.wantCode { + t.Errorf("code = %q, want %q", envelope.Error.Code, tt.wantCode) + } + } + }) + } +} diff --git a/internal/server/response/codes.go b/internal/server/response/codes.go index 5e39de3..be3982a 100644 --- a/internal/server/response/codes.go +++ b/internal/server/response/codes.go @@ -20,4 +20,5 @@ const ( CodePathBlocked = "path_blocked" CodePathPolicyExists = "path_policy_exists" + CodeUserExists = "user_exists" ) diff --git a/internal/services/users/errors.go b/internal/services/users/errors.go index 0240df8..b7f9fe4 100644 --- a/internal/services/users/errors.go +++ b/internal/services/users/errors.go @@ -2,4 +2,7 @@ package users import "errors" -var ErrUserNotFound = errors.New("user not found") +var ( + ErrUserNotFound = errors.New("user not found") + ErrUserExists = errors.New("user already exists") +) diff --git a/internal/services/users/service.go b/internal/services/users/service.go index a90639c..e8199f9 100644 --- a/internal/services/users/service.go +++ b/internal/services/users/service.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "errors" + "fmt" "github.com/Axenos-dev/HeadlessGit/internal/db/gen" "github.com/Axenos-dev/HeadlessGit/internal/domain" @@ -26,6 +27,10 @@ func NewService(registry Registry) *Service { func (s *Service) Create(ctx context.Context, info domain.UserInfo) (domain.Account, error) { user, err := s.registry.CreateUser(ctx, info.Username, string(info.Kind)) + // the insert is "on conflict do nothing returning *" -> on duplicate "no rows" + if errors.Is(err, sql.ErrNoRows) { + return domain.Account{}, fmt.Errorf("%w: %q", ErrUserExists, info.Username) + } if err != nil { return domain.Account{}, err } diff --git a/internal/services/users/service_test.go b/internal/services/users/service_test.go new file mode 100644 index 0000000..de0eb44 --- /dev/null +++ b/internal/services/users/service_test.go @@ -0,0 +1,53 @@ +package users + +import ( + "context" + "database/sql" + "errors" + "testing" + + "github.com/Axenos-dev/HeadlessGit/internal/db/gen" + "github.com/Axenos-dev/HeadlessGit/internal/domain" +) + +type fakeRegistry struct { + user gen.User + err error +} + +func (f fakeRegistry) GetUser(ctx context.Context, userID int64) (gen.User, error) { + return f.user, f.err +} + +func (f fakeRegistry) CreateUser(ctx context.Context, username, kind string) (gen.User, error) { + return f.user, f.err +} + +func TestCreate(t *testing.T) { + t.Run("ok", func(t *testing.T) { + svc := NewService(fakeRegistry{user: gen.User{ID: 7, Username: "alice", Kind: "user"}}) + account, err := svc.Create(context.Background(), domain.UserInfo{Username: "alice", Kind: domain.UserKindUser}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if account.UserID != 7 || account.Username != "alice" { + t.Errorf("unexpected account: %+v", account) + } + }) + + t.Run("duplicate", func(t *testing.T) { + // the insert is "on conflict do nothing returning *" -> on duplicate "no rows" + svc := NewService(fakeRegistry{err: sql.ErrNoRows}) + if _, err := svc.Create(context.Background(), domain.UserInfo{Username: "alice", Kind: domain.UserKindUser}); !errors.Is(err, ErrUserExists) { + t.Errorf("want ErrUserExists, got %v", err) + } + }) + + t.Run("registry error", func(t *testing.T) { + boom := errors.New("boom") + svc := NewService(fakeRegistry{err: boom}) + if _, err := svc.Create(context.Background(), domain.UserInfo{Username: "alice", Kind: domain.UserKindUser}); !errors.Is(err, boom) { + t.Errorf("want boom, got %v", err) + } + }) +} From 9ffa40f3ead0665706d5b08f1d4b13ab570f0b03 Mon Sep 17 00:00:00 2001 From: Axenos-dev Date: Wed, 8 Jul 2026 13:30:32 -0700 Subject: [PATCH 2/3] Return 409 if repository already exists on creation --- README.md | 2 +- internal/db/gen/repositories.sql.go | 3 +- internal/db/queries/repositories.sql | 3 +- .../server/control/repositories/create.go | 2 + .../control/repositories/handlers_test.go | 53 +++++++++++++++++++ internal/server/response/codes.go | 1 + internal/services/repositories/errors.go | 1 + internal/services/repositories/service.go | 4 ++ .../services/repositories/service_test.go | 44 +++++++++++++++ 9 files changed, 110 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 966f761..62d5c34 100644 --- a/README.md +++ b/README.md @@ -133,7 +133,7 @@ Every request requires `Authorization: Bearer `. Responses are enve | Method | Path | Body | Description | | -------- | --------------------------------------------- | ----------------------------- | ----------------------------------------------------------------- | -| `POST` | `/repositories` | `{ownerId, name, visibility}` | Create a repository (`visibility`: `public` \| `private`). | +| `POST` | `/repositories` | `{ownerId, name, visibility}` | Create a repository (`visibility`: `public` \| `private`); `409 repository_exists` if the owner already has one with that name. | | `GET` | `/repositories/{id}` | — | Get repository metadata. | | `PUT` | `/repositories/{id}/visibility` | `{visibility}` | Change visibility (`public` \| `private`). | | `DELETE` | `/repositories/{id}` | — | Delete a repository (row + bare repo). | diff --git a/internal/db/gen/repositories.sql.go b/internal/db/gen/repositories.sql.go index 5fe79d7..2ec9053 100644 --- a/internal/db/gen/repositories.sql.go +++ b/internal/db/gen/repositories.sql.go @@ -14,7 +14,8 @@ insert into repositories ( owner_id, repository_name, storage_path, visibility ) values ( ?, ?, ?, ? -) returning id, owner_id, repository_name, storage_path, visibility, created_at_unix_ms, updated_at_unix_ms +) on conflict(owner_id, repository_name) do nothing +returning id, owner_id, repository_name, storage_path, visibility, created_at_unix_ms, updated_at_unix_ms ` type CreateRepositoryParams struct { diff --git a/internal/db/queries/repositories.sql b/internal/db/queries/repositories.sql index e53e3b0..53f3df9 100644 --- a/internal/db/queries/repositories.sql +++ b/internal/db/queries/repositories.sql @@ -14,7 +14,8 @@ insert into repositories ( owner_id, repository_name, storage_path, visibility ) values ( ?, ?, ?, ? -) returning *; +) on conflict(owner_id, repository_name) do nothing +returning *; -- name: UpdateRepositoryVisibility :one update repositories diff --git a/internal/server/control/repositories/create.go b/internal/server/control/repositories/create.go index 2f2db06..8b3db8c 100644 --- a/internal/server/control/repositories/create.go +++ b/internal/server/control/repositories/create.go @@ -27,6 +27,8 @@ func (h *handlers) createRepository(w http.ResponseWriter, r *http.Request) erro switch { case errors.Is(err, reposervice.ErrInvalidRepositoryName): return response.NewError(http.StatusBadRequest, response.CodeInvalidRequest, "invalid repository name") + case errors.Is(err, reposervice.ErrRepositoryExists): + return response.NewError(http.StatusConflict, response.CodeRepositoryExists, "repository already exists") case err != nil: h.logger.Error("failed to create repository", zap.Error(err)) return response.NewError(http.StatusInternalServerError, response.CodeInternalError, "failed to create repository") diff --git a/internal/server/control/repositories/handlers_test.go b/internal/server/control/repositories/handlers_test.go index e1985f8..36e4092 100644 --- a/internal/server/control/repositories/handlers_test.go +++ b/internal/server/control/repositories/handlers_test.go @@ -44,6 +44,13 @@ type fakeManager struct { policyErr error policyPattern string policyReason string + + createdRepo domain.Repository + createErr error +} + +func (f *fakeManager) Create(ctx context.Context, ownerID int64, info domain.RepositoryInfo) (domain.Repository, error) { + return f.createdRepo, f.createErr } func (f *fakeManager) ListPathPolicies(ctx context.Context, repositoryID int64) ([]domain.PathPolicy, error) { @@ -524,6 +531,52 @@ func TestCreateCommitErrors(t *testing.T) { } } +func TestCreateRepository(t *testing.T) { + cases := []struct { + name string + body string + createErr error + wantStatus int + wantCode string + }{ + {"created", `{"ownerId":3,"name":"demo","visibility":"private"}`, nil, http.StatusCreated, ""}, + {"duplicate", `{"ownerId":3,"name":"demo","visibility":"private"}`, reposervice.ErrRepositoryExists, http.StatusConflict, "repository_exists"}, + {"invalid name", `{"ownerId":3,"name":"..","visibility":"private"}`, reposervice.ErrInvalidRepositoryName, http.StatusBadRequest, "invalid_request"}, + {"internal", `{"ownerId":3,"name":"demo","visibility":"private"}`, io.ErrUnexpectedEOF, http.StatusInternalServerError, "internal_error"}, + {"invalid body", `not json`, nil, http.StatusBadRequest, "invalid_request"}, + {"missing owner", `{"name":"demo","visibility":"private"}`, nil, http.StatusBadRequest, "invalid_request"}, + {"bad visibility", `{"ownerId":3,"name":"demo","visibility":"hidden"}`, nil, http.StatusBadRequest, "invalid_request"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fake := &fakeManager{ + createdRepo: domain.Repository{ID: 7, OwnerID: 3, RepositoryName: "demo", Visibility: domain.RepoVisibilityPrivate}, + createErr: tc.createErr, + } + rec := httptest.NewRecorder() + newTestRouter(fake).ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/repositories", strings.NewReader(tc.body))) + + if rec.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d: %s", rec.Code, tc.wantStatus, rec.Body.String()) + } + if tc.wantCode != "" { + var body struct { + Error struct { + Code string `json:"code"` + } `json:"error"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatal(err) + } + if body.Error.Code != tc.wantCode { + t.Errorf("code = %q, want %q", body.Error.Code, tc.wantCode) + } + } + }) + } +} + func TestPathPolicies(t *testing.T) { policy := domain.PathPolicy{ID: 3, RepositoryID: 7, Pattern: "runtime", Kind: domain.PathPolicyBlock, Reason: "deploy state"} diff --git a/internal/server/response/codes.go b/internal/server/response/codes.go index be3982a..756c6eb 100644 --- a/internal/server/response/codes.go +++ b/internal/server/response/codes.go @@ -21,4 +21,5 @@ const ( CodePathBlocked = "path_blocked" CodePathPolicyExists = "path_policy_exists" CodeUserExists = "user_exists" + CodeRepositoryExists = "repository_exists" ) diff --git a/internal/services/repositories/errors.go b/internal/services/repositories/errors.go index f4b237a..265206f 100644 --- a/internal/services/repositories/errors.go +++ b/internal/services/repositories/errors.go @@ -4,6 +4,7 @@ import "errors" var ( ErrRepositoryNotFound = errors.New("repository not found") + ErrRepositoryExists = errors.New("repository already exists") ErrInvalidRepositoryName = errors.New("invalid repository name") ErrInvalidVisibility = errors.New("invalid visibility") diff --git a/internal/services/repositories/service.go b/internal/services/repositories/service.go index 7e1da5d..c7cc0d7 100644 --- a/internal/services/repositories/service.go +++ b/internal/services/repositories/service.go @@ -125,6 +125,10 @@ func (s *Service) Create(ctx context.Context, ownerID int64, info domain.Reposit // insert row first, check if we pass the main constrains repo, err := s.registry.CreateRepository(ctx, ownerID, info.RepositoryName, storagePath, string(info.Visibility)) + // the insert is "on conflict do nothing returning *" -> on duplicate "no rows" + if errors.Is(err, sql.ErrNoRows) { + return domain.Repository{}, fmt.Errorf("%w: %q", ErrRepositoryExists, info.RepositoryName) + } if err != nil { s.logger.Error("failed to create repository", zap.Error(err)) return domain.Repository{}, err diff --git a/internal/services/repositories/service_test.go b/internal/services/repositories/service_test.go index 229d838..8476593 100644 --- a/internal/services/repositories/service_test.go +++ b/internal/services/repositories/service_test.go @@ -26,6 +26,8 @@ type fakeRegistry struct { repo gen.Repository err error + createRepoErr error + policies []gen.PathPolicy policiesErr error createPolicyErr error @@ -35,6 +37,13 @@ func (f fakeRegistry) GetRepository(ctx context.Context, repositoryID int64) (ge return f.repo, f.err } +func (f fakeRegistry) CreateRepository(ctx context.Context, ownerID int64, name, storagePath, visibility string) (gen.Repository, error) { + if f.createRepoErr != nil { + return gen.Repository{}, f.createRepoErr + } + return f.repo, nil +} + func (f fakeRegistry) ListRepositoryPathPolicies(ctx context.Context, repositoryID int64) ([]gen.PathPolicy, error) { return f.policies, f.policiesErr } @@ -71,6 +80,10 @@ type fakeStorage struct { applyFn func(spec gitbackend.CommitSpec, ops []gitbackend.CommitOp, clean gitbackend.CleanFunc) error } +func (f fakeStorage) InitBare(ctx context.Context, storagePath string) error { + return nil +} + func (f fakeStorage) ResolveCommit(ctx context.Context, storagePath, rev string) (string, error) { if f.resolveErr != nil { return "", f.resolveErr @@ -150,6 +163,37 @@ func (f fakeDispatcher) DispatchEvent(ctx context.Context, event domain.Reposito return nil } +func TestCreateRepository(t *testing.T) { + row := gen.Repository{ID: 7, OwnerID: 3, RepositoryName: "myrepo", StoragePath: "3/myrepo.git", Visibility: "private"} + info := domain.RepositoryInfo{RepositoryName: "myrepo", Visibility: domain.RepoVisibilityPrivate} + + t.Run("ok", func(t *testing.T) { + svc := NewService(zap.NewNop(), fakeRegistry{repo: row}, fakeStorage{}, nil, nil) + repo, err := svc.Create(context.Background(), 3, info) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if repo.ID != 7 || repo.RepositoryName != "myrepo" { + t.Errorf("unexpected repo: %+v", repo) + } + }) + + t.Run("duplicate", func(t *testing.T) { + // the insert is "on conflict do nothing returning *" -> on duplicate "no rows" + svc := NewService(zap.NewNop(), fakeRegistry{createRepoErr: sql.ErrNoRows}, fakeStorage{}, nil, nil) + if _, err := svc.Create(context.Background(), 3, info); !errors.Is(err, ErrRepositoryExists) { + t.Errorf("want ErrRepositoryExists, got %v", err) + } + }) + + t.Run("invalid name", func(t *testing.T) { + svc := NewService(zap.NewNop(), fakeRegistry{repo: row}, fakeStorage{}, nil, nil) + if _, err := svc.Create(context.Background(), 3, domain.RepositoryInfo{RepositoryName: "../evil", Visibility: domain.RepoVisibilityPrivate}); !errors.Is(err, ErrInvalidRepositoryName) { + t.Errorf("want ErrInvalidRepositoryName, got %v", err) + } + }) +} + func TestPrepareArchive(t *testing.T) { row := gen.Repository{ID: 7, RepositoryName: "myrepo", StoragePath: "7/myrepo.git", Visibility: "private"} From 2d0c54cf30c62d8e9bf52d6844ae906dde5e4038 Mon Sep 17 00:00:00 2001 From: Axenos-dev Date: Wed, 8 Jul 2026 13:40:29 -0700 Subject: [PATCH 3/3] Constraint duplicate webhook URLs per repo --- README.md | 4 +- internal/db/db_integration_test.go | 25 ++++++ internal/db/gen/webhooks.sql.go | 3 +- .../migrations/0006_webhooks_unique_url.sql | 12 +++ internal/db/queries/webhooks.sql | 3 +- internal/server/control/webhooks/create.go | 7 +- .../server/control/webhooks/handlers_test.go | 82 +++++++++++++++++++ internal/server/response/codes.go | 1 + internal/services/webhooks/errors.go | 1 + internal/services/webhooks/service.go | 6 ++ internal/services/webhooks/service_test.go | 50 +++++++++++ 11 files changed, 189 insertions(+), 5 deletions(-) create mode 100644 internal/db/migrations/0006_webhooks_unique_url.sql create mode 100644 internal/server/control/webhooks/handlers_test.go create mode 100644 internal/services/webhooks/service_test.go diff --git a/README.md b/README.md index 62d5c34..7537edb 100644 --- a/README.md +++ b/README.md @@ -140,7 +140,7 @@ Every request requires `Authorization: Bearer `. Responses are enve | `GET` | `/repositories/{id}/permissions` | — | List collaborators. | | `PUT` | `/repositories/{id}/permissions` | `{userId, role}` | Grant/update a collaborator role (`read` \| `write` \| `admin`). | | `DELETE` | `/repositories/{id}/permissions/{userId}` | — | Revoke a collaborator. | -| `POST` | `/repositories/{id}/webhooks` | `{url}` | Register a push webhook; the signing secret is returned **once**. | +| `POST` | `/repositories/{id}/webhooks` | `{url}` | Register a push webhook; the signing secret is returned **once**. `409 webhook_exists` if the URL is already registered on the repo. | | `DELETE` | `/repositories/{id}/webhooks/{hookId}` | — | Delete a webhook. | | `GET` | `/repositories/{id}/path-policies` | — | List the repository's path policies. | | `POST` | `/repositories/{id}/path-policies` | `{pattern, reason?}` | Block a path; see [Path policies](#path-policies). | @@ -269,7 +269,7 @@ remote: push rejected: "runtime/state.json" is blocked by policy (.....) ## Webhooks -Register a webhook on a repository and `headlessgit` will `POST` to it after every ref change — a `git push` or a commit created through the [content API](#writing-without-a-clone) produce identical events. +Register a webhook on a repository and `headlessgit` will `POST` to it after every ref change — a `git push` or a commit created through the [content API](#writing-without-a-clone) produce identical events. A repository can have multiple webhooks, but each URL only once (`409 webhook_exists` on duplicates); to rotate a secret, delete the webhook and re-register it. One delivery is sent **per changed ref** (a branch/tag create, update, or delete — not per file or commit). The JSON body: diff --git a/internal/db/db_integration_test.go b/internal/db/db_integration_test.go index 2ce1317..cf95f58 100644 --- a/internal/db/db_integration_test.go +++ b/internal/db/db_integration_test.go @@ -135,6 +135,31 @@ func TestPermissionUpsert(t *testing.T) { } } +func TestWebhookUniquePerRepoURL(t *testing.T) { + d := openTestDB(t) + ctx := context.Background() + owner, _ := d.CreateUser(ctx, gen.CreateUserParams{Username: "o", Kind: "user"}) + repo, _ := d.CreateRepository(ctx, gen.CreateRepositoryParams{ + OwnerID: owner.ID, RepositoryName: "r", StoragePath: "o/r.git", Visibility: "private", + }) + + if _, err := d.CreateWebhook(ctx, gen.CreateWebhookParams{RepositoryID: repo.ID, Secret: "s1", Url: "https://example.com/hook"}); err != nil { + t.Fatalf("create webhook: %v", err) + } + // duplicate (repo, url): "on conflict do nothing returning *" -> no rows + if _, err := d.CreateWebhook(ctx, gen.CreateWebhookParams{RepositoryID: repo.ID, Secret: "s2", Url: "https://example.com/hook"}); !errors.Is(err, sql.ErrNoRows) { + t.Fatalf("duplicate webhook: expected ErrNoRows, got %v", err) + } + + // the same url on a different repo is a separate registration + repo2, _ := d.CreateRepository(ctx, gen.CreateRepositoryParams{ + OwnerID: owner.ID, RepositoryName: "r2", StoragePath: "o/r2.git", Visibility: "private", + }) + if _, err := d.CreateWebhook(ctx, gen.CreateWebhookParams{RepositoryID: repo2.ID, Secret: "s3", Url: "https://example.com/hook"}); err != nil { + t.Fatalf("same url, other repo: %v", err) + } +} + func TestEnsureAdminUserIdempotent(t *testing.T) { d := openTestDB(t) ctx := context.Background() diff --git a/internal/db/gen/webhooks.sql.go b/internal/db/gen/webhooks.sql.go index 5cd49d2..169cb6c 100644 --- a/internal/db/gen/webhooks.sql.go +++ b/internal/db/gen/webhooks.sql.go @@ -14,7 +14,8 @@ insert into webhooks ( repository_id, secret, url ) values ( ?, ?, ? -) returning id, repository_id, secret, url, created_at_unix_ms, updated_at_unix_ms +) on conflict(repository_id, url) do nothing +returning id, repository_id, secret, url, created_at_unix_ms, updated_at_unix_ms ` type CreateWebhookParams struct { diff --git a/internal/db/migrations/0006_webhooks_unique_url.sql b/internal/db/migrations/0006_webhooks_unique_url.sql new file mode 100644 index 0000000..0c27ced --- /dev/null +++ b/internal/db/migrations/0006_webhooks_unique_url.sql @@ -0,0 +1,12 @@ +-- +goose Up +-- drop duplicate registrations before adding the constraint +delete from webhooks +where id not in ( + select min(id) from webhooks + group by repository_id, url +); + +create unique index if not exists idx_webhooks_repository_id_url on webhooks(repository_id, url); + +-- +goose Down +drop index if exists idx_webhooks_repository_id_url; diff --git a/internal/db/queries/webhooks.sql b/internal/db/queries/webhooks.sql index 649ee80..39c82f4 100644 --- a/internal/db/queries/webhooks.sql +++ b/internal/db/queries/webhooks.sql @@ -3,7 +3,8 @@ insert into webhooks ( repository_id, secret, url ) values ( ?, ?, ? -) returning *; +) on conflict(repository_id, url) do nothing +returning *; -- name: ListWebhooksForRepository :many select * from webhooks diff --git a/internal/server/control/webhooks/create.go b/internal/server/control/webhooks/create.go index f761fd5..d4f29df 100644 --- a/internal/server/control/webhooks/create.go +++ b/internal/server/control/webhooks/create.go @@ -2,10 +2,12 @@ package webhooks import ( "encoding/json" + "errors" "net/http" "strconv" "github.com/Axenos-dev/HeadlessGit/internal/server/response" + webhookservice "github.com/Axenos-dev/HeadlessGit/internal/services/webhooks" "github.com/go-chi/chi/v5" "go.uber.org/zap" ) @@ -25,7 +27,10 @@ func (h *handlers) createWebhook(w http.ResponseWriter, r *http.Request) error { } webhook, err := h.webhooks.RegisterWebhook(r.Context(), repoID, req.URL) - if err != nil { + switch { + case errors.Is(err, webhookservice.ErrWebhookExists): + return response.NewError(http.StatusConflict, response.CodeWebhookExists, "webhook already exists") + case err != nil: h.logger.Error("failed to register webhook", zap.Error(err)) return response.NewError(http.StatusInternalServerError, response.CodeInternalError, "failed to register webhook") } diff --git a/internal/server/control/webhooks/handlers_test.go b/internal/server/control/webhooks/handlers_test.go new file mode 100644 index 0000000..4557bcc --- /dev/null +++ b/internal/server/control/webhooks/handlers_test.go @@ -0,0 +1,82 @@ +package webhooks + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/Axenos-dev/HeadlessGit/internal/domain" + webhookservice "github.com/Axenos-dev/HeadlessGit/internal/services/webhooks" + "github.com/go-chi/chi/v5" + "go.uber.org/zap" +) + +// fakeManager stubs WebhookManager for handler tests +type fakeManager struct { + webhook domain.Webhook + err error +} + +func (f fakeManager) RegisterWebhook(ctx context.Context, repoID int64, url string) (domain.Webhook, error) { + return f.webhook, f.err +} + +func (f fakeManager) DeleteWebhook(ctx context.Context, webhookID, repositoryID int64) error { + return f.err +} + +// newTestRouter mounts the handlers the same way the control server does +func newTestRouter(webhooks WebhookManager) http.Handler { + r := chi.NewRouter() + NewHandlers(zap.NewNop(), webhooks).RegisterRoutes(r) + return r +} + +func TestCreateWebhook(t *testing.T) { + cases := []struct { + name string + body string + svcErr error + wantStatus int + wantCode string + }{ + {"created", `{"url":"https://example.com/hook"}`, nil, http.StatusCreated, ""}, + {"duplicate", `{"url":"https://example.com/hook"}`, webhookservice.ErrWebhookExists, http.StatusConflict, "webhook_exists"}, + {"internal", `{"url":"https://example.com/hook"}`, io.ErrUnexpectedEOF, http.StatusInternalServerError, "internal_error"}, + {"invalid body", `not json`, nil, http.StatusBadRequest, "invalid_request"}, + {"missing url", `{}`, nil, http.StatusBadRequest, "invalid_request"}, + {"non-http url", `{"url":"ftp://example.com/hook"}`, nil, http.StatusBadRequest, "invalid_request"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fake := fakeManager{ + webhook: domain.Webhook{ID: 3, RepositoryID: 7, URL: "https://example.com/hook", Secret: "s"}, + err: tc.svcErr, + } + rec := httptest.NewRecorder() + newTestRouter(fake).ServeHTTP(rec, httptest.NewRequest(http.MethodPost, "/repositories/7/webhooks", strings.NewReader(tc.body))) + + if rec.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d: %s", rec.Code, tc.wantStatus, rec.Body.String()) + } + if tc.wantCode != "" { + var body struct { + Error struct { + Code string `json:"code"` + } `json:"error"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatal(err) + } + if body.Error.Code != tc.wantCode { + t.Errorf("code = %q, want %q", body.Error.Code, tc.wantCode) + } + } + }) + } +} diff --git a/internal/server/response/codes.go b/internal/server/response/codes.go index 756c6eb..4c1eed7 100644 --- a/internal/server/response/codes.go +++ b/internal/server/response/codes.go @@ -22,4 +22,5 @@ const ( CodePathPolicyExists = "path_policy_exists" CodeUserExists = "user_exists" CodeRepositoryExists = "repository_exists" + CodeWebhookExists = "webhook_exists" ) diff --git a/internal/services/webhooks/errors.go b/internal/services/webhooks/errors.go index 9fe619a..3b4b507 100644 --- a/internal/services/webhooks/errors.go +++ b/internal/services/webhooks/errors.go @@ -4,4 +4,5 @@ import "errors" var ( ErrEventsChannelFull = errors.New("events channel buffer is full") + ErrWebhookExists = errors.New("webhook already exists") ) diff --git a/internal/services/webhooks/service.go b/internal/services/webhooks/service.go index 59d7dd5..698f19b 100644 --- a/internal/services/webhooks/service.go +++ b/internal/services/webhooks/service.go @@ -6,9 +6,11 @@ import ( "crypto/hmac" "crypto/rand" "crypto/sha256" + "database/sql" "encoding/base64" "encoding/hex" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -49,6 +51,10 @@ func (s *Service) RegisterWebhook(ctx context.Context, repoID int64, url string) } webhook, err := s.registry.CreateWebhook(ctx, repoID, secret, url) + // the insert is "on conflict do nothing returning *" -> on duplicate "no rows" + if errors.Is(err, sql.ErrNoRows) { + return domain.Webhook{}, fmt.Errorf("%w: %q", ErrWebhookExists, url) + } if err != nil { return domain.Webhook{}, err } diff --git a/internal/services/webhooks/service_test.go b/internal/services/webhooks/service_test.go new file mode 100644 index 0000000..3a037f8 --- /dev/null +++ b/internal/services/webhooks/service_test.go @@ -0,0 +1,50 @@ +package webhooks + +import ( + "context" + "database/sql" + "errors" + "testing" + + "github.com/Axenos-dev/HeadlessGit/internal/db/gen" + "go.uber.org/zap" +) + +type fakeRegistry struct { + Registry + webhook gen.Webhook + err error +} + +func (f fakeRegistry) CreateWebhook(ctx context.Context, repoID int64, secret, url string) (gen.Webhook, error) { + return f.webhook, f.err +} + +func TestRegisterWebhook(t *testing.T) { + t.Run("ok", func(t *testing.T) { + svc := NewService(zap.NewNop(), fakeRegistry{webhook: gen.Webhook{ID: 3, RepositoryID: 7, Url: "https://example.com/hook", Secret: "s"}}) + webhook, err := svc.RegisterWebhook(context.Background(), 7, "https://example.com/hook") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if webhook.ID != 3 || webhook.URL != "https://example.com/hook" { + t.Errorf("unexpected webhook: %+v", webhook) + } + }) + + t.Run("duplicate", func(t *testing.T) { + // the insert is "on conflict do nothing returning *" -> on duplicate "no rows" + svc := NewService(zap.NewNop(), fakeRegistry{err: sql.ErrNoRows}) + if _, err := svc.RegisterWebhook(context.Background(), 7, "https://example.com/hook"); !errors.Is(err, ErrWebhookExists) { + t.Errorf("want ErrWebhookExists, got %v", err) + } + }) + + t.Run("registry error", func(t *testing.T) { + boom := errors.New("boom") + svc := NewService(zap.NewNop(), fakeRegistry{err: boom}) + if _, err := svc.RegisterWebhook(context.Background(), 7, "https://example.com/hook"); !errors.Is(err, boom) { + t.Errorf("want boom, got %v", err) + } + }) +}