Skip to content

Commit 8db326b

Browse files
chmouelchmouel
authored
Merge branch 'main' into dependabot/github_actions/actions/setup-go-6.3.0
2 parents aa3025d + 95bb453 commit 8db326b

File tree

2 files changed

+255
-4
lines changed

2 files changed

+255
-4
lines changed

‎pkg/provider/github/github.go‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"time"
1616

1717
"github.com/gobwas/glob"
18+
"github.com/golang-jwt/jwt/v4"
1819
"github.com/google/go-github/v81/github"
1920
"github.com/jonboulle/clockwork"
2021
"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
@@ -62,6 +63,7 @@ type Provider struct {
6263
triggerEvent string
6364
cachedChangedFiles *changedfiles.ChangedFiles
6465
commitInfo *github.Commit
66+
pacUserLogin string // user/bot login used by PAC
6567
}
6668

6769
type skippedRun struct {
@@ -898,6 +900,16 @@ func (v *Provider) listCommentsByMarker(
898900
matchedComments := make([]*github.IssueComment, 0, len(comments))
899901
for _, comment := range comments {
900902
if re.MatchString(comment.GetBody()) {
903+
if err = v.getUserLogin(ctx, event); err != nil {
904+
return nil, fmt.Errorf("unable to fetch user info: %w", err)
905+
}
906+
// Only edit comments created by this PAC installation's credentials.
907+
// Prevents accidentally modifying comments from other users/bots.
908+
if comment.GetUser().GetLogin() != v.pacUserLogin {
909+
v.Logger.Debugf("This comment was not created by PAC, skipping comment edit :%d, created by user %s, PAC user: %s",
910+
comment.GetID(), comment.GetUser().GetLogin(), v.pacUserLogin)
911+
continue
912+
}
901913
matchedComments = append(matchedComments, comment)
902914
}
903915
}
@@ -989,3 +1001,61 @@ func (v *Provider) CreateComment(ctx context.Context, event *info.Event, commit,
9891001
)
9901002
return nil
9911003
}
1004+
1005+
func (v *Provider) getUserLogin(ctx context.Context, event *info.Event) error {
1006+
if v.pacUserLogin != "" {
1007+
return nil
1008+
}
1009+
1010+
// Get the PAC user/bot login.
1011+
if event.InstallationID > 0 {
1012+
// For Apps, get the app slug.
1013+
slug, err := v.fetchAppSlug(ctx, event.Provider.URL)
1014+
if err != nil {
1015+
return fmt.Errorf("failed to fetch app slug: %w", err)
1016+
}
1017+
1018+
v.pacUserLogin = fmt.Sprintf("%s[bot]", slug)
1019+
} else {
1020+
// For PATs, get the authenticated user.
1021+
pacUser, _, err := v.Client().Users.Get(ctx, "")
1022+
if err != nil {
1023+
return fmt.Errorf("unable to fetch user info: %w", err)
1024+
}
1025+
v.pacUserLogin = pacUser.GetLogin()
1026+
}
1027+
1028+
return nil
1029+
}
1030+
1031+
// Refer https://docs.github.com/en/apps/creating-github-apps/authenticating-with-a-github-app/authenticating-as-a-github-app-installation
1032+
func (v *Provider) fetchAppSlug(ctx context.Context, apiURL string) (string, error) {
1033+
ns := info.GetNS(ctx)
1034+
applicationID, privateKey, err := v.GetAppIDAndPrivateKey(ctx, ns, v.Run.Clients.Kube)
1035+
if err != nil {
1036+
return "", err
1037+
}
1038+
parsedPK, err := jwt.ParseRSAPrivateKeyFromPEM(privateKey)
1039+
if err != nil {
1040+
return "", fmt.Errorf("failed to parse private key: %w", err)
1041+
}
1042+
1043+
token := jwt.NewWithClaims(jwt.SigningMethodRS256, &jwt.RegisteredClaims{
1044+
Issuer: fmt.Sprintf("%d", applicationID),
1045+
IssuedAt: jwt.NewNumericDate(time.Now()),
1046+
ExpiresAt: jwt.NewNumericDate(time.Now().Add(5 * time.Minute)),
1047+
})
1048+
1049+
tokenString, err := token.SignedString(parsedPK)
1050+
if err != nil {
1051+
return "", fmt.Errorf("failed to sign private key: %w", err)
1052+
}
1053+
1054+
client, _, _ := MakeClient(ctx, apiURL, tokenString)
1055+
app, _, err := client.Apps.Get(ctx, "")
1056+
if err != nil {
1057+
return "", fmt.Errorf("failed to get app info: %w", err)
1058+
}
1059+
1060+
return app.GetSlug(), nil
1061+
}

‎pkg/provider/github/github_test.go‎

Lines changed: 185 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1468,9 +1468,13 @@ func TestCreateComment(t *testing.T) {
14681468
updateMarker: "MARKER",
14691469
setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
14701470
t.Helper()
1471+
mux.HandleFunc("/user", func(rw http.ResponseWriter, _ *http.Request) {
1472+
fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
1473+
},
1474+
)
14711475
mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
14721476
if r.Method == http.MethodGet {
1473-
fmt.Fprint(rw, `[{"id": 555, "body": "MARKER"}]`)
1477+
fmt.Fprint(rw, `[{"id": 555, "body": "MARKER", "user": {"id": 100, "login": "pac-user"}}]`)
14741478
return
14751479
}
14761480
})
@@ -1487,6 +1491,9 @@ func TestCreateComment(t *testing.T) {
14871491
updateMarker: "MARKER",
14881492
setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
14891493
t.Helper()
1494+
mux.HandleFunc("/user", func(rw http.ResponseWriter, _ *http.Request) {
1495+
fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
1496+
})
14901497
mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
14911498
if r.Method == http.MethodGet {
14921499
fmt.Fprint(rw, `[{"id": 555, "body": "NO_MATCH"}]`)
@@ -1498,18 +1505,54 @@ func TestCreateComment(t *testing.T) {
14981505
return nil
14991506
},
15001507
},
1508+
{
1509+
name: "skip comment from different user and create new",
1510+
event: &info.Event{Organization: "org", Repository: "repo", PullRequestNumber: 123},
1511+
updateMarker: "MARKER",
1512+
commentBody: "New Comment MARKER",
1513+
setup: func(t *testing.T, mux *http.ServeMux) func(t *testing.T) {
1514+
t.Helper()
1515+
editEndpointCalled := false
1516+
commentCreated := false
1517+
mux.HandleFunc("/user", func(rw http.ResponseWriter, _ *http.Request) {
1518+
fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
1519+
})
1520+
mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
1521+
if r.Method == http.MethodGet {
1522+
fmt.Fprint(rw, `[{"id": 555, "body": "Old MARKER", "user": {"id": 999, "login": "other-user"}}]`)
1523+
return
1524+
}
1525+
assert.Equal(t, r.Method, http.MethodPost)
1526+
commentCreated = true
1527+
rw.WriteHeader(http.StatusCreated)
1528+
fmt.Fprint(rw, `{"id": 556}`)
1529+
})
1530+
mux.HandleFunc("/repos/org/repo/issues/comments/555", func(rw http.ResponseWriter, _ *http.Request) {
1531+
editEndpointCalled = true
1532+
t.Error("should not edit comment from different user")
1533+
rw.WriteHeader(http.StatusOK)
1534+
})
1535+
return func(t *testing.T) {
1536+
t.Helper()
1537+
assert.Assert(t, !editEndpointCalled, "edit endpoint should not be called for comment from different user")
1538+
assert.Assert(t, commentCreated, "new comment should be created")
1539+
}
1540+
},
1541+
},
15011542
}
15021543

