diff --git a/docs/migration-1.0.md b/docs/migration-1.0.md index ddb94331..10f4b0db 100644 --- a/docs/migration-1.0.md +++ b/docs/migration-1.0.md @@ -254,6 +254,10 @@ Renamed flags that appear on many commands: | `--inactive` (webhook) | `--active=false` | | `--expires` / `-x` (milestones create) | `--deadline` | | `--kind issue\|pull` (milestones issues) | `--kind issues\|pulls\|all` | +| `--no-admin`, `--no-restricted`, `--inactive`, `--no-allow-git-hook`, `--no-allow-import-local`, `--no-allow-create-organization` (admin users edit) | `--admin=false`, `--restricted=false`, `--active=false`, `--allow-git-hook=false`, and so on | +| `--allow-login` (admin users edit) | `--prohibit-login=false` | +| `--password-stdin`, `--no-must-change-password` (admin users) | `--password-file -`, `--must-change-password=false` | +| `-u`, `-p`, `-e` (admin users create) | the username argument or `--username`, `--password`, `--email` | | `--out` (man) | `--output-file` / `-o` | Other long names changed too, mostly on `admin user`, `repo fork`, `repo edit` and @@ -400,6 +404,8 @@ archive, or run `make docs`. on every call, because Gitea replaces it: pass `--authorization-header`, `--authorization-header-file` or `--clear-authorization`. The signing secret cannot be edited; recreate the hook to rotate it. +- `admin user edit --password` no longer forces a password change at the next sign-in + (`admin user create` still does); add `--must-change-password` to require one. - `notification mark-read`, `mark-unread`, `pin` and `unpin` act on the given IDs; pass `--all` to apply them to every notification matching the filters. - `--no-input` disables prompts, editors and browsers. With `--json`, tea never diff --git a/internal/app/admin_users.go b/internal/app/admin_users.go index e7732efe..2d5a3bca 100644 --- a/internal/app/admin_users.go +++ b/internal/app/admin_users.go @@ -185,16 +185,16 @@ $ tea admin user create ci-bot --email ci@example.com --password-file pw.txt --m if verb == "edit" { stringsToBind = append(stringsToBind, stringBinding{"description", "Profile description", &description, &opts.Description}, stringBinding{"website", "Website URL", &website, &opts.Website}, stringBinding{"location", "Location", &location, &opts.Location}) boolsToBind = append(boolsToBind, boolBinding{"active", "Activate account", &active, &opts.Active}, boolBinding{"allow-git-hook", "Allow Git hooks", &allowGitHook, &opts.AllowGitHook}, boolBinding{"allow-import-local", "Allow local repository imports", &allowImportLocal, &opts.AllowImportLocal}, boolBinding{"allow-create-organization", "Allow creating organizations", &allowCreateOrganization, &opts.AllowCreateOrganization}) - command.Flags().IntVar(&maxRepos, "max-repo-creation", 0, "Repository creation limit (-1 for unlimited)") + command.Flags().IntVar(&maxRepos, "max-repo-creation", 0, "Repository creation limit (-1 uses the instance default)") } for _, binding := range stringsToBind { command.Flags().StringVar(binding.value, binding.name, "", binding.description) } - initialVisibility := "" + visibilityUsage := "Profile visibility" if verb == "create" { - initialVisibility = "public" + visibilityUsage += ", by default the instance's default" } - enumFlag(command, &visibility, "visibility", "", initialVisibility, "Profile visibility", visibilities...) + enumFlag(command, &visibility, "visibility", "", "", visibilityUsage, visibilities...) for _, binding := range boolsToBind { command.Flags().BoolVar(binding.value, binding.name, false, binding.description+" (use =false to revoke)") } @@ -215,7 +215,7 @@ $ tea admin user create ci-bot --email ci@example.com --password-file pw.txt --m if _, err := a.requireInput(cmd, &email, "email", "Email address", validEmail); err != nil { return err } - opts.Email, opts.FullName, opts.Visibility = &email, &fullName, &visibility + opts.Email, opts.FullName = &email, &fullName } else { username = args[0] } @@ -329,7 +329,7 @@ func (a *application) adminUserDeleteCommand() *cobra.Command { Long: `Delete an account. Gitea refuses while the user owns repositories or organizations, unless --purge deletes the account with everything it owns.`, Example: `$ tea admin user delete spammer -# also delete the user's repositories, packages and organization memberships +# also delete the user's repositories, packages and organization memberships (and organizations they solely own) $ tea admin user delete spammer --purge`, Annotations: map[string]string{"tea.text": "account deletion does not return a resource", "tea.confirm": "true"}, RunE: func(cmd *cobra.Command, args []string) error { @@ -348,7 +348,7 @@ $ tea admin user delete spammer --purge`, confirmation := terminal.Confirmation{Prompt: "Delete user " + user.Username + "?"} if purge { confirmation = terminal.Confirmation{ - Prompt: fmt.Sprintf("Delete user %s with their repositories, packages and organization memberships? This cannot be undone.", user.Username), + Prompt: fmt.Sprintf("Delete user %s with their repositories, packages and organization memberships, and the organizations they are the last owner of with those organizations' repositories? This cannot be undone.", user.Username), Typed: user.Username, NonTTY: "purge user " + user.Username, } } @@ -356,7 +356,7 @@ $ tea admin user delete spammer --purge`, return err } err = service.Delete(cmd.Context(), user.Username, purge) - if conflict, ok := errors.AsType[*services.ConflictError](err); ok && conflict.Reason == services.ReasonOwnsRepos { + if conflict, ok := errors.AsType[*services.ConflictError](err); ok && (conflict.Reason == services.ReasonOwnsRepos || conflict.Reason == services.ReasonOwnsOther) { conflict.Hint = "transfer or delete them, or pass --purge to delete them with the user" } if err != nil { @@ -370,6 +370,6 @@ $ tea admin user delete spammer --purge`, return nil }, } - cmd.Flags().BoolVar(&purge, "purge", false, "Also delete the user's repositories, packages and organization memberships") + cmd.Flags().BoolVar(&purge, "purge", false, "Also delete the user's repositories, packages, organization memberships and the organizations they solely own") return cmd } diff --git a/internal/app/admin_users_test.go b/internal/app/admin_users_test.go index 332b4cb3..a1b83792 100644 --- a/internal/app/admin_users_test.go +++ b/internal/app/admin_users_test.go @@ -47,7 +47,7 @@ func TestAdminUserEditSetsMustChangePasswordOnlyWhenPassed(t *testing.T) { require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) edits = append(edits, payload) } - _, _ = w.Write([]byte(`{"id":2,"login":"bob"}`)) + _, _ = w.Write([]byte(`[{"id":2,"login":"bob"}]`)) })) defer server.Close() deps, _, _ := serverDeps(t, server.URL) @@ -85,8 +85,8 @@ func TestAdminUserDeleteOffersPurgeForOwners(t *testing.T) { server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.Method { case http.MethodGet: - require.Equal(t, "/api/v1/users/bob", r.URL.Path) - _, _ = w.Write([]byte(`{"id":2,"login":"bob"}`)) + require.Equal(t, "/api/v1/admin/users", r.URL.Path) + _, _ = w.Write([]byte(`[{"id":2,"login":"bob"}]`)) case http.MethodDelete: deletes = append(deletes, r.URL.RequestURI()) if r.URL.Query().Get("purge") != "true" { @@ -103,10 +103,47 @@ func TestAdminUserDeleteOffersPurgeForOwners(t *testing.T) { require.EqualError(t, err, `user "bob" still owns repositories; transfer or delete them, or pass --purge to delete them with the user`) deps.Streams.InTTY, deps.Streams.ErrTTY = true, true - const typed = "Delete user bob with their repositories, packages and organization memberships? This cannot be undone. Type bob to confirm:" + const typed = "Delete user bob with their repositories, packages and organization memberships, and the organizations they are the last owner of with those organizations' repositories? This cannot be undone. Type bob to confirm:" prompt := &scriptedPrompter{inputs: map[string]string{typed: "bob"}} deps.Prompter = prompt require.NoError(t, Execute(context.Background(), deps, []string{"admin", "user", "delete", "bob", "--purge"})) require.Equal(t, []string{typed}, prompt.asked) require.Equal(t, []string{"/api/v1/admin/users/bob", "/api/v1/admin/users/bob?purge=true"}, deletes) } + +func TestAdminUserDeleteOffersPurgeForMembersAndPackageOwners(t *testing.T) { + message := "user still has membership of organizations [uid: 2]" + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`[{"id":2,"login":"bob"}]`)) + return + } + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message":"` + message + `"}`)) + })) + defer server.Close() + deps, _, _ := serverDeps(t, server.URL) + err := Execute(context.Background(), deps, []string{"admin", "user", "delete", "bob", "--yes"}) + require.EqualError(t, err, `user "bob" is still a member of organizations or owns packages; transfer or delete them, or pass --purge to delete them with the user`) + message = "user still has ownership of packages [uid: 2]" + err = Execute(context.Background(), deps, []string{"admin", "user", "delete", "bob", "--yes"}) + require.ErrorContains(t, err, "pass --purge") +} + +func TestAdminUserCreateLeavesVisibilityToTheInstanceUnlessPassed(t *testing.T) { + var payloads []map[string]any + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var payload map[string]any + require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) + payloads = append(payloads, payload) + _, _ = w.Write([]byte(`{"id":1,"login":"account"}`)) + })) + defer server.Close() + deps, _, _ := serverDeps(t, server.URL) + args := []string{"admin", "user", "create", "account", "--email", "account@example.test", "--password", "secret", "--no-input"} + require.NoError(t, Execute(context.Background(), deps, args)) + require.NoError(t, Execute(context.Background(), deps, append(args, "--visibility", "limited"))) + require.Len(t, payloads, 2) + require.Nil(t, payloads[0]["visibility"], "an omitted visibility lets DEFAULT_USER_VISIBILITY apply") + require.Equal(t, "limited", payloads[1]["visibility"]) +} diff --git a/internal/app/ssh_keys.go b/internal/app/ssh_keys.go index 6e4113f2..ce64489b 100644 --- a/internal/app/ssh_keys.go +++ b/internal/app/ssh_keys.go @@ -6,6 +6,7 @@ package app import ( "bytes" "context" + "encoding/base64" "errors" "fmt" "os" @@ -120,11 +121,42 @@ func publicKey(data []byte, source string) (ssh.PublicKey, string, error) { } key, comment, _, rest, err := ssh.ParseAuthorizedKey(data) if err != nil || len(bytes.TrimSpace(rest)) != 0 { + if key, comment, ok := parseSSH2PublicKey(data); ok { + return key, comment, nil + } return nil, "", clierr.Wrap(clierr.Usage, fmt.Errorf("%s must hold exactly one valid SSH public key", sshKeySource(source))) } return key, comment, nil } +// parseSSH2PublicKey reads the RFC 4716 format of PuTTYgen and ssh-keygen -e, which Gitea accepts +// as well. It returns the key and the Comment header without its quotes. +func parseSSH2PublicKey(data []byte) (ssh.PublicKey, string, bool) { + lines := strings.Split(strings.TrimSpace(strings.ReplaceAll(string(data), "\r", "")), "\n") + if len(lines) < 3 || lines[0] != "---- BEGIN SSH2 PUBLIC KEY ----" || lines[len(lines)-1] != "---- END SSH2 PUBLIC KEY ----" { + return nil, "", false + } + var body strings.Builder + var comment string + continued := false + for _, line := range lines[1 : len(lines)-1] { + header := continued || strings.Contains(line, ":") + continued = header && strings.HasSuffix(line, "\\") + switch { + case !header: + body.WriteString(strings.TrimSpace(line)) + case strings.HasPrefix(line, "Comment:"): + comment = strings.Trim(strings.TrimSpace(strings.TrimPrefix(line, "Comment:")), `"`) + } + } + blob, err := base64.StdEncoding.DecodeString(body.String()) + if err != nil { + return nil, "", false + } + key, err := ssh.ParsePublicKey(blob) + return key, comment, err == nil +} + func sshKeySource(source string) string { if source == "-" { return "stdin" @@ -147,7 +179,7 @@ $ cat key.pub | tea ssh-key add`, if len(args) > 0 { source = args[0] } - if source == "-" && a.deps.Streams.InTTY { + if len(args) == 0 && a.deps.Streams.InTTY { return clierr.Wrap(clierr.Usage, errors.New("supply a public key file, such as ~/.ssh/id_ed25519.pub, or pipe a key to stdin")) } data, err := readFileOrStdin(a.deps.Streams.In, source) @@ -174,6 +206,10 @@ $ cat key.pub | tea ssh-key add`, service := services.SSHKeys{Client: target.client} created, err := service.Create(cmd.Context(), title, strings.TrimSpace(string(data))) if err != nil { + if conflict, ok := errors.AsType[*services.ConflictError](err); ok && conflict.Reason == services.ReasonExists { + conflict.Hint = fmt.Sprintf("another SSH key is titled %q; pass a different --title", title) + return conflict + } return a.registeredKey(cmd.Context(), service, key, err) } a.done(cmd, "Added %s", sshKeyLabel(created)) diff --git a/internal/app/ssh_keys_test.go b/internal/app/ssh_keys_test.go index 811500c6..9a6ddbbe 100644 --- a/internal/app/ssh_keys_test.go +++ b/internal/app/ssh_keys_test.go @@ -8,6 +8,7 @@ import ( "context" "crypto/ed25519" "crypto/rand" + "encoding/base64" "encoding/json" "fmt" "net/http" @@ -96,3 +97,41 @@ func TestSSHKeyDeleteNamesTheKey(t *testing.T) { require.Equal(t, []string{`Delete SSH key "uxkey" (SHA256:aR5L)?`}, prompt.confirmations) require.False(t, deleted) } + +func TestSSHKeyAddAcceptsRFC4716KeysAndExplicitStdinOnATerminal(t *testing.T) { + line, fingerprint := testPublicKey(t, "") + key, _, _, _, err := ssh.ParseAuthorizedKey([]byte(line)) + require.NoError(t, err) + blob := base64.StdEncoding.EncodeToString(key.Marshal()) + rfc4716 := "---- BEGIN SSH2 PUBLIC KEY ----\nComment: \"putty key\"\n" + blob[:30] + "\n" + blob[30:] + "\n---- END SSH2 PUBLIC KEY ----\n" + var titles []string + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var body map[string]any + require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) + titles = append(titles, body["title"].(string)) + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprintf(w, `{"id":3,"title":%q,"fingerprint":%q}`, body["title"], fingerprint) + })) + defer server.Close() + deps, _, _ := serverDeps(t, server.URL) + deps.Streams.InTTY = true + deps.Streams.In = bytes.NewBufferString(rfc4716) + require.NoError(t, Execute(context.Background(), deps, []string{"ssh-key", "create", "-"})) + require.Equal(t, []string{"putty key"}, titles) + + err = Execute(context.Background(), deps, []string{"ssh-key", "create"}) + require.ErrorContains(t, err, "supply a public key file") +} + +func TestSSHKeyAddExplainsATakenTitle(t *testing.T) { + key, _ := testPublicKey(t, "alice@laptop") + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnprocessableEntity) + _, _ = w.Write([]byte(`{"message":"Key title has been used"}`)) + })) + defer server.Close() + deps, _, _ := serverDeps(t, server.URL) + deps.Streams.In = bytes.NewBufferString(key) + err := Execute(context.Background(), deps, []string{"ssh-key", "add"}) + require.EqualError(t, err, `SSH key already exists; another SSH key is titled "alice@laptop"; pass a different --title`) +} diff --git a/internal/app/webhooks.go b/internal/app/webhooks.go index 480ac0ef..36f0db94 100644 --- a/internal/app/webhooks.go +++ b/internal/app/webhooks.go @@ -261,7 +261,8 @@ Events: ` + strings.Join(services.WebhookEvents, ", ") + `; issues and pull_requ include every issue or pull request event. Types: ` + strings.Join(services.WebhookTypes, ", ") + `. Slack webhooks need ---config channel=#name; other types take their settings through --config too.` +--config channel=#name (username, icon_url and color are optional). Gitea reads no other +settings from --config, so set up discord, matrix and packagist webhooks in the web UI.` command.Example = `$ tea webhook create https://ci.example.com/hooks/gitea --events push,pull_request # sign deliveries with a secret read from a file $ tea webhook create https://ci.example.com/hooks/gitea --secret-file secret.txt diff --git a/internal/services/admin_users.go b/internal/services/admin_users.go index 9ec1e0b9..ff5f5d2e 100644 --- a/internal/services/admin_users.go +++ b/internal/services/admin_users.go @@ -9,6 +9,7 @@ import ( "net/http" "net/url" "strconv" + "strings" gitea "gitea.dev/sdk" "gitea.dev/tea/internal/transport" @@ -88,11 +89,26 @@ func (s AdminUsers) Search(ctx context.Context, query string, limit int) ([]Admi return convertAll(users, adminUserFrom), err } +// get finds an account by its exact username through the administrator listing. Unlike +// /users/{name}, that answers only administrators, only with complete role and status fields, +// and only for individual accounts, never for organizations. func (s AdminUsers) get(ctx context.Context, username string) (*gitea.User, error) { - user, _, err := call(ctx, s.Client, func(ctx context.Context, c *gitea.Client) (*gitea.User, *gitea.Response, error) { - return c.Users.GetUserInfo(ctx, username) - }) - return user, err + for page := 1; ; page++ { + users, err := callPage(ctx, s.Client, 50, page, func(ctx context.Context, c *gitea.Client, list gitea.ListOptions) ([]*gitea.User, *gitea.Response, error) { + return c.Admin.ListUsers(ctx, gitea.AdminListUsersOptions{ListOptions: list, Query: username}) + }) + if err != nil { + return nil, err + } + for _, user := range users.Items { + if strings.EqualFold(user.UserName, username) { + return user, nil + } + } + if !users.More { + return nil, NotFound("user", strconv.Quote(username), "") + } + } } // View retrieves one account. diff --git a/internal/services/admin_users_test.go b/internal/services/admin_users_test.go index d2db78fa..d5b5f340 100644 --- a/internal/services/admin_users_test.go +++ b/internal/services/admin_users_test.go @@ -17,12 +17,13 @@ func TestAdminEditPreservesAuthenticationAndExplicitFalse(t *testing.T) { client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method == http.MethodGet { reads++ - require.Equal(t, "/api/v1/users/account", r.URL.Path) + require.Equal(t, "/api/v1/admin/users", r.URL.Path) + require.Equal(t, "account", r.URL.Query().Get("q")) if mutations == 0 { - _, _ = w.Write([]byte(`{"id":1,"login":"account","login_name":"external-identity","source_id":7,"is_admin":true}`)) + _, _ = w.Write([]byte(`[{"id":2,"login":"account-2"},{"id":1,"login":"account","login_name":"external-identity","source_id":7,"is_admin":true}]`)) return } - _, _ = w.Write([]byte(`{"id":1,"login":"account","is_admin":false,"created":"2026-01-02T03:04:05Z"}`)) + _, _ = w.Write([]byte(`[{"id":1,"login":"account","is_admin":false,"created":"2026-01-02T03:04:05Z"}]`)) return } mutations++ @@ -65,7 +66,7 @@ func TestAdminCreateAppliesRestrictionsAndReloads(t *testing.T) { require.Equal(t, true, payload["restricted"]) w.WriteHeader(http.StatusNoContent) default: - _, _ = w.Write([]byte(`{"id":4,"login":"new-account","restricted":true}`)) + _, _ = w.Write([]byte(`[{"id":4,"login":"new-account","restricted":true}]`)) } })) restricted := true @@ -73,7 +74,7 @@ func TestAdminCreateAppliesRestrictionsAndReloads(t *testing.T) { item, err := (AdminUsers{Client: client}).Create(context.Background(), "new-account", AdminUserOptions{Email: &email, Password: "local-password", Visibility: &visibility, Restricted: &restricted}) require.NoError(t, err) require.True(t, item.Restricted) - require.Equal(t, []string{"POST /api/v1/admin/users", "PATCH /api/v1/admin/users/new-account", "GET /api/v1/users/new-account"}, requests) + require.Equal(t, []string{"POST /api/v1/admin/users", "PATCH /api/v1/admin/users/new-account", "GET /api/v1/admin/users"}, requests) } func TestAdminCreateReportsFollowupFailureWithoutRetrying(t *testing.T) { @@ -108,3 +109,12 @@ func TestAdminUserFromMapsServerFields(t *testing.T) { Visibility: "limited", CreatedAt: "2026-10-01T12:00:00+02:00", }, adminUserFrom(user)) } + +func TestAdminViewFindsOnlyIndividualAccountsByExactName(t *testing.T) { + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/api/v1/admin/users", r.URL.Path, "the public /users/{name} endpoint also answers for organizations and hides roles from other accounts") + _, _ = w.Write([]byte(`[{"id":2,"login":"acme-bot"}]`)) + })) + _, err := (AdminUsers{Client: client}).View(context.Background(), "acme") + require.EqualError(t, err, `user "acme" not found`) +} diff --git a/internal/services/errors.go b/internal/services/errors.go index 1b120305..890edcba 100644 --- a/internal/services/errors.go +++ b/internal/services/errors.go @@ -61,6 +61,7 @@ const ( ReasonNotDone = "not done" // the run has not finished yet ReasonKeyInUse = "key in use" // the SSH key is registered already ReasonOwnsRepos = "owns repositories" // the user still owns repositories + ReasonOwnsOther = "owns other" // the user is still in organizations or owns packages ) // ConflictError reports a request the current state of a resource refuses, such as closing a @@ -105,6 +106,8 @@ func (e *ConflictError) Error() string { text = "this SSH key is already registered" case ReasonOwnsRepos: text = cmp.Or(subject, "the user") + " still owns repositories" + case ReasonOwnsOther: + text = cmp.Or(subject, "the user") + " is still a member of organizations or owns packages" default: text = e.Err.Error() } @@ -153,7 +156,8 @@ var refusals = []struct { {ReasonNotDone, []int{http.StatusBadRequest}, []string{"not done"}}, {ReasonKeyInUse, nil, []string{"key content has been used"}}, {ReasonOwnsRepos, nil, []string{"ownership of repositories"}}, - {ReasonExists, []int{http.StatusBadRequest, http.StatusConflict, http.StatusUnprocessableEntity}, []string{"already exist", "release is has no tag"}}, + {ReasonOwnsOther, nil, []string{"membership of organizations", "ownership of packages"}}, + {ReasonExists, []int{http.StatusBadRequest, http.StatusConflict, http.StatusUnprocessableEntity}, []string{"already exist", "release is has no tag", "key title has been used"}}, } var ( diff --git a/internal/services/ssh_keys.go b/internal/services/ssh_keys.go index 6af727a5..a0574076 100644 --- a/internal/services/ssh_keys.go +++ b/internal/services/ssh_keys.go @@ -5,7 +5,9 @@ package services import ( "context" + "fmt" "strconv" + "strings" gitea "gitea.dev/sdk" "gitea.dev/tea/internal/transport" @@ -36,7 +38,8 @@ func (s SSHKeys) List(ctx context.Context, limit, page int) (Page[SSHKey], error return convertPage(keys, sshKeyFrom), err } -// View retrieves a public key by ID. +// View retrieves a public key of the account by ID. Gitea also answers for other accounts' keys +// and lets administrators delete them, so a key that belongs to someone else is refused. func (s SSHKeys) View(ctx context.Context, id int64) (SSHKey, error) { key, _, err := call(ctx, s.Client, func(ctx context.Context, c *gitea.Client) (*gitea.PublicKey, *gitea.Response, error) { return c.Users.GetPublicKey(ctx, id) @@ -44,6 +47,15 @@ func (s SSHKeys) View(ctx context.Context, id int64) (SSHKey, error) { if err != nil { return SSHKey{}, Classify(err, "SSH key", strconv.FormatInt(id, 10), "") } + if key.Owner != nil { + account, err := CurrentAccount(ctx, s.Client) + if err != nil { + return SSHKey{}, err + } + if owner := login(key.Owner); !strings.EqualFold(owner, account) { + return SSHKey{}, fmt.Errorf("SSH key %d belongs to %s, not to %s", id, owner, account) + } + } return sshKeyFrom(key), nil } diff --git a/internal/services/ssh_keys_test.go b/internal/services/ssh_keys_test.go index 6d06c99a..b827c30d 100644 --- a/internal/services/ssh_keys_test.go +++ b/internal/services/ssh_keys_test.go @@ -48,6 +48,10 @@ func TestSSHKeyViewCreateAndDelete(t *testing.T) { case http.MethodDelete: w.WriteHeader(http.StatusNoContent) default: + if r.URL.Path == "/api/v1/user" { + _, _ = w.Write([]byte(`{"login":"alice"}`)) + return + } _, _ = w.Write([]byte(sshKeyJSON)) } })) @@ -59,5 +63,17 @@ func TestSSHKeyViewCreateAndDelete(t *testing.T) { require.NoError(t, err) require.EqualValues(t, 3, created.ID) require.NoError(t, service.Delete(context.Background(), 3)) - require.Equal(t, []string{"GET /api/v1/user/keys/3", "POST /api/v1/user/keys", "DELETE /api/v1/user/keys/3"}, requests) + require.Equal(t, []string{"GET /api/v1/user/keys/3", "GET /api/v1/user", "POST /api/v1/user/keys", "DELETE /api/v1/user/keys/3"}, requests) +} + +func TestSSHKeyViewRefusesKeysOfOtherAccounts(t *testing.T) { + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/user" { + _, _ = w.Write([]byte(`{"login":"admin"}`)) + return + } + _, _ = w.Write([]byte(sshKeyJSON)) + })) + _, err := (SSHKeys{Client: client}).View(context.Background(), 3) + require.EqualError(t, err, "SSH key 3 belongs to alice, not to admin") } diff --git a/internal/services/webhooks.go b/internal/services/webhooks.go index f7936479..d4033bad 100644 --- a/internal/services/webhooks.go +++ b/internal/services/webhooks.go @@ -4,7 +4,6 @@ package services import ( - "cmp" "context" "maps" "net/url" @@ -78,6 +77,27 @@ func webhookFrom(hook *gitea.Hook, reveal bool) Webhook { } } +// eventGroupMembers are the events that sending issues or pull_request enables besides that event. +var eventGroupMembers = map[string][]string{ + "issues": {"issue_assign", "issue_comment", "issue_label", "issue_milestone"}, + "pull_request": {"pull_request_assign", "pull_request_comment", "pull_request_label", "pull_request_milestone", "pull_request_review", "pull_request_review_request", "pull_request_sync"}, +} + +// narrowedEvents turns the events Gitea reports into events that select the same set when sent +// back. Gitea reports a hook of the issue or pull request events alone as issues or pull_request, +// but taking those names back enables their whole group, so they become issues_only and +// pull_request_only unless every member of the group is there too. +func narrowedEvents(events []string) []string { + narrowed := slices.Clone(events) + for head, members := range eventGroupMembers { + index := slices.Index(narrowed, head) + if index >= 0 && slices.ContainsFunc(members, func(member string) bool { return !slices.Contains(events, member) }) { + narrowed[index] = head + "_only" + } + } + return narrowed +} + // WebhookScope selects one repository, organization, account or system scope. type WebhookScope struct { Repository, Organization string @@ -143,7 +163,18 @@ func (s Webhooks) get(ctx context.Context, id int64) (*gitea.Hook, error) { } return c.Hooks.GetRepoHook(ctx, owner, name, id) }) - return hook, Classify(err, "webhook", strconv.FormatInt(id, 10), cmp.Or(s.Scope.Repository, s.Scope.Organization)) + return hook, Classify(err, "webhook", strconv.FormatInt(id, 10), s.errorScope()) +} + +// errorScope names the repository or organization the selected scope looks in, for error messages. +func (s Webhooks) errorScope() string { + switch { + case s.Scope.Global, s.Scope.Account: + return "" + case s.Scope.Organization != "": + return s.Scope.Organization + } + return s.Scope.Repository } // View retrieves hook metadata without exposing its delivery configuration. @@ -199,7 +230,7 @@ func (s Webhooks) Edit(ctx context.Context, id int64, opts WebhookOptions) (Webh if err != nil { return Webhook{}, err } - edit := gitea.EditHookOption{Events: current.Events, BranchFilter: current.BranchFilter, Active: opts.Active, AuthorizationHeader: *opts.Authorization} + edit := gitea.EditHookOption{Events: narrowedEvents(current.Events), BranchFilter: current.BranchFilter, Active: opts.Active, AuthorizationHeader: *opts.Authorization} if opts.Events != nil { edit.Events = *opts.Events } diff --git a/internal/services/webhooks_test.go b/internal/services/webhooks_test.go index 7a983419..2918a308 100644 --- a/internal/services/webhooks_test.go +++ b/internal/services/webhooks_test.go @@ -34,7 +34,7 @@ func TestWebhookEditPreservesSettingsAndRedactsCredentials(t *testing.T) { w.WriteHeader(http.StatusBadRequest) return } - assert.Equal(t, []any{"issues", "release"}, payload["events"]) + assert.Equal(t, []any{"issues_only", "release"}, payload["events"]) assert.Equal(t, "main", payload["branch_filter"]) assert.Equal(t, "", payload["authorization_header"]) assert.Equal(t, false, payload["active"]) @@ -171,3 +171,32 @@ func TestWebhookFromRedactsDeliveryURLs(t *testing.T) { }) } } + +func TestWebhookEditKeepsEventGroupsNarrow(t *testing.T) { + issues := []string{"issue_assign", "issue_comment", "issue_label", "issue_milestone", "issues"} + pulls := []string{"pull_request", "pull_request_assign", "pull_request_comment", "pull_request_label", "pull_request_milestone", "pull_request_review", "pull_request_review_request", "pull_request_sync"} + require.Equal(t, []string{"push", "issues_only", "pull_request_only", "pull_request_review"}, narrowedEvents([]string{"push", "issues", "pull_request", "pull_request_review"})) + require.Equal(t, []string{"issues_only", "issue_label"}, narrowedEvents([]string{"issues", "issue_label"})) + require.Equal(t, issues, narrowedEvents(issues)) + require.Equal(t, pulls, narrowedEvents(pulls)) +} + +func TestWebhookNotFoundNamesTheSelectedScope(t *testing.T) { + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + for name, tc := range map[string]struct { + scope WebhookScope + want string + }{ + "organization": {WebhookScope{Repository: "owner/repo", Organization: "acme"}, "webhook 9 not found in acme"}, + "account": {WebhookScope{Repository: "owner/repo", Account: true}, "webhook 9 not found"}, + "global": {WebhookScope{Repository: "owner/repo", Global: true}, "webhook 9 not found"}, + "repository": {WebhookScope{Repository: "owner/repo"}, "webhook 9 not found in owner/repo"}, + } { + t.Run(name, func(t *testing.T) { + _, err := (Webhooks{Client: client, Scope: tc.scope}).View(context.Background(), 9) + require.EqualError(t, err, tc.want) + }) + } +}