From 00810f6348ffe660e798299ea5a86cc8e6d93fbb Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Thu, 6 Nov 2025 16:12:30 +0530 Subject: [PATCH] fix(gitlab): check permission according to RememberOkToTest setting Refactor ACL check to properly handle RememberOKToTest setting: - When RememberOKToTest is disabled for MergeEvent, skip checking all discussion notes and return false early - When RememberOKToTest is disabled for MergeCommentEvent, check only the current comment instead of all discussion history - Add aclAllowedOkToTestCurrentComment function to validate the specific comment that triggered the event This avoids checking all comments for /ok-to-test regardless of RememberOkToTest setting and optimizes the ACL check by avoiding unnecessary API calls to fetch all discussion notes when RememberOKToTest is disabled. https://issues.redhat.com/browse/SRVKP-9200 Signed-off-by: Zaki Shaikh --- pkg/provider/gitlab/acl.go | 36 +++++++++++ pkg/provider/gitlab/acl_test.go | 106 ++++++++++++++++++++++++------- pkg/provider/gitlab/test/test.go | 35 +++++++++- 3 files changed, 154 insertions(+), 23 deletions(-) diff --git a/pkg/provider/gitlab/acl.go b/pkg/provider/gitlab/acl.go index 0a3f7133bb..ec92a6d86f 100644 --- a/pkg/provider/gitlab/acl.go +++ b/pkg/provider/gitlab/acl.go @@ -65,6 +65,22 @@ func (v *Provider) checkOkToTestCommentFromApprovedMember(ctx context.Context, e nextPage = resp.NextPage } + switch gitEvent := event.Event.(type) { + case *gitlab.MergeEvent: + if !v.pacInfo.RememberOKToTest { + v.Logger.Debug("RememberOKToTest is disabled, skipping MergeRequest notes check as it is not needed") + return false, nil + } + case *gitlab.MergeCommentEvent: + if !v.pacInfo.RememberOKToTest { + v.Logger.Debug("Event is a MergeCommentEvent and RememberOKToTest is disabled, checking current comment only") + return v.aclAllowedOkToTestCurrentComment(ctx, event, gitEvent.ObjectAttributes.ID) + } + default: + v.Logger.Debug("Event is not a MergeEvent or MergeCommentEvent, skipping merge request notes check") + return false, nil + } + for _, discussion := range discussions { // Iterate through every note in the discussion thread and evaluate them. // If a note contains an OK-to-test command, verify the commenter's permission @@ -92,6 +108,26 @@ func (v *Provider) checkOkToTestCommentFromApprovedMember(ctx context.Context, e return false, nil } +func (v *Provider) aclAllowedOkToTestCurrentComment(ctx context.Context, event *info.Event, commentID int) (bool, error) { + comment, _, err := v.Client().Notes.GetMergeRequestNote(v.targetProjectID, event.PullRequestNumber, commentID) + if err != nil { + return false, err + } + + if acl.MatchRegexp(acl.OKToTestCommentRegexp, comment.Body) { + commenterEvent := info.NewEvent() + commenterEvent.Event = event.Event + commenterEvent.Sender = comment.Author.Username + commenterEvent.BaseBranch = event.BaseBranch + commenterEvent.HeadBranch = event.HeadBranch + commenterEvent.DefaultBranch = event.DefaultBranch + if v.checkMembership(ctx, commenterEvent, comment.Author.ID) { + return true, nil + } + } + return false, nil +} + func (v *Provider) IsAllowed(ctx context.Context, event *info.Event) (bool, error) { if v.gitlabClient == nil { return false, fmt.Errorf("no github client has been initialized, " + diff --git a/pkg/provider/gitlab/acl_test.go b/pkg/provider/gitlab/acl_test.go index d72305a8a4..e2059dbff1 100644 --- a/pkg/provider/gitlab/acl_test.go +++ b/pkg/provider/gitlab/acl_test.go @@ -1,12 +1,16 @@ package gitlab import ( + "encoding/json" "fmt" "net/http" "testing" "github.com/openshift-pipelines/pipelines-as-code/pkg/params/info" + "github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings" thelp "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/gitlab/test" + "github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger" + gitlab "gitlab.com/gitlab-org/api/client-go" rtesting "knative.dev/pkg/reconciler/testing" ) @@ -19,19 +23,38 @@ func TestIsAllowed(t *testing.T) { type args struct { event *info.Event } + + testEvent := thelp.TEvent{ + UserID: 1111, + TargetProjectID: 2525, + NoteID: 1111, + Username: "admin", + DefaultBranch: "main", + URL: "https://gitlab.com/admin/testrepo", + SHA: "sha", + SHAurl: "https://url", + SHAtitle: "commit it", + Headbranch: "branch", + Basebranch: "main", + HeadURL: "https://url", + BaseURL: "https://url", + } + tests := []struct { - name string - fields fields - args args - allowed bool - wantErr bool - wantClient bool - allowMemberID int - ownerFile string - commentContent string - commentAuthor string - commentAuthorID int - threadFirstNote string + name string + fields fields + args args + allowed bool + wantErr bool + wantClient bool + allowMemberID int + ownerFile string + commentContent string + commentAuthor string + commentAuthorID int + threadFirstNote string + rememberOKToTest bool + wantEventStruct bool }{ { name: "check client has been set", @@ -64,7 +87,25 @@ func TestIsAllowed(t *testing.T) { ownerFile: "---\n approvers:\n - allowmeplease\n", }, { - name: "allowed from ok-to-test", + name: "allowed from ok-to-test with RememberOKToTest enabled", + allowed: true, + wantClient: true, + fields: fields{ + userID: 6666, + targetProjectID: 2525, + }, + args: args{ + event: &info.Event{Sender: "noowner", PullRequestNumber: 1}, + }, + allowMemberID: 1111, + commentContent: "/ok-to-test", + commentAuthor: "admin", + commentAuthorID: 1111, + wantEventStruct: true, + rememberOKToTest: true, + }, + { + name: "allowed from ok-to-test with RememberOKToTest disabled", allowed: true, wantClient: true, fields: fields{ @@ -74,10 +115,12 @@ func TestIsAllowed(t *testing.T) { args: args{ event: &info.Event{Sender: "noowner", PullRequestNumber: 1}, }, - allowMemberID: 1111, - commentContent: "/ok-to-test", - commentAuthor: "admin", - commentAuthorID: 1111, + allowMemberID: 1111, + commentContent: "/ok-to-test", + commentAuthor: "admin", + commentAuthorID: 1111, + wantEventStruct: true, + rememberOKToTest: false, // make it disabled explicitly to indicate what we're testing 🙃 }, { name: "allowed when /ok-to-test is in a reply note", @@ -90,11 +133,13 @@ func TestIsAllowed(t *testing.T) { args: args{ event: &info.Event{Sender: "noowner", PullRequestNumber: 2}, }, - allowMemberID: 1111, - threadFirstNote: "random comment", - commentContent: "/ok-to-test", - commentAuthor: "admin", - commentAuthorID: 1111, + allowMemberID: 1111, + threadFirstNote: "random comment", + commentContent: "/ok-to-test", + commentAuthor: "admin", + commentAuthorID: 1111, + wantEventStruct: true, + rememberOKToTest: true, }, { name: "disallowed from non authorized note", @@ -113,16 +158,20 @@ func TestIsAllowed(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) + logger, _ := logger.GetLogger() v := &Provider{ targetProjectID: tt.fields.targetProjectID, sourceProjectID: tt.fields.sourceProjectID, userID: tt.fields.userID, + Logger: logger, + pacInfo: &info.PacOpts{Settings: settings.Settings{RememberOKToTest: tt.rememberOKToTest}}, } if tt.wantClient { client, mux, tearDown := thelp.Setup(t) v.gitlabClient = client if tt.allowMemberID != 0 { + v.userID = tt.allowMemberID thelp.MuxAllowUserID(mux, tt.fields.targetProjectID, tt.allowMemberID) } else { thelp.MuxDisallowUserID(mux, tt.fields.targetProjectID, tt.allowMemberID) @@ -143,9 +192,22 @@ func TestIsAllowed(t *testing.T) { } else { thelp.MuxDiscussionsNoteEmpty(mux, tt.fields.targetProjectID, tt.args.event.PullRequestNumber) } + if tt.commentContent != "" { + thelp.MuxMergeRequestNote(mux, tt.fields.targetProjectID, tt.args.event.PullRequestNumber, testEvent.NoteID, testEvent.UserID, tt.commentContent, testEvent.Username) + } defer tearDown() } + + if tt.wantEventStruct { + glEvent := &gitlab.MergeCommentEvent{} + err := json.Unmarshal([]byte(testEvent.MergeCommentEventAsJSON(tt.commentContent)), glEvent) + if err != nil { + t.Fatalf("failed to unmarshal event: %v", err) + } + tt.args.event.Event = glEvent + } + got, err := v.IsAllowed(ctx, tt.args.event) if (err != nil) != tt.wantErr { t.Errorf("IsAllowed() error = %v, wantErr %v", err, tt.wantErr) diff --git a/pkg/provider/gitlab/test/test.go b/pkg/provider/gitlab/test/test.go index ca193f6358..174a29e68a 100644 --- a/pkg/provider/gitlab/test/test.go +++ b/pkg/provider/gitlab/test/test.go @@ -114,6 +114,13 @@ func MuxDiscussionsNoteEmpty(mux *http.ServeMux, pid, mrID int) { }) } +func MuxMergeRequestNote(mux *http.ServeMux, pid, mrID, noteID, userID int, commentContent, username string) { + path := fmt.Sprintf("/projects/%d/merge_requests/%d/notes/%d", pid, mrID, noteID) + mux.HandleFunc(path, func(rw http.ResponseWriter, _ *http.Request) { + fmt.Fprintf(rw, `{"body": "%s", "author": {"username": "%s", "id": %d}}`, commentContent, username, userID) + }) +} + func MuxDiscussionsNote(mux *http.ServeMux, pid, mrID int, author string, authorID int, notecontent string) { path := fmt.Sprintf("/projects/%d/merge_requests/%d/discussions", pid, mrID) mux.HandleFunc(path, func(rw http.ResponseWriter, r *http.Request) { @@ -171,7 +178,7 @@ func MuxGetFile(mux *http.ServeMux, pid int, fname, content string, wantErr bool type TEvent struct { Username, DefaultBranch, URL, SHA, SHAurl, SHAtitle, Headbranch, Basebranch, HeadURL, BaseURL string - UserID, MRID, TargetProjectID, SourceProjectID int + UserID, MRID, TargetProjectID, SourceProjectID, NoteID int PathWithNameSpace, Comment string } @@ -310,3 +317,29 @@ func (t TEvent) CommitNoteEventAsJSON(comment, action, repository string) string } }`, t.SHA, comment, action, t.UserID, t.Username, t.SourceProjectID, t.DefaultBranch, t.URL, t.PathWithNameSpace, repository) } + +func (t TEvent) MergeCommentEventAsJSON(comment string) string { + return fmt.Sprintf(`{ + "object_kind": "note", + "event_type": "note", + "object_attributes": { + "id": %d, + "note": "%s" + }, + "user": { + "id": %d, + "username": "%s" + }, + "project": { + "id": %d, + "web_url": "%s" + }, + "merge_request": { + "iid": %d, + "target_project_id": %d, + "source_project_id": %d, + "target_branch": "%s", + "source_branch": "%s" + } +}`, t.NoteID, comment, t.UserID, t.Username, t.TargetProjectID, t.URL, t.MRID, t.TargetProjectID, t.SourceProjectID, t.Basebranch, t.Headbranch) +}