From 73881aef1859e68a36177a2058ddb803cc817f49 Mon Sep 17 00:00:00 2001 From: Michael Bahr <1830132+bahrmichael@users.noreply.github.com> Date: Fri, 5 Jul 2024 15:56:41 +0200 Subject: [PATCH] feat: implement functionality to create credential GitHub apps (#63635) Closes SRCH-663 This is a follow-up to previous PRs, where we added database fields to support the new github apps integration. See initiative "Batch Changes using GitHub App auth" on linear. ## Test plan - Manual testing ## Changelog --------- Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com> --- .../BatchChangesSettingsArea.story.tsx | 1 + ...tchChangesSiteConfigSettingsPage.story.tsx | 4 + .../settings/CodeHostConnectionNode.story.tsx | 1 + .../settings/RemoveCredentialModal.story.tsx | 1 + .../settings/ViewCredentialModal.story.tsx | 1 + .../enterprise/batches/settings/backend.ts | 1 + client/web/src/integration/batches.test.ts | 2 + cmd/frontend/graphqlbackend/batches.go | 4 +- cmd/frontend/graphqlbackend/batches.graphql | 5 + .../internal/batches/resolvers/BUILD.bazel | 4 +- .../internal/batches/resolvers/batch_spec.go | 1 + .../resolvers/changeset_counts_test.go | 2 +- .../internal/batches/resolvers/code_host.go | 3 +- .../internal/batches/resolvers/credential.go | 19 +- .../internal/batches/resolvers/errors.go | 22 -- .../internal/batches/resolvers/resolver.go | 201 +++++---------- .../batches/webhooks/bitbucketserver_test.go | 2 +- .../internal/batches/webhooks/github_test.go | 2 +- cmd/frontend/internal/githubapp/BUILD.bazel | 5 + cmd/frontend/internal/githubapp/httpapi.go | 185 +++++++++++--- .../internal/githubapp/httpapi_test.go | 76 ++++-- cmd/frontend/internal/githubapp/parser.go | 30 +++ .../worker/installation_backfill_test.go | 2 - internal/batches/reconciler/BUILD.bazel | 1 + internal/batches/reconciler/executor.go | 5 +- internal/batches/service/BUILD.bazel | 8 + internal/batches/service/errors.go | 25 ++ internal/batches/service/service.go | 240 +++++++++++++++++- internal/batches/service/service_test.go | 17 +- internal/batches/sources/BUILD.bazel | 1 + internal/batches/sources/sources.go | 33 ++- internal/batches/sources/sources_test.go | 18 +- internal/batches/sources/testing/BUILD.bazel | 2 +- internal/batches/sources/testing/fake.go | 6 +- internal/batches/syncer/syncer.go | 2 +- internal/batches/types/site_credential.go | 5 +- internal/database/user_credentials.go | 2 +- internal/github_apps/store/mocks_temp.go | 98 +++---- internal/github_apps/store/store.go | 39 +-- internal/github_apps/store/store_test.go | 15 +- internal/github_apps/types/BUILD.bazel | 1 + internal/github_apps/types/types.go | 8 + 42 files changed, 760 insertions(+), 340 deletions(-) create mode 100644 cmd/frontend/internal/githubapp/parser.go create mode 100644 internal/batches/service/errors.go diff --git a/client/web/src/enterprise/batches/settings/BatchChangesSettingsArea.story.tsx b/client/web/src/enterprise/batches/settings/BatchChangesSettingsArea.story.tsx index 880ea76bd96..4dff2087ccb 100644 --- a/client/web/src/enterprise/batches/settings/BatchChangesSettingsArea.story.tsx +++ b/client/web/src/enterprise/batches/settings/BatchChangesSettingsArea.story.tsx @@ -47,6 +47,7 @@ const sshCredential = (isSiteCredential: boolean): BatchChangesCredentialFields isSiteCredential, sshPublicKey: 'rsa-ssh randorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorando', + isGitHubApp: false, }) export const Overview: StoryFn = () => ( diff --git a/client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.story.tsx b/client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.story.tsx index 7ed1b13dd39..3a5db9cd180 100644 --- a/client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.story.tsx +++ b/client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.story.tsx @@ -156,6 +156,7 @@ export const ConfigAdded: StoryFn = () => ( isSiteCredential: true, sshPublicKey: 'rsa-ssh randorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorando', + isGitHubApp: false, }, externalServiceKind: ExternalServiceKind.GITHUB, externalServiceURL: 'https://github.com/', @@ -179,6 +180,7 @@ export const ConfigAdded: StoryFn = () => ( isSiteCredential: true, sshPublicKey: 'rsa-ssh randorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorando', + isGitHubApp: false, }, externalServiceKind: ExternalServiceKind.GITLAB, externalServiceURL: 'https://gitlab.com/', @@ -194,6 +196,7 @@ export const ConfigAdded: StoryFn = () => ( isSiteCredential: true, sshPublicKey: 'rsa-ssh randorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorando', + isGitHubApp: false, }, externalServiceKind: ExternalServiceKind.BITBUCKETSERVER, externalServiceURL: 'https://bitbucket.sgdev.org/', @@ -209,6 +212,7 @@ export const ConfigAdded: StoryFn = () => ( isSiteCredential: true, sshPublicKey: 'rsa-ssh randorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorando', + isGitHubApp: false, }, externalServiceKind: ExternalServiceKind.BITBUCKETCLOUD, externalServiceURL: 'https://bitbucket.org/', diff --git a/client/web/src/enterprise/batches/settings/CodeHostConnectionNode.story.tsx b/client/web/src/enterprise/batches/settings/CodeHostConnectionNode.story.tsx index 507a0fa4b93..ce87bb475d1 100644 --- a/client/web/src/enterprise/batches/settings/CodeHostConnectionNode.story.tsx +++ b/client/web/src/enterprise/batches/settings/CodeHostConnectionNode.story.tsx @@ -34,6 +34,7 @@ const sshCredential = (isSiteCredential: boolean): BatchChangesCredentialFields isSiteCredential, sshPublicKey: 'rsa-ssh randorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorando', + isGitHubApp: false, }) export const Overview: StoryFn = () => ( diff --git a/client/web/src/enterprise/batches/settings/RemoveCredentialModal.story.tsx b/client/web/src/enterprise/batches/settings/RemoveCredentialModal.story.tsx index a3f1e48459c..af9403cb21a 100644 --- a/client/web/src/enterprise/batches/settings/RemoveCredentialModal.story.tsx +++ b/client/web/src/enterprise/batches/settings/RemoveCredentialModal.story.tsx @@ -27,6 +27,7 @@ const credential = { isSiteCredential: false, sshPublicKey: 'ssh-rsa randorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorando', + isGitHubApp: false, } export const NoSsh: StoryFn = () => ( diff --git a/client/web/src/enterprise/batches/settings/ViewCredentialModal.story.tsx b/client/web/src/enterprise/batches/settings/ViewCredentialModal.story.tsx index 0588c621f1e..b8d20fb75cd 100644 --- a/client/web/src/enterprise/batches/settings/ViewCredentialModal.story.tsx +++ b/client/web/src/enterprise/batches/settings/ViewCredentialModal.story.tsx @@ -20,6 +20,7 @@ const credential: BatchChangesCredentialFields = { isSiteCredential: false, sshPublicKey: 'ssh-rsa randorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorandorando', + isGitHubApp: false, } export const View: StoryFn = () => ( diff --git a/client/web/src/enterprise/batches/settings/backend.ts b/client/web/src/enterprise/batches/settings/backend.ts index ed86e5c502e..cbed254a1ec 100644 --- a/client/web/src/enterprise/batches/settings/backend.ts +++ b/client/web/src/enterprise/batches/settings/backend.ts @@ -26,6 +26,7 @@ export const CREDENTIAL_FIELDS_FRAGMENT = gql` id sshPublicKey isSiteCredential + isGitHubApp } ` diff --git a/client/web/src/integration/batches.test.ts b/client/web/src/integration/batches.test.ts index 7cb9661f626..4b724b9b240 100644 --- a/client/web/src/integration/batches.test.ts +++ b/client/web/src/integration/batches.test.ts @@ -1007,6 +1007,7 @@ describe('Batches', () => { id: '123', isSiteCredential: false, sshPublicKey: 'ssh-rsa randorandorandorando', + isGitHubApp: false, } : null, requiresSSH: false, @@ -1025,6 +1026,7 @@ describe('Batches', () => { id: '123', isSiteCredential: false, sshPublicKey: 'ssh-rsa randorandorandorando', + isGitHubApp: false, }, } }, diff --git a/cmd/frontend/graphqlbackend/batches.go b/cmd/frontend/graphqlbackend/batches.go index 7821c6c69a5..ce53a20d412 100644 --- a/cmd/frontend/graphqlbackend/batches.go +++ b/cmd/frontend/graphqlbackend/batches.go @@ -566,11 +566,11 @@ type BatchChangesCredentialResolver interface { ID() graphql.ID ExternalServiceKind() string ExternalServiceURL() string + IsGitHubApp() bool + GitHubAppID() int SSHPublicKey(ctx context.Context) (*string, error) CreatedAt() gqlutil.DateTime IsSiteCredential() bool - - IsGitHubApp() bool } // Only GitHubApps are supported for commit signing for now. diff --git a/cmd/frontend/graphqlbackend/batches.graphql b/cmd/frontend/graphqlbackend/batches.graphql index 66d10613a9c..360462aa895 100644 --- a/cmd/frontend/graphqlbackend/batches.graphql +++ b/cmd/frontend/graphqlbackend/batches.graphql @@ -3557,6 +3557,11 @@ type BatchChangesCredential implements Node { Whether the configured credential is a site credential, that is available globally. """ isSiteCredential: Boolean! + + """ + Whether the credential is tied to a GitHub App installation. + """ + isGitHubApp: Boolean! } """ diff --git a/cmd/frontend/internal/batches/resolvers/BUILD.bazel b/cmd/frontend/internal/batches/resolvers/BUILD.bazel index 05118db2baa..765bab3779e 100644 --- a/cmd/frontend/internal/batches/resolvers/BUILD.bazel +++ b/cmd/frontend/internal/batches/resolvers/BUILD.bazel @@ -53,6 +53,7 @@ go_library( "//internal/batches/rewirer", "//internal/batches/search", "//internal/batches/service", + "//internal/batches/sources", "//internal/batches/state", "//internal/batches/store", "//internal/batches/syncer", @@ -61,12 +62,10 @@ go_library( "//internal/conf", "//internal/database", "//internal/deviceid", - "//internal/encryption", "//internal/errcode", "//internal/executor", "//internal/extsvc", "//internal/extsvc/auth", - "//internal/extsvc/bitbucketserver", "//internal/extsvc/github", "//internal/featureflag", "//internal/github_apps/auth", @@ -84,6 +83,7 @@ go_library( "//lib/batches/execution", "//lib/codeintel/languages", "//lib/errors", + "//lib/pointers", "@com_github_grafana_regexp//:regexp", "@com_github_graph_gophers_graphql_go//:graphql-go", "@com_github_graph_gophers_graphql_go//relay", diff --git a/cmd/frontend/internal/batches/resolvers/batch_spec.go b/cmd/frontend/internal/batches/resolvers/batch_spec.go index f1d12640a98..87f10c01972 100644 --- a/cmd/frontend/internal/batches/resolvers/batch_spec.go +++ b/cmd/frontend/internal/batches/resolvers/batch_spec.go @@ -325,6 +325,7 @@ func (r *batchSpecResolver) ViewerBatchChangesCodeHosts(ctx context.Context, arg Limit: int(args.First), Offset: offset, }, + db: r.store.DatabaseDB(), }, nil } diff --git a/cmd/frontend/internal/batches/resolvers/changeset_counts_test.go b/cmd/frontend/internal/batches/resolvers/changeset_counts_test.go index cb5e735c34d..77c18e75ec8 100644 --- a/cmd/frontend/internal/batches/resolvers/changeset_counts_test.go +++ b/cmd/frontend/internal/batches/resolvers/changeset_counts_test.go @@ -189,7 +189,7 @@ func TestChangesetCountsOverTimeIntegration(t *testing.T) { t.Fatal(err) } - src, err := sourcer.ForChangeset(ctx, bstore, c, sources.AuthenticationStrategyUserCredential, githubRepo) + src, err := sourcer.ForChangeset(ctx, bstore, c, sources.AuthenticationStrategyUserCredential, githubRepo, "") if err != nil { t.Fatalf("failed to build source for repo: %s", err) } diff --git a/cmd/frontend/internal/batches/resolvers/code_host.go b/cmd/frontend/internal/batches/resolvers/code_host.go index 81da61205ea..82c85bee4a2 100644 --- a/cmd/frontend/internal/batches/resolvers/code_host.go +++ b/cmd/frontend/internal/batches/resolvers/code_host.go @@ -43,7 +43,8 @@ func (c *batchChangesCodeHostResolver) CommitSigningConfiguration(ctx context.Co case extsvc.TypeGitHub: gstore := ghstore.GitHubAppsWith(c.store.Store) domain := itypes.BatchesGitHubAppDomain - ghapp, err := gstore.GetByDomain(ctx, domain, c.codeHost.ExternalServiceID) + kind := ghtypes.CommitSigningGitHubAppKind + ghapp, err := gstore.GetByDomainAndKind(ctx, domain, kind, c.codeHost.ExternalServiceID) if err != nil { if _, ok := err.(ghstore.ErrNoGitHubAppFound); ok { return nil, nil diff --git a/cmd/frontend/internal/batches/resolvers/credential.go b/cmd/frontend/internal/batches/resolvers/credential.go index 7eb59e43c2c..c6454d6e4a8 100644 --- a/cmd/frontend/internal/batches/resolvers/credential.go +++ b/cmd/frontend/internal/batches/resolvers/credential.go @@ -3,10 +3,11 @@ package resolvers import ( "context" "fmt" - ghauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" "strconv" "strings" + ghauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" + "github.com/graph-gophers/graphql-go" "github.com/graph-gophers/graphql-go/relay" @@ -16,7 +17,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/extsvc/auth" - ghstore "github.com/sourcegraph/sourcegraph/internal/github_apps/store" + ghastore "github.com/sourcegraph/sourcegraph/internal/github_apps/store" "github.com/sourcegraph/sourcegraph/internal/gqlutil" "github.com/sourcegraph/sourcegraph/internal/types" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -75,7 +76,7 @@ type batchChangesUserCredentialResolver struct { credential *database.UserCredential repo *types.Repo - ghStore ghstore.GitHubAppsStore + ghStore ghastore.GitHubAppsStore } var _ graphqlbackend.BatchChangesCredentialResolver = &batchChangesUserCredentialResolver{} @@ -124,13 +125,17 @@ func (c *batchChangesUserCredentialResolver) authenticator(ctx context.Context) }) } -func (c *batchChangesUserCredentialResolver) IsGitHubApp() bool { return c.credential.GitHubAppID != 0 } +func (c *batchChangesUserCredentialResolver) IsGitHubApp() bool { return c.credential.GitHubAppID > 0 } + +func (c *batchChangesUserCredentialResolver) GitHubAppID() int { + return c.credential.GitHubAppID +} type batchChangesSiteCredentialResolver struct { credential *btypes.SiteCredential repo *types.Repo - ghStore ghstore.GitHubAppsStore + ghStore ghastore.GitHubAppsStore } var _ graphqlbackend.BatchChangesCredentialResolver = &batchChangesSiteCredentialResolver{} @@ -179,4 +184,6 @@ func (c *batchChangesSiteCredentialResolver) authenticator(ctx context.Context) }) } -func (c *batchChangesSiteCredentialResolver) IsGitHubApp() bool { return c.credential.GitHubAppID != 0 } +func (c *batchChangesSiteCredentialResolver) IsGitHubApp() bool { return c.credential.GitHubAppID > 0 } + +func (c *batchChangesSiteCredentialResolver) GitHubAppID() int { return c.credential.GitHubAppID } diff --git a/cmd/frontend/internal/batches/resolvers/errors.go b/cmd/frontend/internal/batches/resolvers/errors.go index 10f68243ea1..b2451ff1d90 100644 --- a/cmd/frontend/internal/batches/resolvers/errors.go +++ b/cmd/frontend/internal/batches/resolvers/errors.go @@ -89,25 +89,3 @@ func (e ErrMatchingBatchChangeExists) Error() string { func (e ErrMatchingBatchChangeExists) Extensions() map[string]any { return map[string]any{"code": "ErrMatchingBatchChangeExists"} } - -type ErrDuplicateCredential struct{} - -func (e ErrDuplicateCredential) Error() string { - return "a credential for this code host already exists" -} - -func (e ErrDuplicateCredential) Extensions() map[string]any { - return map[string]any{"code": "ErrDuplicateCredential"} -} - -type ErrVerifyCredentialFailed struct { - SourceErr error -} - -func (e ErrVerifyCredentialFailed) Error() string { - return fmt.Sprintf("Failed to verify the credential:\n```\n%s\n```\n", e.SourceErr) -} - -func (e ErrVerifyCredentialFailed) Extensions() map[string]any { - return map[string]any{"code": "ErrVerifyCredentialFailed"} -} diff --git a/cmd/frontend/internal/batches/resolvers/resolver.go b/cmd/frontend/internal/batches/resolvers/resolver.go index 736195fad13..01b839aefa2 100644 --- a/cmd/frontend/internal/batches/resolvers/resolver.go +++ b/cmd/frontend/internal/batches/resolvers/resolver.go @@ -14,10 +14,6 @@ import ( "github.com/sourcegraph/log" - "github.com/sourcegraph/sourcegraph/internal/gitserver" - "github.com/sourcegraph/sourcegraph/internal/rbac" - "github.com/sourcegraph/sourcegraph/internal/types" - "github.com/sourcegraph/sourcegraph/cmd/frontend/enterprise" "github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend" sgactor "github.com/sourcegraph/sourcegraph/internal/actor" @@ -25,21 +21,25 @@ import ( "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/batches/search" "github.com/sourcegraph/sourcegraph/internal/batches/service" + "github.com/sourcegraph/sourcegraph/internal/batches/sources" "github.com/sourcegraph/sourcegraph/internal/batches/store" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/deviceid" - "github.com/sourcegraph/sourcegraph/internal/encryption" "github.com/sourcegraph/sourcegraph/internal/errcode" "github.com/sourcegraph/sourcegraph/internal/extsvc" extsvcauth "github.com/sourcegraph/sourcegraph/internal/extsvc/auth" - "github.com/sourcegraph/sourcegraph/internal/extsvc/bitbucketserver" "github.com/sourcegraph/sourcegraph/internal/featureflag" + ghstore "github.com/sourcegraph/sourcegraph/internal/github_apps/store" + "github.com/sourcegraph/sourcegraph/internal/gitserver" "github.com/sourcegraph/sourcegraph/internal/licensing" + "github.com/sourcegraph/sourcegraph/internal/rbac" "github.com/sourcegraph/sourcegraph/internal/trace" + "github.com/sourcegraph/sourcegraph/internal/types" "github.com/sourcegraph/sourcegraph/internal/usagestats" batcheslib "github.com/sourcegraph/sourcegraph/lib/batches" "github.com/sourcegraph/sourcegraph/lib/errors" + "github.com/sourcegraph/sourcegraph/lib/pointers" ) // Resolver is the GraphQL resolver of all things related to batch changes. @@ -1123,145 +1123,32 @@ func (r *Resolver) CreateBatchChangesCredential(ctx context.Context, args *graph return nil, errors.New("empty credential not allowed") } - if userID != 0 { - return r.createBatchChangesUserCredential(ctx, args.ExternalServiceURL, extsvc.KindToType(kind), userID, trimmedCredential, args.Username) - } - - return r.createBatchChangesSiteCredential(ctx, args.ExternalServiceURL, extsvc.KindToType(kind), trimmedCredential, args.Username) -} - -func (r *Resolver) createBatchChangesUserCredential(ctx context.Context, externalServiceURL, externalServiceType string, userID int32, credential string, username *string) (graphqlbackend.BatchChangesCredentialResolver, error) { - // 🚨 SECURITY: Check that the requesting user can create the credential. - if err := auth.CheckSiteAdminOrSameUser(ctx, r.store.DatabaseDB(), userID); err != nil { - return nil, err - } - - // Throw error documented in schema.graphql. - userCredentialScope := database.UserCredentialScope{ - Domain: database.UserCredentialDomainBatches, - ExternalServiceID: externalServiceURL, - ExternalServiceType: externalServiceType, - UserID: userID, - } - existing, err := r.store.UserCredentials().GetByScope(ctx, userCredentialScope) - if err != nil && !errcode.IsNotFound(err) { - return nil, err - } - if existing != nil { - return nil, ErrDuplicateCredential{} - } - - a, err := r.generateAuthenticatorForCredential(ctx, externalServiceType, externalServiceURL, credential, username) - if err != nil { - return nil, err - } - cred, err := r.store.UserCredentials().Create(ctx, userCredentialScope, a) - if err != nil { - return nil, err - } - - return &batchChangesUserCredentialResolver{credential: cred, ghStore: r.db.GitHubApps()}, nil -} - -func (r *Resolver) createBatchChangesSiteCredential(ctx context.Context, externalServiceURL, externalServiceType string, credential string, username *string) (graphqlbackend.BatchChangesCredentialResolver, error) { - // 🚨 SECURITY: Check that a site credential can only be created - // by a site-admin. - if err := auth.CheckCurrentUserIsSiteAdmin(ctx, r.store.DatabaseDB()); err != nil { - return nil, err - } - - // Throw error documented in schema.graphql. - existing, err := r.store.GetSiteCredential(ctx, store.GetSiteCredentialOpts{ - ExternalServiceType: externalServiceType, - ExternalServiceID: externalServiceURL, - }) - if err != nil && err != store.ErrNoResults { - return nil, err - } - if existing != nil { - return nil, ErrDuplicateCredential{} - } - - a, err := r.generateAuthenticatorForCredential(ctx, externalServiceType, externalServiceURL, credential, username) - if err != nil { - return nil, err - } - cred := &btypes.SiteCredential{ - ExternalServiceID: externalServiceURL, - ExternalServiceType: externalServiceType, - } - if err := r.store.CreateSiteCredential(ctx, cred, a); err != nil { - return nil, err - } - - return &batchChangesSiteCredentialResolver{credential: cred, ghStore: r.db.GitHubApps()}, nil -} - -func (r *Resolver) generateAuthenticatorForCredential(ctx context.Context, externalServiceType, externalServiceURL, credential string, username *string) (extsvcauth.Authenticator, error) { svc := service.New(r.store) - var a extsvcauth.Authenticator - keypair, err := encryption.GenerateRSAKey() + if userID != 0 { + cred, err := svc.CreateBatchChangesUserCredential(ctx, sources.AuthenticationStrategyUserCredential, service.CreateBatchChangesUserCredentialArgs{ + ExternalServiceURL: args.ExternalServiceURL, + ExternalServiceType: extsvc.KindToType(kind), + UserID: userID, + Credential: args.Credential, + Username: args.Username, + }) + if err != nil { + return nil, err + } + return &batchChangesUserCredentialResolver{credential: cred, ghStore: r.db.GitHubApps()}, nil + } + + cred, err := svc.CreateBatchChangesSiteCredential(ctx, sources.AuthenticationStrategyUserCredential, service.CreateBatchChangesSiteCredentialArgs{ + ExternalServiceURL: args.ExternalServiceURL, + ExternalServiceType: extsvc.VariantGitHub.AsType(), + Credential: args.Credential, + Username: args.Username, + }) if err != nil { return nil, err } - if externalServiceType == extsvc.TypeBitbucketServer { - // We need to fetch the username for the token, as just an OAuth token isn't enough for some reason.. - username, err := svc.FetchUsernameForBitbucketServerToken(ctx, externalServiceURL, externalServiceType, credential) - if err != nil { - if bitbucketserver.IsUnauthorized(err) { - return nil, &ErrVerifyCredentialFailed{SourceErr: err} - } - return nil, err - } - a = &extsvcauth.BasicAuthWithSSH{ - BasicAuth: extsvcauth.BasicAuth{Username: username, Password: credential}, - PrivateKey: keypair.PrivateKey, - PublicKey: keypair.PublicKey, - Passphrase: keypair.Passphrase, - } - } else if externalServiceType == extsvc.TypeBitbucketCloud { - a = &extsvcauth.BasicAuthWithSSH{ - BasicAuth: extsvcauth.BasicAuth{Username: *username, Password: credential}, - PrivateKey: keypair.PrivateKey, - PublicKey: keypair.PublicKey, - Passphrase: keypair.Passphrase, - } - } else if externalServiceType == extsvc.TypeAzureDevOps { - a = &extsvcauth.BasicAuthWithSSH{ - BasicAuth: extsvcauth.BasicAuth{Username: *username, Password: credential}, - PrivateKey: keypair.PrivateKey, - PublicKey: keypair.PublicKey, - Passphrase: keypair.Passphrase, - } - } else if externalServiceType == extsvc.TypeGerrit { - a = &extsvcauth.BasicAuthWithSSH{ - BasicAuth: extsvcauth.BasicAuth{Username: *username, Password: credential}, - PrivateKey: keypair.PrivateKey, - PublicKey: keypair.PublicKey, - Passphrase: keypair.Passphrase, - } - } else if externalServiceType == extsvc.TypePerforce { - a = &extsvcauth.BasicAuthWithSSH{ - BasicAuth: extsvcauth.BasicAuth{Username: *username, Password: credential}, - PrivateKey: keypair.PrivateKey, - PublicKey: keypair.PublicKey, - Passphrase: keypair.Passphrase, - } - } else { - a = &extsvcauth.OAuthBearerTokenWithSSH{ - OAuthBearerToken: extsvcauth.OAuthBearerToken{Token: credential}, - PrivateKey: keypair.PrivateKey, - PublicKey: keypair.PublicKey, - Passphrase: keypair.Passphrase, - } - } - - // Validate the newly created authenticator. - if err := svc.ValidateAuthenticator(ctx, externalServiceURL, externalServiceType, a); err != nil { - return nil, &ErrVerifyCredentialFailed{SourceErr: err} - } - return a, nil + return &batchChangesSiteCredentialResolver{credential: cred, ghStore: r.db.GitHubApps()}, nil } func (r *Resolver) DeleteBatchChangesCredential(ctx context.Context, args *graphqlbackend.DeleteBatchChangesCredentialArgs) (_ *graphqlbackend.EmptyResponse, err error) { @@ -2059,14 +1946,44 @@ func (r *Resolver) CheckBatchChangesCredential(ctx context.Context, args *graphq return nil, ErrIDIsZero{} } + validateArgs := service.ValidateAuthenticatorArgs{ + ExternalServiceID: cred.ExternalServiceURL(), + ExternalServiceType: extsvc.KindToType(cred.ExternalServiceKind()), + } + as := sources.AuthenticationStrategyUserCredential + if cred.IsGitHubApp() { + as = sources.AuthenticationStrategyGitHubApp + + ghApp, err := r.db.GitHubApps().GetByID(ctx, cred.GitHubAppID()) + if err != nil { + return nil, err + } + + if ghApp == nil { + return nil, ghstore.ErrNoGitHubAppFound{} + } + + installs, err := r.db.GitHubApps().GetInstallations(ctx, ghApp.ID) + if err != nil { + return nil, err + } + + if len(installs) == 0 { + return nil, ghstore.ErrNoGitHubAppFound{} + } + + validateArgs.GitHubAppKind = ghApp.Kind + validateArgs.Username = pointers.Ptr(installs[0].AccountLogin) + } + a, err := cred.authenticator(ctx) if err != nil { return nil, err } svc := service.New(r.store) - if err := svc.ValidateAuthenticator(ctx, cred.ExternalServiceURL(), extsvc.KindToType(cred.ExternalServiceKind()), a); err != nil { - return nil, &ErrVerifyCredentialFailed{SourceErr: err} + if err := svc.ValidateAuthenticator(ctx, a, as, validateArgs); err != nil { + return nil, &service.ErrVerifyCredentialFailed{SourceErr: err} } return &graphqlbackend.EmptyResponse{}, nil diff --git a/cmd/frontend/internal/batches/webhooks/bitbucketserver_test.go b/cmd/frontend/internal/batches/webhooks/bitbucketserver_test.go index d202035b160..02552498963 100644 --- a/cmd/frontend/internal/batches/webhooks/bitbucketserver_test.go +++ b/cmd/frontend/internal/batches/webhooks/bitbucketserver_test.go @@ -166,7 +166,7 @@ func testBitbucketServerWebhook(db database.DB, userID int32) func(*testing.T) { if err := s.CreateChangeset(ctx, ch); err != nil { t.Fatal(err) } - src, err := sourcer.ForChangeset(ctx, s, ch, sources.AuthenticationStrategyUserCredential, bitbucketRepo) + src, err := sourcer.ForChangeset(ctx, s, ch, sources.AuthenticationStrategyUserCredential, bitbucketRepo, "") if err != nil { t.Fatal(err) } diff --git a/cmd/frontend/internal/batches/webhooks/github_test.go b/cmd/frontend/internal/batches/webhooks/github_test.go index 3e6a62293c0..3221a7a6bdb 100644 --- a/cmd/frontend/internal/batches/webhooks/github_test.go +++ b/cmd/frontend/internal/batches/webhooks/github_test.go @@ -155,7 +155,7 @@ func testGitHubWebhook(db database.DB, userID int32) func(*testing.T) { VCS: protocol.VCSInfo{URL: "https://example.com/repo/"}, }) - src, err := sourcer.ForChangeset(ctx, s, changeset, sources.AuthenticationStrategyUserCredential, githubRepo) + src, err := sourcer.ForChangeset(ctx, s, changeset, sources.AuthenticationStrategyUserCredential, githubRepo, "") if err != nil { t.Fatal(err) } diff --git a/cmd/frontend/internal/githubapp/BUILD.bazel b/cmd/frontend/internal/githubapp/BUILD.bazel index b7c72804f91..f54fae82619 100644 --- a/cmd/frontend/internal/githubapp/BUILD.bazel +++ b/cmd/frontend/internal/githubapp/BUILD.bazel @@ -6,6 +6,7 @@ go_library( srcs = [ "httpapi.go", "init.go", + "parser.go", "resolver.go", ], importpath = "github.com/sourcegraph/sourcegraph/cmd/frontend/internal/githubapp", @@ -17,6 +18,9 @@ go_library( "//cmd/frontend/graphqlbackend", "//cmd/frontend/internal/repos/webhooks/resolvers", "//internal/auth", + "//internal/batches/service", + "//internal/batches/sources", + "//internal/batches/store", "//internal/codeintel", "//internal/conf/conftypes", "//internal/database", @@ -32,6 +36,7 @@ go_library( "//internal/rcache", "//internal/types", "//lib/errors", + "//lib/pointers", "//schema", "@com_github_google_uuid//:uuid", "@com_github_gorilla_mux//:mux", diff --git a/cmd/frontend/internal/githubapp/httpapi.go b/cmd/frontend/internal/githubapp/httpapi.go index c41502678e2..d38da2e026c 100644 --- a/cmd/frontend/internal/githubapp/httpapi.go +++ b/cmd/frontend/internal/githubapp/httpapi.go @@ -1,6 +1,7 @@ package githubapp import ( + "context" "crypto/rand" "crypto/sha256" "encoding/hex" @@ -15,21 +16,27 @@ import ( "github.com/google/uuid" "github.com/gorilla/mux" "github.com/graph-gophers/graphql-go" + "github.com/graph-gophers/graphql-go/relay" "github.com/sourcegraph/log" "github.com/sourcegraph/sourcegraph/cmd/frontend/backend" authcheck "github.com/sourcegraph/sourcegraph/internal/auth" + "github.com/sourcegraph/sourcegraph/internal/batches/service" + "github.com/sourcegraph/sourcegraph/internal/batches/sources" + "github.com/sourcegraph/sourcegraph/internal/batches/store" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/encryption" "github.com/sourcegraph/sourcegraph/internal/encryption/keyring" "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/extsvc/github" - ghaauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" + ghauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" "github.com/sourcegraph/sourcegraph/internal/httpcli" + "github.com/sourcegraph/sourcegraph/internal/observation" "github.com/sourcegraph/sourcegraph/internal/rcache" "github.com/sourcegraph/sourcegraph/internal/types" "github.com/sourcegraph/sourcegraph/lib/errors" + "github.com/sourcegraph/sourcegraph/lib/pointers" ) const cacheTTLSeconds = 60 * 60 // 1 hour @@ -123,6 +130,8 @@ type gitHubAppStateDetails struct { Domain string `json:"domain"` AppID int `json:"app_id,omitempty"` BaseURL string `json:"base_url,omitempty"` + Kind string `json:"kind,omitempty"` + UserID int32 `json:"user_id,omitempty"` } func (srv *gitHubAppServer) stateHandler(w http.ResponseWriter, r *http.Request) { @@ -169,11 +178,29 @@ func (srv *gitHubAppServer) stateHandler(w http.ResponseWriter, r *http.Request) _, _ = w.Write([]byte(s)) } +func unmarshalUserID(id graphql.ID) (userID int32, err error) { + err = relay.UnmarshalSpec(id, &userID) + return +} + func (srv *gitHubAppServer) newAppStateHandler(w http.ResponseWriter, r *http.Request) { webhookURN := r.URL.Query().Get("webhookURN") appName := r.URL.Query().Get("appName") domain := r.URL.Query().Get("domain") baseURL := r.URL.Query().Get("baseURL") + kind := r.URL.Query().Get("kind") + marshalledUserID := r.URL.Query().Get("userID") + + var userID int32 + if marshalledUserID != "" { + uid, err := unmarshalUserID(graphql.ID(marshalledUserID)) + if err != nil { + http.Error(w, fmt.Sprintf("Unexpected error while unmarshalling user ID: %s", err.Error()), http.StatusBadRequest) + return + } + userID = uid + } + var webhookUUID string if webhookURN != "" { ws := backend.NewWebhookService(srv.db, keyring.Default()) @@ -195,6 +222,8 @@ func (srv *gitHubAppServer) newAppStateHandler(w http.ResponseWriter, r *http.Re WebhookUUID: webhookUUID, Domain: domain, BaseURL: baseURL, + Kind: kind, + UserID: userID, }) if err != nil { http.Error(w, fmt.Sprintf("Unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) @@ -243,7 +272,6 @@ func (srv *gitHubAppServer) redirectHandler(w http.ResponseWriter, r *http.Reque http.Error(w, "Bad request, invalid state", http.StatusBadRequest) return } - // Wait until we've validated the type of state before deleting it from the cache. srv.cache.Delete(state) webhookUUID, err := uuid.Parse(stateDetails.WebhookUUID) @@ -271,7 +299,18 @@ func (srv *gitHubAppServer) redirectHandler(w http.ResponseWriter, r *http.Reque return } - app, err := createGitHubApp(u, *domain, httpcli.UncachedExternalClient) + kind, err := parseKind(&stateDetails.Kind) + if err != nil { + http.Error(w, fmt.Sprintf("Unable to parse kind: %v", err), http.StatusBadRequest) + return + } + + if kind == nil { + http.Error(w, "invalid kind provided", http.StatusBadRequest) + return + } + + app, err := createGitHubApp(u, *domain, httpcli.UncachedExternalClient, *kind) if err != nil { http.Error(w, fmt.Sprintf("Unexpected error while converting github app: %s", err.Error()), http.StatusInternalServerError) return @@ -303,8 +342,11 @@ func (srv *gitHubAppServer) redirectHandler(w http.ResponseWriter, r *http.Reque } newStateDetails, err := json.Marshal(gitHubAppStateDetails{ - Domain: stateDetails.Domain, - AppID: id, + Domain: stateDetails.Domain, + AppID: id, + Kind: stateDetails.Kind, + BaseURL: stateDetails.BaseURL, + UserID: stateDetails.UserID, }) if err != nil { http.Error(w, fmt.Sprintf("unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError) @@ -344,7 +386,7 @@ func (srv *gitHubAppServer) setupHandler(w http.ResponseWriter, r *http.Request) setupInfo, ok := srv.cache.Get(state) if !ok { - redirectURL := generateRedirectURL(nil, nil, nil, nil, errors.New("Bad request, state query param does not match")) + redirectURL := generateRedirectURL(gitHubAppStateDetails{}, nil, nil, errors.New("Bad request, state query param does not match")) http.Redirect(w, r, redirectURL, http.StatusFound) return } @@ -352,16 +394,27 @@ func (srv *gitHubAppServer) setupHandler(w http.ResponseWriter, r *http.Request) var stateDetails gitHubAppStateDetails err := json.Unmarshal(setupInfo, &stateDetails) if err != nil { - redirectURL := generateRedirectURL(nil, nil, nil, nil, errors.New("Bad request, invalid state")) + redirectURL := generateRedirectURL(gitHubAppStateDetails{}, nil, nil, errors.New("Bad request, invalid state")) http.Redirect(w, r, redirectURL, http.StatusFound) return } // Wait until we've validated the type of state before deleting it from the cache. srv.cache.Delete(state) + kind, err := parseKind(&stateDetails.Kind) + if err != nil { + http.Error(w, fmt.Sprintf("Unable to parse kind: %v", err), http.StatusBadRequest) + return + } + + if kind == nil { + http.Error(w, "invalid kind provided", http.StatusBadRequest) + return + } + installationID, err := strconv.Atoi(instID) if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, nil, &stateDetails.AppID, nil, errors.New("Bad request, could not parse installation ID")) + redirectURL := generateRedirectURL(stateDetails, nil, nil, errors.New("Bad request, could not parse installation ID")) http.Redirect(w, r, redirectURL, http.StatusFound) return } @@ -371,21 +424,21 @@ func (srv *gitHubAppServer) setupHandler(w http.ResponseWriter, r *http.Request) ctx := r.Context() app, err := srv.db.GitHubApps().GetByID(ctx, stateDetails.AppID) if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while fetching GitHub App from DB: %s", err.Error())) + redirectURL := generateRedirectURL(stateDetails, &installationID, nil, errors.Newf("Unexpected error while fetching GitHub App from DB: %s", err.Error())) http.Redirect(w, r, redirectURL, http.StatusFound) return } - auther, err := ghaauth.NewGitHubAppAuthenticator(app.AppID, []byte(app.PrivateKey)) + auther, err := ghauth.NewGitHubAppAuthenticator(app.AppID, []byte(app.PrivateKey)) if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while creating GitHubAppAuthenticator: %s", err.Error())) + redirectURL := generateRedirectURL(stateDetails, &installationID, nil, errors.Newf("Unexpected error while creating GitHubAppAuthenticator: %s", err.Error())) http.Redirect(w, r, redirectURL, http.StatusFound) return } baseURL, err := url.Parse(app.BaseURL) if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while parsing App base URL: %s", err.Error())) + redirectURL := generateRedirectURL(stateDetails, &installationID, nil, errors.Newf("Unexpected error while parsing App base URL: %s", err.Error())) http.Redirect(w, r, redirectURL, http.StatusFound) return } @@ -402,12 +455,12 @@ func (srv *gitHubAppServer) setupHandler(w http.ResponseWriter, r *http.Request) remoteInstall, err := client.GetAppInstallation(ctx, int64(installationID)) if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, nil, errors.Newf("Unexpected error while fetching App installation details from GitHub: %s", err.Error())) + redirectURL := generateRedirectURL(stateDetails, &installationID, nil, errors.Newf("Unexpected error while fetching App installation details from GitHub: %s", err.Error())) http.Redirect(w, r, redirectURL, http.StatusFound) return } - _, err = srv.db.GitHubApps().Install(ctx, ghtypes.GitHubAppInstallation{ + installInfo, err := srv.db.GitHubApps().Install(ctx, ghtypes.GitHubAppInstallation{ InstallationID: installationID, AppID: app.ID, URL: remoteInstall.GetHTMLURL(), @@ -417,12 +470,20 @@ func (srv *gitHubAppServer) setupHandler(w http.ResponseWriter, r *http.Request) AccountType: remoteInstall.Account.GetType(), }) if err != nil { - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &stateDetails.AppID, &app.Name, errors.Newf("Unexpected error while creating GitHub App installation: %s", err.Error())) + redirectURL := generateRedirectURL(stateDetails, &installationID, &app.Name, errors.Newf("Unexpected error while creating GitHub App installation: %s", err.Error())) http.Redirect(w, r, redirectURL, http.StatusFound) return } - redirectURL := generateRedirectURL(&stateDetails.Domain, &installationID, &app.ID, &app.Name, nil) + if *kind == ghtypes.UserCredentialGitHubAppKind || *kind == ghtypes.SiteCredentialGitHubAppKind { + if err := handleCredentialCreation(r.Context(), srv.db, stateDetails, kind, app, installInfo.AccountLogin); err != nil { + redirectURL := generateRedirectURL(stateDetails, &installationID, &app.Name, err) + http.Redirect(w, r, redirectURL, http.StatusFound) + return + } + } + + redirectURL := generateRedirectURL(stateDetails, &installationID, &app.Name, nil) http.Redirect(w, r, redirectURL, http.StatusFound) return } else { @@ -431,48 +492,111 @@ func (srv *gitHubAppServer) setupHandler(w http.ResponseWriter, r *http.Request) } } -func generateRedirectURL(domain *string, installationID, appID *int, appName *string, creationErr error) string { +func handleCredentialCreation(ctx context.Context, db database.DB, stateDetails gitHubAppStateDetails, kind *ghtypes.GitHubAppKind, app *ghtypes.GitHubApp, owner string) error { + observationCtx := observation.NewContext(log.NoOp()) + bstore := store.New(db, observationCtx, keyring.Default().BatchChangesCredentialKey) + svc := service.New(bstore) + + // external service urls always end with a trailing slash. `url.JoinPath` ensures that's the case irrespective + // of whether the base URL ends with a trailing slash or not. + esu, err := url.JoinPath(stateDetails.BaseURL, "/") + if err != nil { + return errors.Newf("Unexpected error while creating external service url for github app: %s", err.Error()) + } + + if *kind == ghtypes.UserCredentialGitHubAppKind { + if _, err = svc.CreateBatchChangesUserCredential( + ctx, + sources.AuthenticationStrategyGitHubApp, + service.CreateBatchChangesUserCredentialArgs{ + ExternalServiceURL: esu, + ExternalServiceType: extsvc.VariantGitHub.AsType(), + GitHubAppKind: *kind, + Username: pointers.Ptr(owner), + GitHubAppID: app.ID, + UserID: stateDetails.UserID, + }); err != nil { + return errors.Wrapf(err, "Unexpected error while creating user credential for github app") + } + } else { + if _, err := svc.CreateBatchChangesSiteCredential( + ctx, + sources.AuthenticationStrategyGitHubApp, + service.CreateBatchChangesSiteCredentialArgs{ + ExternalServiceURL: esu, + ExternalServiceType: extsvc.VariantGitHub.AsType(), + GitHubAppKind: *kind, + Username: pointers.Ptr(owner), + GitHubAppID: app.ID, + }); err != nil { + return errors.Wrapf(err, "Unexpected error while creating site credential for github app") + } + } + + return nil +} + +func generateRedirectURL(stateDetails gitHubAppStateDetails, installationID *int, appName *string, creationErr error) string { // If we got an error but didn't even get far enough to parse a domain for the new // GitHub App, we still want to route the user back to somewhere useful, so we send // them back to the main site admin GitHub Apps page. - if domain == nil && creationErr != nil { - return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(creationErr.Error())) + if stateDetails.Domain == "" && creationErr != nil { + return fmt.Sprintf("/site-admin/github-apps?kind=%s&success=false&error=%s", stateDetails.Kind, url.QueryEscape(creationErr.Error())) } - parsedDomain, err := parseDomain(domain) + parsedDomain, err := parseDomain(&stateDetails.Domain) if err != nil { - return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(fmt.Sprintf("invalid domain: %s", *domain))) + return fmt.Sprintf("/site-admin/github-apps?kind=%s&success=false&error=%s", stateDetails.Kind, url.QueryEscape(fmt.Sprintf("invalid domain: %s", stateDetails.Domain))) + } + + if parsedDomain == nil { + return fmt.Sprintf("/site-admin/github-apps?kind=%s&success=false&error=%s", stateDetails.Kind, "unable to parse domain") + } + + kind, err := parseKind(&stateDetails.Kind) + if err != nil { + return fmt.Sprintf("/site-admin/github-apps?success=false&kind=%s&error=%s", stateDetails.Kind, url.QueryEscape(creationErr.Error())) + } + + if kind == nil { + return fmt.Sprintf("/site-admin/github-apps?kind=%s&success=false&error=%s", stateDetails.Kind, "unable to parse kind") } switch *parsedDomain { case types.ReposGitHubAppDomain: + ghAppPageUrl := "/site-admin/github-apps" if creationErr != nil { - return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(creationErr.Error())) + return fmt.Sprintf("%s?success=false&error=%s", ghAppPageUrl, url.QueryEscape(creationErr.Error())) } - if installationID == nil || appID == nil { - return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape("missing installation ID or app ID")) + if installationID == nil || stateDetails.AppID == 0 { + return fmt.Sprintf("%s?success=false&error=%s", ghAppPageUrl, url.QueryEscape("missing installation ID or app ID")) } - return fmt.Sprintf("/site-admin/github-apps/%s?installation_id=%d", MarshalGitHubAppID(int64(*appID)), *installationID) + return fmt.Sprintf("%s/%s?installation_id=%d", ghAppPageUrl, MarshalGitHubAppID(int64(stateDetails.AppID)), *installationID) case types.BatchesGitHubAppDomain: + ghAppPageUrl := "/site-admin/batch-changes" + if *kind == ghtypes.UserCredentialGitHubAppKind { + ghAppPageUrl = "/user/settings/batch-changes" + } + if creationErr != nil { - return fmt.Sprintf("/site-admin/batch-changes?success=false&error=%s", url.QueryEscape(creationErr.Error())) + return fmt.Sprintf("%s?kind=%s&success=false&error=%s", ghAppPageUrl, *kind, url.QueryEscape(creationErr.Error())) } // This shouldn't really happen unless we also had an error, but we handle it just // in case if appName == nil { - return "/site-admin/batch-changes?success=true" + return fmt.Sprintf("%s?kind=%s&success=true", ghAppPageUrl, *kind) } - return fmt.Sprintf("/site-admin/batch-changes?success=true&app_name=%s", *appName) + return fmt.Sprintf("%s?kind=%s&success=true&app_name=%s", ghAppPageUrl, *kind, *appName) default: - return fmt.Sprintf("/site-admin/github-apps?success=false&error=%s", url.QueryEscape(fmt.Sprintf("unsupported github apps domain: %v", parsedDomain))) + return fmt.Sprintf("/site-admin/github-apps?kind=%s&success=false&error=%s", *kind, url.QueryEscape(fmt.Sprintf("unsupported github apps domain: %v", parsedDomain))) } } var MockCreateGitHubApp func(conversionURL string, domain types.GitHubAppDomain) (*ghtypes.GitHubApp, error) -func createGitHubApp(conversionURL string, domain types.GitHubAppDomain, httpClient *http.Client) (*ghtypes.GitHubApp, error) { +func createGitHubApp(conversionURL string, domain types.GitHubAppDomain, httpClient *http.Client, kind ghtypes.GitHubAppKind) (*ghtypes.GitHubApp, error) { if MockCreateGitHubApp != nil { return MockCreateGitHubApp(conversionURL, domain) } @@ -512,6 +636,7 @@ func createGitHubApp(conversionURL string, domain types.GitHubAppDomain, httpCli BaseURL: htmlURL.Scheme + "://" + htmlURL.Host, AppURL: htmlURL.String(), Domain: domain, + Kind: kind, Logo: fmt.Sprintf("%s://%s/identicons/app/app/%s", htmlURL.Scheme, htmlURL.Host, response.Slug), }, nil } diff --git a/cmd/frontend/internal/githubapp/httpapi_test.go b/cmd/frontend/internal/githubapp/httpapi_test.go index 56841357d5d..365cd8d6575 100644 --- a/cmd/frontend/internal/githubapp/httpapi_test.go +++ b/cmd/frontend/internal/githubapp/httpapi_test.go @@ -39,43 +39,65 @@ func TestGenerateRedirectURL(t *testing.T) { appID int creationErr error expectedURL string + stateDetails gitHubAppStateDetails }{ { name: "repos domain", - domain: &reposDomain, installationID: 1, - appID: 2, - expectedURL: "/site-admin/github-apps/R2l0SHViQXBwOjI=?installation_id=1", + expectedURL: "/site-admin/github-apps/R2l0SHViQXBwOjE=?installation_id=1", + stateDetails: gitHubAppStateDetails{ + Domain: reposDomain, + AppID: 1, + Kind: string(ghtypes.RepoSyncGitHubAppKind), + }, }, { name: "batches domain", - domain: &batchesDomain, installationID: 1, - appID: 2, - expectedURL: "/site-admin/batch-changes?success=true&app_name=my-cool-app", + expectedURL: "/site-admin/batch-changes?kind=COMMIT_SIGNING&success=true&app_name=my-cool-app", + stateDetails: gitHubAppStateDetails{ + Domain: batchesDomain, + AppID: 2, + Kind: string(ghtypes.CommitSigningGitHubAppKind), + }, }, { name: "invalid domain", domain: &invalidDomain, - expectedURL: "/site-admin/github-apps?success=false&error=invalid+domain%3A+invalid", + expectedURL: "/site-admin/github-apps?kind=COMMIT_SIGNING&success=false&error=invalid+domain%3A+invalid", + stateDetails: gitHubAppStateDetails{ + Domain: invalidDomain, + AppID: 1, + Kind: string(ghtypes.CommitSigningGitHubAppKind), + }, }, { name: "repos creation error", domain: &reposDomain, creationErr: creationErr, expectedURL: "/site-admin/github-apps?success=false&error=uh+oh%21", + stateDetails: gitHubAppStateDetails{ + Domain: reposDomain, + AppID: 1, + Kind: string(ghtypes.CommitSigningGitHubAppKind), + }, }, { name: "batches creation error", domain: &batchesDomain, creationErr: creationErr, - expectedURL: "/site-admin/batch-changes?success=false&error=uh+oh%21", + expectedURL: "/site-admin/batch-changes?kind=COMMIT_SIGNING&success=false&error=uh+oh%21", + stateDetails: gitHubAppStateDetails{ + Domain: batchesDomain, + AppID: 1, + Kind: string(ghtypes.CommitSigningGitHubAppKind), + }, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - url := generateRedirectURL(tc.domain, &tc.installationID, &tc.appID, &appName, tc.creationErr) + url := generateRedirectURL(tc.stateDetails, &tc.installationID, &appName, tc.creationErr) require.Equal(t, tc.expectedURL, url) }) } @@ -193,7 +215,9 @@ func TestGithubAppHTTPAPI(t *testing.T) { appName := "TestApp" domain := "batches" baseURL := "https://ghe.example.org" - req := httptest.NewRequest("GET", fmt.Sprintf("/githubapp/new-app-state?webhookURN=%s&appName=%s&domain=%s&baseURL=%s", webhookURN, appName, domain, baseURL), nil) + kind := ghtypes.CommitSigningGitHubAppKind + userID := "VXNlcjox" + req := httptest.NewRequest("GET", fmt.Sprintf("/githubapp/new-app-state?webhookURN=%s&appName=%s&domain=%s&baseURL=%s&kind=%s&userID=%s", webhookURN, appName, domain, baseURL, kind, userID), nil) t.Run("normal user", func(t *testing.T) { req = req.WithContext(actor.WithActor(req.Context(), &actor.Actor{ @@ -217,7 +241,7 @@ func TestGithubAppHTTPAPI(t *testing.T) { mux.ServeHTTP(w, req) if w.Code != http.StatusOK { - t.Fatalf("expected status code %d but got %d", http.StatusOK, w.Code) + t.Fatalf("expected status code %d but got %d: %s", http.StatusOK, w.Code, w.Body) } var resp struct { @@ -322,6 +346,7 @@ func TestGithubAppHTTPAPI(t *testing.T) { WebhookUUID: webhookUUID.String(), Domain: string(domain), BaseURL: stateBaseURL, + Kind: string(ghtypes.CommitSigningGitHubAppKind), }) require.NoError(t, err) cache.Set(state, stateDeets) @@ -330,7 +355,7 @@ func TestGithubAppHTTPAPI(t *testing.T) { mux.ServeHTTP(w, req) if w.Code != http.StatusSeeOther { - t.Fatalf("expected status code %d but got %d", http.StatusOK, w.Code) + t.Fatalf("expected status code %d but got %d: %s", http.StatusOK, w.Code, w.Body) } }) }) @@ -412,6 +437,7 @@ func TestGithubAppHTTPAPI(t *testing.T) { stateDeets, err := json.Marshal(gitHubAppStateDetails{ Domain: string(domain), + Kind: string(ghtypes.CommitSigningGitHubAppKind), }) require.NoError(t, err) cache.Set(state, stateDeets) @@ -420,7 +446,7 @@ func TestGithubAppHTTPAPI(t *testing.T) { mux.ServeHTTP(w, req) if w.Code != http.StatusFound { - t.Fatalf("expected status code %d but got %d", http.StatusOK, w.Code) + t.Fatalf("expected status code %d but got %d: %s", http.StatusOK, w.Code, w.Body) } }) }) @@ -430,13 +456,15 @@ func TestCreateGitHubApp(t *testing.T) { tests := []struct { name string domain types.GitHubAppDomain + gitHubAppKind ghtypes.GitHubAppKind handlerAssert func(t *testing.T) http.HandlerFunc expected *ghtypes.GitHubApp expectedErr error }{ { - name: "success", - domain: types.BatchesGitHubAppDomain, + name: "success", + domain: types.BatchesGitHubAppDomain, + gitHubAppKind: ghtypes.SiteCredentialGitHubAppKind, handlerAssert: func(t *testing.T) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, http.MethodPost, r.Method) @@ -475,11 +503,13 @@ func TestCreateGitHubApp(t *testing.T) { AppURL: "http://my-github-app.com/app", Domain: types.BatchesGitHubAppDomain, Logo: "http://my-github-app.com/identicons/app/app/test/github-app", + Kind: ghtypes.SiteCredentialGitHubAppKind, }, }, { - name: "unexpected status code", - domain: types.BatchesGitHubAppDomain, + name: "unexpected status code", + domain: types.BatchesGitHubAppDomain, + gitHubAppKind: ghtypes.SiteCredentialGitHubAppKind, handlerAssert: func(t *testing.T) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) @@ -488,8 +518,9 @@ func TestCreateGitHubApp(t *testing.T) { expectedErr: errors.New("expected 201 statusCode, got: 200"), }, { - name: "server error", - domain: types.BatchesGitHubAppDomain, + name: "server error", + domain: types.BatchesGitHubAppDomain, + gitHubAppKind: ghtypes.SiteCredentialGitHubAppKind, handlerAssert: func(t *testing.T) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusInternalServerError) @@ -498,8 +529,9 @@ func TestCreateGitHubApp(t *testing.T) { expectedErr: errors.New("expected 201 statusCode, got: 500"), }, { - name: "invalid html url", - domain: types.BatchesGitHubAppDomain, + name: "invalid html url", + domain: types.BatchesGitHubAppDomain, + gitHubAppKind: ghtypes.SiteCredentialGitHubAppKind, handlerAssert: func(t *testing.T) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusCreated) @@ -517,7 +549,7 @@ func TestCreateGitHubApp(t *testing.T) { srv := httptest.NewServer(test.handlerAssert(t)) defer srv.Close() - app, err := createGitHubApp(srv.URL, test.domain, httpcli.TestExternalClient) + app, err := createGitHubApp(srv.URL, test.domain, httpcli.TestExternalClient, test.gitHubAppKind) if test.expectedErr != nil { require.Error(t, err) assert.EqualError(t, err, test.expectedErr.Error()) diff --git a/cmd/frontend/internal/githubapp/parser.go b/cmd/frontend/internal/githubapp/parser.go new file mode 100644 index 00000000000..aa95c312b08 --- /dev/null +++ b/cmd/frontend/internal/githubapp/parser.go @@ -0,0 +1,30 @@ +package githubapp + +import ( + "strings" + + ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" + "github.com/sourcegraph/sourcegraph/lib/errors" +) + +func parseKind(s *string) (*ghtypes.GitHubAppKind, error) { + if s == nil { + return nil, nil + } + switch strings.ToUpper(*s) { + case "SITE_CREDENTIAL": + kind := ghtypes.SiteCredentialGitHubAppKind + return &kind, nil + case "USER_CREDENTIAL": + kind := ghtypes.UserCredentialGitHubAppKind + return &kind, nil + case "COMMIT_SIGNING": + kind := ghtypes.CommitSigningGitHubAppKind + return &kind, nil + case "REPO_SYNC": + kind := ghtypes.RepoSyncGitHubAppKind + return &kind, nil + default: + return nil, errors.Newf("unknown kind %q", *s) + } +} diff --git a/cmd/worker/internal/githubapps/worker/installation_backfill_test.go b/cmd/worker/internal/githubapps/worker/installation_backfill_test.go index fd830008138..670132f2d17 100644 --- a/cmd/worker/internal/githubapps/worker/installation_backfill_test.go +++ b/cmd/worker/internal/githubapps/worker/installation_backfill_test.go @@ -2,7 +2,6 @@ package worker import ( "context" - "fmt" "testing" "github.com/google/go-github/v55/github" @@ -38,7 +37,6 @@ func TestGitHubInstallationWorker(t *testing.T) { return apps, nil }) ghStore.SyncInstallationsFunc.SetDefaultHook(func(ctx context.Context, app ghtypes.GitHubApp, logger log.Logger, client ghtypes.GitHubAppClient) (errs errors.MultiError) { - fmt.Println("sync installations: ", app.ID) return nil }) diff --git a/internal/batches/reconciler/BUILD.bazel b/internal/batches/reconciler/BUILD.bazel index 604e80272e8..a38ea59fcda 100644 --- a/internal/batches/reconciler/BUILD.bazel +++ b/internal/batches/reconciler/BUILD.bazel @@ -22,6 +22,7 @@ go_library( "//internal/conf", "//internal/database", "//internal/errcode", + "//internal/github_apps/types", "//internal/gitserver", "//internal/gitserver/protocol", "//internal/metrics", diff --git a/internal/batches/reconciler/executor.go b/internal/batches/reconciler/executor.go index 0c225556870..d3335222457 100644 --- a/internal/batches/reconciler/executor.go +++ b/internal/batches/reconciler/executor.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "fmt" + ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" "strings" "sync" "text/template" @@ -593,7 +594,7 @@ func (e *executor) decorateChangesetBody(ctx context.Context) (string, error) { } func loadChangesetSource(ctx context.Context, s *store.Store, sourcer sources.Sourcer, ch *btypes.Changeset, repo *types.Repo) (sources.ChangesetSource, error) { - css, err := sourcer.ForChangeset(ctx, s, ch, sources.AuthenticationStrategyUserCredential, repo) + css, err := sourcer.ForChangeset(ctx, s, ch, sources.AuthenticationStrategyUserCredential, repo, "") if err != nil { switch err { case sources.ErrMissingCredentials: @@ -651,7 +652,7 @@ func (e *executor) runAfterCommit(ctx context.Context, css sources.ChangesetSour // configured for Batch Changes to sign commits on this code host with. if _, ok := css.(*sources.GitHubSource); ok { // Attempt to get a ChangesetSource authenticated with a GitHub App. - css, err = e.sourcer.ForChangeset(ctx, e.tx, e.ch, sources.AuthenticationStrategyGitHubApp, e.remote) + css, err = e.sourcer.ForChangeset(ctx, e.tx, e.ch, sources.AuthenticationStrategyGitHubApp, e.remote, ghtypes.SiteCredentialGitHubAppKind) if err != nil { switch err { case sources.ErrNoGitHubAppConfigured: diff --git a/internal/batches/service/BUILD.bazel b/internal/batches/service/BUILD.bazel index 23c852acded..31d25188841 100644 --- a/internal/batches/service/BUILD.bazel +++ b/internal/batches/service/BUILD.bazel @@ -4,6 +4,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library") go_library( name = "service", srcs = [ + "errors.go", "mocks.go", "service.go", "service_apply_batch_change.go", @@ -27,9 +28,14 @@ go_library( "//internal/batches/webhooks", "//internal/database", "//internal/database/locker", + "//internal/encryption", "//internal/errcode", "//internal/extsvc", "//internal/extsvc/auth", + "//internal/extsvc/bitbucketserver", + "//internal/github_apps/auth", + "//internal/github_apps/store", + "//internal/github_apps/types", "//internal/gitserver", "//internal/gitserver/gitdomain", "//internal/httpcli", @@ -45,6 +51,7 @@ go_library( "//lib/batches/on", "//lib/batches/template", "//lib/errors", + "//lib/pointers", "//schema", "@com_github_gobwas_glob//:glob", "@com_github_grafana_regexp//:regexp", @@ -75,6 +82,7 @@ go_test( "//internal/api", "//internal/auth", "//internal/batches/reconciler", + "//internal/batches/sources", "//internal/batches/sources/testing", "//internal/batches/store", "//internal/batches/testing", diff --git a/internal/batches/service/errors.go b/internal/batches/service/errors.go new file mode 100644 index 00000000000..493649d66d0 --- /dev/null +++ b/internal/batches/service/errors.go @@ -0,0 +1,25 @@ +package service + +import "fmt" + +type ErrDuplicateCredential struct{} + +func (e ErrDuplicateCredential) Error() string { + return "a credential for this code host already exists" +} + +func (e ErrDuplicateCredential) Extensions() map[string]any { + return map[string]any{"code": "ErrDuplicateCredential"} +} + +type ErrVerifyCredentialFailed struct { + SourceErr error +} + +func (e ErrVerifyCredentialFailed) Error() string { + return fmt.Sprintf("Failed to verify the credential:\n```\n%s\n```\n", e.SourceErr) +} + +func (e ErrVerifyCredentialFailed) Extensions() map[string]any { + return map[string]any{"code": "ErrVerifyCredentialFailed"} +} diff --git a/internal/batches/service/service.go b/internal/batches/service/service.go index 30f4b3629e4..6e07438af09 100644 --- a/internal/batches/service/service.go +++ b/internal/batches/service/service.go @@ -23,8 +23,14 @@ import ( btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/batches/webhooks" "github.com/sourcegraph/sourcegraph/internal/database" + "github.com/sourcegraph/sourcegraph/internal/encryption" "github.com/sourcegraph/sourcegraph/internal/errcode" + "github.com/sourcegraph/sourcegraph/internal/extsvc" extsvcauth "github.com/sourcegraph/sourcegraph/internal/extsvc/auth" + "github.com/sourcegraph/sourcegraph/internal/extsvc/bitbucketserver" + ghauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" + ghstore "github.com/sourcegraph/sourcegraph/internal/github_apps/store" + ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" "github.com/sourcegraph/sourcegraph/internal/httpcli" "github.com/sourcegraph/sourcegraph/internal/jsonc" "github.com/sourcegraph/sourcegraph/internal/metrics" @@ -34,6 +40,7 @@ import ( batcheslib "github.com/sourcegraph/sourcegraph/lib/batches" "github.com/sourcegraph/sourcegraph/lib/batches/template" "github.com/sourcegraph/sourcegraph/lib/errors" + "github.com/sourcegraph/sourcegraph/lib/pointers" "github.com/sourcegraph/sourcegraph/schema" ) @@ -1255,10 +1262,10 @@ func (s *Service) FetchUsernameForBitbucketServerToken(ctx context.Context, exte defer endObservation(1, observation.Args{}) // Get a changeset source for the external service and use the given authenticator. - css, err := s.sourcer.ForExternalService(ctx, s.store, &extsvcauth.OAuthBearerToken{Token: token}, store.GetExternalServiceIDsOpts{ + css, err := s.sourcer.ForExternalService(ctx, s.store, &extsvcauth.OAuthBearerToken{Token: token}, sources.ForExternalServiceOpts{ ExternalServiceType: externalServiceType, ExternalServiceID: externalServiceID, - }) + }, sources.AuthenticationStrategyUserCredential) if err != nil { return "", err } @@ -1283,21 +1290,30 @@ type usernameSource interface { var _ usernameSource = &sources.BitbucketServerSource{} +type ValidateAuthenticatorArgs struct { + ExternalServiceID string + ExternalServiceType string + Username *string + GitHubAppKind ghtypes.GitHubAppKind +} + // ValidateAuthenticator creates a ChangesetSource, configures it with the given // authenticator and validates it can correctly access the remote server. -func (s *Service) ValidateAuthenticator(ctx context.Context, externalServiceID, externalServiceType string, a extsvcauth.Authenticator) (err error) { +func (s *Service) ValidateAuthenticator(ctx context.Context, a extsvcauth.Authenticator, as sources.AuthenticationStrategy, args ValidateAuthenticatorArgs) (err error) { ctx, _, endObservation := s.operations.validateAuthenticator.With(ctx, &err, observation.Args{}) defer endObservation(1, observation.Args{}) if Mocks.ValidateAuthenticator != nil { - return Mocks.ValidateAuthenticator(ctx, externalServiceID, externalServiceType, a) + return Mocks.ValidateAuthenticator(ctx, args.ExternalServiceID, args.ExternalServiceType, a) } // Get a changeset source for the external service and use the given authenticator. - css, err := s.sourcer.ForExternalService(ctx, s.store, a, store.GetExternalServiceIDsOpts{ - ExternalServiceType: externalServiceType, - ExternalServiceID: externalServiceID, - }) + css, err := s.sourcer.ForExternalService(ctx, s.store, a, sources.ForExternalServiceOpts{ + ExternalServiceType: args.ExternalServiceType, + ExternalServiceID: args.ExternalServiceID, + GitHubAppAccount: pointers.Deref(args.Username, ""), + GitHubAppKind: args.GitHubAppKind, + }, as) if err != nil { return err } @@ -1798,3 +1814,211 @@ func (s *Service) GetChangesetsByIDs(ctx context.Context, batchChange int64, ids func (s *Service) enqueueBatchChangeWebhook(ctx context.Context, eventType string, id graphql.ID) { webhooks.EnqueueBatchChange(ctx, s.logger, s.store, eventType, id) } + +type CreateBatchChangesUserCredentialArgs struct { + ExternalServiceURL string + ExternalServiceType string + UserID int32 + Credential string + Username *string + GitHubAppID int + GitHubAppKind ghtypes.GitHubAppKind +} + +func (s *Service) CreateBatchChangesUserCredential(ctx context.Context, as sources.AuthenticationStrategy, args CreateBatchChangesUserCredentialArgs) (*database.UserCredential, error) { + // 🚨 SECURITY: Check that the requesting user can create the credential. + if err := auth.CheckSiteAdminOrSameUser(ctx, s.store.DatabaseDB(), args.UserID); err != nil { + return nil, err + } + + if as == "" { + return nil, errors.New("authentication strategy must be specified") + } + + if as == sources.AuthenticationStrategyGitHubApp && args.GitHubAppID == 0 { + return nil, errors.Newf("GithubAppID must be specified when authenticationStrategy is %s", as) + } + + // Throw error documented in schema.graphql. + userCredentialScope := database.UserCredentialScope{ + Domain: database.UserCredentialDomainBatches, + ExternalServiceID: args.ExternalServiceURL, + ExternalServiceType: args.ExternalServiceType, + UserID: args.UserID, + GitHubAppID: args.GitHubAppID, + } + existing, err := s.store.UserCredentials().GetByScope(ctx, userCredentialScope) + if err != nil && !errcode.IsNotFound(err) { + return nil, err + } + if existing != nil { + return nil, ErrDuplicateCredential{} + } + + a, err := s.generateAuthenticatorForCredential(ctx, generateAuthenticatorForCredentialArgs{ + externalServiceType: args.ExternalServiceType, + externalServiceURL: args.ExternalServiceURL, + credential: args.Credential, + username: args.Username, + authenticationStrategy: as, + gitHubAppKind: args.GitHubAppKind, + githubAppID: args.GitHubAppID, + githubAppStore: s.store.GitHubAppsStore(), + }) + if err != nil { + return nil, err + } + cred, err := s.store.UserCredentials().Create(ctx, userCredentialScope, a) + if err != nil { + return nil, err + } + + return cred, nil +} + +type CreateBatchChangesSiteCredentialArgs struct { + ExternalServiceURL string + ExternalServiceType string + Credential string + Username *string + GitHubAppID int + GitHubAppKind ghtypes.GitHubAppKind +} + +func (s *Service) CreateBatchChangesSiteCredential(ctx context.Context, as sources.AuthenticationStrategy, args CreateBatchChangesSiteCredentialArgs) (*btypes.SiteCredential, error) { + // 🚨 SECURITY: Check that a site credential can only be created + // by a site-admin. + if err := auth.CheckCurrentUserIsSiteAdmin(ctx, s.store.DatabaseDB()); err != nil { + return nil, err + } + + if as == "" { + return nil, errors.New("authentication strategy must be specified") + } + + // Throw error documented in schema.graphql. + existing, err := s.store.GetSiteCredential(ctx, store.GetSiteCredentialOpts{ + ExternalServiceType: args.ExternalServiceType, + ExternalServiceID: args.ExternalServiceURL, + }) + if err != nil && err != store.ErrNoResults { + return nil, err + } + if existing != nil { + return nil, ErrDuplicateCredential{} + } + + a, err := s.generateAuthenticatorForCredential(ctx, generateAuthenticatorForCredentialArgs{ + externalServiceType: args.ExternalServiceType, + externalServiceURL: args.ExternalServiceURL, + credential: args.Credential, + username: args.Username, + gitHubAppKind: args.GitHubAppKind, + githubAppID: args.GitHubAppID, + authenticationStrategy: as, + githubAppStore: s.store.GitHubAppsStore(), + }) + if err != nil { + return nil, err + } + cred := &btypes.SiteCredential{ + ExternalServiceType: args.ExternalServiceType, + ExternalServiceID: args.ExternalServiceURL, + GitHubAppID: args.GitHubAppID, + } + if err := s.store.CreateSiteCredential(ctx, cred, a); err != nil { + return nil, err + } + + return cred, nil +} + +type generateAuthenticatorForCredentialArgs struct { + externalServiceType string + externalServiceURL string + credential string + username *string + authenticationStrategy sources.AuthenticationStrategy + gitHubAppKind ghtypes.GitHubAppKind + githubAppID int + githubAppStore ghstore.GitHubAppsStore +} + +func (s *Service) generateAuthenticatorForCredential(ctx context.Context, args generateAuthenticatorForCredentialArgs) (extsvcauth.Authenticator, error) { + var a extsvcauth.Authenticator + keypair, err := encryption.GenerateRSAKey() + if err != nil { + return nil, err + } + + if args.authenticationStrategy == sources.AuthenticationStrategyGitHubApp { + auther, err := ghauth.CreateAuthenticatorForCredential(ctx, args.githubAppID, ghauth.CreateAuthenticatorForCredentialOpts{ + GitHubAppStore: args.githubAppStore, + }) + if err != nil { + return nil, err + } + a = auther + } else if args.externalServiceType == extsvc.TypeBitbucketServer { + // We need to fetch the username for the token, as just an OAuth token isn't enough for some reason.. + username, err := s.FetchUsernameForBitbucketServerToken(ctx, args.externalServiceURL, args.externalServiceType, args.credential) + if err != nil { + if bitbucketserver.IsUnauthorized(err) { + return nil, &ErrVerifyCredentialFailed{SourceErr: err} + } + return nil, err + } + a = &extsvcauth.BasicAuthWithSSH{ + BasicAuth: extsvcauth.BasicAuth{Username: username, Password: args.credential}, + PrivateKey: keypair.PrivateKey, + PublicKey: keypair.PublicKey, + Passphrase: keypair.Passphrase, + } + } else if args.externalServiceType == extsvc.TypeBitbucketCloud { + a = &extsvcauth.BasicAuthWithSSH{ + BasicAuth: extsvcauth.BasicAuth{Username: *args.username, Password: args.credential}, + PrivateKey: keypair.PrivateKey, + PublicKey: keypair.PublicKey, + Passphrase: keypair.Passphrase, + } + } else if args.externalServiceType == extsvc.TypeAzureDevOps { + a = &extsvcauth.BasicAuthWithSSH{ + BasicAuth: extsvcauth.BasicAuth{Username: *args.username, Password: args.credential}, + PrivateKey: keypair.PrivateKey, + PublicKey: keypair.PublicKey, + Passphrase: keypair.Passphrase, + } + } else if args.externalServiceType == extsvc.TypeGerrit { + a = &extsvcauth.BasicAuthWithSSH{ + BasicAuth: extsvcauth.BasicAuth{Username: *args.username, Password: args.credential}, + PrivateKey: keypair.PrivateKey, + PublicKey: keypair.PublicKey, + Passphrase: keypair.Passphrase, + } + } else if args.externalServiceType == extsvc.TypePerforce { + a = &extsvcauth.BasicAuthWithSSH{ + BasicAuth: extsvcauth.BasicAuth{Username: *args.username, Password: args.credential}, + PrivateKey: keypair.PrivateKey, + PublicKey: keypair.PublicKey, + Passphrase: keypair.Passphrase, + } + } else { + a = &extsvcauth.OAuthBearerTokenWithSSH{ + OAuthBearerToken: extsvcauth.OAuthBearerToken{Token: args.credential}, + PrivateKey: keypair.PrivateKey, + PublicKey: keypair.PublicKey, + Passphrase: keypair.Passphrase, + } + } + + // Validate the newly created authenticator. + if err := s.ValidateAuthenticator(ctx, a, args.authenticationStrategy, ValidateAuthenticatorArgs{ + ExternalServiceID: args.externalServiceURL, + ExternalServiceType: args.externalServiceType, + Username: args.username, + GitHubAppKind: args.gitHubAppKind, + }); err != nil { + return nil, &ErrVerifyCredentialFailed{SourceErr: err} + } + return a, nil +} diff --git a/internal/batches/service/service_test.go b/internal/batches/service/service_test.go index 156d8f813cb..98250c23fbb 100644 --- a/internal/batches/service/service_test.go +++ b/internal/batches/service/service_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "github.com/sourcegraph/sourcegraph/internal/batches/sources" "strings" "testing" "time" @@ -1179,11 +1180,15 @@ index e5af166..d44c3fc 100644 t.Run("valid", func(t *testing.T) { fakeSource.AuthenticatorIsValid = true fakeSource.ValidateAuthenticatorCalled = false + validateArgs := ValidateAuthenticatorArgs{ + ExternalServiceID: "https://github.com/", + ExternalServiceType: extsvc.KindToType(extsvc.TypeGitHub), + } if err := svc.ValidateAuthenticator( ctx, - "https://github.com/", - extsvc.TypeGitHub, &extsvcauth.OAuthBearerToken{Token: "test123"}, + sources.AuthenticationStrategyUserCredential, + validateArgs, ); err != nil { t.Fatal(err) } @@ -1194,11 +1199,15 @@ index e5af166..d44c3fc 100644 t.Run("invalid", func(t *testing.T) { fakeSource.AuthenticatorIsValid = false fakeSource.ValidateAuthenticatorCalled = false + validateArgs := ValidateAuthenticatorArgs{ + ExternalServiceID: "https://github.com/", + ExternalServiceType: extsvc.KindToType(extsvc.TypeGitHub), + } if err := svc.ValidateAuthenticator( ctx, - "https://github.com/", - extsvc.TypeGitHub, &extsvcauth.OAuthBearerToken{Token: "test123"}, + sources.AuthenticationStrategyUserCredential, + validateArgs, ); err == nil { t.Fatal("unexpected nil-error returned from ValidateAuthenticator") } diff --git a/internal/batches/sources/BUILD.bazel b/internal/batches/sources/BUILD.bazel index 032493b09c6..df063b4e34f 100644 --- a/internal/batches/sources/BUILD.bazel +++ b/internal/batches/sources/BUILD.bazel @@ -42,6 +42,7 @@ go_library( "//internal/extsvc/versions", "//internal/github_apps/auth", "//internal/github_apps/store", + "//internal/github_apps/types", "//internal/gitserver", "//internal/gitserver/gitdomain", "//internal/gitserver/protocol", diff --git a/internal/batches/sources/sources.go b/internal/batches/sources/sources.go index 040dc2ba53f..addbc78852f 100644 --- a/internal/batches/sources/sources.go +++ b/internal/batches/sources/sources.go @@ -3,6 +3,7 @@ package sources import ( "context" "fmt" + ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" "net/url" "sort" "strings" @@ -107,13 +108,13 @@ type Sourcer interface { // // If the changeset was not created by a batch change, then a site credential will be // used. If another AuthenticationStrategy is specified, then it will be used. - ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, as AuthenticationStrategy, repo *types.Repo) (ChangesetSource, error) + ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, as AuthenticationStrategy, repo *types.Repo, gitHubAppKind ghtypes.GitHubAppKind) (ChangesetSource, error) // ForUser returns a ChangesetSource for changesets on the given repo. // It will be authenticated with the given authenticator. ForUser(ctx context.Context, tx SourcerStore, uid int32, repo *types.Repo) (ChangesetSource, error) // ForExternalService returns a ChangesetSource based on the provided external service opts. // It will be authenticated with the given authenticator. - ForExternalService(ctx context.Context, tx SourcerStore, au auth.Authenticator, opts store.GetExternalServiceIDsOpts) (ChangesetSource, error) + ForExternalService(ctx context.Context, tx SourcerStore, au auth.Authenticator, opts ForExternalServiceOpts, as AuthenticationStrategy) (ChangesetSource, error) } // NewSourcer returns a new Sourcer to be used in Batch Changes. @@ -137,7 +138,7 @@ func newSourcer(cf *httpcli.Factory, csf changesetSourceFactory) Sourcer { } } -func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, as AuthenticationStrategy, targetRepo *types.Repo) (ChangesetSource, error) { +func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes.Changeset, as AuthenticationStrategy, targetRepo *types.Repo, gitHubAppKind ghtypes.GitHubAppKind) (ChangesetSource, error) { repo, err := tx.Repos().Get(ctx, ch.RepoID) if err != nil { return nil, errors.Wrap(err, "loading changeset repo") @@ -198,7 +199,10 @@ func (s *sourcer) ForChangeset(ctx context.Context, tx SourcerStore, ch *btypes. } } - return withGitHubAppAuthenticator(ctx, tx, css, extSvc, owner) + if gitHubAppKind == "" { + return nil, errors.New("ForChangeset with AuthenticationStrategyGitHubApp must be called with a gitHubAppKind, but was called with empty string.") + } + return withGitHubAppAuthenticator(ctx, tx, css, extSvc, owner, gitHubAppKind) } if ch.OwnedByBatchChangeID != 0 { @@ -228,13 +232,23 @@ func (s *sourcer) ForUser(ctx context.Context, tx SourcerStore, uid int32, repo return withAuthenticatorForUser(ctx, tx, css, uid, repo) } -func (s *sourcer) ForExternalService(ctx context.Context, tx SourcerStore, au auth.Authenticator, opts store.GetExternalServiceIDsOpts) (ChangesetSource, error) { +type ForExternalServiceOpts struct { + ExternalServiceID string + ExternalServiceType string + GitHubAppAccount string + GitHubAppKind ghtypes.GitHubAppKind +} + +func (s *sourcer) ForExternalService(ctx context.Context, tx SourcerStore, au auth.Authenticator, opts ForExternalServiceOpts, as AuthenticationStrategy) (ChangesetSource, error) { // Empty authenticators are not allowed. if au == nil { return nil, ErrMissingCredentials } - extSvcIDs, err := tx.GetExternalServiceIDs(ctx, opts) + extSvcIDs, err := tx.GetExternalServiceIDs(ctx, store.GetExternalServiceIDsOpts{ + ExternalServiceType: opts.ExternalServiceType, + ExternalServiceID: opts.ExternalServiceID, + }) if err != nil { return nil, errors.Wrap(err, "loading external service IDs") } @@ -250,6 +264,9 @@ func (s *sourcer) ForExternalService(ctx context.Context, tx SourcerStore, au au if err != nil { return nil, err } + if as == AuthenticationStrategyGitHubApp { + return withGitHubAppAuthenticator(ctx, tx, css, extSvc, opts.GitHubAppAccount, opts.GitHubAppKind) + } return css.WithAuthenticator(au) } @@ -352,7 +369,7 @@ func loadBatchChange(ctx context.Context, tx getBatchChanger, id int64) (*btypes // App has been configured for it, ErrNoGitHubAppConfigured is returned. If a batches // domain GitHub App has been configured, but no installation exists for the given // account, ErrNoGitHubAppInstallation is returned. -func withGitHubAppAuthenticator(ctx context.Context, tx SourcerStore, css ChangesetSource, extSvc *types.ExternalService, account string) (ChangesetSource, error) { +func withGitHubAppAuthenticator(ctx context.Context, tx SourcerStore, css ChangesetSource, extSvc *types.ExternalService, account string, kind ghtypes.GitHubAppKind) (ChangesetSource, error) { if extSvc.Kind != extsvc.KindGitHub { return nil, ErrExternalServiceNotGitHub } @@ -372,7 +389,7 @@ func withGitHubAppAuthenticator(ctx context.Context, tx SourcerStore, css Change } baseURL = extsvc.NormalizeBaseURL(baseURL) - app, err := tx.GitHubAppsStore().GetByDomain(ctx, types.BatchesGitHubAppDomain, baseURL.String()) + app, err := tx.GitHubAppsStore().GetByDomainAndKind(ctx, types.BatchesGitHubAppDomain, kind, baseURL.String()) if err != nil { return nil, ErrNoGitHubAppConfigured } diff --git a/internal/batches/sources/sources_test.go b/internal/batches/sources/sources_test.go index cc4d64b67e1..1103eef9c2f 100644 --- a/internal/batches/sources/sources_test.go +++ b/internal/batches/sources/sources_test.go @@ -411,7 +411,7 @@ func TestSourcer_ForChangeset(t *testing.T) { return want, nil }) - have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo) + have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo, "") assert.NoError(t, err) assert.Same(t, want, have) }) @@ -452,7 +452,7 @@ func TestSourcer_ForChangeset(t *testing.T) { return want, nil }) - have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo) + have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo, "") assert.NoError(t, err) assert.Same(t, want, have) }) @@ -487,13 +487,13 @@ func TestSourcer_ForChangeset(t *testing.T) { tx.ExternalServicesFunc.SetDefaultReturn(extsvcStore) css := NewMockChangesetSource() - _, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo) + _, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo, "") assert.Error(t, err) }) t.Run("with GH App", func(t *testing.T) { ghaStore := ghastore.NewMockGitHubAppsStore() - ghaStore.GetByDomainFunc.SetDefaultHook(func(ctx context.Context, domain types.GitHubAppDomain, baseUrl string) (*ghatypes.GitHubApp, error) { + ghaStore.GetByDomainAndKindFunc.SetDefaultHook(func(ctx context.Context, domain types.GitHubAppDomain, kind ghatypes.GitHubAppKind, baseUrl string) (*ghatypes.GitHubApp, error) { assert.EqualValues(t, types.BatchesGitHubAppDomain, domain) assert.EqualValues(t, config.Url, baseUrl) ghApp := &ghatypes.GitHubApp{ @@ -537,7 +537,7 @@ func TestSourcer_ForChangeset(t *testing.T) { return want, nil }) - have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyGitHubApp, repo) + have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyGitHubApp, repo, ghatypes.SiteCredentialGitHubAppKind) assert.NoError(t, err) assert.Same(t, want, have) }) @@ -545,7 +545,7 @@ func TestSourcer_ForChangeset(t *testing.T) { t.Run("with GH Ap (forked changeset)", func(t *testing.T) { forkedRepoNamespace := "some-forked-org" ghaStore := ghastore.NewMockGitHubAppsStore() - ghaStore.GetByDomainFunc.SetDefaultHook(func(ctx context.Context, domain types.GitHubAppDomain, baseUrl string) (*ghatypes.GitHubApp, error) { + ghaStore.GetByDomainAndKindFunc.SetDefaultHook(func(ctx context.Context, domain types.GitHubAppDomain, kind ghatypes.GitHubAppKind, baseUrl string) (*ghatypes.GitHubApp, error) { assert.EqualValues(t, types.BatchesGitHubAppDomain, domain) assert.EqualValues(t, config.Url, baseUrl) ghApp := &ghatypes.GitHubApp{ @@ -600,7 +600,7 @@ func TestSourcer_ForChangeset(t *testing.T) { }, } - have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyGitHubApp, targetRepo) + have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyGitHubApp, targetRepo, ghatypes.SiteCredentialGitHubAppKind) assert.NoError(t, err) assert.Same(t, want, have) }) @@ -633,7 +633,7 @@ func TestSourcer_ForChangeset(t *testing.T) { return want, nil }) - have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo) + have, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo, "") assert.NoError(t, err) assert.Same(t, want, have) }) @@ -659,7 +659,7 @@ func TestSourcer_ForChangeset(t *testing.T) { want := errors.New("validator was called") css.ValidateAuthenticatorFunc.SetDefaultReturn(want) - _, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo) + _, err := newMockSourcer(css).ForChangeset(ctx, tx, ch, AuthenticationStrategyUserCredential, repo, "") assert.Error(t, err) }) }) diff --git a/internal/batches/sources/testing/BUILD.bazel b/internal/batches/sources/testing/BUILD.bazel index 05a7c30cf57..f2c53f328b4 100644 --- a/internal/batches/sources/testing/BUILD.bazel +++ b/internal/batches/sources/testing/BUILD.bazel @@ -8,9 +8,9 @@ go_library( visibility = ["//:__subpackages__"], deps = [ "//internal/batches/sources", - "//internal/batches/store", "//internal/batches/types", "//internal/extsvc/auth", + "//internal/github_apps/types", "//internal/gitserver/protocol", "//internal/types", "//lib/errors", diff --git a/internal/batches/sources/testing/fake.go b/internal/batches/sources/testing/fake.go index ee2618a8d4b..b75a39910d3 100644 --- a/internal/batches/sources/testing/fake.go +++ b/internal/batches/sources/testing/fake.go @@ -2,9 +2,9 @@ package testing import ( "context" + ghtypes "github.com/sourcegraph/sourcegraph/internal/github_apps/types" "github.com/sourcegraph/sourcegraph/internal/batches/sources" - "github.com/sourcegraph/sourcegraph/internal/batches/store" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/extsvc/auth" "github.com/sourcegraph/sourcegraph/internal/gitserver/protocol" @@ -25,7 +25,7 @@ type fakeSourcer struct { source sources.ChangesetSource } -func (s *fakeSourcer) ForChangeset(ctx context.Context, tx sources.SourcerStore, ch *btypes.Changeset, as sources.AuthenticationStrategy, repo *types.Repo) (sources.ChangesetSource, error) { +func (s *fakeSourcer) ForChangeset(ctx context.Context, tx sources.SourcerStore, ch *btypes.Changeset, as sources.AuthenticationStrategy, repo *types.Repo, gitHubAppKind ghtypes.GitHubAppKind) (sources.ChangesetSource, error) { return s.source, s.err } @@ -33,7 +33,7 @@ func (s *fakeSourcer) ForUser(ctx context.Context, tx sources.SourcerStore, uid return s.source, s.err } -func (s *fakeSourcer) ForExternalService(ctx context.Context, tx sources.SourcerStore, au auth.Authenticator, opts store.GetExternalServiceIDsOpts) (sources.ChangesetSource, error) { +func (s *fakeSourcer) ForExternalService(ctx context.Context, tx sources.SourcerStore, au auth.Authenticator, opts sources.ForExternalServiceOpts, as sources.AuthenticationStrategy) (sources.ChangesetSource, error) { return s.source, s.err } diff --git a/internal/batches/syncer/syncer.go b/internal/batches/syncer/syncer.go index 590ef12a18a..a62d6459bd4 100644 --- a/internal/batches/syncer/syncer.go +++ b/internal/batches/syncer/syncer.go @@ -497,7 +497,7 @@ func (s *changesetSyncer) SyncChangeset(ctx context.Context, id int64) error { } srcer := sources.NewSourcer(s.httpFactory) - source, err := srcer.ForChangeset(ctx, s.syncStore, cs, sources.AuthenticationStrategyUserCredential, repo) + source, err := srcer.ForChangeset(ctx, s.syncStore, cs, sources.AuthenticationStrategyUserCredential, repo, "") if err != nil { if errors.Is(err, store.ErrDeletedNamespace) { syncLogger.Debug("SyncChangeset skipping changeset: namespace deleted") diff --git a/internal/batches/types/site_credential.go b/internal/batches/types/site_credential.go index 020d67e33c8..c6dff4a2f42 100644 --- a/internal/batches/types/site_credential.go +++ b/internal/batches/types/site_credential.go @@ -2,9 +2,10 @@ package types import ( "context" - ghauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" "time" + ghauth "github.com/sourcegraph/sourcegraph/internal/github_apps/auth" + "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/extsvc/auth" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -22,7 +23,7 @@ type SiteCredential struct { } // IsGitHubApp returns true if the site credential is a GitHub App. -func (sc *SiteCredential) IsGitHubApp() bool { return sc.GitHubAppID != 0 } +func (sc *SiteCredential) IsGitHubApp() bool { return sc.GitHubAppID > 0 } // Authenticator decrypts and creates the authenticator associated with the site credential. func (sc *SiteCredential) Authenticator(ctx context.Context, opts ghauth.CreateAuthenticatorForCredentialOpts) (auth.Authenticator, error) { diff --git a/internal/database/user_credentials.go b/internal/database/user_credentials.go index e38a4c41527..1b34fd9d8e9 100644 --- a/internal/database/user_credentials.go +++ b/internal/database/user_credentials.go @@ -40,7 +40,7 @@ type UserCredential struct { } // IsGitHubApp returns true if the user credential is a GitHub App. -func (uc *UserCredential) IsGitHubApp() bool { return uc.GitHubAppID != 0 } +func (uc *UserCredential) IsGitHubApp() bool { return uc.GitHubAppID > 0 } // Authenticator decrypts and creates the authenticator associated with the user credential. func (uc *UserCredential) Authenticator(ctx context.Context, opts ghauth.CreateAuthenticatorForCredentialOpts) (auth.Authenticator, error) { diff --git a/internal/github_apps/store/mocks_temp.go b/internal/github_apps/store/mocks_temp.go index 9a649dbd932..4a363d64fb5 100644 --- a/internal/github_apps/store/mocks_temp.go +++ b/internal/github_apps/store/mocks_temp.go @@ -34,9 +34,9 @@ type MockGitHubAppsStore struct { // GetByAppIDFunc is an instance of a mock function object controlling // the behavior of the method GetByAppID. GetByAppIDFunc *GitHubAppsStoreGetByAppIDFunc - // GetByDomainFunc is an instance of a mock function object controlling - // the behavior of the method GetByDomain. - GetByDomainFunc *GitHubAppsStoreGetByDomainFunc + // GetByDomainAndKindFunc is an instance of a mock function object + // controlling the behavior of the method GetByDomainAndKind. + GetByDomainAndKindFunc *GitHubAppsStoreGetByDomainAndKindFunc // GetByIDFunc is an instance of a mock function object controlling the // behavior of the method GetByID. GetByIDFunc *GitHubAppsStoreGetByIDFunc @@ -91,8 +91,8 @@ func NewMockGitHubAppsStore() *MockGitHubAppsStore { return }, }, - GetByDomainFunc: &GitHubAppsStoreGetByDomainFunc{ - defaultHook: func(context.Context, types1.GitHubAppDomain, string) (r0 *types.GitHubApp, r1 error) { + GetByDomainAndKindFunc: &GitHubAppsStoreGetByDomainAndKindFunc{ + defaultHook: func(context.Context, types1.GitHubAppDomain, types.GitHubAppKind, string) (r0 *types.GitHubApp, r1 error) { return }, }, @@ -168,9 +168,9 @@ func NewStrictMockGitHubAppsStore() *MockGitHubAppsStore { panic("unexpected invocation of MockGitHubAppsStore.GetByAppID") }, }, - GetByDomainFunc: &GitHubAppsStoreGetByDomainFunc{ - defaultHook: func(context.Context, types1.GitHubAppDomain, string) (*types.GitHubApp, error) { - panic("unexpected invocation of MockGitHubAppsStore.GetByDomain") + GetByDomainAndKindFunc: &GitHubAppsStoreGetByDomainAndKindFunc{ + defaultHook: func(context.Context, types1.GitHubAppDomain, types.GitHubAppKind, string) (*types.GitHubApp, error) { + panic("unexpected invocation of MockGitHubAppsStore.GetByDomainAndKind") }, }, GetByIDFunc: &GitHubAppsStoreGetByIDFunc{ @@ -238,8 +238,8 @@ func NewMockGitHubAppsStoreFrom(i GitHubAppsStore) *MockGitHubAppsStore { GetByAppIDFunc: &GitHubAppsStoreGetByAppIDFunc{ defaultHook: i.GetByAppID, }, - GetByDomainFunc: &GitHubAppsStoreGetByDomainFunc{ - defaultHook: i.GetByDomain, + GetByDomainAndKindFunc: &GitHubAppsStoreGetByDomainAndKindFunc{ + defaultHook: i.GetByDomainAndKind, }, GetByIDFunc: &GitHubAppsStoreGetByIDFunc{ defaultHook: i.GetByID, @@ -707,35 +707,37 @@ func (c GitHubAppsStoreGetByAppIDFuncCall) Results() []interface{} { return []interface{}{c.Result0, c.Result1} } -// GitHubAppsStoreGetByDomainFunc describes the behavior when the -// GetByDomain method of the parent MockGitHubAppsStore instance is invoked. -type GitHubAppsStoreGetByDomainFunc struct { - defaultHook func(context.Context, types1.GitHubAppDomain, string) (*types.GitHubApp, error) - hooks []func(context.Context, types1.GitHubAppDomain, string) (*types.GitHubApp, error) - history []GitHubAppsStoreGetByDomainFuncCall +// GitHubAppsStoreGetByDomainAndKindFunc describes the behavior when the +// GetByDomainAndKind method of the parent MockGitHubAppsStore instance is +// invoked. +type GitHubAppsStoreGetByDomainAndKindFunc struct { + defaultHook func(context.Context, types1.GitHubAppDomain, types.GitHubAppKind, string) (*types.GitHubApp, error) + hooks []func(context.Context, types1.GitHubAppDomain, types.GitHubAppKind, string) (*types.GitHubApp, error) + history []GitHubAppsStoreGetByDomainAndKindFuncCall mutex sync.Mutex } -// GetByDomain delegates to the next hook function in the queue and stores -// the parameter and result values of this invocation. -func (m *MockGitHubAppsStore) GetByDomain(v0 context.Context, v1 types1.GitHubAppDomain, v2 string) (*types.GitHubApp, error) { - r0, r1 := m.GetByDomainFunc.nextHook()(v0, v1, v2) - m.GetByDomainFunc.appendCall(GitHubAppsStoreGetByDomainFuncCall{v0, v1, v2, r0, r1}) +// GetByDomainAndKind delegates to the next hook function in the queue and +// stores the parameter and result values of this invocation. +func (m *MockGitHubAppsStore) GetByDomainAndKind(v0 context.Context, v1 types1.GitHubAppDomain, v2 types.GitHubAppKind, v3 string) (*types.GitHubApp, error) { + r0, r1 := m.GetByDomainAndKindFunc.nextHook()(v0, v1, v2, v3) + m.GetByDomainAndKindFunc.appendCall(GitHubAppsStoreGetByDomainAndKindFuncCall{v0, v1, v2, v3, r0, r1}) return r0, r1 } -// SetDefaultHook sets function that is called when the GetByDomain method -// of the parent MockGitHubAppsStore instance is invoked and the hook queue -// is empty. -func (f *GitHubAppsStoreGetByDomainFunc) SetDefaultHook(hook func(context.Context, types1.GitHubAppDomain, string) (*types.GitHubApp, error)) { +// SetDefaultHook sets function that is called when the GetByDomainAndKind +// method of the parent MockGitHubAppsStore instance is invoked and the hook +// queue is empty. +func (f *GitHubAppsStoreGetByDomainAndKindFunc) SetDefaultHook(hook func(context.Context, types1.GitHubAppDomain, types.GitHubAppKind, string) (*types.GitHubApp, error)) { f.defaultHook = hook } // PushHook adds a function to the end of hook queue. Each invocation of the -// GetByDomain method of the parent MockGitHubAppsStore instance invokes the -// hook at the front of the queue and discards it. After the queue is empty, -// the default hook function is invoked for any future action. -func (f *GitHubAppsStoreGetByDomainFunc) PushHook(hook func(context.Context, types1.GitHubAppDomain, string) (*types.GitHubApp, error)) { +// GetByDomainAndKind method of the parent MockGitHubAppsStore instance +// invokes the hook at the front of the queue and discards it. After the +// queue is empty, the default hook function is invoked for any future +// action. +func (f *GitHubAppsStoreGetByDomainAndKindFunc) PushHook(hook func(context.Context, types1.GitHubAppDomain, types.GitHubAppKind, string) (*types.GitHubApp, error)) { f.mutex.Lock() f.hooks = append(f.hooks, hook) f.mutex.Unlock() @@ -743,20 +745,20 @@ func (f *GitHubAppsStoreGetByDomainFunc) PushHook(hook func(context.Context, typ // SetDefaultReturn calls SetDefaultHook with a function that returns the // given values. -func (f *GitHubAppsStoreGetByDomainFunc) SetDefaultReturn(r0 *types.GitHubApp, r1 error) { - f.SetDefaultHook(func(context.Context, types1.GitHubAppDomain, string) (*types.GitHubApp, error) { +func (f *GitHubAppsStoreGetByDomainAndKindFunc) SetDefaultReturn(r0 *types.GitHubApp, r1 error) { + f.SetDefaultHook(func(context.Context, types1.GitHubAppDomain, types.GitHubAppKind, string) (*types.GitHubApp, error) { return r0, r1 }) } // PushReturn calls PushHook with a function that returns the given values. -func (f *GitHubAppsStoreGetByDomainFunc) PushReturn(r0 *types.GitHubApp, r1 error) { - f.PushHook(func(context.Context, types1.GitHubAppDomain, string) (*types.GitHubApp, error) { +func (f *GitHubAppsStoreGetByDomainAndKindFunc) PushReturn(r0 *types.GitHubApp, r1 error) { + f.PushHook(func(context.Context, types1.GitHubAppDomain, types.GitHubAppKind, string) (*types.GitHubApp, error) { return r0, r1 }) } -func (f *GitHubAppsStoreGetByDomainFunc) nextHook() func(context.Context, types1.GitHubAppDomain, string) (*types.GitHubApp, error) { +func (f *GitHubAppsStoreGetByDomainAndKindFunc) nextHook() func(context.Context, types1.GitHubAppDomain, types.GitHubAppKind, string) (*types.GitHubApp, error) { f.mutex.Lock() defer f.mutex.Unlock() @@ -769,26 +771,27 @@ func (f *GitHubAppsStoreGetByDomainFunc) nextHook() func(context.Context, types1 return hook } -func (f *GitHubAppsStoreGetByDomainFunc) appendCall(r0 GitHubAppsStoreGetByDomainFuncCall) { +func (f *GitHubAppsStoreGetByDomainAndKindFunc) appendCall(r0 GitHubAppsStoreGetByDomainAndKindFuncCall) { f.mutex.Lock() f.history = append(f.history, r0) f.mutex.Unlock() } -// History returns a sequence of GitHubAppsStoreGetByDomainFuncCall objects -// describing the invocations of this function. -func (f *GitHubAppsStoreGetByDomainFunc) History() []GitHubAppsStoreGetByDomainFuncCall { +// History returns a sequence of GitHubAppsStoreGetByDomainAndKindFuncCall +// objects describing the invocations of this function. +func (f *GitHubAppsStoreGetByDomainAndKindFunc) History() []GitHubAppsStoreGetByDomainAndKindFuncCall { f.mutex.Lock() - history := make([]GitHubAppsStoreGetByDomainFuncCall, len(f.history)) + history := make([]GitHubAppsStoreGetByDomainAndKindFuncCall, len(f.history)) copy(history, f.history) f.mutex.Unlock() return history } -// GitHubAppsStoreGetByDomainFuncCall is an object that describes an -// invocation of method GetByDomain on an instance of MockGitHubAppsStore. -type GitHubAppsStoreGetByDomainFuncCall struct { +// GitHubAppsStoreGetByDomainAndKindFuncCall is an object that describes an +// invocation of method GetByDomainAndKind on an instance of +// MockGitHubAppsStore. +type GitHubAppsStoreGetByDomainAndKindFuncCall struct { // Arg0 is the value of the 1st argument passed to this method // invocation. Arg0 context.Context @@ -797,7 +800,10 @@ type GitHubAppsStoreGetByDomainFuncCall struct { Arg1 types1.GitHubAppDomain // Arg2 is the value of the 3rd argument passed to this method // invocation. - Arg2 string + Arg2 types.GitHubAppKind + // Arg3 is the value of the 4th argument passed to this method + // invocation. + Arg3 string // Result0 is the value of the 1st result returned from this method // invocation. Result0 *types.GitHubApp @@ -808,13 +814,13 @@ type GitHubAppsStoreGetByDomainFuncCall struct { // Args returns an interface slice containing the arguments of this // invocation. -func (c GitHubAppsStoreGetByDomainFuncCall) Args() []interface{} { - return []interface{}{c.Arg0, c.Arg1, c.Arg2} +func (c GitHubAppsStoreGetByDomainAndKindFuncCall) Args() []interface{} { + return []interface{}{c.Arg0, c.Arg1, c.Arg2, c.Arg3} } // Results returns an interface slice containing the results of this // invocation. -func (c GitHubAppsStoreGetByDomainFuncCall) Results() []interface{} { +func (c GitHubAppsStoreGetByDomainAndKindFuncCall) Results() []interface{} { return []interface{}{c.Result0, c.Result1} } diff --git a/internal/github_apps/store/store.go b/internal/github_apps/store/store.go index 7a05da3e1e0..224c9ceaf5e 100644 --- a/internal/github_apps/store/store.go +++ b/internal/github_apps/store/store.go @@ -69,8 +69,8 @@ type GitHubAppsStore interface { // GetBySlug retrieves a GitHub App from the database by slug and base url GetBySlug(ctx context.Context, slug string, baseURL string) (*ghtypes.GitHubApp, error) - // GetByDomain retrieves a GitHub App from the database by domain and base url - GetByDomain(ctx context.Context, domain itypes.GitHubAppDomain, baseURL string) (*ghtypes.GitHubApp, error) + // GetByDomainAndKind retrieves a GitHub App from the database by domain and kind and base url + GetByDomainAndKind(ctx context.Context, domain itypes.GitHubAppDomain, kind ghtypes.GitHubAppKind, baseURL string) (*ghtypes.GitHubApp, error) // WithEncryptionKey sets encryption key on store. Returns a new GitHubAppsStore WithEncryptionKey(key encryption.Key) GitHubAppsStore @@ -126,9 +126,17 @@ func (s *gitHubAppsStore) Create(ctx context.Context, app *ghtypes.GitHubApp) (i domain = itypes.ReposGitHubAppDomain } + // Backwards compatibility for apps that did not set the GitHubAppKind. + kind := app.Kind + if kind == "" { + kind = ghtypes.RepoSyncGitHubAppKind + } else if !kind.Valid() { + return -1, errors.New(fmt.Sprintf("The GitHubAppKind %s is not valid.", kind)) + } + // We enforce that GitHub Apps created in the "batches" domain are for unique instance URLs. - if domain == itypes.BatchesGitHubAppDomain { - existingGHApp, err := s.GetByDomain(ctx, domain, baseURL.String()) + if domain == itypes.BatchesGitHubAppDomain && kind == ghtypes.CommitSigningGitHubAppKind { + existingGHApp, err := s.GetByDomainAndKind(ctx, domain, kind, baseURL.String()) // An error is expected if no existing app was found, but we double-check that // we didn't get a different, unrelated error if _, ok := err.(ErrNoGitHubAppFound); !ok { @@ -139,14 +147,6 @@ func (s *gitHubAppsStore) Create(ctx context.Context, app *ghtypes.GitHubApp) (i } } - // Backwards compatibility for apps that did not set the GitHubAppKind. - kind := app.Kind - if kind == "" { - kind = ghtypes.RepoSyncGitHubAppKind - } else if !kind.Valid() { - return -1, errors.New(fmt.Sprintf("The GitHubAppKind %s is not valid.", kind)) - } - query := sqlf.Sprintf(`INSERT INTO github_apps (app_id, name, domain, slug, base_url, app_url, client_id, client_secret, private_key, encryption_key_id, logo, kind) VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, %s) @@ -178,6 +178,7 @@ func scanGitHubApp(s dbutil.Scanner) (*ghtypes.GitHubApp, error) { &app.WebhookID, &app.PrivateKey, &app.EncryptionKey, + &app.Kind, &app.Logo, &app.CreatedAt, &app.UpdatedAt) @@ -269,10 +270,10 @@ func (s *gitHubAppsStore) Update(ctx context.Context, id int, app *ghtypes.GitHu } query := sqlf.Sprintf(`UPDATE github_apps - SET app_id = %s, name = %s, domain = %s, slug = %s, base_url = %s, app_url = %s, client_id = %s, client_secret = %s, webhook_id = %d, private_key = %s, encryption_key_id = %s, logo = %s, updated_at = NOW() + SET app_id = %s, name = %s, domain = %s, slug = %s, base_url = %s, app_url = %s, client_id = %s, client_secret = %s, webhook_id = %d, private_key = %s, encryption_key_id = %s, kind = %s, logo = %s, updated_at = NOW() WHERE id = %s - RETURNING id, app_id, name, domain, slug, base_url, app_url, client_id, client_secret, webhook_id, private_key, encryption_key_id, logo, created_at, updated_at`, - app.AppID, app.Name, app.Domain, app.Slug, app.BaseURL, app.AppURL, app.ClientID, clientSecret, app.WebhookID, privateKey, keyID, app.Logo, id) + RETURNING id, app_id, name, domain, slug, base_url, app_url, client_id, client_secret, webhook_id, private_key, encryption_key_id, kind, logo, created_at, updated_at`, + app.AppID, app.Name, app.Domain, app.Slug, app.BaseURL, app.AppURL, app.ClientID, clientSecret, app.WebhookID, privateKey, keyID, app.Kind, app.Logo, id) app, ok, err := scanFirstGitHubApp(s.Query(ctx, query)) if err != nil { return nil, err @@ -347,6 +348,7 @@ func (s *gitHubAppsStore) get(ctx context.Context, where *sqlf.Query) (*ghtypes. webhook_id, private_key, encryption_key_id, + kind, logo, created_at, updated_at @@ -387,6 +389,7 @@ func (s *gitHubAppsStore) list(ctx context.Context, where *sqlf.Query) ([]*ghtyp webhook_id, private_key, encryption_key_id, + kind, logo, created_at, updated_at @@ -421,9 +424,9 @@ func (s *gitHubAppsStore) GetBySlug(ctx context.Context, slug string, baseURL st return s.get(ctx, sqlf.Sprintf(`slug = %s AND %s`, slug, baseURLWhere(baseURL))) } -// GetByDomain retrieves a GitHub App from the database by domain and base url -func (s *gitHubAppsStore) GetByDomain(ctx context.Context, domain itypes.GitHubAppDomain, baseURL string) (*ghtypes.GitHubApp, error) { - return s.get(ctx, sqlf.Sprintf(`domain = %s AND %s`, domain, baseURLWhere(baseURL))) +// GetByDomainAndKind retrieves a GitHub App from the database by domain, kind and base url +func (s *gitHubAppsStore) GetByDomainAndKind(ctx context.Context, domain itypes.GitHubAppDomain, kind ghtypes.GitHubAppKind, baseURL string) (*ghtypes.GitHubApp, error) { + return s.get(ctx, sqlf.Sprintf(`domain = %s AND kind = %s AND %s`, domain, kind, baseURLWhere(baseURL))) } // List lists all GitHub Apps in the store diff --git a/internal/github_apps/store/store_test.go b/internal/github_apps/store/store_test.go index 7b11739691d..9f2c69bccbd 100644 --- a/internal/github_apps/store/store_test.go +++ b/internal/github_apps/store/store_test.go @@ -26,7 +26,6 @@ import ( func newTestStore(t *testing.T) *gitHubAppsStore { logger := logtest.Scoped(t) return &gitHubAppsStore{Store: basestore.NewWithHandle(basestore.NewHandleWithDB(logger, dbtest.NewDB(t), sql.TxOptions{}))} - } func TestCreateGitHubApp(t *testing.T) { @@ -124,6 +123,7 @@ func TestUpdateGitHubApp(t *testing.T) { ClientID: "abc123", ClientSecret: "secret", PrivateKey: "private-key", + Kind: ghtypes.RepoSyncGitHubAppKind, } id, err := store.Create(ctx, app) @@ -142,6 +142,7 @@ func TestUpdateGitHubApp(t *testing.T) { ClientID: "def456", ClientSecret: "updated-secret", PrivateKey: "updated-private-key", + Kind: ghtypes.RepoSyncGitHubAppKind, } fetched, err := store.Update(ctx, 1, updated) @@ -368,6 +369,7 @@ func TestGetByDomain(t *testing.T) { ClientSecret: "secret", PrivateKey: "private-key", Logo: "logo.png", + Kind: ghtypes.RepoSyncGitHubAppKind, } batchesApp := &ghtypes.GitHubApp{ @@ -381,6 +383,7 @@ func TestGetByDomain(t *testing.T) { ClientSecret: "secret", PrivateKey: "private-key", Logo: "logo.png", + Kind: ghtypes.CommitSigningGitHubAppKind, } _, err := store.Create(ctx, repoApp) @@ -389,7 +392,7 @@ func TestGetByDomain(t *testing.T) { require.NoError(t, err) domain := types.ReposGitHubAppDomain - fetched, err := store.GetByDomain(ctx, domain, "https://github.com/") + fetched, err := store.GetByDomainAndKind(ctx, domain, ghtypes.RepoSyncGitHubAppKind, "https://github.com/") require.NoError(t, err) require.Equal(t, repoApp.AppID, fetched.AppID) require.Equal(t, repoApp.Name, fetched.Name) @@ -404,15 +407,15 @@ func TestGetByDomain(t *testing.T) { require.NotZero(t, fetched.UpdatedAt) // does not exist - fetched, err = store.GetByDomain(ctx, domain, "https://myCompany.github.com/") + fetched, err = store.GetByDomainAndKind(ctx, domain, ghtypes.RepoSyncGitHubAppKind, "https://myCompany.github.com/") require.Nil(t, fetched) require.Error(t, err) notFoundErr, ok := err.(ErrNoGitHubAppFound) require.Equal(t, ok, true) - require.Equal(t, notFoundErr.Error(), "no app exists matching criteria: 'domain = repos AND trim(trailing '/' from base_url) = https://myCompany.github.com'") + require.Equal(t, notFoundErr.Error(), "no app exists matching criteria: 'domain = repos AND kind = REPO_SYNC AND trim(trailing '/' from base_url) = https://myCompany.github.com'") domain = types.BatchesGitHubAppDomain - fetched, err = store.GetByDomain(ctx, domain, "https://github.com/") + fetched, err = store.GetByDomainAndKind(ctx, domain, ghtypes.CommitSigningGitHubAppKind, "https://github.com/") require.NoError(t, err) require.Equal(t, batchesApp.AppID, fetched.AppID) } @@ -448,6 +451,7 @@ func TestListGitHubApp(t *testing.T) { ClientSecret: "secret", PrivateKey: "private-key", Logo: "logo.png", + Kind: ghtypes.RepoSyncGitHubAppKind, } _, err := store.Create(ctx, repoApp) @@ -873,6 +877,7 @@ func TestTrailingSlashesInBaseURL(t *testing.T) { ClientSecret: "secret", PrivateKey: "private-key", Logo: "logo.png", + Kind: ghtypes.RepoSyncGitHubAppKind, } id, err := store.Create(ctx, app) diff --git a/internal/github_apps/types/BUILD.bazel b/internal/github_apps/types/BUILD.bazel index 22c766ce48c..af2f2719c8e 100644 --- a/internal/github_apps/types/BUILD.bazel +++ b/internal/github_apps/types/BUILD.bazel @@ -8,6 +8,7 @@ go_library( visibility = ["//:__subpackages__"], deps = [ "//internal/types", + "//lib/errors", "@com_github_google_go_github_v55//github", ], ) diff --git a/internal/github_apps/types/types.go b/internal/github_apps/types/types.go index b472f212cc2..7bc23e5afdd 100644 --- a/internal/github_apps/types/types.go +++ b/internal/github_apps/types/types.go @@ -7,6 +7,7 @@ import ( gogithub "github.com/google/go-github/v55/github" "github.com/sourcegraph/sourcegraph/internal/types" + "github.com/sourcegraph/sourcegraph/lib/errors" ) // GitHubApp represents a GitHub App. @@ -51,6 +52,13 @@ func (s GitHubAppKind) Valid() bool { } } +func (s GitHubAppKind) Validate() (GitHubAppKind, error) { + if !s.Valid() { + return "", errors.Newf("Not a valid GitHubAppKind: %s", s) + } + return s, nil +} + // GitHubAppInstallation represents an installation of a GitHub App. type GitHubAppInstallation struct { ID int