fix(org,team): guard the owner team rename, show permission-only team access and tidy member lookups

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

internal/app/app.go

@@ -539,6 +539,9 @@ func commandOutputFields(cmd *cobra.Command) []string {
 	case "org":
 		return jsonFields[services.Organization]()
 	case "member":
+		if cmd.Parent().Parent() != nil && cmd.Parent().Parent().Name() == "team" {
+			return jsonFields[services.TeamMember]()
+		}
 		return jsonFields[services.OrganizationMember]()
 	case "asset":
 		return jsonFields[services.ReleaseAsset]()

internal/app/human_output.go

@@ -83,6 +83,10 @@ func writeTable[T any](a *application, cmd *cobra.Command, items []T, spec listS
 	}
 	d := a.display()
 	if len(items) == 0 {
+		// A filtered page can be empty while later pages are not.
+		if page.More {
+			return a.morePagesHint(cmd, true, page.Number)
+		}
 		if !d.TTY || spec.Empty == "" {
 			return nil
 		}

internal/app/human_output_test.go

@@ -282,6 +282,8 @@ func TestWriteTable(t *testing.T) {
 		},
 		{name: "empty message on a terminal", tty: true, page: listPage{Number: 1}, stderr: "no open issues in alice/x\n"},
 		{name: "nothing when empty and piped", page: listPage{Number: 1}},
+		{name: "next-page hint when an empty page has more", tty: true, page: listPage{Number: 1, More: true}, stderr: "more results available; use --page 2\n"},
+		{name: "next-page hint when an empty page has more and piped", page: listPage{Number: 1, More: true}, stderr: "more results available; use --page 2\n"},
 		{name: "JSON", tty: true, json: "number", items: items[:1], page: listPage{Number: 1, More: true}, out: "[\n  {\n    \"number\": 12\n  }\n]\n"},
 	} {
 		t.Run(tc.name, func(t *testing.T) {

internal/app/pull_integration_test.go

@@ -326,7 +326,7 @@ func testTeamReviewRequests(t *testing.T, deps *Dependencies, execute func(...st
 			t.Errorf("team review repository cleanup: %v", err)
 		}
 	})
-	execute("team", "repo", "add", teamID, "-R", repo.FullName)
+	execute("team", "repo", "add", teamID, repo.FullName)
 	execute("branch", "create", "feature", "-R", repo.FullName)
 	deps.Streams.In = strings.NewReader(`{"branch":"feature","content":"dGVzdAo=","message":"Disposable team review change"}`)
 	execute("api", "repos/"+repo.FullName+"/contents/change.txt", "--input", "-")

internal/app/teams.go

@@ -197,13 +197,13 @@ $ tea team view 2 --json units`,
 			return writeView(a, cmd, team, func(team services.Team) terminal.View {
 				var units []string
 				for _, unit := range slices.Concat(services.TeamUnits, services.ExternalTeamUnits) {
-					if access := team.Units[unit]; access != "" && access != "none" {
+					if access := team.UnitLevel(unit); access != "none" {
 						units = append(units, strings.TrimPrefix(unit, "repo.")+": "+access)
 					}
 				}
 				memberNames := ""
 				if !membersHidden {
-					memberNames = listedNames(members, func(member services.OrganizationMember) string { return member.Username })
+					memberNames = listedNames(members, func(member services.TeamMember) string { return member.Username })
 				}
 				repositories := listedNames(repos, func(repo services.Repository) string { return repo.FullName })
 				if team.AllRepositories {
@@ -425,6 +425,10 @@ $ tea team edit acme/developers --name devs --description "Core developers"`,
 			if opts.Units != nil && !opts.ReplaceUnits && (team.Permission == "admin" || team.Permission == "owner") {
 				return clierr.Wrap(clierr.Usage, fmt.Errorf("--unit cannot restrict the %s access of team %s; pass --permission read or write, or --unit-replace", team.Permission, team.Slug()))
 			}
+			// Gitea finds the owner team by its name, so renaming it leaves the organization without owners.
+			if team.Permission == "owner" && opts.Name != nil && !strings.EqualFold(*opts.Name, team.Name) {
+				return clierr.Wrap(clierr.Usage, fmt.Errorf("team %s is the organization's owner team and cannot be renamed", team.Slug()))
+			}
 			edited, err := service.Edit(cmd.Context(), team.ID, opts)
 			if err != nil {
 				return err
@@ -502,9 +506,9 @@ also makes them a member of the organization.`}
 			if err != nil {
 				return err
 			}
-			return writeTable(a, cmd, members.Items, listSpec[services.OrganizationMember]{
+			return writeTable(a, cmd, members.Items, listSpec[services.TeamMember]{
 				Columns: []terminal.Column{{Header: "Username"}, {Header: "Name", Flex: true}},
-				Row: func(member services.OrganizationMember) []terminal.Cell {
+				Row: func(member services.TeamMember) []terminal.Cell {
 					return []terminal.Cell{{Text: member.Username, Link: webURL(target, member.Username), Bold: true}, {Text: member.FullName}}
 				},
 				Context: "members of team " + team.Slug(), Empty: "no members in team " + team.Slug(),
@@ -543,7 +547,7 @@ $ tea team member add 2 bob carol`,
 		Annotations: map[string]string{"tea.text": "team membership changes do not return a resource", "tea.confirm": "true", "tea.arguments": teamArguments},
 		ValidArgsFunction: a.completeTeamThen(func(ctx context.Context, t target, team services.Team) ([]cobra.Completion, error) {
 			members, err := services.Teams{Client: t.client}.Members(ctx, team.ID, completionLimit, 1)
-			return describe(members.Items, func(member services.OrganizationMember) (string, string) { return member.Username, member.FullName }), err
+			return describe(members.Items, func(member services.TeamMember) (string, string) { return member.Username, member.FullName }), err
 		}),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			_, service, team, err := a.team(cmd, args[0])

internal/app/teams_test.go

@@ -81,6 +81,24 @@ func TestTeamEditMergesUnitsIntoTheCurrentPolicy(t *testing.T) {
 	require.Len(t, sent, 2)
 }
 
+func TestTeamEditRefusesToRenameTheOwnerTeam(t *testing.T) {
+	var patched int
+	server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodPatch {
+			patched++
+		}
+		_, _ = w.Write([]byte(`{"id":3,"name":"Owners","permission":"owner","organization":{"username":"acme"}}`))
+	}))
+	defer server.Close()
+	deps, _, _ := serverDeps(t, server.URL)
+	err := Execute(context.Background(), deps, []string{"team", "edit", "3", "--name", "Admins"})
+	require.Equal(t, clierr.Usage, clierr.ExitCode(err), "%v", err)
+	require.ErrorContains(t, err, "owner team and cannot be renamed")
+	require.Zero(t, patched)
+	require.NoError(t, Execute(context.Background(), deps, []string{"team", "edit", "3", "--description", "Owners", "--json"}))
+	require.Equal(t, 1, patched)
+}
+
 func TestTeamRepoAddTakesRepositoriesAsArguments(t *testing.T) {
 	var granted []string
 	server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

internal/services/organizations.go

@@ -163,7 +163,7 @@ func (s Organizations) Create(ctx context.Context, opts OrganizationOptions) (Or
 		})
 	})
 	if err != nil {
-		return Organization{}, Classify(err, "organization", "", "")
+		return Organization{}, Classify(err, "name", strconv.Quote(deref(opts.Name)), "")
 	}
 	return organizationFrom(org), nil
 }
