From fe49cb0243bed03f565269d84836bf21a0597665 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 22 Nov 2024 07:44:48 -0800 Subject: [PATCH 1/4] Fix get reviewers' bug (#32415) This PR rewrites `GetReviewer` function and move it to service layer. Reviewers should not be watchers, so that this PR removed all watchers from reviewers. When the repository is under an organization, the pull request unit read permission will be checked to resolve the bug of #32394 Fix #32394 --- models/organization/team_repo.go | 14 +++++ models/organization/team_repo_test.go | 31 ++++++++++ models/repo/user_repo.go | 52 ---------------- models/repo/user_repo_test.go | 43 ------------- routers/api/v1/repo/collaborators.go | 10 ++- routers/web/repo/issue_page_meta.go | 8 +-- services/issue/assignee.go | 8 +-- services/pull/reviewer.go | 89 +++++++++++++++++++++++++++ services/pull/reviewer_test.go | 72 ++++++++++++++++++++++ services/repository/review.go | 24 -------- services/repository/review_test.go | 28 --------- tests/integration/api_repo_test.go | 4 +- 12 files changed, 225 insertions(+), 158 deletions(-) create mode 100644 models/organization/team_repo_test.go create mode 100644 services/pull/reviewer.go create mode 100644 services/pull/reviewer_test.go delete mode 100644 services/repository/review.go delete mode 100644 services/repository/review_test.go diff --git a/models/organization/team_repo.go b/models/organization/team_repo.go index 1184e39263..c90dfdeda0 100644 --- a/models/organization/team_repo.go +++ b/models/organization/team_repo.go @@ -9,6 +9,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" "xorm.io/builder" ) @@ -83,3 +84,16 @@ func GetTeamsWithAccessToRepo(ctx context.Context, orgID, repoID int64, mode per OrderBy("name"). Find(&teams) } + +// GetTeamsWithAccessToRepoUnit returns all teams in an organization that have given access level to the repository special unit. +func GetTeamsWithAccessToRepoUnit(ctx context.Context, orgID, repoID int64, mode perm.AccessMode, unitType unit.Type) ([]*Team, error) { + teams := make([]*Team, 0, 5) + return teams, db.GetEngine(ctx).Where("team_unit.access_mode >= ?", mode). + Join("INNER", "team_repo", "team_repo.team_id = team.id"). + Join("INNER", "team_unit", "team_unit.team_id = team.id"). + And("team_repo.org_id = ?", orgID). + And("team_repo.repo_id = ?", repoID). + And("team_unit.type = ?", unitType). + OrderBy("name"). + Find(&teams) +} diff --git a/models/organization/team_repo_test.go b/models/organization/team_repo_test.go new file mode 100644 index 0000000000..c0d6750df9 --- /dev/null +++ b/models/organization/team_repo_test.go @@ -0,0 +1,31 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package organization_test + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" + "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" + "code.gitea.io/gitea/models/unittest" + + "github.com/stretchr/testify/assert" +) + +func TestGetTeamsWithAccessToRepoUnit(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + org41 := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 41}) + repo61 := unittest.AssertExistsAndLoadBean(t, &repo.Repository{ID: 61}) + + teams, err := organization.GetTeamsWithAccessToRepoUnit(db.DefaultContext, org41.ID, repo61.ID, perm.AccessModeRead, unit.TypePullRequests) + assert.NoError(t, err) + if assert.Len(t, teams, 2) { + assert.EqualValues(t, 21, teams[0].ID) + assert.EqualValues(t, 22, teams[1].ID) + } +} diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go index ecc9216950..a9b1360df1 100644 --- a/models/repo/user_repo.go +++ b/models/repo/user_repo.go @@ -11,7 +11,6 @@ import ( "code.gitea.io/gitea/models/unit" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" - api "code.gitea.io/gitea/modules/structs" "xorm.io/builder" ) @@ -146,57 +145,6 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us return users, nil } -// GetReviewers get all users can be requested to review: -// * for private repositories this returns all users that have read access or higher to the repository. -// * for public repositories this returns all users that have read access or higher to the repository, -// all repo watchers and all organization members. -// TODO: may be we should have a busy choice for users to block review request to them. -func GetReviewers(ctx context.Context, repo *Repository, doerID, posterID int64) ([]*user_model.User, error) { - // Get the owner of the repository - this often already pre-cached and if so saves complexity for the following queries - if err := repo.LoadOwner(ctx); err != nil { - return nil, err - } - - cond := builder.And(builder.Neq{"`user`.id": posterID}). - And(builder.Eq{"`user`.is_active": true}) - - if repo.IsPrivate || repo.Owner.Visibility == api.VisibleTypePrivate { - // This a private repository: - // Anyone who can read the repository is a requestable reviewer - - cond = cond.And(builder.In("`user`.id", - builder.Select("user_id").From("access").Where( - builder.Eq{"repo_id": repo.ID}. - And(builder.Gte{"mode": perm.AccessModeRead}), - ), - )) - - if repo.Owner.Type == user_model.UserTypeIndividual && repo.Owner.ID != posterID { - // as private *user* repos don't generate an entry in the `access` table, - // the owner of a private repo needs to be explicitly added. - cond = cond.Or(builder.Eq{"`user`.id": repo.Owner.ID}) - } - } else { - // This is a "public" repository: - // Any user that has read access, is a watcher or organization member can be requested to review - cond = cond.And(builder.And(builder.In("`user`.id", - builder.Select("user_id").From("access"). - Where(builder.Eq{"repo_id": repo.ID}. - And(builder.Gte{"mode": perm.AccessModeRead})), - ).Or(builder.In("`user`.id", - builder.Select("user_id").From("watch"). - Where(builder.Eq{"repo_id": repo.ID}. - And(builder.In("mode", WatchModeNormal, WatchModeAuto))), - ).Or(builder.In("`user`.id", - builder.Select("uid").From("org_user"). - Where(builder.Eq{"org_id": repo.OwnerID}), - ))))) - } - - users := make([]*user_model.User, 0, 8) - return users, db.GetEngine(ctx).Where(cond).OrderBy(user_model.GetOrderByName()).Find(&users) -} - // GetIssuePostersWithSearch returns users with limit of 30 whose username started with prefix that have authored an issue/pull request for the given repository // If isShowFullName is set to true, also include full name prefix search func GetIssuePostersWithSearch(ctx context.Context, repo *Repository, isPull bool, search string, isShowFullName bool) ([]*user_model.User, error) { diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go index d2bf6dc912..f2abc2ffa0 100644 --- a/models/repo/user_repo_test.go +++ b/models/repo/user_repo_test.go @@ -38,46 +38,3 @@ func TestRepoAssignees(t *testing.T) { assert.NotContains(t, []int64{users[0].ID, users[1].ID, users[2].ID}, 15) } } - -func TestRepoGetReviewers(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - - // test public repo - repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) - - ctx := db.DefaultContext - reviewers, err := repo_model.GetReviewers(ctx, repo1, 2, 2) - assert.NoError(t, err) - if assert.Len(t, reviewers, 3) { - assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}) - } - - // should include doer if doer is not PR poster. - reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 2) - assert.NoError(t, err) - assert.Len(t, reviewers, 3) - - // should not include PR poster, if PR poster would be otherwise eligible - reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 4) - assert.NoError(t, err) - assert.Len(t, reviewers, 2) - - // test private user repo - repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) - - reviewers, err = repo_model.GetReviewers(ctx, repo2, 2, 4) - assert.NoError(t, err) - assert.Len(t, reviewers, 1) - assert.EqualValues(t, reviewers[0].ID, 2) - - // test private org repo - repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) - - reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 1) - assert.NoError(t, err) - assert.Len(t, reviewers, 2) - - reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 2) - assert.NoError(t, err) - assert.Len(t, reviewers, 1) -} diff --git a/routers/api/v1/repo/collaborators.go b/routers/api/v1/repo/collaborators.go index ea9d8b0f37..0bbf5a1ea4 100644 --- a/routers/api/v1/repo/collaborators.go +++ b/routers/api/v1/repo/collaborators.go @@ -17,6 +17,8 @@ import ( "code.gitea.io/gitea/routers/api/v1/utils" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/convert" + issue_service "code.gitea.io/gitea/services/issue" + pull_service "code.gitea.io/gitea/services/pull" repo_service "code.gitea.io/gitea/services/repository" ) @@ -320,7 +322,13 @@ func GetReviewers(ctx *context.APIContext) { // "404": // "$ref": "#/responses/notFound" - reviewers, err := repo_model.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0) + canChooseReviewer := issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, ctx.Repo.Repository, 0) + if !canChooseReviewer { + ctx.Error(http.StatusForbidden, "GetReviewers", errors.New("doer has no permission to get reviewers")) + return + } + + reviewers, err := pull_service.GetReviewers(ctx, ctx.Repo.Repository, ctx.Doer.ID, 0) if err != nil { ctx.Error(http.StatusInternalServerError, "ListCollaborators", err) return diff --git a/routers/web/repo/issue_page_meta.go b/routers/web/repo/issue_page_meta.go index ac0b1c6425..e04d76b287 100644 --- a/routers/web/repo/issue_page_meta.go +++ b/routers/web/repo/issue_page_meta.go @@ -19,7 +19,7 @@ import ( shared_user "code.gitea.io/gitea/routers/web/shared/user" "code.gitea.io/gitea/services/context" issue_service "code.gitea.io/gitea/services/issue" - repo_service "code.gitea.io/gitea/services/repository" + pull_service "code.gitea.io/gitea/services/pull" ) type issueSidebarMilestoneData struct { @@ -186,7 +186,7 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) { if d.Issue == nil { data.CanChooseReviewer = true } else { - data.CanChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue) + data.CanChooseReviewer = issue_service.CanDoerChangeReviewRequests(ctx, ctx.Doer, repo, d.Issue.PosterID) } } @@ -231,13 +231,13 @@ func (d *IssuePageMetaData) retrieveReviewersData(ctx *context.Context) { if data.CanChooseReviewer { var err error - reviewers, err = repo_model.GetReviewers(ctx, repo, ctx.Doer.ID, posterID) + reviewers, err = pull_service.GetReviewers(ctx, repo, ctx.Doer.ID, posterID) if err != nil { ctx.ServerError("GetReviewers", err) return } - teamReviewers, err = repo_service.GetReviewerTeams(ctx, repo) + teamReviewers, err = pull_service.GetReviewerTeams(ctx, repo) if err != nil { ctx.ServerError("GetReviewerTeams", err) return diff --git a/services/issue/assignee.go b/services/issue/assignee.go index 52ee9f2b22..c7e2495568 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -119,7 +119,7 @@ func isValidReviewRequest(ctx context.Context, reviewer, doer *user_model.User, return err } - canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue) + canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) if isAdd { if !permReviewer.CanAccessAny(perm.AccessModeRead, unit.TypePullRequests) { @@ -178,7 +178,7 @@ func isValidTeamReviewRequest(ctx context.Context, reviewer *organization.Team, } } - canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue) + canDoerChangeReviewRequests := CanDoerChangeReviewRequests(ctx, doer, issue.Repo, issue.PosterID) if isAdd { if issue.Repo.IsPrivate { @@ -276,12 +276,12 @@ func teamReviewRequestNotify(ctx context.Context, issue *issues_model.Issue, doe } // CanDoerChangeReviewRequests returns if the doer can add/remove review requests of a PR -func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue) bool { +func CanDoerChangeReviewRequests(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, posterID int64) bool { if repo.IsArchived { return false } // The poster of the PR can change the reviewers - if doer.ID == issue.PosterID { + if doer.ID == posterID { return true } diff --git a/services/pull/reviewer.go b/services/pull/reviewer.go new file mode 100644 index 0000000000..bf0d8cb298 --- /dev/null +++ b/services/pull/reviewer.go @@ -0,0 +1,89 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull + +import ( + "context" + + "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/organization" + "code.gitea.io/gitea/models/perm" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unit" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/container" + + "xorm.io/builder" +) + +// GetReviewers get all users can be requested to review: +// - Poster should not be listed +// - For collaborator, all users that have read access or higher to the repository. +// - For repository under organization, users under the teams which have read permission or higher of pull request unit +// - Owner will be listed if it's not an organization, not the poster and not in the list of reviewers +func GetReviewers(ctx context.Context, repo *repo_model.Repository, doerID, posterID int64) ([]*user_model.User, error) { + if err := repo.LoadOwner(ctx); err != nil { + return nil, err + } + + e := db.GetEngine(ctx) + uniqueUserIDs := make(container.Set[int64]) + + collaboratorIDs := make([]int64, 0, 10) + if err := e.Table("collaboration").Where("repo_id=?", repo.ID). + And("mode >= ?", perm.AccessModeRead). + Select("user_id"). + Find(&collaboratorIDs); err != nil { + return nil, err + } + uniqueUserIDs.AddMultiple(collaboratorIDs...) + + if repo.Owner.IsOrganization() { + additionalUserIDs := make([]int64, 0, 10) + if err := e.Table("team_user"). + Join("INNER", "team_repo", "`team_repo`.team_id = `team_user`.team_id"). + Join("INNER", "team_unit", "`team_unit`.team_id = `team_user`.team_id"). + Where("`team_repo`.repo_id = ? AND (`team_unit`.access_mode >= ? AND `team_unit`.`type` = ?)", + repo.ID, perm.AccessModeRead, unit.TypePullRequests). + Distinct("`team_user`.uid"). + Select("`team_user`.uid"). + Find(&additionalUserIDs); err != nil { + return nil, err + } + uniqueUserIDs.AddMultiple(additionalUserIDs...) + } + + uniqueUserIDs.Remove(posterID) // posterID should not be in the list of reviewers + + // Leave a seat for owner itself to append later, but if owner is an organization + // and just waste 1 unit is cheaper than re-allocate memory once. + users := make([]*user_model.User, 0, len(uniqueUserIDs)+1) + if len(uniqueUserIDs) > 0 { + if err := e.In("id", uniqueUserIDs.Values()). + Where(builder.Eq{"`user`.is_active": true}). + OrderBy(user_model.GetOrderByName()). + Find(&users); err != nil { + return nil, err + } + } + + // add owner after all users are loaded because we can avoid load owner twice + if repo.OwnerID != posterID && !repo.Owner.IsOrganization() && !uniqueUserIDs.Contains(repo.OwnerID) { + users = append(users, repo.Owner) + } + + return users, nil +} + +// GetReviewerTeams get all teams can be requested to review +func GetReviewerTeams(ctx context.Context, repo *repo_model.Repository) ([]*organization.Team, error) { + if err := repo.LoadOwner(ctx); err != nil { + return nil, err + } + if !repo.Owner.IsOrganization() { + return nil, nil + } + + return organization.GetTeamsWithAccessToRepoUnit(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead, unit.TypePullRequests) +} diff --git a/services/pull/reviewer_test.go b/services/pull/reviewer_test.go new file mode 100644 index 0000000000..1ff373bafb --- /dev/null +++ b/services/pull/reviewer_test.go @@ -0,0 +1,72 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package pull_test + +import ( + "testing" + + "code.gitea.io/gitea/models/db" + repo_model "code.gitea.io/gitea/models/repo" + "code.gitea.io/gitea/models/unittest" + pull_service "code.gitea.io/gitea/services/pull" + + "github.com/stretchr/testify/assert" +) + +func TestRepoGetReviewers(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + // test public repo + repo1 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1}) + + ctx := db.DefaultContext + reviewers, err := pull_service.GetReviewers(ctx, repo1, 2, 0) + assert.NoError(t, err) + if assert.Len(t, reviewers, 1) { + assert.ElementsMatch(t, []int64{2}, []int64{reviewers[0].ID}) + } + + // should not include doer and remove the poster + reviewers, err = pull_service.GetReviewers(ctx, repo1, 11, 2) + assert.NoError(t, err) + assert.Len(t, reviewers, 0) + + // should not include PR poster, if PR poster would be otherwise eligible + reviewers, err = pull_service.GetReviewers(ctx, repo1, 11, 4) + assert.NoError(t, err) + assert.Len(t, reviewers, 1) + + // test private user repo + repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + + reviewers, err = pull_service.GetReviewers(ctx, repo2, 2, 4) + assert.NoError(t, err) + assert.Len(t, reviewers, 1) + assert.EqualValues(t, reviewers[0].ID, 2) + + // test private org repo + repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + + reviewers, err = pull_service.GetReviewers(ctx, repo3, 2, 1) + assert.NoError(t, err) + assert.Len(t, reviewers, 2) + + reviewers, err = pull_service.GetReviewers(ctx, repo3, 2, 2) + assert.NoError(t, err) + assert.Len(t, reviewers, 1) +} + +func TestRepoGetReviewerTeams(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + teams, err := pull_service.GetReviewerTeams(db.DefaultContext, repo2) + assert.NoError(t, err) + assert.Empty(t, teams) + + repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) + teams, err = pull_service.GetReviewerTeams(db.DefaultContext, repo3) + assert.NoError(t, err) + assert.Len(t, teams, 2) +} diff --git a/services/repository/review.go b/services/repository/review.go deleted file mode 100644 index 40513e6bc6..0000000000 --- a/services/repository/review.go +++ /dev/null @@ -1,24 +0,0 @@ -// Copyright 2022 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package repository - -import ( - "context" - - "code.gitea.io/gitea/models/organization" - "code.gitea.io/gitea/models/perm" - repo_model "code.gitea.io/gitea/models/repo" -) - -// GetReviewerTeams get all teams can be requested to review -func GetReviewerTeams(ctx context.Context, repo *repo_model.Repository) ([]*organization.Team, error) { - if err := repo.LoadOwner(ctx); err != nil { - return nil, err - } - if !repo.Owner.IsOrganization() { - return nil, nil - } - - return organization.GetTeamsWithAccessToRepo(ctx, repo.OwnerID, repo.ID, perm.AccessModeRead) -} diff --git a/services/repository/review_test.go b/services/repository/review_test.go deleted file mode 100644 index 2db56d4e8a..0000000000 --- a/services/repository/review_test.go +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright 2022 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package repository - -import ( - "testing" - - "code.gitea.io/gitea/models/db" - repo_model "code.gitea.io/gitea/models/repo" - "code.gitea.io/gitea/models/unittest" - - "github.com/stretchr/testify/assert" -) - -func TestRepoGetReviewerTeams(t *testing.T) { - assert.NoError(t, unittest.PrepareTestDatabase()) - - repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) - teams, err := GetReviewerTeams(db.DefaultContext, repo2) - assert.NoError(t, err) - assert.Empty(t, teams) - - repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}) - teams, err = GetReviewerTeams(db.DefaultContext, repo3) - assert.NoError(t, err) - assert.Len(t, teams, 2) -} diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index 93c9ca0920..122afbfa08 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -718,8 +718,8 @@ func TestAPIRepoGetReviewers(t *testing.T) { resp := MakeRequest(t, req, http.StatusOK) var reviewers []*api.User DecodeJSON(t, resp, &reviewers) - if assert.Len(t, reviewers, 3) { - assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}) + if assert.Len(t, reviewers, 1) { + assert.ElementsMatch(t, []int64{2}, []int64{reviewers[0].ID}) } } From ae90b21db036507b8df9ed22e7ba416139037547 Mon Sep 17 00:00:00 2001 From: hiifong Date: Sat, 23 Nov 2024 02:26:05 +0800 Subject: [PATCH 2/4] Apply to became a maintainer (#32614) [PRs list](https://github.com/go-gitea/gitea/pulls?q=is%3Apr+author%3Ahiifong+is%3Aclosed+is%3Amerged) --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 160bffcdb7..426181cbcf 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -63,3 +63,4 @@ Tim-Niclas Oelschläger (@zokkis) Yu Liu <1240335630@qq.com> (@HEREYUA) Kemal Zebari (@kemzeb) Rowan Bohde (@bohde) +hiifong (@hiifong) From f2a995174101b9fa9409acb2b47b065262098b28 Mon Sep 17 00:00:00 2001 From: Yarden Shoham Date: Fri, 22 Nov 2024 20:51:51 +0200 Subject: [PATCH 3/4] Update the list of watchers and stargazers when clicking watch/unwatch or star/unstar (#32570) We make sure the user cards are updated - Fixes https://github.com/go-gitea/gitea/issues/32561 I also removed `ctx.Data["PageIsWatchers"] = true` and `ctx.Data["PageIsStargazers"] = true` as they are not used anywhere. # Before ![before](https://github.com/user-attachments/assets/e3bc3235-35eb-4eda-862d-bdf2510282ea) # After ![after](https://github.com/user-attachments/assets/bc0488a5-8399-4cf6-95c9-17328a9702eb) --------- Signed-off-by: Yarden Shoham Co-authored-by: wxiaoguang Co-authored-by: silverwind --- routers/web/repo/repo.go | 3 +++ routers/web/repo/view.go | 3 --- templates/repo/user_cards.tmpl | 12 +++++++++++- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/routers/web/repo/repo.go b/routers/web/repo/repo.go index be6f2d483f..b62fd21585 100644 --- a/routers/web/repo/repo.go +++ b/routers/web/repo/repo.go @@ -352,6 +352,9 @@ func Action(ctx *context.Context) { ctx.Data["IsStaringRepo"] = repo_model.IsStaring(ctx, ctx.Doer.ID, ctx.Repo.Repository.ID) } + // see the `hx-trigger="refreshUserCards ..."` comments in tmpl + ctx.RespHeader().Add("hx-trigger", "refreshUserCards") + switch ctx.PathParam(":action") { case "watch", "unwatch", "star", "unstar": // we have to reload the repository because NumStars or NumWatching (used in the templates) has just changed diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index aacd7de6b1..ec2ddfd79f 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -1123,8 +1123,6 @@ func RenderUserCards(ctx *context.Context, total int, getter func(opts db.ListOp func Watchers(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("repo.watchers") ctx.Data["CardsTitle"] = ctx.Tr("repo.watchers") - ctx.Data["PageIsWatchers"] = true - RenderUserCards(ctx, ctx.Repo.Repository.NumWatches, func(opts db.ListOptions) ([]*user_model.User, error) { return repo_model.GetRepoWatchers(ctx, ctx.Repo.Repository.ID, opts) }, tplWatchers) @@ -1134,7 +1132,6 @@ func Watchers(ctx *context.Context) { func Stars(ctx *context.Context) { ctx.Data["Title"] = ctx.Tr("repo.stargazers") ctx.Data["CardsTitle"] = ctx.Tr("repo.stargazers") - ctx.Data["PageIsStargazers"] = true RenderUserCards(ctx, ctx.Repo.Repository.NumStars, func(opts db.ListOptions) ([]*user_model.User, error) { return repo_model.GetStargazers(ctx, ctx.Repo.Repository, opts) }, tplWatchers) diff --git a/templates/repo/user_cards.tmpl b/templates/repo/user_cards.tmpl index 7cd3d4517a..360aeaf619 100644 --- a/templates/repo/user_cards.tmpl +++ b/templates/repo/user_cards.tmpl @@ -1,4 +1,14 @@ -
+ +
+
{{if .CardsTitle}}

