fix: address verified review findings (admin-webhooks-keys-1)

Signed-off-by: bircni <bircni@icloud.com>

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
 }

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"])
+}

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))

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`)
+}

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