@@ -285,7 +285,15 @@ func (s Organizations) IsPublicMember(ctx context.Context, name, username string
 // check runs a membership check that answers no with a 404.
 func (s Organizations) check(ctx context.Context, op func(context.Context, *gitea.Client) (*gitea.Response, error)) (bool, error) {
 	err := do(ctx, s.Client, op)
+	// Gitea redirects callers who are not members, and tea does not follow the redirect.
+	if failure, ok := errors.AsType[*transport.APIError](err); ok && failure.Status == http.StatusSeeOther {
+		return false, &transport.APIError{Status: http.StatusForbidden, Method: failure.Method, Endpoint: failure.Endpoint, Message: "only organization members can check memberships"}
+	}
 	if IsNotFound(err) {
+		// A mistyped user is not a missing membership.
+		if missing, ok := errors.AsType[*NotFoundError](Classify(err, "", "", "")); ok && missing.Kind == "user" {
+			return false, missing
+		}
 		return false, nil
 	}
 	return err == nil, err

internal/services/organizations_test.go

@@ -108,6 +108,38 @@ func TestOrganizationCreateSendsSuppliedPreferences(t *testing.T) {
 	require.Equal(t, Organization{ID: 5, Name: "acme", Visibility: "limited", RepoAdminChangeTeamAccess: true}, item)
 }
 
+func TestOrganizationMembershipChecksReportWhatGiteaRefuses(t *testing.T) {
+	client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/orgs/acme/members/bob":
+			http.Redirect(w, r, "/org/acme/public_members", http.StatusSeeOther)
+		case "/api/v1/orgs/acme/members/bbo":
+			w.WriteHeader(http.StatusNotFound)
+			_, _ = w.Write([]byte(`{"message":"user does not exist [uid: 0, name: bbo]"}`))
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	service := Organizations{Client: client}
+	_, err := service.IsMember(context.Background(), "acme", "bob")
+	require.True(t, IsForbidden(err), "%v", err)
+	_, err = service.IsMember(context.Background(), "acme", "bbo")
+	require.EqualError(t, err, `user "bbo" not found`)
+	member, err := service.IsMember(context.Background(), "acme", "carol")
+	require.NoError(t, err)
+	require.False(t, member)
+}
+
+func TestOrganizationCreateNamesTheTakenName(t *testing.T) {
+	client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusUnprocessableEntity)
+		_, _ = w.Write([]byte(`{"message":"user already exists [name: bob]"}`))
+	}))
+	name := "bob"
+	_, err := (Organizations{Client: client}).Create(context.Background(), OrganizationOptions{Name: &name})
+	require.EqualError(t, err, `name "bob" already exists`)
+}
+
 func TestOrganizationMembershipRequests(t *testing.T) {
 	var requests []string
 	client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

internal/services/teams.go

@@ -67,8 +67,8 @@ func (t Team) Access() string {
 	var level string
 	var granted []string
 	for _, unit := range TeamUnits {
-		access := t.Units[unit]
-		if access == "" || access == "none" {
+		access := t.UnitLevel(unit)
+		if access == "none" {
 			continue
 		}
 		if level != "" && access != level {
@@ -86,6 +86,22 @@ func (t Team) Access() string {
 	return level + " (" + strings.Join(granted, ", ") + ")"
 }
 
+// UnitLevel is the access the team has to unit: the higher of the unit's own level and the team
+// permission, which Gitea grants on every unit, capped at the levels the unit allows.
+func (t Team) UnitLevel(unit string) string {
+	levels := UnitAccess(unit)
+	level := "none"
+	for _, candidate := range []string{t.Units[unit], t.Permission} {
+		if candidate == "write" && !slices.Contains(levels, "write") {
+			candidate = "read"
+		}
+		if slices.Index(levels, candidate) > slices.Index(levels, level) {
+			level = candidate
+		}
+	}
+	return level
+}
+
 // Slug names the team as ORG/TEAM, or by name alone when its organization is unknown.
 func (t Team) Slug() string {
 	if t.Organization == "" {
@@ -273,12 +289,23 @@ func (s Teams) Delete(ctx context.Context, id int64) error {
 	})
 }
 
+// TeamMember identifies a team member without exposing private profile fields.
+type TeamMember struct {
+	ID       int64  `json:"id"`
+	Username string `json:"username"`
+	FullName string `json:"full_name"`
+}
+
+func teamMemberFrom(user *gitea.User) TeamMember {
+	return TeamMember{ID: user.ID, Username: user.UserName, FullName: user.FullName}
+}
+
 // Members lists members of a team.
-func (s Teams) Members(ctx context.Context, id int64, limit, page int) (Page[OrganizationMember], error) {
+func (s Teams) Members(ctx context.Context, id int64, limit, page int) (Page[TeamMember], error) {
 	users, err := callPage(ctx, s.Client, limit, page, func(ctx context.Context, c *gitea.Client, list gitea.ListOptions) ([]*gitea.User, *gitea.Response, error) {
 		return c.Organizations.ListTeamMembers(ctx, id, gitea.ListTeamMembersOptions{ListOptions: list})
 	})
-	return convertPage(users, memberFrom), err
+	return convertPage(users, teamMemberFrom), err
 }
 
 // SetMember adds or removes one team membership.

internal/services/teams_test.go

@@ -102,7 +102,7 @@ func TestTeamAccessRequests(t *testing.T) {
 	service := Teams{Client: client}
 	members, err := service.Members(context.Background(), 3, 5, 2)
 	require.NoError(t, err)
-	require.Equal(t, []OrganizationMember{{ID: 2, Username: "bob"}}, members.Items)
+	require.Equal(t, []TeamMember{{ID: 2, Username: "bob"}}, members.Items)
 	repos, err := service.Repositories(context.Background(), 3, 5, 1)
 	require.NoError(t, err)
 	require.Equal(t, "https://gitea.test/acme/project", repos.Items[0].URL)
@@ -119,6 +119,15 @@ func TestTeamFromKeepsAbsentValuesEmpty(t *testing.T) {
 	require.Equal(t, Team{ID: 1, Name: "Owners", Permission: "owner", Units: map[string]string{}}, item)
 }
 
+func TestTeamUnitLevelAppliesThePermissionToEveryUnit(t *testing.T) {
+	team := Team{Permission: "write", Units: map[string]string{"repo.wiki": "none"}}
+	require.Equal(t, "write", team.UnitLevel("repo.code"))
+	require.Equal(t, "write", team.UnitLevel("repo.wiki"))
+	require.Equal(t, "read", team.UnitLevel("repo.ext_wiki"))
+	require.Equal(t, "write", Team{Permission: "none", Units: map[string]string{"repo.code": "write"}}.UnitLevel("repo.code"))
+	require.Equal(t, "none", Team{Permission: "none"}.UnitLevel("repo.code"))
+}
+
 func TestTeamAccessSummarizesUnits(t *testing.T) {
 	for _, tc := range []struct {
 		permission string
@@ -132,6 +141,11 @@ func TestTeamAccessSummarizesUnits(t *testing.T) {
 		{"none", map[string]string{"repo.code": "write", "repo.issues": "write", "repo.pulls": "write", "repo.wiki": "none", "repo.ext_wiki": "read"}, "write (code, issues, pulls)"},
 		{"none", map[string]string{"repo.code": "write", "repo.wiki": "read"}, "custom"},
 		{"none", map[string]string{"repo.ext_issues": "read"}, "none"},
+		// Gitea grants the team permission on every unit when the team has no unit rows.
+		{"write", nil, "write"},
+		{"read", map[string]string{}, "read"},
+		{"write", map[string]string{"repo.wiki": "read"}, "write"},
+		{"read", map[string]string{"repo.code": "write"}, "custom"},
 	} {
 		require.Equal(t, tc.want, Team{Permission: tc.permission, Units: tc.units}.Access(), "%v", tc.units)
 	}