{{.CardsTitle}} From 713364fc718d1d53840bd83ba6f6c307bd213fa8 Mon Sep 17 00:00:00 2001 From: Michael Owoc <130198442+mowoc-ocp@users.noreply.github.com> Date: Fri, 22 Nov 2024 15:12:06 -0500 Subject: [PATCH 4/4] Support optional/configurable IAMEndpoint for Minio Client (#32581) (#32581) Targeting issue #32271 This modification allows native Kubernetes + AWS (EKS) authentication with the Minio client, to Amazon S3 using the IRSA role assigned to a Service account by replacing the hard coded reference to the `DefaultIAMRoleEndpoint` with an optional configurable endpoint. Internally, Minio's `credentials.IAM` provider implements a discovery flow for IAM Endpoints if it is not set. For backwards compatibility: - We have added a configuration mechanism for an `IamEndpoint` to retain the unit test safety in `minio_test.go`. - We believe existing clients will continue to function the same without needing to provide a new config property since the internals of Minio client also often resolve to the `http://169.254.169.254` default endpoint that was being hard coded before To test, we were able to build a docker image from source and, observe it choosing the expected IAM endpoint, and see files uploaded via the client. --- custom/conf/app.example.ini | 14 ++++++++++++++ modules/setting/storage.go | 1 + modules/setting/storage_test.go | 13 +++++++++++++ modules/storage/minio.go | 8 +++++--- modules/storage/minio_test.go | 21 +++++++++++++-------- 5 files changed, 46 insertions(+), 11 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index ef5684237d..c3b78e60bb 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1944,6 +1944,13 @@ LEVEL = Info ;; Minio secretAccessKey to connect only available when STORAGE_TYPE is `minio` ;MINIO_SECRET_ACCESS_KEY = ;; +;; Preferred IAM Endpoint to override Minio's default IAM Endpoint resolution only available when STORAGE_TYPE is `minio`. +;; If not provided and STORAGE_TYPE is `minio`, will search for and derive endpoint from known environment variables +;; (AWS_CONTAINER_AUTHORIZATION_TOKEN, AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE, AWS_CONTAINER_CREDENTIALS_RELATIVE_URI, +;; AWS_CONTAINER_CREDENTIALS_FULL_URI, AWS_WEB_IDENTITY_TOKEN_FILE, AWS_ROLE_ARN, AWS_ROLE_SESSION_NAME, AWS_REGION), +;; or the DefaultIAMRoleEndpoint if not provided otherwise. +;MINIO_IAM_ENDPOINT = +;; ;; Minio bucket to store the attachments only available when STORAGE_TYPE is `minio` ;MINIO_BUCKET = gitea ;; @@ -2688,6 +2695,13 @@ LEVEL = Info ;; Minio secretAccessKey to connect only available when STORAGE_TYPE is `minio` ;MINIO_SECRET_ACCESS_KEY = ;; +;; Preferred IAM Endpoint to override Minio's default IAM Endpoint resolution only available when STORAGE_TYPE is `minio`. +;; If not provided and STORAGE_TYPE is `minio`, will search for and derive endpoint from known environment variables +;; (AWS_CONTAINER_AUTHORIZATION_TOKEN, AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE, AWS_CONTAINER_CREDENTIALS_RELATIVE_URI, +;; AWS_CONTAINER_CREDENTIALS_FULL_URI, AWS_WEB_IDENTITY_TOKEN_FILE, AWS_ROLE_ARN, AWS_ROLE_SESSION_NAME, AWS_REGION), +;; or the DefaultIAMRoleEndpoint if not provided otherwise. +;MINIO_IAM_ENDPOINT = +;; ;; Minio bucket to store the attachments only available when STORAGE_TYPE is `minio` ;MINIO_BUCKET = gitea ;; diff --git a/modules/setting/storage.go b/modules/setting/storage.go index d6f7672b61..d3d1fb9f30 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -43,6 +43,7 @@ type MinioStorageConfig struct { Endpoint string `ini:"MINIO_ENDPOINT" json:",omitempty"` AccessKeyID string `ini:"MINIO_ACCESS_KEY_ID" json:",omitempty"` SecretAccessKey string `ini:"MINIO_SECRET_ACCESS_KEY" json:",omitempty"` + IamEndpoint string `ini:"MINIO_IAM_ENDPOINT" json:",omitempty"` Bucket string `ini:"MINIO_BUCKET" json:",omitempty"` Location string `ini:"MINIO_LOCATION" json:",omitempty"` BasePath string `ini:"MINIO_BASE_PATH" json:",omitempty"` diff --git a/modules/setting/storage_test.go b/modules/setting/storage_test.go index 44a5de6826..8ee37fd2b6 100644 --- a/modules/setting/storage_test.go +++ b/modules/setting/storage_test.go @@ -470,6 +470,19 @@ MINIO_BASE_PATH = /prefix cfg, err = NewConfigProviderFromData(` [storage] STORAGE_TYPE = minio +MINIO_IAM_ENDPOINT = 127.0.0.1 +MINIO_USE_SSL = true +MINIO_BASE_PATH = /prefix +`) + assert.NoError(t, err) + assert.NoError(t, loadRepoArchiveFrom(cfg)) + assert.EqualValues(t, "127.0.0.1", RepoArchive.Storage.MinioConfig.IamEndpoint) + assert.EqualValues(t, true, RepoArchive.Storage.MinioConfig.UseSSL) + assert.EqualValues(t, "/prefix/repo-archive/", RepoArchive.Storage.MinioConfig.BasePath) + + cfg, err = NewConfigProviderFromData(` +[storage] +STORAGE_TYPE = minio MINIO_ACCESS_KEY_ID = my_access_key MINIO_SECRET_ACCESS_KEY = my_secret_key MINIO_USE_SSL = true diff --git a/modules/storage/minio.go b/modules/storage/minio.go index 8acb7b0354..6b92be61fb 100644 --- a/modules/storage/minio.go +++ b/modules/storage/minio.go @@ -97,7 +97,7 @@ func NewMinioStorage(ctx context.Context, cfg *setting.Storage) (ObjectStorage, } minioClient, err := minio.New(config.Endpoint, &minio.Options{ - Creds: buildMinioCredentials(config, credentials.DefaultIAMRoleEndpoint), + Creds: buildMinioCredentials(config), Secure: config.UseSSL, Transport: &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: config.InsecureSkipVerify}}, Region: config.Location, @@ -164,7 +164,7 @@ func (m *MinioStorage) buildMinioDirPrefix(p string) string { return p } -func buildMinioCredentials(config setting.MinioStorageConfig, iamEndpoint string) *credentials.Credentials { +func buildMinioCredentials(config setting.MinioStorageConfig) *credentials.Credentials { // If static credentials are provided, use those if config.AccessKeyID != "" { return credentials.NewStaticV4(config.AccessKeyID, config.SecretAccessKey, "") @@ -184,7 +184,9 @@ func buildMinioCredentials(config setting.MinioStorageConfig, iamEndpoint string &credentials.FileAWSCredentials{}, // read IAM role from EC2 metadata endpoint if available &credentials.IAM{ - Endpoint: iamEndpoint, + // passing in an empty Endpoint lets the IAM Provider + // decide which endpoint to resolve internally + Endpoint: config.IamEndpoint, Client: &http.Client{ Transport: http.DefaultTransport, }, diff --git a/modules/storage/minio_test.go b/modules/storage/minio_test.go index 6eb03c4a45..395da051e8 100644 --- a/modules/storage/minio_test.go +++ b/modules/storage/minio_test.go @@ -107,8 +107,9 @@ func TestMinioCredentials(t *testing.T) { cfg := setting.MinioStorageConfig{ AccessKeyID: ExpectedAccessKey, SecretAccessKey: ExpectedSecretAccessKey, + IamEndpoint: FakeEndpoint, } - creds := buildMinioCredentials(cfg, FakeEndpoint) + creds := buildMinioCredentials(cfg) v, err := creds.Get() assert.NoError(t, err) @@ -117,13 +118,15 @@ func TestMinioCredentials(t *testing.T) { }) t.Run("Chain", func(t *testing.T) { - cfg := setting.MinioStorageConfig{} + cfg := setting.MinioStorageConfig{ + IamEndpoint: FakeEndpoint, + } t.Run("EnvMinio", func(t *testing.T) { t.Setenv("MINIO_ACCESS_KEY", ExpectedAccessKey+"Minio") t.Setenv("MINIO_SECRET_KEY", ExpectedSecretAccessKey+"Minio") - creds := buildMinioCredentials(cfg, FakeEndpoint) + creds := buildMinioCredentials(cfg) v, err := creds.Get() assert.NoError(t, err) @@ -135,7 +138,7 @@ func TestMinioCredentials(t *testing.T) { t.Setenv("AWS_ACCESS_KEY", ExpectedAccessKey+"AWS") t.Setenv("AWS_SECRET_KEY", ExpectedSecretAccessKey+"AWS") - creds := buildMinioCredentials(cfg, FakeEndpoint) + creds := buildMinioCredentials(cfg) v, err := creds.Get() assert.NoError(t, err) @@ -144,11 +147,11 @@ func TestMinioCredentials(t *testing.T) { }) t.Run("FileMinio", func(t *testing.T) { - t.Setenv("MINIO_SHARED_CREDENTIALS_FILE", "testdata/minio.json") // prevent loading any actual credentials files from the user + t.Setenv("MINIO_SHARED_CREDENTIALS_FILE", "testdata/minio.json") t.Setenv("AWS_SHARED_CREDENTIALS_FILE", "testdata/fake") - creds := buildMinioCredentials(cfg, FakeEndpoint) + creds := buildMinioCredentials(cfg) v, err := creds.Get() assert.NoError(t, err) @@ -161,7 +164,7 @@ func TestMinioCredentials(t *testing.T) { t.Setenv("MINIO_SHARED_CREDENTIALS_FILE", "testdata/fake.json") t.Setenv("AWS_SHARED_CREDENTIALS_FILE", "testdata/aws_credentials") - creds := buildMinioCredentials(cfg, FakeEndpoint) + creds := buildMinioCredentials(cfg) v, err := creds.Get() assert.NoError(t, err) @@ -187,7 +190,9 @@ func TestMinioCredentials(t *testing.T) { defer server.Close() // Use the provided EC2 Instance Metadata server - creds := buildMinioCredentials(cfg, server.URL) + creds := buildMinioCredentials(setting.MinioStorageConfig{ + IamEndpoint: server.URL, + }) v, err := creds.Get() assert.NoError(t, err)