From b4041b89193d46a113c053fd515e9ef02e60b5cf Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 18 Dec 2023 09:58:50 +0100 Subject: [PATCH] Use a dedicated GH API call to get the e-mail address Depending on the privacy settings, the `/user` endpoint might or might not contain the primary e-mail even if the user makes an authenticated call on behalf of themselves. Let's use the `/email` API endpoint instead. Fixes: #1950 --- .../remediate/pull_request/pull_request.go | 26 ++++++++++++++++++- .../pull_request/pull_request_test.go | 15 +++++++++++ internal/providers/github/github_rest.go | 9 +++++++ internal/providers/github/mock/github.go | 15 +++++++++++ pkg/api/openapi/minder/v1/minder.swagger.json | 6 ++--- pkg/api/protobuf/go/minder/v1/minder.pb.go | 2 +- pkg/providers/v1/providers.go | 1 + 7 files changed, 69 insertions(+), 5 deletions(-) diff --git a/internal/engine/actions/remediate/pull_request/pull_request.go b/internal/engine/actions/remediate/pull_request/pull_request.go index 6330aad4ef..33e188e78c 100644 --- a/internal/engine/actions/remediate/pull_request/pull_request.go +++ b/internal/engine/actions/remediate/pull_request/pull_request.go @@ -262,6 +262,11 @@ func (r *Remediator) runGit( return fmt.Errorf("cannot get authenticated user: %w", err) } + email, err := getPrimaryEmail(ctx, r.ghCli) + if err != nil { + return fmt.Errorf("cannot get primary email: %w", err) + } + logger.Debug().Str("branch", branchBaseName(title)).Msg("Checking out branch") err = wt.Checkout(&git.CheckoutOptions{ Branch: plumbing.NewBranchReferenceName(branchBaseName(title)), @@ -288,7 +293,7 @@ func (r *Remediator) runGit( _, err = wt.Commit(title, &git.CommitOptions{ Author: &object.Signature{ Name: u.GetName(), - Email: u.GetEmail(), + Email: email, When: time.Now(), }, }) @@ -484,3 +489,22 @@ func prFromBranchAlreadyExists( return len(openPrs) > 0, nil } + +func getPrimaryEmail(ctx context.Context, cli provifv1.GitHub) (string, error) { + emails, err := cli.ListEmails(ctx, &github.ListOptions{}) + if err != nil { + return "", fmt.Errorf("cannot get email: %w", err) + } + + fallback := "" + for _, email := range emails { + if fallback == "" { + fallback = email.GetEmail() + } + if email.GetPrimary() { + return email.GetEmail(), nil + } + } + + return fallback, nil +} diff --git a/internal/engine/actions/remediate/pull_request/pull_request_test.go b/internal/engine/actions/remediate/pull_request/pull_request_test.go index 7792c3a11b..4d0a1ea401 100644 --- a/internal/engine/actions/remediate/pull_request/pull_request_test.go +++ b/internal/engine/actions/remediate/pull_request/pull_request_test.go @@ -200,6 +200,13 @@ func happyPathMockSetup(mockGitHub *mock_ghclient.MockGitHub) { Email: github.String("test@stacklok.com"), Login: github.String("stacklok-bot"), }, nil) + mockGitHub.EXPECT(). + ListEmails(gomock.Any(), gomock.Any()).Return([]*github.UserEmail{ + { + Email: github.String("test@stacklok.com"), + Primary: github.Bool(true), + }, + }, nil) mockGitHub.EXPECT(). GetToken().Return("token") mockGitHub.EXPECT(). @@ -464,6 +471,14 @@ func TestPullRequestRemediate(t *testing.T) { Email: github.String("test@stacklok.com"), Login: github.String("stacklok-bot"), }, nil) + // likewise we need to update the branch with a valid e-mail + mockGitHub.EXPECT(). + ListEmails(gomock.Any(), gomock.Any()).Return([]*github.UserEmail{ + { + Email: github.String("test@stacklok.com"), + Primary: github.Bool(true), + }, + }, nil) mockGitHub.EXPECT(). GetToken().Return("token") // this is the last call we expect to make. It returns existing PRs from this branch, so we diff --git a/internal/providers/github/github_rest.go b/internal/providers/github/github_rest.go index bcf29481b7..85d197fe75 100644 --- a/internal/providers/github/github_rest.go +++ b/internal/providers/github/github_rest.go @@ -614,3 +614,12 @@ func (c *RestClient) CreateComment(ctx context.Context, owner, repo string, numb }) return err } + +// ListEmails lists all emails for the authenticated user. +func (c *RestClient) ListEmails(ctx context.Context, opts *github.ListOptions) ([]*github.UserEmail, error) { + emails, _, err := c.client.Users.ListEmails(ctx, opts) + if err != nil { + return nil, err + } + return emails, nil +} diff --git a/internal/providers/github/mock/github.go b/internal/providers/github/mock/github.go index 932f9da488..0c71f1f534 100644 --- a/internal/providers/github/mock/github.go +++ b/internal/providers/github/mock/github.go @@ -599,6 +599,21 @@ func (mr *MockGitHubMockRecorder) ListAllRepositories(arg0, arg1, arg2 interface return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListAllRepositories", reflect.TypeOf((*MockGitHub)(nil).ListAllRepositories), arg0, arg1, arg2) } +// ListEmails mocks base method. +func (m *MockGitHub) ListEmails(ctx context.Context, opts *github.ListOptions) ([]*github.UserEmail, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ListEmails", ctx, opts) + ret0, _ := ret[0].([]*github.UserEmail) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ListEmails indicates an expected call of ListEmails. +func (mr *MockGitHubMockRecorder) ListEmails(ctx, opts interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListEmails", reflect.TypeOf((*MockGitHub)(nil).ListEmails), ctx, opts) +} + // ListFiles mocks base method. func (m *MockGitHub) ListFiles(ctx context.Context, owner, repo string, prNumber, perPage, pageNumber int) ([]*github.CommitFile, *github.Response, error) { m.ctrl.T.Helper() diff --git a/pkg/api/openapi/minder/v1/minder.swagger.json b/pkg/api/openapi/minder/v1/minder.swagger.json index bf781f249e..d3c0fcbcef 100644 --- a/pkg/api/openapi/minder/v1/minder.swagger.json +++ b/pkg/api/openapi/minder/v1/minder.swagger.json @@ -1965,11 +1965,11 @@ "properties": { "@type": { "type": "string", - "description": "A URL/resource name that uniquely identifies the type of the serialized\nprotocol buffer message. This string must contain at least\none \"/\" character. The last segment of the URL's path must represent\nthe fully qualified name of the type (as in\n`path/google.protobuf.Duration`). The name should be in a canonical form\n(e.g., leading \".\" is not accepted).\n\nIn practice, teams usually precompile into the binary all types that they\nexpect it to use in the context of Any. However, for URLs which use the\nscheme `http`, `https`, or no scheme, one can optionally set up a type\nserver that maps type URLs to message definitions as follows:\n\n* If no scheme is provided, `https` is assumed.\n* An HTTP GET on the URL must yield a [google.protobuf.Type][]\n value in binary format, or produce an error.\n* Applications are allowed to cache lookup results based on the\n URL, or have them precompiled into a binary to avoid any\n lookup. Therefore, binary compatibility needs to be preserved\n on changes to types. (Use versioned type names to manage\n breaking changes.)\n\nNote: this functionality is not currently available in the official\nprotobuf release, and it is not used for type URLs beginning with\ntype.googleapis.com. As of May 2023, there are no widely used type server\nimplementations and no plans to implement one.\n\nSchemes other than `http`, `https` (or the empty scheme) might be\nused with implementation specific semantics." + "description": "A URL/resource name that uniquely identifies the type of the serialized\nprotocol buffer message. This string must contain at least\none \"/\" character. The last segment of the URL's path must represent\nthe fully qualified name of the type (as in\n`path/google.protobuf.Duration`). The name should be in a canonical form\n(e.g., leading \".\" is not accepted).\n\nIn practice, teams usually precompile into the binary all types that they\nexpect it to use in the context of Any. However, for URLs which use the\nscheme `http`, `https`, or no scheme, one can optionally set up a type\nserver that maps type URLs to message definitions as follows:\n\n* If no scheme is provided, `https` is assumed.\n* An HTTP GET on the URL must yield a [google.protobuf.Type][]\n value in binary format, or produce an error.\n* Applications are allowed to cache lookup results based on the\n URL, or have them precompiled into a binary to avoid any\n lookup. Therefore, binary compatibility needs to be preserved\n on changes to types. (Use versioned type names to manage\n breaking changes.)\n\nNote: this functionality is not currently available in the official\nprotobuf release, and it is not used for type URLs beginning with\ntype.googleapis.com.\n\nSchemes other than `http`, `https` (or the empty scheme) might be\nused with implementation specific semantics." } }, "additionalProperties": {}, - "description": "`Any` contains an arbitrary serialized protocol buffer message along with a\nURL that describes the type of the serialized message.\n\nProtobuf library provides support to pack/unpack Any values in the form\nof utility functions or additional generated methods of the Any type.\n\nExample 1: Pack and unpack a message in C++.\n\n Foo foo = ...;\n Any any;\n any.PackFrom(foo);\n ...\n if (any.UnpackTo(\u0026foo)) {\n ...\n }\n\nExample 2: Pack and unpack a message in Java.\n\n Foo foo = ...;\n Any any = Any.pack(foo);\n ...\n if (any.is(Foo.class)) {\n foo = any.unpack(Foo.class);\n }\n // or ...\n if (any.isSameTypeAs(Foo.getDefaultInstance())) {\n foo = any.unpack(Foo.getDefaultInstance());\n }\n\n Example 3: Pack and unpack a message in Python.\n\n foo = Foo(...)\n any = Any()\n any.Pack(foo)\n ...\n if any.Is(Foo.DESCRIPTOR):\n any.Unpack(foo)\n ...\n\n Example 4: Pack and unpack a message in Go\n\n foo := \u0026pb.Foo{...}\n any, err := anypb.New(foo)\n if err != nil {\n ...\n }\n ...\n foo := \u0026pb.Foo{}\n if err := any.UnmarshalTo(foo); err != nil {\n ...\n }\n\nThe pack methods provided by protobuf library will by default use\n'type.googleapis.com/full.type.name' as the type URL and the unpack\nmethods only use the fully qualified type name after the last '/'\nin the type URL, for example \"foo.bar.com/x/y.z\" will yield type\nname \"y.z\".\n\nJSON\n====\nThe JSON representation of an `Any` value uses the regular\nrepresentation of the deserialized, embedded message, with an\nadditional field `@type` which contains the type URL. Example:\n\n package google.profile;\n message Person {\n string first_name = 1;\n string last_name = 2;\n }\n\n {\n \"@type\": \"type.googleapis.com/google.profile.Person\",\n \"firstName\": \u003cstring\u003e,\n \"lastName\": \u003cstring\u003e\n }\n\nIf the embedded message type is well-known and has a custom JSON\nrepresentation, that representation will be embedded adding a field\n`value` which holds the custom JSON in addition to the `@type`\nfield. Example (for message [google.protobuf.Duration][]):\n\n {\n \"@type\": \"type.googleapis.com/google.protobuf.Duration\",\n \"value\": \"1.212s\"\n }" + "description": "`Any` contains an arbitrary serialized protocol buffer message along with a\nURL that describes the type of the serialized message.\n\nProtobuf library provides support to pack/unpack Any values in the form\nof utility functions or additional generated methods of the Any type.\n\nExample 1: Pack and unpack a message in C++.\n\n Foo foo = ...;\n Any any;\n any.PackFrom(foo);\n ...\n if (any.UnpackTo(\u0026foo)) {\n ...\n }\n\nExample 2: Pack and unpack a message in Java.\n\n Foo foo = ...;\n Any any = Any.pack(foo);\n ...\n if (any.is(Foo.class)) {\n foo = any.unpack(Foo.class);\n }\n // or ...\n if (any.isSameTypeAs(Foo.getDefaultInstance())) {\n foo = any.unpack(Foo.getDefaultInstance());\n }\n\nExample 3: Pack and unpack a message in Python.\n\n foo = Foo(...)\n any = Any()\n any.Pack(foo)\n ...\n if any.Is(Foo.DESCRIPTOR):\n any.Unpack(foo)\n ...\n\nExample 4: Pack and unpack a message in Go\n\n foo := \u0026pb.Foo{...}\n any, err := anypb.New(foo)\n if err != nil {\n ...\n }\n ...\n foo := \u0026pb.Foo{}\n if err := any.UnmarshalTo(foo); err != nil {\n ...\n }\n\nThe pack methods provided by protobuf library will by default use\n'type.googleapis.com/full.type.name' as the type URL and the unpack\nmethods only use the fully qualified type name after the last '/'\nin the type URL, for example \"foo.bar.com/x/y.z\" will yield type\nname \"y.z\".\n\nJSON\n\nThe JSON representation of an `Any` value uses the regular\nrepresentation of the deserialized, embedded message, with an\nadditional field `@type` which contains the type URL. Example:\n\n package google.profile;\n message Person {\n string first_name = 1;\n string last_name = 2;\n }\n\n {\n \"@type\": \"type.googleapis.com/google.profile.Person\",\n \"firstName\": \u003cstring\u003e,\n \"lastName\": \u003cstring\u003e\n }\n\nIf the embedded message type is well-known and has a custom JSON\nrepresentation, that representation will be embedded adding a field\n`value` which holds the custom JSON in addition to the `@type`\nfield. Example (for message [google.protobuf.Duration][]):\n\n {\n \"@type\": \"type.googleapis.com/google.protobuf.Duration\",\n \"value\": \"1.212s\"\n }" }, "protobufNullValue": { "type": "string", @@ -1977,7 +1977,7 @@ "NULL_VALUE" ], "default": "NULL_VALUE", - "description": "`NullValue` is a singleton enumeration to represent the null value for the\n`Value` type union.\n\nThe JSON representation for `NullValue` is JSON `null`.\n\n - NULL_VALUE: Null value." + "description": "`NullValue` is a singleton enumeration to represent the null value for the\n`Value` type union.\n\n The JSON representation for `NullValue` is JSON `null`.\n\n - NULL_VALUE: Null value." }, "v1Artifact": { "type": "object", diff --git a/pkg/api/protobuf/go/minder/v1/minder.pb.go b/pkg/api/protobuf/go/minder/v1/minder.pb.go index 9589724b84..3f5acae8d9 100644 --- a/pkg/api/protobuf/go/minder/v1/minder.pb.go +++ b/pkg/api/protobuf/go/minder/v1/minder.pb.go @@ -15,7 +15,7 @@ // Code generated by protoc-gen-go. DO NOT EDIT. // versions: -// protoc-gen-go v1.31.0-devel +// protoc-gen-go v1.31.0 // protoc (unknown) // source: minder/v1/minder.proto diff --git a/pkg/providers/v1/providers.go b/pkg/providers/v1/providers.go index 7a53bf5c48..f6a96d19ab 100644 --- a/pkg/providers/v1/providers.go +++ b/pkg/providers/v1/providers.go @@ -107,6 +107,7 @@ type GitHub interface { CreatePullRequest(ctx context.Context, owner, repo, title, body, head, base string) (*github.PullRequest, error) ListPullRequests(ctx context.Context, owner, repo string, opt *github.PullRequestListOptions) ([]*github.PullRequest, error) CreateComment(ctx context.Context, owner, repo string, number int, comment string) error + ListEmails(ctx context.Context, opts *github.ListOptions) ([]*github.UserEmail, error) } // ParseAndValidate parses the given provider configuration and validates it.