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

vulnerability evaluator: Submit reviews with just comment if the author is the same as the mediator identity #952

Merged
merged 1 commit into from
Sep 15, 2023
Merged

vulnerability evaluator: Submit reviews with just comment if the author is the same as the mediator identity #952

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
1 change: 1 addition & 0 deletions docs/docs/protodocs/proto.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 9 additions & 3 deletions internal/controlplane/handlers_githubwebhooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,10 +802,16 @@ func getPullRequestInfoFromPayload(
return nil, fmt.Errorf("error getting pull request number from payload: %w", err)
}

prAuthorId, err := util.JQReadFrom[float64](ctx, ".pull_request.user.id", payload)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is reading a float64 and then storing it as an int64. Can we extract this as an int64? I doubt we'll lose precision, but it makes me nervous.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I think that must by copy-pasta. Let me look into changing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this is indeed copy-paste, we use float64 everywhere in the Go accessor and also used float64 before we introduced the go-generics-powered JQReadFrom.

However, just trying to convert to int64 gives me an error like:

could not type assert 1 to int64

which is coming from:

       out, ok := outAny.(T)
	if !ok {
		return out, fmt.Errorf("could not type assert %v to %v", outAny, reflect.TypeOf(out))
	}

I'll admit I don't know why doesn't Go like this type conversion, do you have some idea? I wonder if it's because the JQ library might be using encoding/json under the hood which says in the docs that the default value for numerical types in float64?

But at the same time, plain ol' int seems to work fine, so maybe I'm just Go-impaired and don't see the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with adding a TODO if it requires a lot of debugging. Just keeping track of where we have this debt will be the first step in cleaning it up in December.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you I filed #965 and will move on with this PR.

if err != nil {
return nil, fmt.Errorf("error getting pull request author ID from payload: %w", err)
}

return &pb.PullRequest{
Url: prUrl,
Number: int32(prNumber),
Patches: nil, // to be filled later with a separate call
Url: prUrl,
Number: int32(prNumber),
AuthorId: int64(prAuthorId),
Patches: nil, // to be filled later with a separate call
}, nil
}

Expand Down
28 changes: 23 additions & 5 deletions internal/engine/eval/vulncheck/review.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ type reviewPrHandler struct {
pr *pb.PullRequest

mediatorReview *github.PullRequestReview
failStatus *string

comments []*github.DraftReviewComment
status *string
Expand All @@ -157,11 +158,28 @@ func newReviewPrHandler(
Str("repo-name", pr.RepoName).
Logger()

cliUser, err := cli.GetAuthenticatedUser(ctx)
if err != nil {
return nil, fmt.Errorf("could not get authenticated user: %w", err)
}

// if the user wants mediator to request changes on a pull request, they need to
// be different identities
var failStatus *string
if pr.AuthorId == cliUser.GetID() {
failStatus = github.String("COMMENT")
logger.Debug().Msg("author is the same as the authenticated user, can only comment")
} else {
failStatus = github.String("REQUEST_CHANGES")
logger.Debug().Msg("author is different than the authenticated user, can request changes")
}

return &reviewPrHandler{
cli: cli,
pr: pr,
comments: []*github.DraftReviewComment{},
logger: logger,
cli: cli,
pr: pr,
comments: []*github.DraftReviewComment{},
logger: logger,
failStatus: failStatus,
}, nil
}

Expand Down Expand Up @@ -219,7 +237,7 @@ func (ra *reviewPrHandler) submit(ctx context.Context) error {
func (ra *reviewPrHandler) setStatus() {
if len(ra.comments) > 0 {
// if this pass produced comments, request changes
ra.status = github.String("REQUEST_CHANGES")
ra.status = ra.failStatus
ra.text = github.String(reviewBodyRequestChangesCommentText)
} else {
// if this pass produced no comments, resolve the mediator review
Expand Down
Loading