diff --git a/pkg/provider/gitea/acl.go b/pkg/provider/gitea/acl.go index a1523e364..ab7e6fac4 100644 --- a/pkg/provider/gitea/acl.go +++ b/pkg/provider/gitea/acl.go @@ -17,20 +17,12 @@ func (v *Provider) CheckPolicyAllowing(_ context.Context, event *info.Event, all if event.Organization == event.Repository { return true, "" } - // TODO: caching - orgTeams, resp, err := v.Client().ListOrgTeams(event.Organization, forgejo.ListTeamsOptions{}) - if resp.StatusCode == http.StatusNotFound { - // we explicitly disallow the policy when there is no team on org - return false, fmt.Sprintf("no teams on org %s", event.Organization) - } - if resp.StatusCode == http.StatusForbidden { - v.Logger.Warnf("policy check: ListOrgTeams returned 403 for org %s, sender %s: %v", event.Organization, event.Sender, err) - return false, fmt.Sprintf("unable to list teams on org %s: the token used doesn't have permission to list teams in this org, make sure the token owner is a member of the org", event.Organization) - } + + orgTeams, err := v.listOrgTeams(event.Organization, event.Sender) if err != nil { - // probably a 500 or another api error, no need to try again and again with other teams - return false, fmt.Sprintf("error while getting org team, error: %s", err.Error()) + return false, err.Error() } + for _, allowedTeam := range allowedTeams { for _, orgTeam := range orgTeams { if orgTeam.Name == allowedTeam { @@ -207,6 +199,32 @@ func (v *Provider) IsAllowedOwnersFile(ctx context.Context, rev *info.Event) (bo return acl.UserInOwnerFile(ownerContent, ownerAliasesContent, rev.Sender) } +func (v *Provider) listOrgTeams(org, sender string) ([]*forgejo.Team, error) { + if v.cachedOrgTeams == nil { + v.cachedOrgTeams = make(map[string][]*forgejo.Team) + } + if teams, ok := v.cachedOrgTeams[org]; ok { + return teams, nil + } + + teams, resp, err := v.Client().ListOrgTeams(org, forgejo.ListTeamsOptions{}) + if err != nil { + if resp != nil && resp.Response != nil { + switch resp.StatusCode { + case http.StatusNotFound: + return nil, fmt.Errorf("no teams on org %s", org) + case http.StatusForbidden: + v.Logger.Warnf("policy check: ListOrgTeams returned 403 for org %s, sender %s: %v", org, sender, err) + return nil, fmt.Errorf("unable to list teams on org %s: the token used doesn't have permission to list teams in this org, make sure the token owner is a member of the org", org) + } + } + return nil, fmt.Errorf("error while getting org team, error: %w", err) + } + + v.cachedOrgTeams[org] = teams + return teams, nil +} + func (v *Provider) checkSenderRepoMembership(_ context.Context, runevent *info.Event) (bool, error) { ret, _, err := v.Client().IsCollaborator(runevent.Organization, runevent.Repository, runevent.Sender) return ret, err diff --git a/pkg/provider/gitea/acl_test.go b/pkg/provider/gitea/acl_test.go index 51348368f..275f11da2 100644 --- a/pkg/provider/gitea/acl_test.go +++ b/pkg/provider/gitea/acl_test.go @@ -110,6 +110,40 @@ func TestCheckPolicyAllowing(t *testing.T) { } } +func TestCheckPolicyAllowingCaching(t *testing.T) { + fakeclient, mux, teardown := tgitea.Setup(t) + defer teardown() + + apiCallCount := 0 + event := &info.Event{ + Organization: "myorg", + Sender: "allowedUser", + } + + mux.HandleFunc("/orgs/myorg/teams", func(rw http.ResponseWriter, _ *http.Request) { + apiCallCount++ + fmt.Fprint(rw, `[{"name": "team1", "id": 1}]`) + }) + mux.HandleFunc("/teams/1/members/allowedUser", func(rw http.ResponseWriter, _ *http.Request) { + fmt.Fprint(rw, `{"id": 2}`) + }) + + ctx, _ := rtesting.SetupFakeContext(t) + observer, _ := zapobserver.New(zap.InfoLevel) + logger := zap.New(observer).Sugar() + gprovider := Provider{giteaClient: fakeclient, Logger: logger} + + allowed1, reason1 := gprovider.CheckPolicyAllowing(ctx, event, []string{"team1"}) + assert.Assert(t, allowed1) + assert.Equal(t, "allowing user: allowedUser as a member of the team: team1", reason1) + + allowed2, reason2 := gprovider.CheckPolicyAllowing(ctx, event, []string{"team1"}) + assert.Assert(t, allowed2) + assert.Equal(t, "allowing user: allowedUser as a member of the team: team1", reason2) + + assert.Equal(t, 1, apiCallCount) +} + func TestOkToTestComment(t *testing.T) { issueCommentPayload := &forgejostructs.IssueCommentPayload{ Comment: &forgejostructs.Comment{ diff --git a/pkg/provider/gitea/gitea.go b/pkg/provider/gitea/gitea.go index 5ebadc76c..7df2bb3e2 100644 --- a/pkg/provider/gitea/gitea.go +++ b/pkg/provider/gitea/gitea.go @@ -70,6 +70,7 @@ type Provider struct { triggerEvent string pacUserID int64 // user login used by PAC cachedChangedFiles *changedfiles.ChangedFiles + cachedOrgTeams map[string][]*forgejo.Team clock clockwork.Clock }