15031544
for _, tt := range tests {
15041545
t.Run(tt.name, func(t *testing.T) {
15051546
ctx, _ := rtesting.SetupFakeContext(t)
1547+
observer, _ := zapobserver.New(zap.InfoLevel)
1548+
fakelogger := zap.New(observer).Sugar()
15061549

15071550
var provider *Provider
15081551
var postAssert func(t *testing.T)
15091552
if !tt.clientNil {
15101553
fakeclient, mux, _, teardown := ghtesthelper.SetupGH()
15111554
defer teardown()
1512-
provider = &Provider{ghClient: fakeclient}
1555+
provider = &Provider{ghClient: fakeclient, Logger: fakelogger}
15131556

15141557
if tt.setup != nil {
15151558
postAssert = tt.setup(t, mux)
@@ -1577,9 +1620,12 @@ func TestCreateCommentDedupLogging(t *testing.T) {
15771620
updateMarker: "MARKER",
15781621
setup: func(t *testing.T, mux *http.ServeMux) {
15791622
t.Helper()
1623+
mux.HandleFunc("/user", func(rw http.ResponseWriter, _ *http.Request) {
1624+
fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
1625+
})
15801626
mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
15811627
assert.Equal(t, r.Method, http.MethodGet)
1582-
fmt.Fprint(rw, `[{"id": 111, "body": "MARKER old", "created_at": "2024-01-01T00:00:00Z"}]`)
1628+
fmt.Fprint(rw, `[{"id": 111, "body": "MARKER old", "created_at": "2024-01-01T00:00:00Z", "user": {"id": 100, "login": "pac-user"}}]`)
15831629
})
15841630
mux.HandleFunc("/repos/org/repo/issues/comments/111", func(rw http.ResponseWriter, r *http.Request) {
15851631
assert.Equal(t, r.Method, http.MethodPatch)
@@ -1598,9 +1644,12 @@ func TestCreateCommentDedupLogging(t *testing.T) {
15981644
updateMarker: "MARKER",
15991645
setup: func(t *testing.T, mux *http.ServeMux) {
16001646
t.Helper()
1647+
mux.HandleFunc("/user", func(rw http.ResponseWriter, _ *http.Request) {
1648+
fmt.Fprint(rw, `{"id": 100, "login": "pac-user"}`)
1649+
})
16011650
mux.HandleFunc("/repos/org/repo/issues/123/comments", func(rw http.ResponseWriter, r *http.Request) {
16021651
assert.Equal(t, r.Method, http.MethodGet)
1603-
fmt.Fprint(rw, `[{"id": 111, "body": "MARKER old", "created_at": "2024-01-01T00:00:00Z"}, {"id": 222, "body": "MARKER old", "created_at": "2024-01-01T00:01:00Z"}]`)
1652+
fmt.Fprint(rw, `[{"id": 111, "body": "MARKER old", "created_at": "2024-01-01T00:00:00Z", "user": {"id": 100, "login": "pac-user"}}, {"id": 222, "body": "MARKER old", "created_at": "2024-01-01T00:01:00Z", "user": {"id": 100, "login": "pac-user"}}]`)
16041653
})
16051654
mux.HandleFunc("/repos/org/repo/issues/comments/111", func(rw http.ResponseWriter, r *http.Request) {
16061655
assert.Equal(t, r.Method, http.MethodPatch)
@@ -1905,3 +1954,135 @@ func TestSkipPushEventForPRCommits(t *testing.T) {
19051954
})
19061955
}
19071956
}
1957+
1958+
func TestFetchAppSlug(t *testing.T) {
1959+
testAppID := int64(12345)
1960+
validSlug := "my-github-app"
1961+
1962+
tests := []struct {
1963+
name string
1964+
privateKey []byte
1965+
applicationID int64
1966+
setupMux func(mux *http.ServeMux)
1967+
wantSlug string
1968+
wantErrSubstring string
1969+
}{
1970+
{
1971+
name: "success fetch app slug",
1972+
privateKey: []byte(fakePrivateKey),
1973+
applicationID: testAppID,
1974+
setupMux: func(mux *http.ServeMux) {
1975+
mux.HandleFunc("/app", func(w http.ResponseWriter, _ *http.Request) {
1976+
_, _ = fmt.Fprintf(w, `{"slug": "%s", "name": "My GitHub App"}`, validSlug)
1977+
})
1978+
},
1979+
wantSlug: validSlug,
1980+
},
1981+
{
1982+
name: "app endpoint returns 404",
1983+
privateKey: []byte(fakePrivateKey),
1984+
applicationID: testAppID,
1985+
setupMux: func(mux *http.ServeMux) {
1986+
mux.HandleFunc("/app", func(w http.ResponseWriter, _ *http.Request) {
1987+
w.WriteHeader(http.StatusNotFound)
1988+
_, _ = fmt.Fprint(w, `{"message": "Not Found"}`)
1989+
})
1990+
},
1991+
wantErrSubstring: "failed to get app info",
1992+
},
1993+
{
1994+
name: "invalid private key",
1995+
privateKey: []byte("invalid-key"),
1996+
applicationID: testAppID,
1997+
setupMux: func(_ *http.ServeMux) {},
1998+
wantErrSubstring: "failed to parse private key",
1999+
},
2000+
{
2001+
name: "app returns empty slug",
2002+
privateKey: []byte(fakePrivateKey),
2003+
applicationID: testAppID,
2004+
setupMux: func(mux *http.ServeMux) {
2005+
mux.HandleFunc("/app", func(w http.ResponseWriter, _ *http.Request) {
2006+
_, _ = fmt.Fprint(w, `{"slug": "", "name": "My GitHub App"}`)
2007+
})
2008+
},
2009+
wantSlug: "",
2010+
},
2011+
{
2012+
name: "app endpoint returns malformed JSON",
2013+
privateKey: []byte(fakePrivateKey),
2014+
applicationID: testAppID,
2015+
setupMux: func(mux *http.ServeMux) {
2016+
mux.HandleFunc("/app", func(w http.ResponseWriter, _ *http.Request) {
2017+
_, _ = fmt.Fprint(w, `{invalid json`)
2018+
})
2019+
},
2020+
wantErrSubstring: "failed to get app info",
2021+
},
2022+
}
2023+
2024+
for _, tt := range tests {
2025+
t.Run(tt.name, func(t *testing.T) {
2026+
_, mux, serverURL, teardown := ghtesthelper.SetupGH()
2027+
defer teardown()
2028+
2029+
if tt.setupMux != nil {
2030+
tt.setupMux(mux)
2031+
}
2032+
2033+
// Set up context with namespace
2034+
ctx, _ := rtesting.SetupFakeContext(t)
2035+
testNamespace := &corev1.Namespace{
2036+
ObjectMeta: metav1.ObjectMeta{
2037+
Name: "test-namespace",
2038+
},
2039+
}
2040+
ctx = info.StoreNS(ctx, testNamespace.GetName())
2041+
2042+
// Create a secret with the test application ID and private key
2043+
appIDBytes := make([]byte, 0, 20)
2044+
appIDBytes = fmt.Appendf(appIDBytes, "%d", tt.applicationID)
2045+
testSecret := &corev1.Secret{
2046+
ObjectMeta: metav1.ObjectMeta{
2047+
Name: "pac-secret",
2048+
Namespace: testNamespace.GetName(),
2049+
},
2050+
Data: map[string][]byte{
2051+
"github-application-id": appIDBytes,
2052+
"github-private-key": tt.privateKey,
2053+
},
2054+
}
2055+
2056+
// Set up test data with kubernetes client
2057+
tdata := testclient.Data{
2058+
Namespaces: []*corev1.Namespace{testNamespace},
2059+
Secret: []*corev1.Secret{testSecret},
2060+
}
2061+
stdata, _ := testclient.SeedTestData(t, ctx, tdata)
2062+
2063+
// Create a provider with mocked kubernetes client
2064+
provider := &Provider{}
2065+
provider.Run = &params.Run{
2066+
Clients: clients.Clients{
2067+
Kube: stdata.Kube,
2068+
},
2069+
Info: info.Info{
2070+
Controller: &info.ControllerInfo{
2071+
Secret: testSecret.GetName(),
2072+
},
2073+
},
2074+
}
2075+
2076+
slug, err := provider.fetchAppSlug(ctx, serverURL)
2077+
2078+
if tt.wantErrSubstring != "" {
2079+
assert.Assert(t, err != nil, "expected error but got none")
2080+
assert.ErrorContains(t, err, tt.wantErrSubstring)
2081+
return
2082+
}
2083+
2084+
assert.NilError(t, err)
2085+
assert.Equal(t, slug, tt.wantSlug)
2086+
})
2087+
}
2088+
}

0 commit comments

Comments
 (0)
⚡