diff --git a/internal/app/app.go b/internal/app/app.go index 4a05a176..713b6bbd 100644 --- a/internal/app/app.go +++ b/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]() diff --git a/internal/app/human_output.go b/internal/app/human_output.go index a44bac95..3ceb6e34 100644 --- a/internal/app/human_output.go +++ b/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 } diff --git a/internal/app/human_output_test.go b/internal/app/human_output_test.go index 5c13489b..a458c822 100644 --- a/internal/app/human_output_test.go +++ b/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) { diff --git a/internal/app/pull_integration_test.go b/internal/app/pull_integration_test.go index b5b7a3f4..e06d6956 100644 --- a/internal/app/pull_integration_test.go +++ b/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", "-") diff --git a/internal/app/teams.go b/internal/app/teams.go index 270f75c6..ffa79284 100644 --- a/internal/app/teams.go +++ b/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]) diff --git a/internal/app/teams_test.go b/internal/app/teams_test.go index 6f8777f1..117e86cd 100644 --- a/internal/app/teams_test.go +++ b/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) { diff --git a/internal/services/organizations.go b/internal/services/organizations.go index 095433f6..c1526973 100644 --- a/internal/services/organizations.go +++ b/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 diff --git a/internal/services/organizations_test.go b/internal/services/organizations_test.go index 445b9618..e86ddef0 100644 --- a/internal/services/organizations_test.go +++ b/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) { diff --git a/internal/services/teams.go b/internal/services/teams.go index 9be986fd..5831aa34 100644 --- a/internal/services/teams.go +++ b/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. diff --git a/internal/services/teams_test.go b/internal/services/teams_test.go index 000dc29a..97452771 100644 --- a/internal/services/teams_test.go +++ b/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) }