Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PullRequestCommenter provider trait #5188

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add PullRequestCommenter provider trait #5188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/engine/actions/alert/alert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func NewRuleAlert(
if alertCfg.GetPullRequestComment() == nil {
return nil, fmt.Errorf("alert engine missing pull_request_review configuration")
}
client, err := provinfv1.As[provinfv1.GitHub](provider)
client, err := provinfv1.As[provinfv1.PullRequestCommenter](provider)
if err != nil {
zerolog.Ctx(ctx).Debug().Str("rule-type", ruletype.GetName()).
Msg("provider is not a GitHub provider. Silently skipping alerts.")
Expand Down
144 changes: 51 additions & 93 deletions internal/engine/actions/alert/pull_request_comment/pull_request_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,7 @@ package pull_request_comment
import (
"context"
"encoding/json"
"errors"
"fmt"
"math"
"strconv"
"time"

"github.com/google/go-github/v63/github"
"github.com/rs/zerolog"
Expand All @@ -21,6 +17,7 @@ import (
"github.com/mindersec/minder/internal/db"
enginerr "github.com/mindersec/minder/internal/engine/errors"
"github.com/mindersec/minder/internal/engine/interfaces"
"github.com/mindersec/minder/internal/entities/properties"
pbinternal "github.com/mindersec/minder/internal/proto"
"github.com/mindersec/minder/internal/util"
pb "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
Expand All @@ -39,7 +36,7 @@ const (
// Alert is the structure backing the noop alert
type Alert struct {
actionType interfaces.ActionType
gh provifv1.GitHub
commenter provifv1.PullRequestCommenter
reviewCfg *pb.RuleType_Definition_Alert_AlertTypePRComment
setting models.ActionOpt
}
Expand All @@ -54,26 +51,17 @@ type PrCommentTemplateParams struct {
}

type paramsPR struct {
Owner string
Repo string
CommitSha string
Number int
Comment string
Metadata *alertMetadata
props *properties.Properties
Metadata *provifv1.CommentResultMeta
prevStatus *db.ListRuleEvaluationsByProfileIdRow
}

type alertMetadata struct {
ReviewID string `json:"review_id,omitempty"`
SubmittedAt *time.Time `json:"submitted_at,omitempty"`
PullRequestUrl *string `json:"pull_request_url,omitempty"`
}

// NewPullRequestCommentAlert creates a new pull request comment alert action
func NewPullRequestCommentAlert(
actionType interfaces.ActionType,
reviewCfg *pb.RuleType_Definition_Alert_AlertTypePRComment,
gh provifv1.GitHub,
gh provifv1.PullRequestCommenter,
setting models.ActionOpt,
) (*Alert, error) {
if actionType == "" {
Expand All @@ -82,7 +70,7 @@ func NewPullRequestCommentAlert(

return &Alert{
actionType: actionType,
gh: gh,
commenter: gh,
reviewCfg: reviewCfg,
setting: setting,
}, nil
Expand Down Expand Up @@ -134,70 +122,20 @@ func (alert *Alert) Do(
}

func (alert *Alert) run(ctx context.Context, params *paramsPR, cmd interfaces.ActionCmd) (json.RawMessage, error) {
logger := zerolog.Ctx(ctx)

// Process the command
switch cmd {
// Create a review
case interfaces.ActionCmdOn:
review := &github.PullRequestReviewRequest{
CommitID: github.String(params.CommitSha),
Event: github.String("COMMENT"),
Body: github.String(params.Comment),
}

r, err := alert.gh.CreateReview(
ctx,
params.Owner,
params.Repo,
params.Number,
review,
)
if err != nil {
return nil, fmt.Errorf("error creating PR review: %w, %w", err, enginerr.ErrActionFailed)
}

newMeta, err := json.Marshal(alertMetadata{
ReviewID: strconv.FormatInt(r.GetID(), 10),
SubmittedAt: r.SubmittedAt.GetTime(),
PullRequestUrl: r.PullRequestURL,
})
if err != nil {
return nil, fmt.Errorf("error marshalling alert metadata json: %w", err)
}

logger.Info().Int64("review_id", *r.ID).Msg("PR review created")
return newMeta, nil
// Dismiss the review
// Create a review
return alert.runDoReview(ctx, params)
case interfaces.ActionCmdOff:
if params.Metadata == nil {
// We cannot do anything without the PR review ID, so we assume that turning the alert off is a success
return nil, fmt.Errorf("no PR comment ID provided: %w", enginerr.ErrActionTurnedOff)
}

reviewID, err := strconv.ParseInt(params.Metadata.ReviewID, 10, 64)
if err != nil {
zerolog.Ctx(ctx).Error().Err(err).Str("review_id", params.Metadata.ReviewID).Msg("failed to parse review_id")
return nil, fmt.Errorf("no PR comment ID provided: %w, %w", err, enginerr.ErrActionTurnedOff)
}

_, err = alert.gh.DismissReview(ctx, params.Owner, params.Repo, params.Number, reviewID,
&github.PullRequestReviewDismissalRequest{
Message: github.String("Dismissed due to alert being turned off"),
})
if err != nil {
if errors.Is(err, enginerr.ErrNotFound) {
// There's no PR review with that ID anymore.
// We exit by stating that the action was turned off.
return nil, fmt.Errorf("PR comment already dismissed: %w, %w", err, enginerr.ErrActionTurnedOff)
}
return nil, fmt.Errorf("error dismissing PR comment: %w, %w", err, enginerr.ErrActionFailed)
}
logger.Info().Str("review_id", params.Metadata.ReviewID).Msg("PR comment dismissed")
// Success - return ErrActionTurnedOff to indicate the action was successful
return nil, fmt.Errorf("%s : %w", alert.Class(), enginerr.ErrActionTurnedOff)
return json.RawMessage(`{}`), nil
case interfaces.ActionCmdDoNothing:
// Return the previous alert status.
// If the previous status didn't change (still a failure, for instance) we
// want to refresh the alert.
if alert.setting == models.ActionOptOn {
return alert.runDoReview(ctx, params)
}
// Else, we just do nothing.
return alert.runDoNothing(ctx, params)
}
return nil, enginerr.ErrActionSkipped
Expand All @@ -211,16 +149,16 @@ func (alert *Alert) runDry(ctx context.Context, params *paramsPR, cmd interfaces
switch cmd {
case interfaces.ActionCmdOn:
body := github.String(params.Comment)
logger.Info().Msgf("dry run: create a PR comment on PR %d in repo %s/%s with the following body: %s",
params.Number, params.Owner, params.Repo, *body)
logger.Info().Dict("properties", params.props.ToLogDict()).
Msgf("dry run: create a PR comment on PR with body: %s", *body)
return nil, nil
case interfaces.ActionCmdOff:
if params.Metadata == nil {
// We cannot do anything without the PR review ID, so we assume that turning the alert off is a success
return nil, fmt.Errorf("no PR comment ID provided: %w", enginerr.ErrActionTurnedOff)
}
logger.Info().Msgf("dry run: dismiss PR comment %s on PR PR %d in repo %s/%s", params.Metadata.ReviewID,
params.Number, params.Owner, params.Repo)
logger.Info().Dict("properties", params.props.ToLogDict()).
Msgf("dry run: dismiss PR comment on PR")
case interfaces.ActionCmdDoNothing:
// Return the previous alert status.
return alert.runDoNothing(ctx, params)
Expand All @@ -231,7 +169,7 @@ func (alert *Alert) runDry(ctx context.Context, params *paramsPR, cmd interfaces

// runDoNothing returns the previous alert status
func (_ *Alert) runDoNothing(ctx context.Context, params *paramsPR) (json.RawMessage, error) {
logger := zerolog.Ctx(ctx).With().Str("repo", params.Repo).Logger()
logger := zerolog.Ctx(ctx).With().Dict("properties", params.props.ToLogDict()).Logger()

logger.Debug().Msg("Running do nothing")

Expand All @@ -245,6 +183,30 @@ func (_ *Alert) runDoNothing(ctx context.Context, params *paramsPR) (json.RawMes
return nil, err
}

func (alert *Alert) runDoReview(ctx context.Context, params *paramsPR) (json.RawMessage, error) {
logger := zerolog.Ctx(ctx)

r, err := alert.commenter.CommentOnPullRequest(ctx, params.props, provifv1.PullRequestCommentInfo{
// TODO: We should add the header to identify the alert. We could use the
// rule type name.
Commit: params.props.GetProperty(properties.PullRequestCommitSHA).GetString(),
Body: params.Comment,
// TODO: Determine the priority from the rule type severity
})
if err != nil {
return nil, fmt.Errorf("error creating PR review: %w, %w", err, enginerr.ErrActionFailed)
}
logger.Info().Str("review_id", r.ID).Msg("PR review created")

// serialize r to json
meta, err := json.Marshal(r)
if err != nil {
return nil, fmt.Errorf("error marshalling PR review metadata: %w", err)
}

return meta, nil
}

// getParamsForSecurityAdvisory extracts the details from the entity
func (alert *Alert) getParamsForPRComment(
ctx context.Context,
Expand All @@ -253,19 +215,15 @@ func (alert *Alert) getParamsForPRComment(
metadata *json.RawMessage,
) (*paramsPR, error) {
logger := zerolog.Ctx(ctx)
result := &paramsPR{
prevStatus: params.GetEvalStatusFromDb(),
Owner: pr.GetRepoOwner(),
Repo: pr.GetRepoName(),
CommitSha: pr.GetCommitSha(),
props, err := properties.NewProperties(pr.GetProperties().AsMap())
if err != nil {
return nil, fmt.Errorf("error creating properties: %w", err)
}

// The GitHub Go API takes an int32, but our proto stores an int64; make sure we don't overflow
// The PR number is an int in GitHub and Gitlab; in practice overflow will never happen.
if pr.Number > math.MaxInt {
return nil, fmt.Errorf("pr number is too large")
result := &paramsPR{
prevStatus: params.GetEvalStatusFromDb(),
props: props,
}
result.Number = int(pr.Number)

commentTmpl, err := util.NewSafeHTMLTemplate(&alert.reviewCfg.ReviewMessage, "message")
if err != nil {
Expand All @@ -289,7 +247,7 @@ func (alert *Alert) getParamsForPRComment(

// Unmarshal the existing alert metadata, if any
if metadata != nil {
meta := &alertMetadata{}
meta := &provifv1.CommentResultMeta{}
err := json.Unmarshal(*metadata, meta)
if err != nil {
// There's nothing saved apparently, so no need to fail here, but do log the error
Expand Down
26 changes: 13 additions & 13 deletions internal/engine/actions/alert/pull_request_comment/pull_request_comment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ import (
enginerr "github.com/mindersec/minder/internal/engine/errors"
engif "github.com/mindersec/minder/internal/engine/interfaces"
pbinternal "github.com/mindersec/minder/internal/proto"
mockghclient "github.com/mindersec/minder/internal/providers/github/mock"
pb "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
"github.com/mindersec/minder/pkg/engine/v1/interfaces"
"github.com/mindersec/minder/pkg/profiles/models"
mockcommenter "github.com/mindersec/minder/pkg/providers/v1/mock"
)

var TestActionTypeValid engif.ActionType = "alert-test"
Expand All @@ -44,7 +44,7 @@ func TestPullRequestCommentAlert(t *testing.T) {
cmd engif.ActionCmd
reviewMsg string
inputMetadata *json.RawMessage
mockSetup func(*mockghclient.MockGitHub)
mockSetup func(commenter *mockcommenter.MockPullRequestCommenter)
expectedErr error
expectedMetadata json.RawMessage
}{
Expand All @@ -53,9 +53,9 @@ func TestPullRequestCommentAlert(t *testing.T) {
actionType: TestActionTypeValid,
reviewMsg: "This is a constant review message",
cmd: engif.ActionCmdOn,
mockSetup: func(mockGitHub *mockghclient.MockGitHub) {
mockSetup: func(mockGitHub *mockcommenter.MockPullRequestCommenter) {
mockGitHub.EXPECT().
CreateReview(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
CommentOnPullRequest(gomock.Any(), gomock.Any(), gomock.Any()).
Return(&github.PullRequestReview{ID: &reviewID}, nil)
},
expectedMetadata: json.RawMessage(fmt.Sprintf(`{"review_id":"%s"}`, reviewIDStr)),
Expand All @@ -65,9 +65,9 @@ func TestPullRequestCommentAlert(t *testing.T) {
actionType: TestActionTypeValid,
reviewMsg: "{{ .EvalErrorDetails }}",
cmd: engif.ActionCmdOn,
mockSetup: func(mockGitHub *mockghclient.MockGitHub) {
mockSetup: func(mockGitHub *mockcommenter.MockPullRequestCommenter) {
mockGitHub.EXPECT().
CreateReview(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.AssignableToTypeOf(&github.PullRequestReviewRequest{})).
CommentOnPullRequest(gomock.Any(), gomock.Any(), gomock.Any()).
DoAndReturn(validateReviewBodyAndReturn(evaluationFailureDetails, reviewID))
},
expectedMetadata: json.RawMessage(fmt.Sprintf(`{"review_id":"%s"}`, reviewIDStr)),
Expand All @@ -77,9 +77,9 @@ func TestPullRequestCommentAlert(t *testing.T) {
actionType: TestActionTypeValid,
reviewMsg: "{{ .EvalResultOutput.ViolationMsg }}",
cmd: engif.ActionCmdOn,
mockSetup: func(mockGitHub *mockghclient.MockGitHub) {
mockSetup: func(mockGitHub *mockcommenter.MockPullRequestCommenter) {
mockGitHub.EXPECT().
CreateReview(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.AssignableToTypeOf(&github.PullRequestReviewRequest{})).
CommentOnPullRequest(gomock.Any(), gomock.Any(), gomock.Any()).
DoAndReturn(validateReviewBodyAndReturn(violationMsg, reviewID))
},
expectedMetadata: json.RawMessage(fmt.Sprintf(`{"review_id":"%s"}`, reviewIDStr)),
Expand All @@ -89,9 +89,9 @@ func TestPullRequestCommentAlert(t *testing.T) {
actionType: TestActionTypeValid,
reviewMsg: "This is a constant review message",
cmd: engif.ActionCmdOn,
mockSetup: func(mockGitHub *mockghclient.MockGitHub) {
mockSetup: func(mockGitHub *mockcommenter.MockPullRequestCommenter) {
mockGitHub.EXPECT().
CreateReview(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
CommentOnPullRequest(gomock.Any(), gomock.Any(), gomock.Any()).
Return(nil, fmt.Errorf("failed to create PR comment"))
},
expectedErr: enginerr.ErrActionFailed,
Expand All @@ -102,9 +102,9 @@ func TestPullRequestCommentAlert(t *testing.T) {
reviewMsg: "This is a constant review message",
cmd: engif.ActionCmdOff,
inputMetadata: &successfulRunMetadata,
mockSetup: func(mockGitHub *mockghclient.MockGitHub) {
mockSetup: func(mockGitHub *mockcommenter.MockPullRequestCommenter) {
mockGitHub.EXPECT().
DismissReview(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
CommentOnPullRequest(gomock.Any(), gomock.Any(), gomock.Any()).
Return(&github.PullRequestReview{}, nil)
},
expectedErr: enginerr.ErrActionTurnedOff,
Expand All @@ -126,7 +126,7 @@ func TestPullRequestCommentAlert(t *testing.T) {
ReviewMessage: tt.reviewMsg,
}

mockClient := mockghclient.NewMockGitHub(ctrl)
mockClient := mockcommenter.NewMockPullRequestCommenter(ctrl)
tt.mockSetup(mockClient)

prCommentAlert, err := NewPullRequestCommentAlert(
Expand Down
14 changes: 14 additions & 0 deletions internal/providers/github/clients/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,20 @@ func (g *GitHubAppDelegate) GetUserId(ctx context.Context) (int64, error) {
return user.GetID(), nil
}

// GetMinderUserId returns the user id for the GitHub App user
func (g *GitHubAppDelegate) GetMinderUserId(ctx context.Context) (int64, error) {
// Try to get this user ID from the GitHub API
//nolint:errcheck // this will never error
appUserName, _ := g.GetName(ctx)
user, _, err := g.client.Users.Get(ctx, appUserName)
if err != nil {
// Fallback to the configured user ID
// note: this is different from the App ID
return g.defaultUserId, nil
}
return user.GetID(), nil
}

// GetName returns the username for the GitHub App user
func (g *GitHubAppDelegate) GetName(_ context.Context) (string, error) {
return fmt.Sprintf("%s[bot]", g.appName), nil
Expand Down
9 changes: 9 additions & 0 deletions internal/providers/github/clients/oauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,15 @@ func (o *GitHubOAuthDelegate) GetUserId(ctx context.Context) (int64, error) {
return user.GetID(), nil
}

// GetMinderUserId returns the user id for the authenticated user
func (o *GitHubOAuthDelegate) GetMinderUserId(ctx context.Context) (int64, error) {
user, _, err := o.client.Users.Get(ctx, "")
if err != nil {
return 0, err
}
return user.GetID(), nil
}

// GetName returns the username for the authenticated user
func (o *GitHubOAuthDelegate) GetName(ctx context.Context) (string, error) {
user, _, err := o.client.Users.Get(ctx, "")
Expand Down
Loading
Loading