From 757b5fa1b97bb1a880958a5ffec30fc867ba5c3e Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 18 Oct 2026 21:30:21 +0200 Subject: [PATCH] fix: send a team's permission before its units (Gitea 28), hidden private-org team reviews, long non-ASCII repo descriptions, -R NAME vs directory guard, flake fallback Signed-off-by: bircni --- docs/migration-1.0.md | 3 +++ flake.nix | 3 ++- internal/app/repository_create.go | 5 +++-- internal/app/target.go | 6 ++++++ internal/app/target_test.go | 18 ++++++++++++++++++ internal/app/teams_test.go | 6 ++++-- internal/services/repositories.go | 21 ++++++++++++++++++++- internal/services/repositories_test.go | 22 ++++++++++++++++++++++ internal/services/review_requests.go | 16 ++++++++++++++-- internal/services/review_requests_test.go | 22 ++++++++++++++++++++++ internal/services/teams.go | 17 +++++++++++++++++ internal/services/teams_test.go | 19 +++++++++++++++++++ 12 files changed, 150 insertions(+), 8 deletions(-) diff --git a/docs/migration-1.0.md b/docs/migration-1.0.md index 216b907f..aa37b553 100644 --- a/docs/migration-1.0.md +++ b/docs/migration-1.0.md @@ -384,6 +384,9 @@ archive, or run `make docs`. `--editor`, not as a third argument. `repo create` names its template options `--template` and `--trust-model`; see `tea repo create --help`. - `--repo` takes `OWNER/REPO` or a URL; use `--cwd DIR` to point at a local checkout. + A bare name (`-R project`) means a repository of your account, so it is refused when a + directory with that name exists in the working directory: write `-R OWNER/project`, or + `--cwd project` for the checkout. - `pr checkout N` (alias `co`) checks out a local branch named after the PR's head branch when that name is free or already points at the head; otherwise it uses `OWNER/BRANCH` for a fork or `pr/N`. The branch tracks the head diff --git a/flake.nix b/flake.nix index aa222a4c..0f8cc25e 100644 --- a/flake.nix +++ b/flake.nix @@ -15,7 +15,8 @@ devShells.default = pkgs.mkShell { name = "tea-dev-environment"; buildInputs = with pkgs; [ - go_1_27 + # flake.lock may pin a nixpkgs that predates Go 1.27; go.mod's toolchain line then fetches it. + (pkgs.go_1_27 or pkgs.go) gopls gnumake # Add other dependencies here if needed diff --git a/internal/app/repository_create.go b/internal/app/repository_create.go index 2a46ee7e..a223faeb 100644 --- a/internal/app/repository_create.go +++ b/internal/app/repository_create.go @@ -11,6 +11,7 @@ import ( "path/filepath" "slices" "strings" + "unicode/utf8" "gitea.dev/tea/internal/clierr" "gitea.dev/tea/internal/services" @@ -209,8 +210,8 @@ func validateCreateFlags(cmd *cobra.Command, opts services.RepositoryOptions, or return clierr.Wrap(clierr.Usage, errors.New("--team requires --org and a team name")) case changed("template") && template == "": return clierr.Wrap(clierr.Usage, errors.New("--template requires owner/repo or a repository URL")) - case len(opts.Description) > 2048: - return clierr.Wrap(clierr.Usage, errors.New("description cannot exceed 2048 bytes")) + case utf8.RuneCountInString(opts.Description) > 2048: + return clierr.Wrap(clierr.Usage, errors.New("description cannot exceed 2048 characters")) case template == "" && (opts.Gitignores != "" || opts.License != "" || opts.Readme != "" || opts.DefaultBranch != "") && !opts.AutoInit: return clierr.Wrap(clierr.Usage, errors.New("initial content and default branch options require --auto-init")) case template != "" && changed("auto-init", "label-template", "gitignores", "license", "readme", "as-template", "trust-model", "object-format"): diff --git a/internal/app/target.go b/internal/app/target.go index de9592e9..1b92375d 100644 --- a/internal/app/target.go +++ b/internal/app/target.go @@ -9,6 +9,8 @@ import ( "errors" "fmt" "maps" + "os" + "path/filepath" "slices" "strconv" "strings" @@ -102,6 +104,10 @@ func (a *application) resolveRequest(ctx context.Context, req targetRequest) (ta } // A bare repository NAME belongs to the account tea acts as, which is known only now. if t.Repo != "" && !strings.Contains(t.Repo, "/") { + // A name that is also a directory here may be meant as that checkout, as in tea -R ./project. + if info, statErr := os.Stat(filepath.Join(cmp.Or(a.options.cwd, "."), t.Repo)); statErr == nil && info.IsDir() && t.RepoSource == "--repo" { + return t, clierr.Wrap(clierr.Usage, fmt.Errorf("%q is both a repository name and a directory here; use OWNER/%s for the repository, or --cwd %s for that checkout", t.Repo, t.Repo, t.Repo)) + } owner := cmp.Or(a.options.sudo, t.User) if owner == "" { return t, clierr.Wrap(clierr.Usage, fmt.Errorf("cannot tell who owns repository %q; use OWNER/%s", t.Repo, t.Repo)) diff --git a/internal/app/target_test.go b/internal/app/target_test.go index fbd53bc0..55a7ba34 100644 --- a/internal/app/target_test.go +++ b/internal/app/target_test.go @@ -376,3 +376,21 @@ func TestEnvironmentCredentialOverridesTheActiveAccount(t *testing.T) { require.NoError(t, Execute(context.Background(), deps, []string{"issue", "create", "-R", "alice/project", "--title", "Issue", "--body", "Body", "--no-input", "--json"})) require.True(t, created) } + +func TestRepositoryFlagNamedLikeADirectoryIsAmbiguous(t *testing.T) { + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"full_name":"` + strings.TrimPrefix(r.URL.Path, "/api/v1/repos/") + `"}`)) + })) + defer server.Close() + dir := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(dir, "project"), 0o755)) + deps, out, _ := testDeps(t) + configureAccounts(t, deps, server.URL, "alice", "alice") + err := Execute(context.Background(), deps, []string{"repo", "view", "-R", "project", "--cwd", dir, "--no-input", "--json=full_name"}) + require.Equal(t, clierr.Usage, clierr.ExitCode(err)) + require.ErrorContains(t, err, `"project" is both a repository name and a directory here; use OWNER/project`) + // A qualified name, or a bare name without such a directory, is unambiguous. + require.NoError(t, Execute(context.Background(), deps, []string{"repo", "view", "-R", "alice/project", "--cwd", dir, "--no-input", "--json=full_name"})) + require.NoError(t, Execute(context.Background(), deps, []string{"repo", "view", "-R", "other", "--cwd", dir, "--no-input", "--json=full_name"})) + require.Contains(t, out.String(), "alice/other") +} diff --git a/internal/app/teams_test.go b/internal/app/teams_test.go index 8c326f87..a288318e 100644 --- a/internal/app/teams_test.go +++ b/internal/app/teams_test.go @@ -216,12 +216,14 @@ func TestTeamPermissionSetsEveryUnit(t *testing.T) { } require.NoError(t, Execute(context.Background(), deps, []string{"team", "create", "acme/devs", "--permission", "write"})) require.NoError(t, Execute(context.Background(), deps, []string{"team", "edit", "5", "--permission", "read", "--unit", "repo.wiki=write"})) - require.Len(t, sent, 2) + require.Len(t, sent, 3) require.Equal(t, "devs", sent[0]["name"]) require.Equal(t, every("write"), sent[0]["units_map"]) + // The permission goes first on its own, because Gitea 28 keeps an admin team admin otherwise. + require.Equal(t, map[string]any{"name": "devs", "permission": "read"}, sent[1]) wikiWrite := every("read") wikiWrite["repo.wiki"] = "write" - require.Equal(t, map[string]any{"permission": "read", "units_map": wikiWrite}, sent[1]) + require.Equal(t, map[string]any{"permission": "read", "units_map": wikiWrite}, sent[2]) } func TestTeamMemberRemoveConfirmsOnce(t *testing.T) { diff --git a/internal/services/repositories.go b/internal/services/repositories.go index b3073c05..cbc15907 100644 --- a/internal/services/repositories.go +++ b/internal/services/repositories.go @@ -6,6 +6,7 @@ package services import ( "context" "errors" + "fmt" "net/http" "slices" "strconv" @@ -238,6 +239,13 @@ func (s Repositories) Create(ctx context.Context, org string, opts RepositoryOpt Readme: opts.Readme, DefaultBranch: opts.DefaultBranch, TrustModel: gitea.TrustModel(opts.TrustModel), ObjectFormatName: opts.ObjectFormat, } + // TODO(sdk): CreateRepoOption.Validate counts the description in bytes, but Gitea allows 2048 + // characters; a longer one is set by an edit once the repository exists. + description := opts.Description + later := len(description) > sdkDescriptionLimit + if later { + options.Description = "" + } item, _, err := call(ctx, s.Client, func(ctx context.Context, c *gitea.Client) (*gitea.Repository, *gitea.Response, error) { if org != "" { return c.Repositories.CreateOrgRepo(ctx, org, options) @@ -251,9 +259,20 @@ func (s Repositories) Create(ctx context.Context, org string, opts RepositoryOpt } return Repository{}, Classify(err, "repository", "", "") } - return repositoryFrom(item), nil + created := repositoryFrom(item) + if later { + edited, err := s.Edit(ctx, created.FullName, RepositoryEdit{Description: &description}) + if err != nil { + return created, fmt.Errorf("repository %s was created, but setting its description failed: %w", created.FullName, err) + } + return edited, nil + } + return created, nil } +// sdkDescriptionLimit is the length in bytes that the SDK accepts for a new repository's description. +const sdkDescriptionLimit = 2048 + // Edit updates only explicitly supplied resource fields. func (s Repositories) Edit(ctx context.Context, repo string, opts RepositoryEdit) (Repository, error) { var mergeStyle *gitea.MergeStyle diff --git a/internal/services/repositories_test.go b/internal/services/repositories_test.go index a67dd83b..f870ef7a 100644 --- a/internal/services/repositories_test.go +++ b/internal/services/repositories_test.go @@ -6,6 +6,7 @@ package services import ( "context" "net/http" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -118,3 +119,24 @@ func TestRepositoryReadme(t *testing.T) { _, _, err = (Repositories{Client: client}).Readme(context.Background(), "alice/project", "develp") require.EqualError(t, err, `branch "develp" not found in alice/project`) } + +func TestRepositoryCreateSetsADescriptionBeyondTheSDKByteLimitByEdit(t *testing.T) { + description := strings.Repeat("日", 800) // 2400 bytes, 800 characters: Gitea accepts it, the SDK would not + var methods []string + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + methods = append(methods, r.Method) + body := requestJSON(t, r) + switch r.Method { + case http.MethodPost: + require.Empty(t, body["description"]) + _, _ = w.Write([]byte(`{"id":1,"name":"project","full_name":"alice/project"}`)) + case http.MethodPatch: + require.Equal(t, description, body["description"]) + _, _ = w.Write([]byte(`{"id":1,"name":"project","full_name":"alice/project","description":"` + description + `"}`)) + } + })) + created, err := (Repositories{Client: client}).Create(context.Background(), "", RepositoryOptions{Name: "project", Description: description}) + require.NoError(t, err) + require.Equal(t, []string{http.MethodPost, http.MethodPatch}, methods) + require.Equal(t, description, created.Description) +} diff --git a/internal/services/review_requests.go b/internal/services/review_requests.go index 2e4de9f7..3b7b1154 100644 --- a/internal/services/review_requests.go +++ b/internal/services/review_requests.go @@ -99,7 +99,8 @@ func (s Reviews) changeRequests(ctx context.Context, n int64, changes ReviewRequ // CheckRequestTeams verifies that every named team exists in the repository's organization. Gitea // lets only organization members list teams, though outside collaborators and the pull request -// author may request reviews; when the lookup is forbidden the server checks the names, as for gh. +// author may request reviews; when the lookup is forbidden, or the organization is hidden from the +// caller (a private organization answers 404 to non-members), the server checks the names, as for gh. func (s Reviews) CheckRequestTeams(ctx context.Context, names []string) error { owner, _, _ := strings.Cut(s.Repository, "/") _, isOrg, err := organization(ctx, s.Client, owner) @@ -110,10 +111,21 @@ func (s Reviews) CheckRequestTeams(ctx context.Context, names []string) error { return err } if !isOrg { + // A private organization answers 404 to non-members, like a name nobody owns; a user that + // exists is certainly not an organization. + _, _, err := call(ctx, s.Client, func(ctx context.Context, c *gitea.Client) (*gitea.User, *gitea.Response, error) { + return c.Users.GetUserInfo(ctx, owner) + }) + if IsNotFound(err) || IsForbidden(err) { + return nil + } + if err != nil { + return err + } return errors.New("team reviews require an organization repository") } teams, err := (Teams{Client: s.Client}).ByName(ctx, owner) - if IsForbidden(err) { + if IsForbidden(err) || IsNotFound(err) { return nil } if err != nil { diff --git a/internal/services/review_requests_test.go b/internal/services/review_requests_test.go index 6a0e7217..bf2be1e6 100644 --- a/internal/services/review_requests_test.go +++ b/internal/services/review_requests_test.go @@ -95,3 +95,25 @@ func TestUnknownReviewerTeamRejectsBeforeUserRemoval(t *testing.T) { _, err := (Reviews{Client: transport.New(server.URL, "", false), Repository: "owner/repo"}).ChangeRequests(context.Background(), 1, changes) require.ErrorContains(t, err, `team "missing" is unavailable`) } + +func TestReviewerTeamCheckSkippedForAHiddenPrivateOrganization(t *testing.T) { + var posted bool + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.URL.Path == "/api/v1/orgs/owner" || r.URL.Path == "/api/v1/users/owner": + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"GetUserByName"}`)) + case r.Method == http.MethodPost && r.URL.Path == "/api/v1/repos/owner/repo/pulls/1/requested_reviewers": + posted = true + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`[]`)) + default: + _, _ = w.Write([]byte(`{"number":1}`)) + } + })) + defer server.Close() + changes := ReviewRequestChanges{Add: ReviewRequestInput{Teams: []string{"devs"}}} + _, err := (Reviews{Client: transport.New(server.URL, "", false), Repository: "owner/repo"}).ChangeRequests(context.Background(), 1, changes) + require.NoError(t, err) + require.True(t, posted, "a non-member cannot see the organization, so the server decides") +} diff --git a/internal/services/teams.go b/internal/services/teams.go index 5831aa34..9175c1e5 100644 --- a/internal/services/teams.go +++ b/internal/services/teams.go @@ -250,6 +250,7 @@ func (s Teams) Create(ctx context.Context, org string, opts TeamOptions) (Team, // permissions when changing access to all repositories. Gitea replaces the whole policy on edit. func (s Teams) Edit(ctx context.Context, id int64, opts TeamOptions) (Team, error) { repositoryPolicy := opts.AllRepositories != nil && opts.Permission == nil + changesPermission := opts.Permission != nil if repositoryPolicy || (opts.Units != nil && !opts.ReplaceUnits) { current, err := s.View(ctx, id) if err != nil { @@ -274,6 +275,22 @@ func (s Teams) Edit(ctx context.Context, id int64, opts TeamOptions) (Team, erro opts.Units = units } } + // Gitea 28 keeps an admin team admin when one request changes its permission and sets units_map, + // so change the permission alone first and then apply the units. + if changesPermission && opts.Units != nil { + first := TeamOptions{Name: opts.Name, Permission: opts.Permission} + if first.Name == nil { + current, err := s.View(ctx, id) + if err != nil { + return Team{}, err + } + first.Name = ¤t.Name + } + // TODO(sdk): EditTeamOption requires a name of at most 30 characters and a permission, and EditTeam should return the team. + if err := sendJSON(ctx, s.Client, http.MethodPatch, fmt.Sprintf("teams/%d", id), first, nil); err != nil { + return Team{}, err + } + } var team gitea.Team // TODO(sdk): EditTeamOption requires a name of at most 30 characters and a permission, and EditTeam should return the team. if err := sendJSON(ctx, s.Client, http.MethodPatch, fmt.Sprintf("teams/%d", id), opts, &team); err != nil { diff --git a/internal/services/teams_test.go b/internal/services/teams_test.go index 97452771..97f5e093 100644 --- a/internal/services/teams_test.go +++ b/internal/services/teams_test.go @@ -179,3 +179,22 @@ func TestTeamFindSearchesTheOrganization(t *testing.T) { require.EqualError(t, err, `organization "nope" not found`) require.Equal(t, []string{"devops", "devops", "", "", "DEV", "DEV"}, queries) } + +func TestTeamEditSendsThePermissionBeforeTheUnits(t *testing.T) { + var bodies []map[string]any + client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/api/v1/teams/5", r.URL.Path) + if r.Method == http.MethodGet { + _, _ = w.Write([]byte(`{"id":5,"name":"devs","permission":"admin"}`)) + return + } + bodies = append(bodies, requestJSON(t, r)) + _, _ = w.Write([]byte(`{"id":5,"name":"devs","permission":"write"}`)) + })) + permission := "write" + _, err := (Teams{Client: client}).Edit(context.Background(), 5, TeamOptions{Permission: &permission, Units: map[string]string{"repo.wiki": "read"}, ReplaceUnits: true}) + require.NoError(t, err) + require.Len(t, bodies, 2) + require.Equal(t, map[string]any{"name": "devs", "permission": "write"}, bodies[0], "Gitea 28 keeps an admin team admin when one request sets both") + require.Equal(t, map[string]any{"repo.wiki": "read"}, bodies[1]["units_map"]) +}