Skip to content

Commit 94397af

Browse files
zakiskzakisk
committed
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 <[email protected]>
1 parent a8212d3 commit 94397af

File tree

3 files changed

+149
-23
lines changed

3 files changed

+149
-23
lines changed

‎pkg/provider/gitlab/acl.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,22 @@ func (v *Provider) checkOkToTestCommentFromApprovedMember(ctx context.Context, e
6565
nextPage = resp.NextPage
6666
}
6767

68+
switch gitEvent := event.Event.(type) {
69+
case *gitlab.MergeEvent:
70+
if !v.pacInfo.RememberOKToTest {
71+
v.Logger.Info("RememberOKToTest is disabled, skipping MergeRequest notes check as it is not needed")
72+
return false, nil
73+
}
74+
case *gitlab.MergeCommentEvent:
75+
if !v.pacInfo.RememberOKToTest {
76+
v.Logger.Info("Event is a MergeCommentEvent and RememberOKToTest is disabled, checking current comment only")
77+
return v.aclAllowedOkToTestCurrentComment(ctx, event, gitEvent.ObjectAttributes.ID)
78+
}
79+
default:
80+
v.Logger.Info("Event is not a MergeEvent or MergeCommentEvent, skipping merge request notes check")
81+
return false, nil
82+
}
83+
6884
for _, discussion := range discussions {
6985
// Iterate through every note in the discussion thread and evaluate them.
7086
// If a note contains an OK-to-test command, verify the commenter's permission
@@ -92,6 +108,21 @@ func (v *Provider) checkOkToTestCommentFromApprovedMember(ctx context.Context, e
92108
return false, nil
93109
}
94110

111+
func (v *Provider) aclAllowedOkToTestCurrentComment(ctx context.Context, event *info.Event, commentID int) (bool, error) {
112+
comment, _, err := v.Client().Notes.GetMergeRequestNote(v.targetProjectID, event.PullRequestNumber, commentID)
113+
if err != nil {
114+
return false, err
115+
}
116+
117+
if acl.MatchRegexp(acl.OKToTestCommentRegexp, comment.Body) {
118+
event.Sender = comment.Author.Username
119+
if v.checkMembership(ctx, event, comment.Author.ID) {
120+
return true, nil
121+
}
122+
}
123+
return false, nil
124+
}
125+
95126
func (v *Provider) IsAllowed(ctx context.Context, event *info.Event) (bool, error) {
96127
if v.gitlabClient == nil {
97128
return false, fmt.Errorf("no github client has been initialized, " +

‎pkg/provider/gitlab/acl_test.go‎

Lines changed: 84 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,16 @@
11
package gitlab
22

33
import (
4+
"encoding/json"
45
"fmt"
56
"net/http"
67
"testing"
78

89
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
10+
"github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings"
911
thelp "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/gitlab/test"
12+
"github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger"
13+
gitlab "gitlab.com/gitlab-org/api/client-go"
1014
rtesting "knative.dev/pkg/reconciler/testing"
1115
)
1216

@@ -19,19 +23,38 @@ func TestIsAllowed(t *testing.T) {
1923
type args struct {
2024
event *info.Event
2125
}
26+
27+
testEvent := thelp.TEvent{
28+
UserID: 1111,
29+
TargetProjectID: 2525,
30+
NoteID: 1111,
31+
Username: "admin",
32+
DefaultBranch: "main",
33+
URL: "https://gitlab.com/admin/testrepo",
34+
SHA: "sha",
35+
SHAurl: "https://url",
36+
SHAtitle: "commit it",
37+
Headbranch: "branch",
38+
Basebranch: "main",
39+
HeadURL: "https://url",
40+
BaseURL: "https://url",
41+
}
42+
2243
tests := []struct {
23-
name string
24-
fields fields
25-
args args
26-
allowed bool
27-
wantErr bool
28-
wantClient bool
29-
allowMemberID int
30-
ownerFile string
31-
commentContent string
32-
commentAuthor string
33-
commentAuthorID int
34-
threadFirstNote string
44+
name string
45+
fields fields
46+
args args
47+
allowed bool
48+
wantErr bool
49+
wantClient bool
50+
allowMemberID int
51+
ownerFile string
52+
commentContent string
53+
commentAuthor string
54+
commentAuthorID int
55+
threadFirstNote string
56+
rememberOKToTest bool
57+
wantEventStruct bool
3558
}{
3659
{
3760
name: "check client has been set",
@@ -64,7 +87,25 @@ func TestIsAllowed(t *testing.T) {
6487
ownerFile: "---\n approvers:\n - allowmeplease\n",
6588
},
6689
{
67-
name: "allowed from ok-to-test",
90+
name: "allowed from ok-to-test with RememberOKToTest enabled",
91+
allowed: true,
92+
wantClient: true,
93+
fields: fields{
94+
userID: 6666,
95+
targetProjectID: 2525,
96+
},
97+
args: args{
98+
event: &info.Event{Sender: "noowner", PullRequestNumber: 1},
99+
},
100+
allowMemberID: 1111,
101+
commentContent: "/ok-to-test",
102+
commentAuthor: "admin",
103+
commentAuthorID: 1111,
104+
wantEventStruct: true,
105+
rememberOKToTest: true,
106+
},
107+
{
108+
name: "allowed from ok-to-test with RememberOKToTest disabled",
68109
allowed: true,
69110
wantClient: true,
70111
fields: fields{
@@ -74,10 +115,12 @@ func TestIsAllowed(t *testing.T) {
74115
args: args{
75116
event: &info.Event{Sender: "noowner", PullRequestNumber: 1},
76117
},
77-
allowMemberID: 1111,
78-
commentContent: "/ok-to-test",
79-
commentAuthor: "admin",
80-
commentAuthorID: 1111,
118+
allowMemberID: 1111,
119+
commentContent: "/ok-to-test",
120+
commentAuthor: "admin",
121+
commentAuthorID: 1111,
122+
wantEventStruct: true,
123+
rememberOKToTest: false, // make it disabled explicitly to indicate what we're testing 🙃
81124
},
82125
{
83126
name: "allowed when /ok-to-test is in a reply note",
@@ -90,11 +133,13 @@ func TestIsAllowed(t *testing.T) {
90133
args: args{
91134
event: &info.Event{Sender: "noowner", PullRequestNumber: 2},
92135
},
93-
allowMemberID: 1111,
94-
threadFirstNote: "random comment",
95-
commentContent: "/ok-to-test",
96-
commentAuthor: "admin",
97-
commentAuthorID: 1111,
136+
allowMemberID: 1111,
137+
threadFirstNote: "random comment",
138+
commentContent: "/ok-to-test",
139+
commentAuthor: "admin",
140+
commentAuthorID: 1111,
141+
wantEventStruct: true,
142+
rememberOKToTest: true,
98143
},
99144
{
100145
name: "disallowed from non authorized note",
@@ -113,16 +158,20 @@ func TestIsAllowed(t *testing.T) {
113158
for _, tt := range tests {
114159
t.Run(tt.name, func(t *testing.T) {
115160
ctx, _ := rtesting.SetupFakeContext(t)
161+
logger, _ := logger.GetLogger()
116162

117163
v := &Provider{
118164
targetProjectID: tt.fields.targetProjectID,
119165
sourceProjectID: tt.fields.sourceProjectID,
120166
userID: tt.fields.userID,
167+
Logger: logger,
168+
pacInfo: &info.PacOpts{Settings: settings.Settings{RememberOKToTest: tt.rememberOKToTest}},
121169
}
122170
if tt.wantClient {
123171
client, mux, tearDown := thelp.Setup(t)
124172
v.gitlabClient = client
125173
if tt.allowMemberID != 0 {
174+
v.userID = tt.allowMemberID
126175
thelp.MuxAllowUserID(mux, tt.fields.targetProjectID, tt.allowMemberID)
127176
} else {
128177
thelp.MuxDisallowUserID(mux, tt.fields.targetProjectID, tt.allowMemberID)
@@ -143,9 +192,22 @@ func TestIsAllowed(t *testing.T) {
143192
} else {
144193
thelp.MuxDiscussionsNoteEmpty(mux, tt.fields.targetProjectID, tt.args.event.PullRequestNumber)
145194
}
195+
if tt.commentContent != "" {
196+
thelp.MuxMergeRequestNote(mux, tt.fields.targetProjectID, tt.args.event.PullRequestNumber, testEvent.NoteID, testEvent.UserID, tt.commentContent, testEvent.Username)
197+
}
146198

147199
defer tearDown()
148200
}
201+
202+
if tt.wantEventStruct {
203+
glEvent := &gitlab.MergeCommentEvent{}
204+
err := json.Unmarshal([]byte(testEvent.MergeCommentEventAsJSON(tt.commentContent)), glEvent)
205+
if err != nil {
206+
t.Fatalf("failed to unmarshal event: %v", err)
207+
}
208+
tt.args.event.Event = glEvent
209+
}
210+
149211
got, err := v.IsAllowed(ctx, tt.args.event)
150212
if (err != nil) != tt.wantErr {
151213
t.Errorf("IsAllowed() error = %v, wantErr %v", err, tt.wantErr)

‎pkg/provider/gitlab/test/test.go‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,13 @@ func MuxDiscussionsNoteEmpty(mux *http.ServeMux, pid, mrID int) {
114114
})
115115
}
116116

117+
func MuxMergeRequestNote(mux *http.ServeMux, pid, mrID, noteID, userID int, commentContent, username string) {
118+
path := fmt.Sprintf("/projects/%d/merge_requests/%d/notes/%d", pid, mrID, noteID)
119+
mux.HandleFunc(path, func(rw http.ResponseWriter, _ *http.Request) {
120+
fmt.Fprintf(rw, `{"body": "%s", "author": {"username": "%s", "id": %d}}`, commentContent, username, userID)
121+
})
122+
}
123+
117124
func MuxDiscussionsNote(mux *http.ServeMux, pid, mrID int, author string, authorID int, notecontent string) {
118125
path := fmt.Sprintf("/projects/%d/merge_requests/%d/discussions", pid, mrID)
119126
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
171178

172179
type TEvent struct {
173180
Username, DefaultBranch, URL, SHA, SHAurl, SHAtitle, Headbranch, Basebranch, HeadURL, BaseURL string
174-
UserID, MRID, TargetProjectID, SourceProjectID int
181+
UserID, MRID, TargetProjectID, SourceProjectID, NoteID int
175182
PathWithNameSpace, Comment string
176183
}
177184

@@ -310,3 +317,29 @@ func (t TEvent) CommitNoteEventAsJSON(comment, action, repository string) string
310317
}
311318
}`, t.SHA, comment, action, t.UserID, t.Username, t.SourceProjectID, t.DefaultBranch, t.URL, t.PathWithNameSpace, repository)
312319
}
320+
321+
func (t TEvent) MergeCommentEventAsJSON(comment string) string {
322+
return fmt.Sprintf(`{
323+
"object_kind": "note",
324+
"event_type": "note",
325+
"object_attributes": {
326+
"id": %d,
327+
"note": "%s"
328+
},
329+
"user": {
330+
"id": %d,
331+
"username": "%s"
332+
},
333+
"project": {
334+
"id": %d,
335+
"web_url": "%s"
336+
},
337+
"merge_request": {
338+
"iid": %d,
339+
"target_project_id": %d,
340+
"source_project_id": %d,
341+
"target_branch": "%s",
342+
"source_branch": "%s"
343+
}
344+
}`, t.NoteID, comment, t.UserID, t.Username, t.TargetProjectID, t.URL, t.MRID, t.TargetProjectID, t.SourceProjectID, t.Basebranch, t.Headbranch)
345+
}

0 commit comments

Comments
 (0)