diff --git a/cmd/embeddings/shared/BUILD.bazel b/cmd/embeddings/shared/BUILD.bazel index 96b554655e2..79275c0a096 100644 --- a/cmd/embeddings/shared/BUILD.bazel +++ b/cmd/embeddings/shared/BUILD.bazel @@ -17,7 +17,6 @@ go_library( "//internal/actor", "//internal/api", "//internal/authz", - "//internal/authz/providers", "//internal/authz/subrepoperms", "//internal/conf", "//internal/conf/conftypes", diff --git a/cmd/embeddings/shared/main.go b/cmd/embeddings/shared/main.go index a15de1cb8a1..f9bc7abdee1 100644 --- a/cmd/embeddings/shared/main.go +++ b/cmd/embeddings/shared/main.go @@ -13,7 +13,6 @@ import ( "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/authz" - "github.com/sourcegraph/sourcegraph/internal/authz/providers" srp "github.com/sourcegraph/sourcegraph/internal/authz/subrepoperms" "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/conf/conftypes" @@ -50,8 +49,6 @@ func Main(ctx context.Context, observationCtx *observation.Context, ready servic sqlDB := mustInitializeFrontendDB(observationCtx) db := database.NewDB(logger, sqlDB) - go setAuthzProviders(ctx, db) - repoStore := db.Repos() repoEmbeddingJobsStore := repo.NewRepoEmbeddingJobsStore(db) @@ -170,17 +167,6 @@ func mustInitializeFrontendDB(observationCtx *observation.Context) *sql.DB { return db } -// SetAuthzProviders periodically refreshes the global authz providers. This changes the repositories that are visible for reads based on the -// current actor stored in an operation's context, which is likely an internal actor for many of -// the jobs configured in this service. This also enables repository update operations to fetch -// permissions from code hosts. -func setAuthzProviders(ctx context.Context, db database.DB) { - for range time.NewTicker(providers.RefreshInterval(conf.Get())).C { - allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db) - authz.SetProviders(allowAccessByDefault, authzProviders) - } -} - func handlePanic(logger log.Logger, next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { defer func() { diff --git a/cmd/frontend/internal/app/jscontext/jscontext.go b/cmd/frontend/internal/app/jscontext/jscontext.go index a13fb54150c..5e981459dfd 100644 --- a/cmd/frontend/internal/app/jscontext/jscontext.go +++ b/cmd/frontend/internal/app/jscontext/jscontext.go @@ -298,7 +298,7 @@ func NewJSContextFromRequest(req *http.Request, db database.DB) JSContext { // Auth providers authProviders := []authProviderInfo{} // Explicitly initialise array, otherwise it gets marshalled to null instead of [] - _, authzProviders := authz.GetProviders() + authzProviders := authz.GetProviders() for _, p := range providers.SortedProviders() { commonConfig := providers.GetAuthProviderCommon(p) if commonConfig.Hidden { diff --git a/cmd/frontend/internal/authz/init.go b/cmd/frontend/internal/authz/init.go index bd45c67b517..2b413fbe54f 100644 --- a/cmd/frontend/internal/authz/init.go +++ b/cmd/frontend/internal/authz/init.go @@ -69,10 +69,9 @@ func Init( return nil } - _, _, _, _, invalidConnections := providers.ProvidersFromConfig(ctx, conf.Get(), db) + _, _, _, invalidConnections := providers.ProvidersFromConfig(ctx, conf.Get(), db) - // We currently support three types of authz providers: GitHub, GitLab and Bitbucket Server. - authzTypes := make(map[string]struct{}, 3) + authzTypes := map[string]struct{}{} for _, conn := range invalidConnections { authzTypes[conn] = struct{}{} } @@ -133,10 +132,9 @@ func Init( }) go func() { - t := time.NewTicker(5 * time.Second) - for range t.C { - allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db) - authz.SetProviders(allowAccessByDefault, authzProviders) + for range time.NewTicker(providers.RefreshInterval(conf.Get())).C { + authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db) + authz.SetProviders(authzProviders) } }() diff --git a/cmd/frontend/internal/authz/resolvers/resolver.go b/cmd/frontend/internal/authz/resolvers/resolver.go index 471979f03c4..1e7fd935c11 100644 --- a/cmd/frontend/internal/authz/resolvers/resolver.go +++ b/cmd/frontend/internal/authz/resolvers/resolver.go @@ -492,7 +492,7 @@ func (r *Resolver) AuthzProviderTypes(ctx context.Context) ([]string, error) { if err := auth.CheckCurrentUserIsSiteAdmin(ctx, r.db); err != nil { return nil, err } - _, providers := authz.GetProviders() + providers := authz.GetProviders() providerTypes := make([]string, 0, len(providers)) for _, p := range providers { providerTypes = append(providerTypes, p.ServiceType()) diff --git a/cmd/frontend/internal/authz/resolvers/resolver_test.go b/cmd/frontend/internal/authz/resolvers/resolver_test.go index 6fc7ef3212e..d7e312fbd2b 100644 --- a/cmd/frontend/internal/authz/resolvers/resolver_test.go +++ b/cmd/frontend/internal/authz/resolvers/resolver_test.go @@ -1200,8 +1200,8 @@ func TestResolver_AuthzProviderTypes(t *testing.T) { ctx := actor.WithActor(context.Background(), &actor.Actor{UID: 1}) ghProvider := github.NewProvider("https://github.com", github.ProviderOptions{GitHubURL: mustURL(t, "https://github.com")}) - authz.SetProviders(false, []authz.Provider{ghProvider}) - defer authz.SetProviders(true, nil) + authz.SetProviders([]authz.Provider{ghProvider}) + defer authz.SetProviders(nil) result, err := (&Resolver{db: db}).AuthzProviderTypes(ctx) assert.NoError(t, err) assert.Equal(t, []string{"github"}, result) @@ -1471,7 +1471,6 @@ func TestResolver_RepositoryPermissionsInfo(t *testing.T) { perms.LoadRepoPermissionsFunc.SetDefaultHook(func(_ context.Context, repoID int32) ([]authz.Permission, error) { return []authz.Permission{{RepoID: repoID, UserID: 42, UpdatedAt: clock()}}, nil }) - perms.IsRepoUnrestrictedFunc.SetDefaultReturn(false, nil) perms.ListRepoPermissionsFunc.SetDefaultReturn([]*database.RepoPermission{{User: &types.User{ID: 42}}}, nil) syncJobs := dbmocks.NewStrictMockPermissionSyncJobStore() diff --git a/cmd/frontend/internal/batches/resolvers/batch_spec_test.go b/cmd/frontend/internal/batches/resolvers/batch_spec_test.go index bf70261bf72..da95c281927 100644 --- a/cmd/frontend/internal/batches/resolvers/batch_spec_test.go +++ b/cmd/frontend/internal/batches/resolvers/batch_spec_test.go @@ -56,6 +56,7 @@ func TestBatchSpecResolver(t *testing.T) { userID := bt.CreateTestUser(t, db, false).ID adminID := bt.CreateTestUser(t, db, true).ID orgID := bt.CreateTestOrg(t, db, orgname, userID).ID + bt.MockRepoPermissions(t, db, userID, repo.ID) spec, err := btypes.NewBatchSpecFromRaw(bt.TestRawBatchSpec) if err != nil { @@ -275,6 +276,7 @@ func TestBatchSpecResolver_BatchSpecCreatedFromRaw(t *testing.T) { userCtx := actor.WithActor(ctx, actor.FromUser(user.ID)) rs, extSvc := bt.CreateTestRepos(t, ctx, db, 3) + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID) bstore := store.New(db, observation.TestContextTB(t), nil) @@ -495,6 +497,7 @@ func TestBatchSpecResolver_BatchSpecCreatedFromRaw(t *testing.T) { want.ViewerCanAdminister = false want.ViewerCanRetry = false otherUser := bt.CreateTestUser(t, db, false) + bt.MockRepoPermissions(t, db, otherUser.ID, rs[0].ID, rs[1].ID, rs[2].ID) otherUserCtx := actor.WithActor(ctx, actor.FromUser(otherUser.ID)) queryAndAssertBatchSpec(t, otherUserCtx, s, apiID, want) } diff --git a/cmd/frontend/internal/batches/resolvers/permissions_test.go b/cmd/frontend/internal/batches/resolvers/permissions_test.go index 387e09cea3d..db7652c8de2 100644 --- a/cmd/frontend/internal/batches/resolvers/permissions_test.go +++ b/cmd/frontend/internal/batches/resolvers/permissions_test.go @@ -84,6 +84,8 @@ func TestPermissionLevels(t *testing.T) { if err := repoStore.Create(ctx, repo); err != nil { t.Fatal(err) } + bt.MockRepoPermissions(t, db, userID, repo.ID) + bt.MockRepoPermissions(t, db, nonOrgUserID, repo.ID) changeset := &btypes.Changeset{ RepoID: repo.ID, @@ -1392,6 +1394,7 @@ func TestRepositoryPermissions(t *testing.T) { } t.Run("BatchChange and changesets", func(t *testing.T) { + bt.MockRepoPermissions(t, db, userID, repos[0].ID, repos[1].ID) // Create 2 changesets for 2 repositories changesetBaseRefOid := "f00b4r" changesetHeadRefOid := "b4rf00" @@ -1523,6 +1526,7 @@ func TestRepositoryPermissions(t *testing.T) { }) t.Run("BatchSpec and changesetSpecs", func(t *testing.T) { + bt.MockRepoPermissions(t, db, userID, repos[0].ID, repos[1].ID) batchSpec := &btypes.BatchSpec{ UserID: userID, NamespaceUserID: userID, @@ -1602,6 +1606,7 @@ func TestRepositoryPermissions(t *testing.T) { }) t.Run("BatchSpec and workspaces", func(t *testing.T) { + bt.MockRepoPermissions(t, db, userID, repos[0].ID, repos[1].ID) batchSpec := &btypes.BatchSpec{ UserID: userID, NamespaceUserID: userID, diff --git a/cmd/frontend/internal/cli/serve_cmd.go b/cmd/frontend/internal/cli/serve_cmd.go index a3b1d227f85..b2c6d1321e4 100644 --- a/cmd/frontend/internal/cli/serve_cmd.go +++ b/cmd/frontend/internal/cli/serve_cmd.go @@ -286,7 +286,7 @@ func Main(ctx context.Context, observationCtx *observation.Context, ready servic ExternalServiceURL string `json:"external_service_url"` } - _, providers := authz.GetProviders() + providers := authz.GetProviders() infos := make([]providerInfo, len(providers)) for i, p := range providers { _, id := extsvc.DecodeURN(p.URN()) diff --git a/cmd/gitserver/shared/shared.go b/cmd/gitserver/shared/shared.go index 17ccc06157a..6197a5a17ed 100644 --- a/cmd/gitserver/shared/shared.go +++ b/cmd/gitserver/shared/shared.go @@ -290,12 +290,6 @@ func makeGRPCServer(logger log.Logger, s *server.Server, c *Config) *grpc.Server // getDB initializes a connection to the database and returns a dbutil.DB func getDB(observationCtx *observation.Context) (*sql.DB, error) { - // Gitserver is an internal actor. We rely on the frontend to do authz checks for - // user requests. - // - // This call to SetProviders is here so that calls to GetProviders don't block. - authz.SetProviders(true, []authz.Provider{}) - dsn := conf.GetServiceConnectionValueAndRestartOnChange(func(serviceConnections conftypes.ServiceConnections) string { return serviceConnections.PostgresDSN }) diff --git a/cmd/precise-code-intel-worker/shared/BUILD.bazel b/cmd/precise-code-intel-worker/shared/BUILD.bazel index f6f48ba94b1..7e4c459382c 100644 --- a/cmd/precise-code-intel-worker/shared/BUILD.bazel +++ b/cmd/precise-code-intel-worker/shared/BUILD.bazel @@ -12,7 +12,6 @@ go_library( visibility = ["//visibility:public"], deps = [ "//internal/authz", - "//internal/authz/providers", "//internal/authz/subrepoperms", "//internal/codeintel", "//internal/codeintel/shared", diff --git a/cmd/precise-code-intel-worker/shared/shared.go b/cmd/precise-code-intel-worker/shared/shared.go index f07b3bf8c04..15a65c7957b 100644 --- a/cmd/precise-code-intel-worker/shared/shared.go +++ b/cmd/precise-code-intel-worker/shared/shared.go @@ -10,7 +10,6 @@ import ( "github.com/sourcegraph/log" "github.com/sourcegraph/sourcegraph/internal/authz" - "github.com/sourcegraph/sourcegraph/internal/authz/providers" srp "github.com/sourcegraph/sourcegraph/internal/authz/subrepoperms" "github.com/sourcegraph/sourcegraph/internal/codeintel" codeintelshared "github.com/sourcegraph/sourcegraph/internal/codeintel/shared" @@ -106,21 +105,6 @@ func mustInitializeDB(observationCtx *observation.Context) *sql.DB { log.Scoped("init db").Fatal("Failed to connect to frontend database", log.Error(err)) } - // - // START FLAILING - - ctx := context.Background() - db := database.NewDB(observationCtx.Logger, sqlDB) - go func() { - for range time.NewTicker(providers.RefreshInterval(conf.Get())).C { - allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db) - authz.SetProviders(allowAccessByDefault, authzProviders) - } - }() - - // END FLAILING - // - return sqlDB } diff --git a/cmd/repo-updater/shared/BUILD.bazel b/cmd/repo-updater/shared/BUILD.bazel index 17ca4388363..2aed43be5b2 100644 --- a/cmd/repo-updater/shared/BUILD.bazel +++ b/cmd/repo-updater/shared/BUILD.bazel @@ -19,8 +19,6 @@ go_library( "//cmd/repo-updater/internal/repoupdater", "//cmd/repo-updater/internal/scheduler", "//internal/actor", - "//internal/authz", - "//internal/authz/providers", "//internal/batches", "//internal/batches/syncer", "//internal/codeintel/dependencies", diff --git a/cmd/repo-updater/shared/main.go b/cmd/repo-updater/shared/main.go index 0ec9383ef4f..b3a0c4f8257 100644 --- a/cmd/repo-updater/shared/main.go +++ b/cmd/repo-updater/shared/main.go @@ -23,8 +23,6 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/repo-updater/internal/repoupdater" "github.com/sourcegraph/sourcegraph/cmd/repo-updater/internal/scheduler" "github.com/sourcegraph/sourcegraph/internal/actor" - "github.com/sourcegraph/sourcegraph/internal/authz" - "github.com/sourcegraph/sourcegraph/internal/authz/providers" "github.com/sourcegraph/sourcegraph/internal/batches" "github.com/sourcegraph/sourcegraph/internal/batches/syncer" "github.com/sourcegraph/sourcegraph/internal/codeintel/dependencies" @@ -121,7 +119,6 @@ func Main(ctx context.Context, observationCtx *observation.Context, ready servic } go globals.WatchExternalURL() - go watchAuthzProviders(ctx, db) go watchSyncer(ctx, logger, syncer, updateScheduler, server.ChangesetSyncRegistry) routines := []goroutine.BackgroundRoutine{ @@ -349,22 +346,6 @@ func newUnclonedReposManager(ctx context.Context, logger log.Logger, sched *sche ) } -// TODO: This might clash with what osscmd.Main does. -// watchAuthzProviders updates authz providers if config changes. -func watchAuthzProviders(ctx context.Context, db database.DB) { - go func() { - t := time.NewTicker(providers.RefreshInterval(conf.Get())) - for range t.C { - allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig( - ctx, - conf.Get(), - db, - ) - authz.SetProviders(allowAccessByDefault, authzProviders) - } - }() -} - func mustRegisterMetrics(logger log.Logger, db dbutil.DB) { scanCount := func(sql string) (float64, error) { row := db.QueryRowContext(context.Background(), sql) diff --git a/cmd/syntactic-code-intel-worker/shared/BUILD.bazel b/cmd/syntactic-code-intel-worker/shared/BUILD.bazel index b567ffcab8f..5d297e47c48 100644 --- a/cmd/syntactic-code-intel-worker/shared/BUILD.bazel +++ b/cmd/syntactic-code-intel-worker/shared/BUILD.bazel @@ -14,7 +14,6 @@ go_library( visibility = ["//visibility:public"], deps = [ "//internal/api", - "//internal/authz", "//internal/codeintel/shared", "//internal/codeintel/shared/lsifuploadstore", "//internal/codeintel/syntactic_indexing/jobstore", diff --git a/cmd/syntactic-code-intel-worker/shared/shared.go b/cmd/syntactic-code-intel-worker/shared/shared.go index 8fa14596292..55c375e0dd0 100644 --- a/cmd/syntactic-code-intel-worker/shared/shared.go +++ b/cmd/syntactic-code-intel-worker/shared/shared.go @@ -8,7 +8,6 @@ import ( "github.com/sourcegraph/log" - "github.com/sourcegraph/sourcegraph/internal/authz" codeintelshared "github.com/sourcegraph/sourcegraph/internal/codeintel/shared" "github.com/sourcegraph/sourcegraph/internal/codeintel/syntactic_indexing/jobstore" "github.com/sourcegraph/sourcegraph/internal/conf" @@ -79,16 +78,6 @@ func Main(ctx context.Context, observationCtx *observation.Context, ready servic } func initCodeintelDB(observationCtx *observation.Context, name string) (*sql.DB, error) { - // This is an internal service, so we rely on the - // frontend to do authz checks for user requests. - // Authz checks are enforced by the DB layer - // - // This call to SetProviders is here so that calls to GetProviders don't block. - // Relevant PR: https://github.com/sourcegraph/sourcegraph/pull/15755 - // Relevant issue: https://github.com/sourcegraph/sourcegraph/issues/15962 - - authz.SetProviders(true, []authz.Provider{}) - dsn := conf.GetServiceConnectionValueAndRestartOnChange(func(serviceConnections conftypes.ServiceConnections) string { return serviceConnections.PostgresDSN }) @@ -104,16 +93,6 @@ func initCodeintelDB(observationCtx *observation.Context, name string) (*sql.DB, } func initDB(observationCtx *observation.Context, name string) (*sql.DB, error) { - // This is an internal service, so we rely on the - // frontend to do authz checks for user requests. - // Authz checks are enforced by the DB layer - // - // This call to SetProviders is here so that calls to GetProviders don't block. - // Relevant PR: https://github.com/sourcegraph/sourcegraph/pull/15755 - // Relevant issue: https://github.com/sourcegraph/sourcegraph/issues/15962 - - authz.SetProviders(true, []authz.Provider{}) - dsn := conf.GetServiceConnectionValueAndRestartOnChange(func(serviceConnections conftypes.ServiceConnections) string { return serviceConnections.PostgresDSN }) diff --git a/cmd/worker/internal/authz/integration_test.go b/cmd/worker/internal/authz/integration_test.go index a1b304beacd..df1703614fe 100644 --- a/cmd/worker/internal/authz/integration_test.go +++ b/cmd/worker/internal/authz/integration_test.go @@ -215,8 +215,8 @@ func TestIntegration_GitHubPermissions(t *testing.T) { DB: testDB, }) - authz.SetProviders(false, []authz.Provider{provider}) - defer authz.SetProviders(true, nil) + authz.SetProviders([]authz.Provider{provider}) + defer authz.SetProviders(nil) assertGitHubRepoPermissions(t, ctx, repo.ID, user.ID, uri.String(), syncer, permsStore, []int32{1}) }) @@ -232,8 +232,8 @@ func TestIntegration_GitHubPermissions(t *testing.T) { DB: testDB, }) - authz.SetProviders(false, []authz.Provider{provider}) - defer authz.SetProviders(true, nil) + authz.SetProviders([]authz.Provider{provider}) + defer authz.SetProviders(nil) assertGitHubRepoPermissions(t, ctx, repo.ID, user.ID, uri.String(), syncer, permsStore, []int32{1}) }) @@ -254,8 +254,8 @@ func TestIntegration_GitHubPermissions(t *testing.T) { DB: testDB, }) - authz.SetProviders(false, []authz.Provider{provider}) - defer authz.SetProviders(true, nil) + authz.SetProviders([]authz.Provider{provider}) + defer authz.SetProviders(nil) assertGitHubUserPermissions(t, ctx, user.ID, uri.String(), syncer, permsStore, []int32{1}) }) @@ -271,8 +271,8 @@ func TestIntegration_GitHubPermissions(t *testing.T) { DB: testDB, }) - authz.SetProviders(false, []authz.Provider{provider}) - defer authz.SetProviders(true, nil) + authz.SetProviders([]authz.Provider{provider}) + defer authz.SetProviders(nil) assertGitHubUserPermissions(t, ctx, user.ID, uri.String(), syncer, permsStore, []int32{1}) }) @@ -361,8 +361,8 @@ func TestIntegration_GitHubInternalRepositories(t *testing.T) { DB: testDB, }) - authz.SetProviders(false, []authz.Provider{provider}) - defer authz.SetProviders(true, nil) + authz.SetProviders([]authz.Provider{provider}) + defer authz.SetProviders(nil) repo := types.Repo{ Name: "ghe.sgdev.org/sourcegraph/sourcegraph_internal_repo", @@ -505,8 +505,8 @@ func TestIntegration_GitLabPermissions(t *testing.T) { SyncInternalRepoPermissions: true, }) - authz.SetProviders(false, []authz.Provider{provider}) - defer authz.SetProviders(true, nil) + authz.SetProviders([]authz.Provider{provider}) + defer authz.SetProviders(nil) for _, repo := range testRepos { err = reposStore.RepoStore().Create(ctx, &repo) require.NoError(t, err) diff --git a/cmd/worker/internal/authz/perms_syncer.go b/cmd/worker/internal/authz/perms_syncer.go index 6fe586b78e9..c5e9a25bf84 100644 --- a/cmd/worker/internal/authz/perms_syncer.go +++ b/cmd/worker/internal/authz/perms_syncer.go @@ -340,7 +340,7 @@ func (s *permsSyncerImpl) syncUserPerms(ctx context.Context, userID int32, noPer // providersByServiceID returns a list of authz.Provider configured in the external services. // Keys are ServiceID, e.g. "https://github.com/". func (s *permsSyncerImpl) providersByServiceID() map[string]authz.Provider { - _, ps := authz.GetProviders() + ps := authz.GetProviders() providers := make(map[string]authz.Provider, len(ps)) for _, p := range ps { providers[p.ServiceID()] = p @@ -351,7 +351,7 @@ func (s *permsSyncerImpl) providersByServiceID() map[string]authz.Provider { // providersByURNs returns a list of authz.Provider configured in the external services. // Keys are URN, e.g. "extsvc:github:1". func (s *permsSyncerImpl) providersByURNs() map[string]authz.Provider { - _, ps := authz.GetProviders() + ps := authz.GetProviders() providers := make(map[string]authz.Provider, len(ps)) for _, p := range ps { providers[p.URN()] = p diff --git a/cmd/worker/internal/authz/perms_syncer_test.go b/cmd/worker/internal/authz/perms_syncer_test.go index b26fe120a55..ac2d2af0a7d 100644 --- a/cmd/worker/internal/authz/perms_syncer_test.go +++ b/cmd/worker/internal/authz/perms_syncer_test.go @@ -67,9 +67,9 @@ func TestPermsSyncer_syncUserPerms(t *testing.T) { serviceType: extsvc.TypeGitLab, serviceID: "https://gitlab.com/", } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) extAccount := extsvc.Account{ @@ -157,9 +157,9 @@ func TestPermsSyncer_syncUserPerms_listExternalAccountsError(t *testing.T) { serviceType: extsvc.TypeGitLab, serviceID: "https://gitlab.com/", } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) users := dbmocks.NewMockUserStore() @@ -226,9 +226,9 @@ func TestPermsSyncer_syncUserPerms_fetchAccount(t *testing.T) { serviceType: extsvc.TypeGitHub, serviceID: "https://github.com/", } - authz.SetProviders(false, []authz.Provider{p1, p2}) + authz.SetProviders([]authz.Provider{p1, p2}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) users := dbmocks.NewMockUserStore() @@ -397,9 +397,9 @@ func TestPermsSyncer_syncUserPermsTemporaryProviderError(t *testing.T) { serviceType: extsvc.TypeGitLab, serviceID: "https://gitlab.com/", } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) extAccount := extsvc.Account{ @@ -488,9 +488,9 @@ func TestPermsSyncer_syncUserPermsTemporaryProviderError(t *testing.T) { serviceType: extsvc.TypeGitLab, serviceID: "https://gitlab.com/", } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) extAccount := extsvc.Account{ @@ -605,9 +605,9 @@ func TestPermsSyncer_syncUserPermsTemporaryProviderError(t *testing.T) { serviceType: extsvc.TypeGitLab, serviceID: "https://gitlab.com/", } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) extAccount := extsvc.Account{ @@ -742,9 +742,9 @@ func TestPermsSyncer_syncUserPerms_noPerms(t *testing.T) { serviceType: extsvc.TypeGitLab, serviceID: "https://gitlab.com/", } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) extAccount := extsvc.Account{ @@ -838,9 +838,9 @@ func TestPermsSyncer_syncUserPerms_tokenExpire(t *testing.T) { serviceType: extsvc.TypeGitHub, serviceID: "https://github.com/", } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) extAccount := extsvc.Account{ @@ -935,9 +935,9 @@ func TestPermsSyncer_syncUserPerms_prefixSpecs(t *testing.T) { serviceType: extsvc.TypePerforce, serviceID: "ssl:111.222.333.444:1666", } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) extAccount := extsvc.Account{ @@ -1012,9 +1012,9 @@ func TestPermsSyncer_syncUserPerms_subRepoPermissions(t *testing.T) { serviceType: extsvc.TypePerforce, serviceID: "ssl:111.222.333.444:1666", } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) users := dbmocks.NewMockUserStore() @@ -1183,9 +1183,9 @@ func TestPermsSyncer_syncRepoPerms(t *testing.T) { return nil, errors.New("not supposed to be called") }, } - authz.SetProviders(false, []authz.Provider{p1, p2}) + authz.SetProviders([]authz.Provider{p1, p2}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) mockRepos.ListFunc.SetDefaultReturn( @@ -1269,9 +1269,9 @@ func TestPermsSyncer_syncRepoPerms(t *testing.T) { }, } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) mockRepos.GetFunc.SetDefaultReturn( &types.Repo{ @@ -1319,9 +1319,9 @@ func TestPermsSyncer_syncRepoPerms(t *testing.T) { serviceType: extsvc.TypeGitLab, serviceID: "https://gitlab.com/", } - authz.SetProviders(false, []authz.Provider{p}) + authz.SetProviders([]authz.Provider{p}) t.Cleanup(func() { - authz.SetProviders(true, nil) + authz.SetProviders(nil) }) mockRepos.ListFunc.SetDefaultReturn( diff --git a/cmd/worker/internal/batches/workers/batch_spec_workspace_creator_test.go b/cmd/worker/internal/batches/workers/batch_spec_workspace_creator_test.go index 91b859ae942..1c629d71bfb 100644 --- a/cmd/worker/internal/batches/workers/batch_spec_workspace_creator_test.go +++ b/cmd/worker/internal/batches/workers/batch_spec_workspace_creator_test.go @@ -800,10 +800,11 @@ changesetTemplate: } func TestBatchSpecWorkspaceCreatorProcess_Importing(t *testing.T) { + ctx := actor.WithInternalActor(context.Background()) logger := logtest.Scoped(t) db := database.NewDB(logger, dbtest.NewDB(t)) - repos, _ := bt.CreateTestRepos(t, context.Background(), db, 1) + repos, _ := bt.CreateTestRepos(t, ctx, db, 1) user := bt.CreateTestUser(t, db, true) @@ -820,7 +821,7 @@ importChangesets: ` batchSpec := &btypes.BatchSpec{UserID: user.ID, NamespaceUserID: user.ID, RawSpec: testSpecYAML} - if err := s.CreateBatchSpec(context.Background(), batchSpec); err != nil { + if err := s.CreateBatchSpec(ctx, batchSpec); err != nil { t.Fatal(err) } @@ -829,11 +830,11 @@ importChangesets: resolver := &dummyWorkspaceResolver{} creator := &batchSpecWorkspaceCreator{store: s, logger: logtest.Scoped(t)} - if err := creator.process(context.Background(), resolver.DummyBuilder, job); err != nil { + if err := creator.process(ctx, resolver.DummyBuilder, job); err != nil { t.Fatalf("proces failed: %s", err) } - have, _, err := s.ListChangesetSpecs(context.Background(), store.ListChangesetSpecsOpts{BatchSpecID: batchSpec.ID}) + have, _, err := s.ListChangesetSpecs(ctx, store.ListChangesetSpecsOpts{BatchSpecID: batchSpec.ID}) if err != nil { t.Fatalf("listing specs failed: %s", err) } @@ -859,9 +860,10 @@ importChangesets: func TestBatchSpecWorkspaceCreatorProcess_NoDiff(t *testing.T) { logger := logtest.Scoped(t) + ctx := actor.WithInternalActor(context.Background()) db := database.NewDB(logger, dbtest.NewDB(t)) - repos, _ := bt.CreateTestRepos(t, context.Background(), db, 1) + repos, _ := bt.CreateTestRepos(t, ctx, db, 1) user := bt.CreateTestUser(t, db, true) @@ -878,7 +880,7 @@ importChangesets: ` batchSpec := &btypes.BatchSpec{UserID: user.ID, NamespaceUserID: user.ID, RawSpec: testSpecYAML} - if err := s.CreateBatchSpec(context.Background(), batchSpec); err != nil { + if err := s.CreateBatchSpec(ctx, batchSpec); err != nil { t.Fatal(err) } @@ -887,11 +889,11 @@ importChangesets: resolver := &dummyWorkspaceResolver{} creator := &batchSpecWorkspaceCreator{store: s, logger: logtest.Scoped(t)} - if err := creator.process(context.Background(), resolver.DummyBuilder, job); err != nil { + if err := creator.process(ctx, resolver.DummyBuilder, job); err != nil { t.Fatalf("proces failed: %s", err) } - have, _, err := s.ListChangesetSpecs(context.Background(), store.ListChangesetSpecsOpts{BatchSpecID: batchSpec.ID}) + have, _, err := s.ListChangesetSpecs(ctx, store.ListChangesetSpecsOpts{BatchSpecID: batchSpec.ID}) if err != nil { t.Fatalf("listing specs failed: %s", err) } diff --git a/cmd/worker/internal/permissions/BUILD.bazel b/cmd/worker/internal/permissions/BUILD.bazel index 027f4604362..ee648ca7508 100644 --- a/cmd/worker/internal/permissions/BUILD.bazel +++ b/cmd/worker/internal/permissions/BUILD.bazel @@ -19,8 +19,8 @@ go_library( "//internal/api", "//internal/auth", "//internal/authz", - "//internal/authz/providers", "//internal/conf", + "//internal/conf/conftypes", "//internal/database", "//internal/database/basestore", "//internal/env", @@ -30,6 +30,7 @@ go_library( "//internal/goroutine", "//internal/httpcli", "//internal/jsonc", + "//internal/licensing", "//internal/metrics", "//internal/observation", "//internal/timeutil", @@ -73,6 +74,7 @@ go_test( "//internal/errcode", "//internal/extsvc", "//internal/extsvc/bitbucketserver", + "//internal/licensing", "//internal/observation", "//internal/timeutil", "//internal/types", @@ -81,6 +83,7 @@ go_test( "@com_github_google_go_cmp//cmp", "@com_github_sourcegraph_log//:log", "@com_github_sourcegraph_log//logtest", + "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", ], ) diff --git a/cmd/worker/internal/permissions/perms_syncer_scheduler.go b/cmd/worker/internal/permissions/perms_syncer_scheduler.go index 8baee543e14..db0da4b1792 100644 --- a/cmd/worker/internal/permissions/perms_syncer_scheduler.go +++ b/cmd/worker/internal/permissions/perms_syncer_scheduler.go @@ -12,11 +12,13 @@ import ( workerdb "github.com/sourcegraph/sourcegraph/cmd/worker/shared/init/db" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/auth" - "github.com/sourcegraph/sourcegraph/internal/authz/providers" + "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/conf" + "github.com/sourcegraph/sourcegraph/internal/conf/conftypes" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/env" "github.com/sourcegraph/sourcegraph/internal/goroutine" + "github.com/sourcegraph/sourcegraph/internal/licensing" "github.com/sourcegraph/sourcegraph/internal/metrics" "github.com/sourcegraph/sourcegraph/internal/observation" "github.com/sourcegraph/sourcegraph/internal/timeutil" @@ -79,7 +81,7 @@ func (p *permissionSyncJobScheduler) Routines(_ context.Context, observationCtx context.Background(), goroutine.HandlerFunc( func(ctx context.Context) error { - if providers.PermissionSyncingDisabled(conf.Get()) { + if permissionSyncingDisabled(conf.Get()) { logger.Debug("scheduler disabled due to permission syncing disabled") return nil } @@ -289,3 +291,15 @@ func oldestRepoPermissionsBatchSize() int { } return batchSize } + +// permissionSyncingDisabled returns true if the background permissions syncing is not enabled. +// It is not enabled if: +// - There are no code host connections with authorization or enforcePermissions enabled +// - Not purchased with the current license +// - `disableAutoCodeHostSyncs` site setting is set to true +func permissionSyncingDisabled(cfg conftypes.UnifiedQuerier) bool { + p := authz.GetProviders() + return len(p) == 0 || + licensing.Check(licensing.FeatureACLs) != nil || + cfg.SiteConfig().DisableAutoCodeHostSyncs +} diff --git a/cmd/worker/internal/permissions/perms_syncer_scheduler_test.go b/cmd/worker/internal/permissions/perms_syncer_scheduler_test.go index 48ab479f11a..d9cd590bf2c 100644 --- a/cmd/worker/internal/permissions/perms_syncer_scheduler_test.go +++ b/cmd/worker/internal/permissions/perms_syncer_scheduler_test.go @@ -10,6 +10,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sourcegraph/log" "github.com/sourcegraph/log/logtest" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/sourcegraph/sourcegraph/internal/auth" @@ -18,6 +19,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/database/dbtest" "github.com/sourcegraph/sourcegraph/internal/extsvc" + "github.com/sourcegraph/sourcegraph/internal/licensing" "github.com/sourcegraph/sourcegraph/internal/timeutil" "github.com/sourcegraph/sourcegraph/internal/types" "github.com/sourcegraph/sourcegraph/lib/pointers" @@ -349,3 +351,87 @@ func TestOldestRepoPermissionsBatchSize(t *testing.T) { }) } } + +func TestPermissionSyncingDisabled(t *testing.T) { + authz.SetProviders([]authz.Provider{&mockProvider{}}) + cleanupLicense := licensing.MockCheckFeatureError("") + + t.Cleanup(func() { + authz.SetProviders(nil) + cleanupLicense() + }) + + t.Run("no authz providers", func(t *testing.T) { + authz.SetProviders(nil) + t.Cleanup(func() { + authz.SetProviders([]authz.Provider{&mockProvider{}}) + }) + + assert.True(t, permissionSyncingDisabled(&conf.Unified{})) + }) + + t.Run("permissions user mapping enabled", func(t *testing.T) { + cleanup := mockExplicitPermissions(true) + t.Cleanup(func() { + cleanup() + conf.Mock(nil) + }) + + assert.False(t, permissionSyncingDisabled(&conf.Unified{})) + }) + + t.Run("license does not have acls feature", func(t *testing.T) { + licensing.MockCheckFeatureError("failed") + t.Cleanup(func() { + licensing.MockCheckFeatureError("") + }) + assert.True(t, permissionSyncingDisabled(&conf.Unified{})) + }) + + t.Run("Auto code host syncs disabled", func(t *testing.T) { + assert.True(t, permissionSyncingDisabled(&conf.Unified{SiteConfiguration: schema.SiteConfiguration{DisableAutoCodeHostSyncs: true}})) + }) + + t.Run("Auto code host syncs enabled", func(t *testing.T) { + assert.False(t, permissionSyncingDisabled(&conf.Unified{SiteConfiguration: schema.SiteConfiguration{DisableAutoCodeHostSyncs: false}})) + }) +} + +type mockProvider struct { + codeHost *extsvc.CodeHost + extAcct *extsvc.Account +} + +func (p *mockProvider) FetchAccount(context.Context, *types.User, []*extsvc.Account, []string) (mine *extsvc.Account, err error) { + return p.extAcct, nil +} + +func (p *mockProvider) ServiceType() string { return p.codeHost.ServiceType } +func (p *mockProvider) ServiceID() string { return p.codeHost.ServiceID } +func (p *mockProvider) URN() string { return extsvc.URN(p.codeHost.ServiceType, 0) } + +func (p *mockProvider) ValidateConnection(context.Context) error { return nil } + +func (p *mockProvider) FetchUserPerms(context.Context, *extsvc.Account, authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { + return nil, nil +} + +func (p *mockProvider) FetchUserPermsByToken(context.Context, string, authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { + return nil, nil +} + +func (p *mockProvider) FetchRepoPerms(context.Context, *extsvc.Repository, authz.FetchPermsOptions) ([]extsvc.AccountID, error) { + return nil, nil +} + +func mockExplicitPermissions(enabled bool) func() { + conf.Mock(&conf.Unified{ + SiteConfiguration: schema.SiteConfiguration{ + PermissionsUserMapping: &schema.PermissionsUserMapping{ + Enabled: enabled, + BindID: "email", + }, + }, + }) + return func() { conf.Mock(nil) } +} diff --git a/cmd/worker/shared/main.go b/cmd/worker/shared/main.go index 07a61efc6d9..04ded35dc12 100644 --- a/cmd/worker/shared/main.go +++ b/cmd/worker/shared/main.go @@ -405,7 +405,7 @@ func setAuthzProviders(ctx context.Context, observationCtx *observation.Context) } for range time.NewTicker(providers.RefreshInterval(conf.Get())).C { - allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db) - authz.SetProviders(allowAccessByDefault, authzProviders) + authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db) + authz.SetProviders(authzProviders) } } diff --git a/dev/ci/integration/executors/tester/BUILD.bazel b/dev/ci/integration/executors/tester/BUILD.bazel index 1a258f6dc46..94fc06045ad 100644 --- a/dev/ci/integration/executors/tester/BUILD.bazel +++ b/dev/ci/integration/executors/tester/BUILD.bazel @@ -12,7 +12,6 @@ go_library( visibility = ["//visibility:private"], deps = [ "//dev/ci/integration/executors/tester/config", - "//internal/authz", "//internal/batches/store", "//internal/batches/types", "//internal/database", diff --git a/dev/ci/integration/executors/tester/main.go b/dev/ci/integration/executors/tester/main.go index b118252dba6..f3f2dfce026 100644 --- a/dev/ci/integration/executors/tester/main.go +++ b/dev/ci/integration/executors/tester/main.go @@ -6,7 +6,6 @@ import ( "github.com/sourcegraph/log" - "github.com/sourcegraph/sourcegraph/internal/authz" batchesstore "github.com/sourcegraph/sourcegraph/internal/batches/store" "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/database" @@ -343,9 +342,6 @@ var expectedState = gqltestutil.BatchSpecDeep{ } func initDB(logger log.Logger) (database.DB, error) { - // This call to SetProviders is here so that calls to GetProviders don't block. - authz.SetProviders(true, []authz.Provider{}) - obsCtx := observation.TestContext obsCtx.Logger = logger sqlDB, err := connections.RawNewFrontendDB(&obsCtx, "postgres://sg@127.0.0.1:5433/sg", "") diff --git a/internal/authz/providers/BUILD.bazel b/internal/authz/providers/BUILD.bazel index 8693d27fa8f..df3b583a4e6 100644 --- a/internal/authz/providers/BUILD.bazel +++ b/internal/authz/providers/BUILD.bazel @@ -20,7 +20,6 @@ go_library( "//internal/database", "//internal/extsvc", "//internal/httpcli", - "//internal/licensing", "//internal/types", "//lib/errors", "//schema", diff --git a/internal/authz/providers/authz.go b/internal/authz/providers/authz.go index 9ea878db0ce..1c4b73c9cf8 100644 --- a/internal/authz/providers/authz.go +++ b/internal/authz/providers/authz.go @@ -19,7 +19,6 @@ import ( "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/httpcli" - "github.com/sourcegraph/sourcegraph/internal/licensing" "github.com/sourcegraph/sourcegraph/internal/types" "github.com/sourcegraph/sourcegraph/lib/errors" "github.com/sourcegraph/sourcegraph/schema" @@ -40,7 +39,6 @@ func ProvidersFromConfig( cfg conftypes.SiteConfigQuerier, db database.DB, ) ( - allowAccessByDefault bool, providers []authz.Provider, seriousProblems []string, warnings []string, @@ -48,11 +46,9 @@ func ProvidersFromConfig( ) { logger := log.Scoped("authz") - allowAccessByDefault = true defer func() { if len(seriousProblems) > 0 { logger.Error("Repository authz config was invalid (errors are visible in the UI as an admin user, you should fix ASAP). Restricting access to repositories by default for now to be safe.", log.Strings("seriousProblems", seriousProblems)) - allowAccessByDefault = false } }() @@ -157,14 +153,14 @@ func ProvidersFromConfig( } initResult := github.NewAuthzProviders(ctx, db, gitHubConns, cfg.SiteConfig().AuthProviders, enableGithubInternalRepoVisibility) - initResult.Append(gitlab.NewAuthzProviders(db, cfg.SiteConfig(), gitLabConns)) + initResult.Append(gitlab.NewAuthzProviders(db, gitLabConns, cfg.SiteConfig().AuthProviders)) initResult.Append(bitbucketserver.NewAuthzProviders(bitbucketServerConns)) initResult.Append(perforce.NewAuthzProviders(perforceConns)) - initResult.Append(bitbucketcloud.NewAuthzProviders(db, bitbucketCloudConns, cfg.SiteConfig().AuthProviders)) - initResult.Append(gerrit.NewAuthzProviders(gerritConns, cfg.SiteConfig().AuthProviders)) + initResult.Append(bitbucketcloud.NewAuthzProviders(db, bitbucketCloudConns)) + initResult.Append(gerrit.NewAuthzProviders(gerritConns)) initResult.Append(azuredevops.NewAuthzProviders(db, azuredevopsConns, httpcli.ExternalClient)) - return allowAccessByDefault, initResult.Providers, initResult.Problems, initResult.Warnings, initResult.InvalidConnections + return initResult.Providers, initResult.Problems, initResult.Warnings, initResult.InvalidConnections } func RefreshInterval(cfg conftypes.UnifiedQuerier) time.Duration { @@ -175,18 +171,6 @@ func RefreshInterval(cfg conftypes.UnifiedQuerier) time.Duration { return time.Duration(interval) * time.Second } -// PermissionSyncingDisabled returns true if the background permissions syncing is not enabled. -// It is not enabled if: -// - There are no code host connections with authorization or enforcePermissions enabled -// - Not purchased with the current license -// - `disableAutoCodeHostSyncs` site setting is set to true -func PermissionSyncingDisabled(cfg conftypes.UnifiedQuerier) bool { - _, p := authz.GetProviders() - return len(p) == 0 || - licensing.Check(licensing.FeatureACLs) != nil || - cfg.SiteConfig().DisableAutoCodeHostSyncs -} - var ValidateExternalServiceConfig = database.MakeValidateExternalServiceConfigFunc( []database.GitHubValidatorFunc{github.ValidateAuthz}, []database.GitLabValidatorFunc{gitlab.ValidateAuthz}, diff --git a/internal/authz/providers/authz_test.go b/internal/authz/providers/authz_test.go index 0645bd4fd02..c5b668188e5 100644 --- a/internal/authz/providers/authz_test.go +++ b/internal/authz/providers/authz_test.go @@ -87,13 +87,12 @@ func TestAuthzProvidersFromConfig(t *testing.T) { } tests := []struct { - description string - cfg conf.Unified - gitlabConnections []*schema.GitLabConnection - bitbucketServerConnections []*schema.BitbucketServerConnection - expAuthzAllowAccessByDefault bool - expAuthzProviders func(*testing.T, []authz.Provider) - expSeriousProblems []string + description string + cfg conf.Unified + gitlabConnections []*schema.GitLabConnection + bitbucketServerConnections []*schema.BitbucketServerConnection + expAuthzProviders func(*testing.T, []authz.Provider) + expSeriousProblems []string }{ { description: "1 GitLab connection with authz enabled, 1 GitLab matching auth provider", @@ -119,7 +118,6 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "asdf", }, }, - expAuthzAllowAccessByDefault: true, expAuthzProviders: providersEqual( gitlabAuthzProviderParams{ OAuthOp: gitlab.OAuthProviderOp{ @@ -155,8 +153,7 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "asdf", }, }, - expAuthzAllowAccessByDefault: false, - expSeriousProblems: []string{"Did not find authentication provider matching \"https://gitlab.mine\". Check the [**site configuration**](/site-admin/configuration) to verify an entry in [`auth.providers`](https://sourcegraph.com/docs/admin/auth) exists for https://gitlab.mine."}, + expSeriousProblems: []string{"Did not find authentication provider matching \"https://gitlab.mine\". Check the [**site configuration**](/site-admin/configuration) to verify an entry in [`auth.providers`](https://sourcegraph.com/docs/admin/auth) exists for https://gitlab.mine."}, }, { description: "1 GitLab connection with authz enabled, no GitLab auth provider", @@ -176,8 +173,7 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "asdf", }, }, - expAuthzAllowAccessByDefault: false, - expSeriousProblems: []string{"Did not find authentication provider matching \"https://gitlab.mine\". Check the [**site configuration**](/site-admin/configuration) to verify an entry in [`auth.providers`](https://sourcegraph.com/docs/admin/auth) exists for https://gitlab.mine."}, + expSeriousProblems: []string{"Did not find authentication provider matching \"https://gitlab.mine\". Check the [**site configuration**](/site-admin/configuration) to verify an entry in [`auth.providers`](https://sourcegraph.com/docs/admin/auth) exists for https://gitlab.mine."}, }, { description: "Two GitLab connections with authz enabled, two matching GitLab auth providers", @@ -220,7 +216,6 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "asdf", }, }, - expAuthzAllowAccessByDefault: true, expAuthzProviders: providersEqual( gitlabAuthzProviderParams{ OAuthOp: gitlab.OAuthProviderOp{ @@ -262,8 +257,7 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "asdf", }, }, - expAuthzAllowAccessByDefault: true, - expAuthzProviders: nil, + expAuthzProviders: nil, }, { description: "external auth provider", @@ -291,7 +285,6 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "asdf", }, }, - expAuthzAllowAccessByDefault: true, expAuthzProviders: providersEqual( gitlabAuthzProviderParams{ SudoOp: gitlab.SudoProviderOp{ @@ -325,7 +318,6 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "asdf", }, }, - expAuthzAllowAccessByDefault: true, expAuthzProviders: providersEqual( gitlabAuthzProviderParams{ SudoOp: gitlab.SudoProviderOp{ @@ -348,8 +340,7 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "secret-token", }, }, - expAuthzAllowAccessByDefault: true, - expAuthzProviders: providersEqual(), + expAuthzProviders: providersEqual(), }, { description: "Bitbucket Server Oauth config error", @@ -372,8 +363,7 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "secret-token", }, }, - expAuthzAllowAccessByDefault: false, - expSeriousProblems: []string{"authorization.oauth.signingKey: illegal base64 data at input byte 7"}, + expSeriousProblems: []string{"authorization.oauth.signingKey: illegal base64 data at input byte 7"}, }, { description: "Bitbucket Server exact username matching", @@ -396,7 +386,6 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "secret-token", }, }, - expAuthzAllowAccessByDefault: true, expAuthzProviders: func(t *testing.T, have []authz.Provider) { if len(have) == 0 { t.Fatalf("no providers") @@ -437,7 +426,6 @@ func TestAuthzProvidersFromConfig(t *testing.T) { Token: "asdf", }, }, - expAuthzAllowAccessByDefault: true, expAuthzProviders: providersEqual( gitlabAuthzProviderParams{ OAuthOp: gitlab.OAuthProviderOp{ @@ -487,12 +475,11 @@ func TestAuthzProvidersFromConfig(t *testing.T) { } return svcs, nil }) - allowAccessByDefault, authzProviders, seriousProblems, _, _ := ProvidersFromConfig( + authzProviders, seriousProblems, _, _ := ProvidersFromConfig( context.Background(), staticConfig(test.cfg.SiteConfiguration), db, ) - assert.Equal(t, test.expAuthzAllowAccessByDefault, allowAccessByDefault) if test.expAuthzProviders != nil { test.expAuthzProviders(t, authzProviders) } @@ -733,7 +720,7 @@ func TestAuthzProvidersEnabledACLsDisabled(t *testing.T) { return svcs, nil }) - _, _, seriousProblems, _, invalidConnections := ProvidersFromConfig( + _, seriousProblems, _, invalidConnections := ProvidersFromConfig( context.Background(), staticConfig(test.cfg.SiteConfiguration), db, @@ -759,90 +746,6 @@ func mustURLParse(t *testing.T, u string) *url.URL { return parsed } -type mockProvider struct { - codeHost *extsvc.CodeHost - extAcct *extsvc.Account -} - -func (p *mockProvider) FetchAccount(context.Context, *types.User, []*extsvc.Account, []string) (mine *extsvc.Account, err error) { - return p.extAcct, nil -} - -func (p *mockProvider) ServiceType() string { return p.codeHost.ServiceType } -func (p *mockProvider) ServiceID() string { return p.codeHost.ServiceID } -func (p *mockProvider) URN() string { return extsvc.URN(p.codeHost.ServiceType, 0) } - -func (p *mockProvider) ValidateConnection(context.Context) error { return nil } - -func (p *mockProvider) FetchUserPerms(context.Context, *extsvc.Account, authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { - return nil, nil -} - -func (p *mockProvider) FetchUserPermsByToken(context.Context, string, authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { - return nil, nil -} - -func (p *mockProvider) FetchRepoPerms(context.Context, *extsvc.Repository, authz.FetchPermsOptions) ([]extsvc.AccountID, error) { - return nil, nil -} - -func mockExplicitPermissions(enabled bool) func() { - conf.Mock(&conf.Unified{ - SiteConfiguration: schema.SiteConfiguration{ - PermissionsUserMapping: &schema.PermissionsUserMapping{ - Enabled: enabled, - BindID: "email", - }, - }, - }) - return func() { conf.Mock(nil) } -} - -func TestPermissionSyncingDisabled(t *testing.T) { - authz.SetProviders(true, []authz.Provider{&mockProvider{}}) - cleanupLicense := licensing.MockCheckFeatureError("") - - t.Cleanup(func() { - authz.SetProviders(true, nil) - cleanupLicense() - }) - - t.Run("no authz providers", func(t *testing.T) { - authz.SetProviders(true, nil) - t.Cleanup(func() { - authz.SetProviders(true, []authz.Provider{&mockProvider{}}) - }) - - assert.True(t, PermissionSyncingDisabled(&conf.Unified{})) - }) - - t.Run("permissions user mapping enabled", func(t *testing.T) { - cleanup := mockExplicitPermissions(true) - t.Cleanup(func() { - cleanup() - conf.Mock(nil) - }) - - assert.False(t, PermissionSyncingDisabled(&conf.Unified{})) - }) - - t.Run("license does not have acls feature", func(t *testing.T) { - licensing.MockCheckFeatureError("failed") - t.Cleanup(func() { - licensing.MockCheckFeatureError("") - }) - assert.True(t, PermissionSyncingDisabled(&conf.Unified{})) - }) - - t.Run("Auto code host syncs disabled", func(t *testing.T) { - assert.True(t, PermissionSyncingDisabled(&conf.Unified{SiteConfiguration: schema.SiteConfiguration{DisableAutoCodeHostSyncs: true}})) - }) - - t.Run("Auto code host syncs enabled", func(t *testing.T) { - assert.False(t, PermissionSyncingDisabled(&conf.Unified{SiteConfiguration: schema.SiteConfiguration{DisableAutoCodeHostSyncs: false}})) - }) -} - func TestValidateExternalServiceConfig(t *testing.T) { t.Parallel() t.Cleanup(licensing.TestingSkipFeatureChecks()) diff --git a/internal/authz/providers/bitbucketcloud/authz.go b/internal/authz/providers/bitbucketcloud/authz.go index 856b3ad1eb2..b9e8e6214ec 100644 --- a/internal/authz/providers/bitbucketcloud/authz.go +++ b/internal/authz/providers/bitbucketcloud/authz.go @@ -1,8 +1,6 @@ package bitbucketcloud import ( - "net/url" - "github.com/sourcegraph/sourcegraph/internal/licensing" "github.com/sourcegraph/sourcegraph/internal/authz" @@ -23,24 +21,8 @@ import ( // This constructor does not and should not directly check connectivity to external services - if // desired, callers should use `(*Provider).ValidateConnection` directly to get warnings related // to connection issues. -func NewAuthzProviders(db database.DB, conns []*types.BitbucketCloudConnection, authProviders []schema.AuthProviders) *atypes.ProviderInitResult { +func NewAuthzProviders(db database.DB, conns []*types.BitbucketCloudConnection) *atypes.ProviderInitResult { initResults := &atypes.ProviderInitResult{} - bbcloudAuthProviders := make(map[string]*schema.BitbucketCloudAuthProvider) - for _, p := range authProviders { - if p.Bitbucketcloud != nil { - var id string - bbURL, err := url.Parse(p.Bitbucketcloud.GetURL()) - if err != nil { - // error reporting for this should happen elsewhere, for now just use what is given - id = p.Bitbucketcloud.GetURL() - } else { - // use codehost normalized URL as ID - ch := extsvc.NewCodeHost(bbURL, p.Bitbucketcloud.Type) - id = ch.ServiceID - } - bbcloudAuthProviders[id] = p.Bitbucketcloud - } - } for _, c := range conns { p, err := newAuthzProvider(db, c) diff --git a/internal/authz/providers/bitbucketcloud/authz_test.go b/internal/authz/providers/bitbucketcloud/authz_test.go index a745f2e43a5..b826a898a78 100644 --- a/internal/authz/providers/bitbucketcloud/authz_test.go +++ b/internal/authz/providers/bitbucketcloud/authz_test.go @@ -23,7 +23,6 @@ func TestNewAuthzProviders(t *testing.T) { Url: schema.DefaultBitbucketCloudURL, }, }}, - []schema.AuthProviders{}, ) assertion := assert.New(t) @@ -34,75 +33,47 @@ func TestNewAuthzProviders(t *testing.T) { assertion.Len(initResults.InvalidConnections, 0, "unexpected invalidConnections: %+v", initResults.InvalidConnections) }) - t.Run("no matching auth provider", func(t *testing.T) { + t.Run("default case", func(t *testing.T) { t.Cleanup(licensing.TestingSkipFeatureChecks()) initResults := NewAuthzProviders( db, []*types.BitbucketCloudConnection{ { BitbucketCloudConnection: &schema.BitbucketCloudConnection{ - Url: "https://bitbucket.org/my-org", // incorrect + Url: schema.DefaultBitbucketCloudURL, Authorization: &schema.BitbucketCloudAuthorization{}, }, }, }, - []schema.AuthProviders{{Bitbucketcloud: &schema.BitbucketCloudAuthProvider{}}}, ) require.Len(t, initResults.Providers, 1, "expect exactly one provider") assert.NotNil(t, initResults.Providers[0]) assert.Empty(t, initResults.Problems) + assert.Empty(t, initResults.Warnings) assert.Empty(t, initResults.InvalidConnections) - - require.Len(t, initResults.Warnings, 0, "expect no warnings") }) - t.Run("matching auth provider found", func(t *testing.T) { - t.Run("default case", func(t *testing.T) { - t.Cleanup(licensing.TestingSkipFeatureChecks()) - initResults := NewAuthzProviders( - db, - []*types.BitbucketCloudConnection{ - { - BitbucketCloudConnection: &schema.BitbucketCloudConnection{ - Url: schema.DefaultBitbucketCloudURL, - Authorization: &schema.BitbucketCloudAuthorization{}, - }, + t.Run("license does not have ACLs feature", func(t *testing.T) { + t.Cleanup(licensing.MockCheckFeatureError("failed")) + initResults := NewAuthzProviders( + db, + []*types.BitbucketCloudConnection{ + { + BitbucketCloudConnection: &schema.BitbucketCloudConnection{ + Url: schema.DefaultBitbucketCloudURL, + Authorization: &schema.BitbucketCloudAuthorization{}, }, }, - []schema.AuthProviders{{Bitbucketcloud: &schema.BitbucketCloudAuthProvider{}}}, - ) + }, + ) - require.Len(t, initResults.Providers, 1, "expect exactly one provider") - assert.NotNil(t, initResults.Providers[0]) - - assert.Empty(t, initResults.Problems) - assert.Empty(t, initResults.Warnings) - assert.Empty(t, initResults.InvalidConnections) - }) - - t.Run("license does not have ACLs feature", func(t *testing.T) { - t.Cleanup(licensing.MockCheckFeatureError("failed")) - initResults := NewAuthzProviders( - db, - []*types.BitbucketCloudConnection{ - { - BitbucketCloudConnection: &schema.BitbucketCloudConnection{ - Url: schema.DefaultBitbucketCloudURL, - Authorization: &schema.BitbucketCloudAuthorization{}, - }, - }, - }, - []schema.AuthProviders{{Bitbucketcloud: &schema.BitbucketCloudAuthProvider{}}}, - ) - - expectedError := []string{"failed"} - expInvalidConnectionErr := []string{"bitbucketCloud"} - assert.Equal(t, expectedError, initResults.Problems) - assert.Equal(t, expInvalidConnectionErr, initResults.InvalidConnections) - assert.Empty(t, initResults.Providers) - assert.Empty(t, initResults.Warnings) - }) + expectedError := []string{"failed"} + expInvalidConnectionErr := []string{"bitbucketCloud"} + assert.Equal(t, expectedError, initResults.Problems) + assert.Equal(t, expInvalidConnectionErr, initResults.InvalidConnections) + assert.Empty(t, initResults.Providers) + assert.Empty(t, initResults.Warnings) }) } diff --git a/internal/authz/providers/gerrit/BUILD.bazel b/internal/authz/providers/gerrit/BUILD.bazel index 54c5b0ca9af..cfff984a830 100644 --- a/internal/authz/providers/gerrit/BUILD.bazel +++ b/internal/authz/providers/gerrit/BUILD.bazel @@ -20,7 +20,6 @@ go_library( "//internal/licensing", "//internal/types", "//lib/errors", - "//schema", ], ) diff --git a/internal/authz/providers/gerrit/authz.go b/internal/authz/providers/gerrit/authz.go index a35219db34e..e855bd1570f 100644 --- a/internal/authz/providers/gerrit/authz.go +++ b/internal/authz/providers/gerrit/authz.go @@ -1,43 +1,28 @@ package gerrit import ( - "net/url" - atypes "github.com/sourcegraph/sourcegraph/internal/authz/types" "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/licensing" "github.com/sourcegraph/sourcegraph/internal/types" - "github.com/sourcegraph/sourcegraph/schema" ) // NewAuthzProviders returns the set of Gerrit authz providers derived from the connections. -func NewAuthzProviders(conns []*types.GerritConnection, authProviders []schema.AuthProviders) *atypes.ProviderInitResult { +func NewAuthzProviders(conns []*types.GerritConnection) *atypes.ProviderInitResult { initResults := &atypes.ProviderInitResult{} - gerritAuthProviders := make(map[string]*schema.GerritAuthProvider) - for _, p := range authProviders { - if p.Gerrit == nil { - continue - } - - gerritURL, err := url.Parse(p.Gerrit.Url) - if err != nil { - continue - } - - // Use normalised base URL as ID. - gerritAuthProviders[extsvc.NormalizeBaseURL(gerritURL).String()] = p.Gerrit - } for _, c := range conns { if c.Authorization == nil { // No authorization required continue } + if err := licensing.Check(licensing.FeatureACLs); err != nil { initResults.InvalidConnections = append(initResults.InvalidConnections, extsvc.TypeGerrit) initResults.Problems = append(initResults.Problems, err.Error()) continue } + p, err := NewProvider(c) if err != nil { initResults.InvalidConnections = append(initResults.InvalidConnections, extsvc.TypeGerrit) diff --git a/internal/authz/providers/gitlab/authz.go b/internal/authz/providers/gitlab/authz.go index 9492687e521..f1c1a0076c0 100644 --- a/internal/authz/providers/gitlab/authz.go +++ b/internal/authz/providers/gitlab/authz.go @@ -26,13 +26,13 @@ import ( // to connection issues. func NewAuthzProviders( db database.DB, - cfg schema.SiteConfiguration, conns []*types.GitLabConnection, + ap []schema.AuthProviders, ) *atypes.ProviderInitResult { initResults := &atypes.ProviderInitResult{} // Authorization (i.e., permissions) providers for _, c := range conns { - p, err := newAuthzProvider(db, c, cfg.AuthProviders) + p, err := newAuthzProvider(db, c, ap) if err != nil { initResults.InvalidConnections = append(initResults.InvalidConnections, extsvc.TypeGitLab) initResults.Problems = append(initResults.Problems, err.Error()) diff --git a/internal/authz/register.go b/internal/authz/register.go index 5dca6dce5da..701cc16570e 100644 --- a/internal/authz/register.go +++ b/internal/authz/register.go @@ -7,12 +7,6 @@ import ( ) var ( - // allowAccessByDefault, if set to true, grants all users access to repositories that are - // not matched by any authz provider. The default value is true. It is only set to false in - // error modes (when the configuration is in a state where interpreting it literally could lead - // to leakage of private repositories). - allowAccessByDefault = true - // authzProvidersReady and authzProvidersReadyOnce together indicate when // GetProviders should no longer block. It should block until SetProviders // is called at least once. @@ -27,12 +21,11 @@ var ( ) // SetProviders sets the current authz parameters. It is concurrency-safe. -func SetProviders(authzAllowByDefault bool, z []Provider) { +func SetProviders(z []Provider) { authzMu.Lock() defer authzMu.Unlock() authzProviders = z - allowAccessByDefault = authzAllowByDefault authzProvidersReadyOnce.Do(func() { close(authzProvidersReady) @@ -42,7 +35,7 @@ func SetProviders(authzAllowByDefault bool, z []Provider) { // GetProviders returns the current authz parameters. It is concurrency-safe. // // It blocks until SetProviders has been called at least once. -func GetProviders() (authzAllowByDefault bool, providers []Provider) { +func GetProviders() (providers []Provider) { if !testutil.IsTest { <-authzProvidersReady } @@ -50,9 +43,9 @@ func GetProviders() (authzAllowByDefault bool, providers []Provider) { defer authzMu.Unlock() if authzProviders == nil { - return allowAccessByDefault, nil + return nil } providers = make([]Provider, len(authzProviders)) copy(providers, authzProviders) - return allowAccessByDefault, providers + return providers } diff --git a/internal/batches/reconciler/executor_test.go b/internal/batches/reconciler/executor_test.go index beb0633c89c..8eb6991dfa9 100644 --- a/internal/batches/reconciler/executor_test.go +++ b/internal/batches/reconciler/executor_test.go @@ -873,7 +873,7 @@ func TestExecutor_ExecutePlan_PublishedChangesetDuplicateBranch(t *testing.T) { } logger := logtest.Scoped(t) - ctx := context.Background() + ctx := actor.WithInternalActor(context.Background()) db := database.NewDB(logger, dbtest.NewDB(t)) bstore := store.New(db, observation.TestContextTB(t), et.TestKey{}) @@ -916,7 +916,7 @@ func TestExecutor_ExecutePlan_PublishedChangesetDuplicateBranch(t *testing.T) { func TestExecutor_ExecutePlan_AvoidLoadingChangesetSource(t *testing.T) { logger := logtest.Scoped(t) - ctx := context.Background() + ctx := actor.WithInternalActor(context.Background()) db := database.NewDB(logger, dbtest.NewDB(t)) bstore := store.New(db, observation.TestContextTB(t), et.TestKey{}) repo, _ := bt.CreateTestRepo(t, ctx, db) @@ -1178,7 +1178,7 @@ func TestExecutor_UserCredentialsForGitserver(t *testing.T) { }) _, err := executePlan( - actor.WithActor(ctx, actor.FromUser(tt.user.ID)), + actor.WithInternalActor(ctx), logtest.Scoped(t), gitserverClient, sourcer, diff --git a/internal/batches/service/service_test.go b/internal/batches/service/service_test.go index 98250c23fbb..a1c7faa72f4 100644 --- a/internal/batches/service/service_test.go +++ b/internal/batches/service/service_test.go @@ -4,11 +4,12 @@ import ( "context" "encoding/json" "fmt" - "github.com/sourcegraph/sourcegraph/internal/batches/sources" "strings" "testing" "time" + "github.com/sourcegraph/sourcegraph/internal/batches/sources" + "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/graph-gophers/graphql-go/relay" @@ -56,6 +57,9 @@ func TestServicePermissionLevels(t *testing.T) { nonOrgMember := bt.CreateTestUser(t, db, false) repo, _ := bt.CreateTestRepo(t, ctx, db) + bt.MockRepoPermissions(t, db, user.ID, repo.ID) + bt.MockRepoPermissions(t, db, otherUser.ID, repo.ID) + bt.MockRepoPermissions(t, db, nonOrgMember.ID, repo.ID) org := bt.CreateTestOrg(t, db, "test-org-1", admin.ID, user.ID, otherUser.ID) @@ -317,6 +321,8 @@ func TestService(t *testing.T) { svc.sourcer = sourcer t.Run("CheckViewerCanAdminister", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + org := bt.CreateTestOrg(t, db, "test-org-1", user.ID, user2.ID) spec := testBatchSpec(user.ID) @@ -424,6 +430,8 @@ func TestService(t *testing.T) { }) t.Run("DeleteBatchChange", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + spec := testBatchSpec(admin.ID) if err := s.CreateBatchSpec(ctx, spec); err != nil { t.Fatal(err) @@ -444,6 +452,8 @@ func TestService(t *testing.T) { }) t.Run("CloseBatchChange", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + createBatchChange := func(t *testing.T) *btypes.BatchChange { t.Helper() @@ -516,6 +526,8 @@ func TestService(t *testing.T) { }) t.Run("EnqueueChangesetSync", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + spec := testBatchSpec(user.ID) if err := s.CreateBatchSpec(ctx, spec); err != nil { t.Fatal(err) @@ -559,6 +571,8 @@ func TestService(t *testing.T) { }) t.Run("ReenqueueChangeset", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + spec := testBatchSpec(user.ID) if err := s.CreateBatchSpec(ctx, spec); err != nil { t.Fatal(err) @@ -616,6 +630,8 @@ func TestService(t *testing.T) { } t.Run("success", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + opts := CreateBatchSpecOpts{ NamespaceUserID: admin.ID, RawSpec: bt.TestRawBatchSpec, @@ -657,6 +673,8 @@ func TestService(t *testing.T) { }) t.Run("success with YAML raw spec", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + opts := CreateBatchSpecOpts{ NamespaceUserID: admin.ID, RawSpec: bt.TestRawBatchSpecYAML, @@ -696,6 +714,8 @@ func TestService(t *testing.T) { }) t.Run("invalid changesetspec id", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + containsInvalidID := []string{changesetSpecRandIDs[0], "foobar"} opts := CreateBatchSpecOpts{ NamespaceUserID: admin.ID, @@ -709,6 +729,8 @@ func TestService(t *testing.T) { }) t.Run("namespace user is not admin and not creator", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + opts := CreateBatchSpecOpts{ NamespaceUserID: admin.ID, RawSpec: bt.TestRawBatchSpecYAML, @@ -729,6 +751,8 @@ func TestService(t *testing.T) { }) t.Run("missing access to namespace org", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + orgID := bt.CreateTestOrg(t, db, "test-org").ID opts := CreateBatchSpecOpts{ @@ -754,6 +778,8 @@ func TestService(t *testing.T) { }) t.Run("no side-effects if no changeset spec IDs are given", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + // We already have ChangesetSpecs in the database. Here we // want to make sure that the new BatchSpec is created, // without accidently attaching the existing ChangesetSpecs. @@ -780,6 +806,8 @@ func TestService(t *testing.T) { }) t.Run("CreateChangesetSpec", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + repo := rs[0] rawSpec := bt.NewRawChangesetSpecGitBranch(relay.MarshalID("Repository", repo.ID), "d34db33f") @@ -865,6 +893,8 @@ index e5af166..d44c3fc 100644 }) t.Run("CreateChangesetSpecs", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + rawSpec := bt.NewRawChangesetSpecGitBranch(relay.MarshalID("Repository", rs[0].ID), "d34db33f") t.Run("success", func(t *testing.T) { @@ -949,6 +979,8 @@ index e5af166..d44c3fc 100644 }) t.Run("MoveBatchChange", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + createBatchChange := func(t *testing.T, name string, authorID, userID, orgID int32) *btypes.BatchChange { t.Helper() @@ -1061,6 +1093,8 @@ index e5af166..d44c3fc 100644 }) t.Run("GetBatchChangeMatchingBatchSpec", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + batchSpec := bt.CreateBatchSpec(t, ctx, s, "matching-batch-spec", admin.ID, 0) haveBatchChange, err := svc.GetBatchChangeMatchingBatchSpec(ctx, batchSpec) @@ -1116,6 +1150,8 @@ index e5af166..d44c3fc 100644 }) t.Run("GetNewestBatchSpec", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + older := bt.CreateBatchSpec(t, ctx, s, "superseding", user.ID, 0) newer := bt.CreateBatchSpec(t, ctx, s, "superseding", user.ID, 0) @@ -1148,6 +1184,8 @@ index e5af166..d44c3fc 100644 }) t.Run("FetchUsernameForBitbucketServerToken", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + fakeSource := &stesting.FakeChangesetSource{Username: "my-bbs-username"} sourcer := stesting.NewFakeSourcer(nil, fakeSource) @@ -1177,6 +1215,8 @@ index e5af166..d44c3fc 100644 }) t.Run("ValidateAuthenticator", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + t.Run("valid", func(t *testing.T) { fakeSource.AuthenticatorIsValid = true fakeSource.ValidateAuthenticatorCalled = false @@ -1218,6 +1258,8 @@ index e5af166..d44c3fc 100644 }) t.Run("CreateChangesetJobs", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + spec := testBatchSpec(admin.ID) if err := s.CreateBatchSpec(ctx, spec); err != nil { t.Fatal(err) @@ -1474,6 +1516,8 @@ index e5af166..d44c3fc 100644 }) t.Run("ExecuteBatchSpec", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + adminCtx := actor.WithActor(ctx, actor.FromUser(admin.ID)) t.Run("success", func(t *testing.T) { spec := testBatchSpec(admin.ID) @@ -1722,6 +1766,8 @@ index e5af166..d44c3fc 100644 }) t.Run("CancelBatchSpec", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + t.Run("success", func(t *testing.T) { spec := testBatchSpec(admin.ID) spec.CreatedFromRaw = true @@ -1829,6 +1875,8 @@ index e5af166..d44c3fc 100644 }) t.Run("ReplaceBatchSpecInput", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + createBatchSpecWithWorkspaces := func(t *testing.T) *btypes.BatchSpec { t.Helper() spec := testBatchSpec(admin.ID) @@ -2045,6 +2093,8 @@ changesetTemplate: }) t.Run("CreateBatchSpecFromRaw", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + t.Run("batch change isn't owned by non-admin user", func(t *testing.T) { spec := testBatchSpec(user.ID) if err := s.CreateBatchSpec(ctx, spec); err != nil { @@ -2175,6 +2225,8 @@ changesetTemplate: }) t.Run("UpsertBatchSpecInput", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + adminCtx := actor.WithActor(ctx, actor.FromUser(admin.ID)) t.Run("new spec", func(t *testing.T) { newSpec, err := svc.UpsertBatchSpecInput(adminCtx, UpsertBatchSpecInputOpts{ @@ -2241,6 +2293,8 @@ changesetTemplate: }) t.Run("ValidateChangesetSpecs", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + batchSpec := bt.CreateBatchSpec(t, ctx, s, "matching-batch-spec", admin.ID, 0) conflictingRef := "refs/heads/conflicting-head-ref" for _, opts := range []bt.TestSpecOpts{ @@ -2268,6 +2322,8 @@ changesetTemplate: }) t.Run("ComputeBatchSpecState", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + t.Run("success", func(t *testing.T) { spec := testBatchSpec(admin.ID) spec.CreatedFromRaw = true @@ -2318,6 +2374,8 @@ changesetTemplate: }) t.Run("RetryBatchSpecWorkspaces", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + failureMessage := "this failed" t.Run("success", func(t *testing.T) { @@ -2521,6 +2579,8 @@ changesetTemplate: }) t.Run("RetryBatchSpecExecution", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + failureMessage := "this failed" createSpec := func(t *testing.T) *btypes.BatchSpec { @@ -2817,6 +2877,8 @@ changesetTemplate: }) t.Run("GetAvailableBulkOperations", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + spec := testBatchSpec(admin.ID) if err := s.CreateBatchSpec(ctx, spec); err != nil { t.Fatal(err) @@ -3138,6 +3200,8 @@ changesetTemplate: }) t.Run("UpsertEmptyBatchChange", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + t.Run("creates new batch change if it is non-existent", func(t *testing.T) { name := "random-bc-name" @@ -3210,6 +3274,8 @@ changesetTemplate: }) t.Run("GetChangesetsByIDs", func(t *testing.T) { + bt.MockRepoPermissions(t, db, user.ID, rs[0].ID, rs[1].ID, rs[2].ID, rs[3].ID) + spec := testBatchSpec(admin.ID) if err := s.CreateBatchSpec(ctx, spec); err != nil { t.Fatal(err) diff --git a/internal/batches/store/batch_changes_test.go b/internal/batches/store/batch_changes_test.go index b77c071aa86..00d17b6286f 100644 --- a/internal/batches/store/batch_changes_test.go +++ b/internal/batches/store/batch_changes_test.go @@ -259,7 +259,7 @@ func testStoreBatchChanges(t *testing.T, ctx context.Context, s *Store, clock bt t.Run(strconv.Itoa(i), func(t *testing.T) { opts := CountBatchChangesOpts{RepoID: tc.repoID} - count, err := s.CountBatchChanges(ctx, opts) + count, err := s.CountBatchChanges(actor.WithInternalActor(ctx), opts) if err != nil { t.Fatal(err) } @@ -430,7 +430,7 @@ func testStoreBatchChanges(t *testing.T, ctx context.Context, s *Store, clock bt t.Run(strconv.Itoa(i), func(t *testing.T) { opts := ListBatchChangesOpts{RepoID: tc.repoID} - ts, next, err := s.ListBatchChanges(ctx, opts) + ts, next, err := s.ListBatchChanges(actor.WithInternalActor(ctx), opts) if err != nil { t.Fatal(err) } @@ -800,6 +800,8 @@ func testStoreBatchChanges(t *testing.T, ctx context.Context, s *Store, clock bt batchChangeID := bcs[0].ID var testDiffStatCount int32 = 10 + bt.MockRepoPermissions(t, s.DatabaseDB(), userID, repo.ID) + bt.MockRepoPermissions(t, s.DatabaseDB(), otherUserID, repo.ID) bt.CreateChangeset(t, ctx, s, bt.TestChangesetOpts{ Repo: repo.ID, BatchChanges: []btypes.BatchChangeAssoc{{BatchChangeID: batchChangeID}}, @@ -825,7 +827,7 @@ func testStoreBatchChanges(t *testing.T, ctx context.Context, s *Store, clock bt // Now give repo access only to otherUserID, and check that // userID cannot see it in the diff stat anymore. - bt.MockRepoPermissions(t, s.DatabaseDB(), otherUserID, repo.ID) + bt.MockRepoPermissions(t, s.DatabaseDB(), userID) { want := &godiff.Stat{ Added: 0, @@ -856,6 +858,7 @@ func testStoreBatchChanges(t *testing.T, ctx context.Context, s *Store, clock bt if err := repoStore.Create(ctx, repo1, repo2, repo3); err != nil { t.Fatal(err) } + bt.MockRepoPermissions(t, s.DatabaseDB(), userID, repo1.ID, repo2.ID, repo3.ID) batchChangeID := bcs[0].ID var testDiffStatCount1 int32 = 10 @@ -930,6 +933,7 @@ func testStoreBatchChanges(t *testing.T, ctx context.Context, s *Store, clock bt // Now give repo access only to otherUserID, and check that // userID cannot see it in the diff stat anymore. + bt.MockRepoPermissions(t, s.DatabaseDB(), userID) bt.MockRepoPermissions(t, s.DatabaseDB(), otherUserID, repo1.ID) { want := &godiff.Stat{ diff --git a/internal/batches/store/changesets_test.go b/internal/batches/store/changesets_test.go index ae573fe5bb2..afed05766a3 100644 --- a/internal/batches/store/changesets_test.go +++ b/internal/batches/store/changesets_test.go @@ -16,6 +16,7 @@ import ( "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/batches/search" bt "github.com/sourcegraph/sourcegraph/internal/batches/testing" @@ -1439,7 +1440,7 @@ func testStoreChangesets(t *testing.T, ctx context.Context, s *Store, clock bt.C bt.CreateChangeset(t, ctx, s, opts9) wantStats.Draft += 1 - haveStats, err := s.GetRepoChangesetsStats(ctx, r.ID) + haveStats, err := s.GetRepoChangesetsStats(actor.WithInternalActor(ctx), r.ID) if err != nil { t.Fatal(err) } diff --git a/internal/batches/testing/mock_repo_perms.go b/internal/batches/testing/mock_repo_perms.go index e85628a16e8..1d87c60cf21 100644 --- a/internal/batches/testing/mock_repo_perms.go +++ b/internal/batches/testing/mock_repo_perms.go @@ -32,9 +32,4 @@ func MockRepoPermissions(t *testing.T, db database.DB, userID int32, repoIDs ... UserID: userID, }, maps.Keys(repoIDMap), authz.SourceUserSync) require.NoError(t, err) - - authz.SetProviders(false, nil) - t.Cleanup(func() { - authz.SetProviders(true, nil) - }) } diff --git a/internal/codeintel/autoindexing/internal/store/BUILD.bazel b/internal/codeintel/autoindexing/internal/store/BUILD.bazel index 67cc9ffaa43..f6e20e15833 100644 --- a/internal/codeintel/autoindexing/internal/store/BUILD.bazel +++ b/internal/codeintel/autoindexing/internal/store/BUILD.bazel @@ -67,5 +67,6 @@ go_test( "@com_github_keegancsmith_sqlf//:sqlf", "@com_github_lib_pq//:pq", "@com_github_sourcegraph_log//logtest", + "@com_github_stretchr_testify//require", ], ) diff --git a/internal/codeintel/autoindexing/internal/store/enqueuer_test.go b/internal/codeintel/autoindexing/internal/store/enqueuer_test.go index c6ffc75f1a2..1510cfc9bfb 100644 --- a/internal/codeintel/autoindexing/internal/store/enqueuer_test.go +++ b/internal/codeintel/autoindexing/internal/store/enqueuer_test.go @@ -8,6 +8,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sourcegraph/log/logtest" + "github.com/stretchr/testify/require" "github.com/sourcegraph/sourcegraph/internal/actor" uploadsshared "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/shared" @@ -234,9 +235,11 @@ func TestInsertIndexWithActor(t *testing.T) { store := New(observation.TestContextTB(t), db) insertRepo(t, db, 50, "") + u, err := db.Users().Create(context.Background(), database.NewUser{Username: "alice"}) + require.NoError(t, err) for i, ctx := range []context.Context{ - actor.WithActor(context.Background(), actor.FromMockUser(100)), + actor.WithActor(context.Background(), actor.FromUser(u.ID)), actor.WithInternalActor(context.Background()), context.Background(), } { diff --git a/internal/codeintel/policies/internal/store/BUILD.bazel b/internal/codeintel/policies/internal/store/BUILD.bazel index 78a5ebe4097..90a1d845503 100644 --- a/internal/codeintel/policies/internal/store/BUILD.bazel +++ b/internal/codeintel/policies/internal/store/BUILD.bazel @@ -45,13 +45,12 @@ go_test( "requires-network", ], deps = [ + "//internal/actor", "//internal/codeintel/policies/shared", - "//internal/conf", "//internal/database", "//internal/database/basestore", "//internal/database/dbtest", "//internal/observation", - "//schema", "@com_github_google_go_cmp//cmp", "@com_github_keegancsmith_sqlf//:sqlf", "@com_github_sourcegraph_log//logtest", diff --git a/internal/codeintel/policies/internal/store/configurations_test.go b/internal/codeintel/policies/internal/store/configurations_test.go index 8ef3b02ccb7..2d4a6e8b9e6 100644 --- a/internal/codeintel/policies/internal/store/configurations_test.go +++ b/internal/codeintel/policies/internal/store/configurations_test.go @@ -9,6 +9,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/actor" policiesshared "github.com/sourcegraph/sourcegraph/internal/codeintel/policies/shared" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/database/dbtest" @@ -239,6 +240,7 @@ func TestDeleteConfigurationPolicyByID(t *testing.T) { func TestDeleteConfigurationProtectedPolicy(t *testing.T) { logger := logtest.Scoped(t) + ctx := actor.WithInternalActor(context.Background()) db := database.NewDB(logger, dbtest.NewDB(t)) store := testStoreWithoutConfigurationPolicies(t, db) @@ -260,7 +262,7 @@ func TestDeleteConfigurationProtectedPolicy(t *testing.T) { IndexIntermediateCommits: true, } - hydratedConfigurationPolicy, err := store.CreateConfigurationPolicy(context.Background(), configurationPolicy) + hydratedConfigurationPolicy, err := store.CreateConfigurationPolicy(ctx, configurationPolicy) if err != nil { t.Fatalf("unexpected error creating configuration policy: %s", err) } @@ -270,15 +272,15 @@ func TestDeleteConfigurationProtectedPolicy(t *testing.T) { } // Mark configuration policy as protected (no other way to do so outside of migrations) - if _, err := db.ExecContext(context.Background(), "UPDATE lsif_configuration_policies SET protected = true"); err != nil { + if _, err := db.ExecContext(ctx, "UPDATE lsif_configuration_policies SET protected = true"); err != nil { t.Fatalf("unexpected error marking configuration policy as protected: %s", err) } - if err := store.DeleteConfigurationPolicyByID(context.Background(), hydratedConfigurationPolicy.ID); err == nil { + if err := store.DeleteConfigurationPolicyByID(ctx, hydratedConfigurationPolicy.ID); err == nil { t.Fatalf("expected error deleting configuration policy: %s", err) } - _, ok, err := store.GetConfigurationPolicyByID(context.Background(), hydratedConfigurationPolicy.ID) + _, ok, err := store.GetConfigurationPolicyByID(ctx, hydratedConfigurationPolicy.ID) if err != nil { t.Fatalf("unexpected error fetching configuration policy: %s", err) } diff --git a/internal/codeintel/policies/internal/store/repository_matches_test.go b/internal/codeintel/policies/internal/store/repository_matches_test.go index 69952abafdb..8448ee3ff3b 100644 --- a/internal/codeintel/policies/internal/store/repository_matches_test.go +++ b/internal/codeintel/policies/internal/store/repository_matches_test.go @@ -9,16 +9,15 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/actor" policiesshared "github.com/sourcegraph/sourcegraph/internal/codeintel/policies/shared" - "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/database/dbtest" "github.com/sourcegraph/sourcegraph/internal/observation" - "github.com/sourcegraph/sourcegraph/schema" ) func TestRepoIDsByGlobPatterns(t *testing.T) { - ctx := context.Background() + ctx := actor.WithInternalActor(context.Background()) logger := logtest.Scoped(t) db := database.NewDB(logger, dbtest.NewDB(t)) store := New(observation.TestContextTB(t), db) @@ -72,20 +71,8 @@ func TestRepoIDsByGlobPatterns(t *testing.T) { } t.Run("enforce repository permissions", func(t *testing.T) { - // Turning on explicit permissions forces checking repository permissions - // against permissions tables in the database, which should effectively block - // all access because permissions tables are empty and repos are private. - conf.Mock(&conf.Unified{ - SiteConfiguration: schema.SiteConfiguration{ - PermissionsUserMapping: &schema.PermissionsUserMapping{ - Enabled: true, - BindID: "email", - }, - }, - }) - t.Cleanup(func() { conf.Mock(nil) }) - - repoIDs, _, err := store.GetRepoIDsByGlobPatterns(ctx, []string{"*"}, 10, 0) + // We use an actor-less context here. Without an actor, no repositories should be visible. + repoIDs, _, err := store.GetRepoIDsByGlobPatterns(context.Background(), []string{"*"}, 10, 0) if err != nil { t.Fatal(err) } diff --git a/internal/codeintel/syntactic_indexing/jobstore/store.go b/internal/codeintel/syntactic_indexing/jobstore/store.go index 5957aca640e..147c9d7e353 100644 --- a/internal/codeintel/syntactic_indexing/jobstore/store.go +++ b/internal/codeintel/syntactic_indexing/jobstore/store.go @@ -170,6 +170,7 @@ SELECT u.should_reindex, u.enqueuer_user_id FROM syntactic_scip_indexing_jobs_with_repository_name u +JOIN repo ON repo.id = u.repository_id AND repo.deleted_at IS NULL AND repo.blocked IS NULL WHERE u.id IN (%s) and %s ORDER BY u.id ` diff --git a/internal/codeintel/uploads/internal/store/BUILD.bazel b/internal/codeintel/uploads/internal/store/BUILD.bazel index c50704bc419..c8698297aca 100644 --- a/internal/codeintel/uploads/internal/store/BUILD.bazel +++ b/internal/codeintel/uploads/internal/store/BUILD.bazel @@ -71,6 +71,7 @@ go_test( "requires-network", ], deps = [ + "//internal/actor", "//internal/api", "//internal/byteutils", "//internal/codeintel/core", diff --git a/internal/codeintel/uploads/internal/store/cleanup_test.go b/internal/codeintel/uploads/internal/store/cleanup_test.go index 807213ab9a6..c37b5400aab 100644 --- a/internal/codeintel/uploads/internal/store/cleanup_test.go +++ b/internal/codeintel/uploads/internal/store/cleanup_test.go @@ -11,6 +11,7 @@ import ( logger "github.com/sourcegraph/log" "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/shared" uploadsshared "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/shared" "github.com/sourcegraph/sourcegraph/internal/database" @@ -67,7 +68,7 @@ func TestDeleteUploadsStuckUploading(t *testing.T) { shared.Upload{ID: 5, Commit: makeCommit(1115), UploadedAt: t5, State: "uploading"}, // old ) - _, count, err := store.DeleteUploadsStuckUploading(context.Background(), t1.Add(time.Minute*3)) + _, count, err := store.DeleteUploadsStuckUploading(actor.WithInternalActor(context.Background()), t1.Add(time.Minute*3)) if err != nil { t.Fatalf("unexpected error deleting uploads stuck uploading: %s", err) } @@ -75,7 +76,7 @@ func TestDeleteUploadsStuckUploading(t *testing.T) { t.Errorf("unexpected count. want=%d have=%d", 2, count) } - uploads, totalCount, err := store.GetUploads(context.Background(), shared.GetUploadsOptions{Limit: 5}) + uploads, totalCount, err := store.GetUploads(actor.WithInternalActor(context.Background()), shared.GetUploadsOptions{Limit: 5}) if err != nil { t.Fatalf("unexpected error getting uploads: %s", err) } @@ -121,12 +122,12 @@ func TestDeleteUploadsWithoutRepository(t *testing.T) { for repositoryID, deletedAt := range deletions { query := sqlf.Sprintf(`UPDATE repo SET deleted_at=%s WHERE id=%s`, deletedAt, repositoryID) - if _, err := db.QueryContext(context.Background(), query.Query(sqlf.PostgresBindVar), query.Args()...); err != nil { + if _, err := db.QueryContext(actor.WithInternalActor(context.Background()), query.Query(sqlf.PostgresBindVar), query.Args()...); err != nil { t.Fatalf("Failed to update repository: %s", err) } } - _, count, err := store.DeleteUploadsWithoutRepository(context.Background(), t1) + _, count, err := store.DeleteUploadsWithoutRepository(actor.WithInternalActor(context.Background()), t1) if err != nil { t.Fatalf("unexpected error deleting uploads: %s", err) } @@ -163,7 +164,7 @@ func TestDeleteOldAuditLogs(t *testing.T) { store := New(observation.TestContextTB(t), db) // Sanity check for syntax only - if _, _, err := store.DeleteOldAuditLogs(context.Background(), time.Second, time.Now()); err != nil { + if _, _, err := store.DeleteOldAuditLogs(actor.WithInternalActor(context.Background()), time.Second, time.Now()); err != nil { t.Fatalf("unexpected error deleting old audit logs: %s", err) } } @@ -452,29 +453,29 @@ func TestGetQueuedUploadRank(t *testing.T) { shared.Upload{ID: 7, UploadedAt: t1, State: "queued", ProcessAfter: &t7}, ) - if upload, _, _ := store.GetUploadByID(context.Background(), 1); upload.Rank == nil || *upload.Rank != 1 { + if upload, _, _ := store.GetUploadByID(actor.WithInternalActor(context.Background()), 1); upload.Rank == nil || *upload.Rank != 1 { t.Errorf("unexpected rank. want=%d have=%s", 1, printableRank{upload.Rank}) } - if upload, _, _ := store.GetUploadByID(context.Background(), 2); upload.Rank == nil || *upload.Rank != 6 { + if upload, _, _ := store.GetUploadByID(actor.WithInternalActor(context.Background()), 2); upload.Rank == nil || *upload.Rank != 6 { t.Errorf("unexpected rank. want=%d have=%s", 5, printableRank{upload.Rank}) } - if upload, _, _ := store.GetUploadByID(context.Background(), 3); upload.Rank == nil || *upload.Rank != 3 { + if upload, _, _ := store.GetUploadByID(actor.WithInternalActor(context.Background()), 3); upload.Rank == nil || *upload.Rank != 3 { t.Errorf("unexpected rank. want=%d have=%s", 3, printableRank{upload.Rank}) } - if upload, _, _ := store.GetUploadByID(context.Background(), 4); upload.Rank == nil || *upload.Rank != 2 { + if upload, _, _ := store.GetUploadByID(actor.WithInternalActor(context.Background()), 4); upload.Rank == nil || *upload.Rank != 2 { t.Errorf("unexpected rank. want=%d have=%s", 2, printableRank{upload.Rank}) } - if upload, _, _ := store.GetUploadByID(context.Background(), 5); upload.Rank == nil || *upload.Rank != 4 { + if upload, _, _ := store.GetUploadByID(actor.WithInternalActor(context.Background()), 5); upload.Rank == nil || *upload.Rank != 4 { t.Errorf("unexpected rank. want=%d have=%s", 4, printableRank{upload.Rank}) } // Only considers queued uploads to determine rank - if upload, _, _ := store.GetUploadByID(context.Background(), 6); upload.Rank != nil { + if upload, _, _ := store.GetUploadByID(actor.WithInternalActor(context.Background()), 6); upload.Rank != nil { t.Errorf("unexpected rank. want=%s have=%s", "nil", printableRank{upload.Rank}) } // Process after takes priority over upload time - if upload, _, _ := store.GetUploadByID(context.Background(), 7); upload.Rank == nil || *upload.Rank != 5 { + if upload, _, _ := store.GetUploadByID(actor.WithInternalActor(context.Background()), 7); upload.Rank == nil || *upload.Rank != 5 { t.Errorf("unexpected rank. want=%d have=%s", 4, printableRank{upload.Rank}) } } diff --git a/internal/codeintel/uploads/internal/store/indexes_test.go b/internal/codeintel/uploads/internal/store/indexes_test.go index 35f4e14a014..d2e92f24850 100644 --- a/internal/codeintel/uploads/internal/store/indexes_test.go +++ b/internal/codeintel/uploads/internal/store/indexes_test.go @@ -11,18 +11,17 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/shared" uploadsshared "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/shared" - "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/database/dbtest" "github.com/sourcegraph/sourcegraph/internal/executor" "github.com/sourcegraph/sourcegraph/internal/observation" - "github.com/sourcegraph/sourcegraph/schema" ) func TestGetIndexes(t *testing.T) { - ctx := context.Background() + ctx := actor.WithInternalActor(context.Background()) logger := logtest.Scoped(t) db := database.NewDB(logger, dbtest.NewDB(t)) store := New(observation.TestContextTB(t), db) @@ -132,20 +131,8 @@ func TestGetIndexes(t *testing.T) { } t.Run("enforce repository permissions", func(t *testing.T) { - // Enable permissions user mapping forces checking repository permissions - // against permissions tables in the database, which should effectively block - // all access because permissions tables are empty. - conf.Mock(&conf.Unified{ - SiteConfiguration: schema.SiteConfiguration{ - PermissionsUserMapping: &schema.PermissionsUserMapping{ - Enabled: true, - BindID: "email", - }, - }, - }) - t.Cleanup(func() { conf.Mock(nil) }) - - indexes, totalCount, err := store.GetAutoIndexJobs(ctx, + // Use an actorless context to test permissions. + indexes, totalCount, err := store.GetAutoIndexJobs(context.Background(), shared.GetAutoIndexJobsOptions{ Limit: 1, }, @@ -160,7 +147,7 @@ func TestGetIndexes(t *testing.T) { } func TestGetAutoIndexJobByID(t *testing.T) { - ctx := context.Background() + ctx := actor.WithInternalActor(context.Background()) logger := logtest.Scoped(t) db := database.NewDB(logger, dbtest.NewDB(t)) store := New(observation.TestContextTB(t), db) @@ -216,20 +203,8 @@ func TestGetAutoIndexJobByID(t *testing.T) { } t.Run("enforce repository permissions", func(t *testing.T) { - // Enable permissions user mapping forces checking repository permissions - // against permissions tables in the database, which should effectively block - // all access because permissions tables are empty. - conf.Mock(&conf.Unified{ - SiteConfiguration: schema.SiteConfiguration{ - PermissionsUserMapping: &schema.PermissionsUserMapping{ - Enabled: true, - BindID: "email", - }, - }, - }) - t.Cleanup(func() { conf.Mock(nil) }) - - _, exists, err := store.GetAutoIndexJobByID(ctx, 1) + // Use an actorless context to test permissions. + _, exists, err := store.GetAutoIndexJobByID(context.Background(), 1) if err != nil { t.Fatal(err) } @@ -262,35 +237,35 @@ func TestGetQueuedIndexRank(t *testing.T) { uploadsshared.AutoIndexJob{ID: 7, QueuedAt: t1, State: "queued", ProcessAfter: &t7}, ) - if index, _, _ := store.GetAutoIndexJobByID(context.Background(), 1); index.Rank == nil || *index.Rank != 1 { + if index, _, _ := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 1); index.Rank == nil || *index.Rank != 1 { t.Errorf("unexpected rank. want=%d have=%s", 1, printableRank{index.Rank}) } - if index, _, _ := store.GetAutoIndexJobByID(context.Background(), 2); index.Rank == nil || *index.Rank != 6 { + if index, _, _ := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 2); index.Rank == nil || *index.Rank != 6 { t.Errorf("unexpected rank. want=%d have=%s", 5, printableRank{index.Rank}) } - if index, _, _ := store.GetAutoIndexJobByID(context.Background(), 3); index.Rank == nil || *index.Rank != 3 { + if index, _, _ := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 3); index.Rank == nil || *index.Rank != 3 { t.Errorf("unexpected rank. want=%d have=%s", 3, printableRank{index.Rank}) } - if index, _, _ := store.GetAutoIndexJobByID(context.Background(), 4); index.Rank == nil || *index.Rank != 2 { + if index, _, _ := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 4); index.Rank == nil || *index.Rank != 2 { t.Errorf("unexpected rank. want=%d have=%s", 2, printableRank{index.Rank}) } - if index, _, _ := store.GetAutoIndexJobByID(context.Background(), 5); index.Rank == nil || *index.Rank != 4 { + if index, _, _ := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 5); index.Rank == nil || *index.Rank != 4 { t.Errorf("unexpected rank. want=%d have=%s", 4, printableRank{index.Rank}) } // Only considers queued indexes to determine rank - if index, _, _ := store.GetAutoIndexJobByID(context.Background(), 6); index.Rank != nil { + if index, _, _ := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 6); index.Rank != nil { t.Errorf("unexpected rank. want=%s have=%s", "nil", printableRank{index.Rank}) } // Process after takes priority over upload time - if upload, _, _ := store.GetAutoIndexJobByID(context.Background(), 7); upload.Rank == nil || *upload.Rank != 5 { + if upload, _, _ := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 7); upload.Rank == nil || *upload.Rank != 5 { t.Errorf("unexpected rank. want=%d have=%s", 4, printableRank{upload.Rank}) } } func TestGetAutoIndexJobsByIDs(t *testing.T) { - ctx := context.Background() + ctx := actor.WithInternalActor(context.Background()) logger := logtest.Scoped(t) db := database.NewDB(logger, dbtest.NewDB(t)) store := New(observation.TestContextTB(t), db) @@ -335,20 +310,8 @@ func TestGetAutoIndexJobsByIDs(t *testing.T) { }) t.Run("enforce repository permissions", func(t *testing.T) { - // Enable permissions user mapping forces checking repository permissions - // against permissions tables in the database, which should effectively block - // all access because permissions tables are empty. - conf.Mock(&conf.Unified{ - SiteConfiguration: schema.SiteConfiguration{ - PermissionsUserMapping: &schema.PermissionsUserMapping{ - Enabled: true, - BindID: "email", - }, - }, - }) - t.Cleanup(func() { conf.Mock(nil) }) - - indexes, err := store.GetAutoIndexJobsByIDs(ctx, 1, 2, 3, 4) + // Use an actorless context to test permissions. + indexes, err := store.GetAutoIndexJobsByIDs(context.Background(), 1, 2, 3, 4) if err != nil { t.Fatal(err) } @@ -372,7 +335,7 @@ func TestDeleteAutoIndexJobByID(t *testing.T) { } // AutoIndexJob no longer exists - if _, exists, err := store.GetAutoIndexJobByID(context.Background(), 1); err != nil { + if _, exists, err := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error getting index: %s", err) } else if exists { t.Fatal("unexpected record") @@ -387,7 +350,7 @@ func TestDeleteAutoIndexJobs(t *testing.T) { insertAutoIndexJobs(t, db, uploadsshared.AutoIndexJob{ID: 1, State: "completed"}) insertAutoIndexJobs(t, db, uploadsshared.AutoIndexJob{ID: 2, State: "errored"}) - if err := store.DeleteAutoIndexJobs(context.Background(), shared.DeleteAutoIndexJobsOptions{ + if err := store.DeleteAutoIndexJobs(actor.WithInternalActor(context.Background()), shared.DeleteAutoIndexJobsOptions{ States: []string{"errored"}, Term: "", RepositoryID: 0, @@ -396,7 +359,7 @@ func TestDeleteAutoIndexJobs(t *testing.T) { } // AutoIndexJob no longer exists - if _, exists, err := store.GetAutoIndexJobByID(context.Background(), 2); err != nil { + if _, exists, err := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 2); err != nil { t.Fatalf("unexpected error getting index: %s", err) } else if exists { t.Fatal("unexpected record") @@ -413,7 +376,7 @@ func TestDeleteAutoIndexJobsWithIndexerKey(t *testing.T) { insertAutoIndexJobs(t, db, uploadsshared.AutoIndexJob{ID: 3, Indexer: "sourcegraph/scip-typescript"}) insertAutoIndexJobs(t, db, uploadsshared.AutoIndexJob{ID: 4, Indexer: "sourcegraph/scip-typescript"}) - if err := store.DeleteAutoIndexJobs(context.Background(), shared.DeleteAutoIndexJobsOptions{ + if err := store.DeleteAutoIndexJobs(actor.WithInternalActor(context.Background()), shared.DeleteAutoIndexJobsOptions{ IndexerNames: []string{"scip-go"}, }); err != nil { t.Fatalf("unexpected error deleting indexes: %s", err) @@ -421,7 +384,7 @@ func TestDeleteAutoIndexJobsWithIndexerKey(t *testing.T) { // Target indexes no longer exist for _, id := range []int{1, 2} { - if _, exists, err := store.GetAutoIndexJobByID(context.Background(), id); err != nil { + if _, exists, err := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), id); err != nil { t.Fatalf("unexpected error getting index: %s", err) } else if exists { t.Fatal("unexpected record") @@ -430,7 +393,7 @@ func TestDeleteAutoIndexJobsWithIndexerKey(t *testing.T) { // Unmatched indexes remain for _, id := range []int{3, 4} { - if _, exists, err := store.GetAutoIndexJobByID(context.Background(), id); err != nil { + if _, exists, err := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), id); err != nil { t.Fatalf("unexpected error getting index: %s", err) } else if !exists { t.Fatal("expected record, got none") @@ -451,7 +414,7 @@ func TestSetRerunAutoIndexJobByID(t *testing.T) { } // AutoIndexJob has been marked for reindexing - if index, exists, err := store.GetAutoIndexJobByID(context.Background(), 2); err != nil { + if index, exists, err := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 2); err != nil { t.Fatalf("unexpected error getting index: %s", err) } else if !exists { t.Fatal("index missing") @@ -468,7 +431,7 @@ func TestSetRerunAutoIndexJobs(t *testing.T) { insertAutoIndexJobs(t, db, uploadsshared.AutoIndexJob{ID: 1, State: "completed"}) insertAutoIndexJobs(t, db, uploadsshared.AutoIndexJob{ID: 2, State: "errored"}) - if err := store.SetRerunAutoIndexJobs(context.Background(), shared.SetRerunAutoIndexJobsOptions{ + if err := store.SetRerunAutoIndexJobs(actor.WithInternalActor(context.Background()), shared.SetRerunAutoIndexJobsOptions{ States: []string{"errored"}, Term: "", RepositoryID: 0, @@ -477,7 +440,7 @@ func TestSetRerunAutoIndexJobs(t *testing.T) { } // AutoIndexJob has been marked for reindexing - if index, exists, err := store.GetAutoIndexJobByID(context.Background(), 2); err != nil { + if index, exists, err := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), 2); err != nil { t.Fatalf("unexpected error getting index: %s", err) } else if !exists { t.Fatal("index missing") @@ -496,7 +459,7 @@ func TestSetRerunAutoIndexJobsWithIndexerKey(t *testing.T) { insertAutoIndexJobs(t, db, uploadsshared.AutoIndexJob{ID: 3, Indexer: "sourcegraph/scip-typescript"}) insertAutoIndexJobs(t, db, uploadsshared.AutoIndexJob{ID: 4, Indexer: "sourcegraph/scip-typescript"}) - if err := store.SetRerunAutoIndexJobs(context.Background(), shared.SetRerunAutoIndexJobsOptions{ + if err := store.SetRerunAutoIndexJobs(actor.WithInternalActor(context.Background()), shared.SetRerunAutoIndexJobsOptions{ IndexerNames: []string{"scip-go"}, Term: "", RepositoryID: 0, @@ -509,7 +472,7 @@ func TestSetRerunAutoIndexJobsWithIndexerKey(t *testing.T) { 1: true, 2: true, 3: false, 4: false, } { - if index, exists, err := store.GetAutoIndexJobByID(context.Background(), id); err != nil { + if index, exists, err := store.GetAutoIndexJobByID(actor.WithInternalActor(context.Background()), id); err != nil { t.Fatalf("unexpected error getting index: %s", err) } else if !exists { t.Fatal("index missing") diff --git a/internal/codeintel/uploads/internal/store/processing_test.go b/internal/codeintel/uploads/internal/store/processing_test.go index 4b400a977aa..418e7b477cd 100644 --- a/internal/codeintel/uploads/internal/store/processing_test.go +++ b/internal/codeintel/uploads/internal/store/processing_test.go @@ -9,6 +9,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/shared" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/database/dbtest" @@ -51,7 +52,7 @@ func TestInsertUploadUploading(t *testing.T) { UploadedParts: []int{}, } - if upload, exists, err := store.GetUploadByID(context.Background(), id); err != nil { + if upload, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), id); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if !exists { t.Fatal("expected record to exist") @@ -104,7 +105,7 @@ func TestInsertUploadQueued(t *testing.T) { Rank: &rank, } - if upload, exists, err := store.GetUploadByID(context.Background(), id); err != nil { + if upload, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), id); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if !exists { t.Fatal("expected record to exist") @@ -161,7 +162,7 @@ func TestInsertUploadWithAssociatedIndexID(t *testing.T) { AssociatedIndexID: &associatedIndexIDResult, } - if upload, exists, err := store.GetUploadByID(context.Background(), id); err != nil { + if upload, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), id); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if !exists { t.Fatal("expected record to exist") @@ -187,7 +188,7 @@ func TestAddUploadPart(t *testing.T) { t.Fatalf("unexpected error adding upload part: %s", err) } } - if upload, exists, err := store.GetUploadByID(context.Background(), 1); err != nil { + if upload, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if !exists { t.Fatal("expected record to exist") @@ -207,11 +208,11 @@ func TestMarkQueued(t *testing.T) { insertUploads(t, db, shared.Upload{ID: 1, State: "uploading"}) uploadSize := int64(300) - if err := store.MarkQueued(context.Background(), 1, &uploadSize); err != nil { + if err := store.MarkQueued(actor.WithInternalActor(context.Background()), 1, &uploadSize); err != nil { t.Fatalf("unexpected error marking upload as queued: %s", err) } - if upload, exists, err := store.GetUploadByID(context.Background(), 1); err != nil { + if upload, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if !exists { t.Fatal("expected record to exist") @@ -233,11 +234,11 @@ func TestMarkQueuedNoSize(t *testing.T) { insertUploads(t, db, shared.Upload{ID: 1, State: "uploading"}) - if err := store.MarkQueued(context.Background(), 1, nil); err != nil { + if err := store.MarkQueued(actor.WithInternalActor(context.Background()), 1, nil); err != nil { t.Fatalf("unexpected error marking upload as queued: %s", err) } - if upload, exists, err := store.GetUploadByID(context.Background(), 1); err != nil { + if upload, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if !exists { t.Fatal("expected record to exist") @@ -260,7 +261,7 @@ func TestMarkFailed(t *testing.T) { t.Fatalf("unexpected error marking upload as failed: %s", err) } - if upload, exists, err := store.GetUploadByID(context.Background(), 1); err != nil { + if upload, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if !exists { t.Fatal("expected record to exist") @@ -361,7 +362,7 @@ func TestDeleteOverlappingCompletedUploadsIgnoresIncompleteUploads(t *testing.T) } // Original upload still exists - if _, exists, err := store.GetUploadByID(context.Background(), 1); err != nil { + if _, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error getting dump: %s", err) } else if !exists { t.Fatal("expected dump record to still exist") diff --git a/internal/codeintel/uploads/internal/store/summary_test.go b/internal/codeintel/uploads/internal/store/summary_test.go index 1a385368cec..7af1d2736e6 100644 --- a/internal/codeintel/uploads/internal/store/summary_test.go +++ b/internal/codeintel/uploads/internal/store/summary_test.go @@ -8,6 +8,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/shared" uploadsshared "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/shared" "github.com/sourcegraph/sourcegraph/internal/database" @@ -19,7 +20,7 @@ func TestGetIndexers(t *testing.T) { logger := logtest.Scoped(t) db := database.NewDB(logger, dbtest.NewDB(t)) store := New(observation.TestContextTB(t), db) - ctx := context.Background() + ctx := actor.WithInternalActor(context.Background()) insertUploads(t, db, shared.Upload{ID: 1, Indexer: "scip-typescript"}, diff --git a/internal/codeintel/uploads/internal/store/uploads_test.go b/internal/codeintel/uploads/internal/store/uploads_test.go index 1aeab3c426d..52cf9418f0b 100644 --- a/internal/codeintel/uploads/internal/store/uploads_test.go +++ b/internal/codeintel/uploads/internal/store/uploads_test.go @@ -13,6 +13,7 @@ import ( "github.com/lib/pq" "github.com/sourcegraph/log/logtest" + "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/internal/commitgraph" "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/shared" @@ -28,7 +29,7 @@ func TestGetUploads(t *testing.T) { logger := logtest.Scoped(t) db := database.NewDB(logger, dbtest.NewDB(t)) store := New(observation.TestContextTB(t), db) - ctx := context.Background() + ctx := actor.WithInternalActor(context.Background()) t1 := time.Unix(1587396557, 0).UTC() t2 := t1.Add(-time.Minute * 1) @@ -215,20 +216,8 @@ func TestGetUploads(t *testing.T) { } t.Run("enforce repository permissions", func(t *testing.T) { - // Enable permissions user mapping forces checking repository permissions - // against permissions tables in the database, which should effectively block - // all access because permissions tables are empty. - conf.Mock(&conf.Unified{ - SiteConfiguration: schema.SiteConfiguration{ - PermissionsUserMapping: &schema.PermissionsUserMapping{ - Enabled: true, - BindID: "email", - }, - }, - }) - t.Cleanup(func() { conf.Mock(nil) }) - - uploads, totalCount, err := store.GetUploads(ctx, + // Use an actorless context to test permissions. + uploads, totalCount, err := store.GetUploads(context.Background(), shared.GetUploadsOptions{ Limit: 1, }, @@ -243,7 +232,7 @@ func TestGetUploads(t *testing.T) { } func TestGetUploadByID(t *testing.T) { - ctx := context.Background() + ctx := actor.WithInternalActor(context.Background()) logger := logtest.Scoped(t) db := database.NewDB(logger, dbtest.NewDB(t)) store := New(observation.TestContextTB(t), db) @@ -288,20 +277,8 @@ func TestGetUploadByID(t *testing.T) { } t.Run("enforce repository permissions", func(t *testing.T) { - // Enable permissions user mapping forces checking repository permissions - // against permissions tables in the database, which should effectively block - // all access because permissions tables are empty. - conf.Mock(&conf.Unified{ - SiteConfiguration: schema.SiteConfiguration{ - PermissionsUserMapping: &schema.PermissionsUserMapping{ - Enabled: true, - BindID: "email", - }, - }, - }) - t.Cleanup(func() { conf.Mock(nil) }) - - _, exists, err := store.GetUploadByID(ctx, 1) + // Use an actorless context to test permissions. + _, exists, err := store.GetUploadByID(context.Background(), 1) if err != nil { t.Fatal(err) } @@ -317,7 +294,7 @@ func TestGetUploadByIDDeleted(t *testing.T) { store := New(observation.TestContextTB(t), db) // Upload does not exist initially - if _, exists, err := store.GetUploadByID(context.Background(), 1); err != nil { + if _, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if exists { t.Fatal("unexpected record") @@ -346,7 +323,7 @@ func TestGetUploadByIDDeleted(t *testing.T) { insertUploads(t, db, expected) // Should still not be queryable - if _, exists, err := store.GetUploadByID(context.Background(), 1); err != nil { + if _, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if exists { t.Fatal("unexpected record") @@ -425,7 +402,7 @@ func TestGetCompletedUploadsByIDs(t *testing.T) { } func TestGetUploadsByIDs(t *testing.T) { - ctx := context.Background() + ctx := actor.WithInternalActor(context.Background()) logger := logtest.Scoped(t) db := database.NewDB(logger, dbtest.NewDB(t)) store := New(observation.TestContextTB(t), db) @@ -461,20 +438,8 @@ func TestGetUploadsByIDs(t *testing.T) { }) t.Run("enforce repository permissions", func(t *testing.T) { - // Enable permissions user mapping forces checking repository permissions - // against permissions tables in the database, which should effectively block - // all access because permissions tables are empty. - conf.Mock(&conf.Unified{ - SiteConfiguration: schema.SiteConfiguration{ - PermissionsUserMapping: &schema.PermissionsUserMapping{ - Enabled: true, - BindID: "email", - }, - }, - }) - t.Cleanup(func() { conf.Mock(nil) }) - - indexes, err := store.GetUploadsByIDs(ctx, 1, 2, 3, 4) + // Use an actorless context to test permissions. + indexes, err := store.GetUploadsByIDs(context.Background(), 1, 2, 3, 4) if err != nil { t.Fatal(err) } @@ -567,7 +532,7 @@ func TestGetVisibleUploadsMatchingMonikers(t *testing.T) { for i, testCase := range testCases { t.Run(fmt.Sprintf("i=%d", i), func(t *testing.T) { - scanner, totalCount, err := store.GetVisibleUploadsMatchingMonikers(context.Background(), 50, makeCommit(1), []precise.QualifiedMonikerData{moniker}, testCase.limit, testCase.offset) + scanner, totalCount, err := store.GetVisibleUploadsMatchingMonikers(actor.WithInternalActor(context.Background()), 50, makeCommit(1), []precise.QualifiedMonikerData{moniker}, testCase.limit, testCase.offset) if err != nil { t.Fatalf("unexpected error getting scanner: %s", err) } @@ -637,7 +602,7 @@ func TestDefinitionDumps(t *testing.T) { } // Package does not exist initially - if uploads, err := store.GetCompletedUploadsWithDefinitionsForMonikers(context.Background(), []precise.QualifiedMonikerData{moniker1}); err != nil { + if uploads, err := store.GetCompletedUploadsWithDefinitionsForMonikers(actor.WithInternalActor(context.Background()), []precise.QualifiedMonikerData{moniker1}); err != nil { t.Fatalf("unexpected error getting package: %s", err) } else if len(uploads) != 0 { t.Fatal("unexpected record") @@ -715,7 +680,7 @@ func TestDefinitionDumps(t *testing.T) { t.Fatalf("unexpected error updating packages: %s", err) } - if uploads, err := store.GetCompletedUploadsWithDefinitionsForMonikers(context.Background(), []precise.QualifiedMonikerData{moniker1}); err != nil { + if uploads, err := store.GetCompletedUploadsWithDefinitionsForMonikers(actor.WithInternalActor(context.Background()), []precise.QualifiedMonikerData{moniker1}); err != nil { t.Fatalf("unexpected error getting package: %s", err) } else if len(uploads) != 1 { t.Fatal("expected one record") @@ -723,7 +688,7 @@ func TestDefinitionDumps(t *testing.T) { t.Errorf("unexpected dump (-want +got):\n%s", diff) } - if uploads, err := store.GetCompletedUploadsWithDefinitionsForMonikers(context.Background(), []precise.QualifiedMonikerData{moniker1, moniker2}); err != nil { + if uploads, err := store.GetCompletedUploadsWithDefinitionsForMonikers(actor.WithInternalActor(context.Background()), []precise.QualifiedMonikerData{moniker1, moniker2}); err != nil { t.Fatalf("unexpected error getting package: %s", err) } else if len(uploads) != 2 { t.Fatal("expected two records") @@ -734,20 +699,7 @@ func TestDefinitionDumps(t *testing.T) { } t.Run("enforce repository permissions", func(t *testing.T) { - // Turning on explicit permissions forces checking repository permissions - // against permissions tables in the database, which should effectively block - // all access because permissions tables are empty and repo that dumps belong - // to are private. - conf.Mock(&conf.Unified{ - SiteConfiguration: schema.SiteConfiguration{ - PermissionsUserMapping: &schema.PermissionsUserMapping{ - Enabled: true, - BindID: "email", - }, - }, - }) - t.Cleanup(func() { conf.Mock(nil) }) - + // Use an actorless context to test permissions. if uploads, err := store.GetCompletedUploadsWithDefinitionsForMonikers(context.Background(), []precise.QualifiedMonikerData{moniker1, moniker2}); err != nil { t.Fatalf("unexpected error getting package: %s", err) } else if len(uploads) != 0 { @@ -765,7 +717,7 @@ func TestUploadAuditLogs(t *testing.T) { insertUploads(t, db, shared.Upload{ID: 1}) updateUploads(t, db, shared.Upload{ID: 1, State: "deleting"}) - logs, err := store.GetAuditLogsForUpload(context.Background(), 1) + logs, err := store.GetAuditLogsForUpload(actor.WithInternalActor(context.Background()), 1) if err != nil { t.Fatalf("unexpected error fetching audit logs: %s", err) } @@ -809,7 +761,7 @@ func TestDeleteUploads(t *testing.T) { shared.Upload{ID: 5, Commit: makeCommit(1115), UploadedAt: t5, State: "uploading"}, // will be deleted ) - err := store.DeleteUploads(context.Background(), shared.DeleteUploadsOptions{ + err := store.DeleteUploads(actor.WithInternalActor(context.Background()), shared.DeleteUploadsOptions{ States: []string{"uploading"}, Term: "", VisibleAtTip: false, @@ -818,7 +770,7 @@ func TestDeleteUploads(t *testing.T) { t.Fatalf("unexpected error deleting uploads: %s", err) } - uploads, totalCount, err := store.GetUploads(context.Background(), shared.GetUploadsOptions{Limit: 5}) + uploads, totalCount, err := store.GetUploads(actor.WithInternalActor(context.Background()), shared.GetUploadsOptions{Limit: 5}) if err != nil { t.Fatalf("unexpected error getting uploads: %s", err) } @@ -850,7 +802,7 @@ func TestDeleteUploadsWithIndexerKey(t *testing.T) { insertUploads(t, db, shared.Upload{ID: 3, State: "queued", Indexer: "sourcegraph/scip-typescript"}) insertUploads(t, db, shared.Upload{ID: 4, State: "queued", Indexer: "sourcegraph/scip-typescript"}) - err := store.DeleteUploads(context.Background(), shared.DeleteUploadsOptions{ + err := store.DeleteUploads(actor.WithInternalActor(context.Background()), shared.DeleteUploadsOptions{ IndexerNames: []string{"scip-go"}, Term: "", VisibleAtTip: false, @@ -859,7 +811,7 @@ func TestDeleteUploadsWithIndexerKey(t *testing.T) { t.Fatalf("unexpected error deleting uploads: %s", err) } - uploads, totalCount, err := store.GetUploads(context.Background(), shared.GetUploadsOptions{Limit: 5}) + uploads, totalCount, err := store.GetUploads(actor.WithInternalActor(context.Background()), shared.GetUploadsOptions{Limit: 5}) if err != nil { t.Fatalf("unexpected error getting uploads: %s", err) } @@ -889,7 +841,7 @@ func TestDeleteUploadByID(t *testing.T) { shared.Upload{ID: 1, RepositoryID: 50}, ) - if found, err := store.DeleteUploadByID(context.Background(), 1); err != nil { + if found, err := store.DeleteUploadByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error deleting upload: %s", err) } else if !found { t.Fatalf("expected record to exist") @@ -923,7 +875,7 @@ func TestDeleteUploadByIDMissingRow(t *testing.T) { db := database.NewDB(logger, dbtest.NewDB(t)) store := New(observation.TestContextTB(t), db) - if found, err := store.DeleteUploadByID(context.Background(), 1); err != nil { + if found, err := store.DeleteUploadByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error deleting upload: %s", err) } else if found { t.Fatalf("unexpected record") @@ -939,7 +891,7 @@ func TestDeleteUploadByIDNotCompleted(t *testing.T) { shared.Upload{ID: 1, RepositoryID: 50, State: "uploading"}, ) - if found, err := store.DeleteUploadByID(context.Background(), 1); err != nil { + if found, err := store.DeleteUploadByID(actor.WithInternalActor(context.Background()), 1); err != nil { t.Fatalf("unexpected error deleting upload: %s", err) } else if !found { t.Fatalf("expected record to exist") @@ -976,7 +928,7 @@ func TestReindexUploads(t *testing.T) { insertUploads(t, db, shared.Upload{ID: 1, State: "completed"}) insertUploads(t, db, shared.Upload{ID: 2, State: "errored"}) - if err := store.ReindexUploads(context.Background(), shared.ReindexUploadsOptions{ + if err := store.ReindexUploads(actor.WithInternalActor(context.Background()), shared.ReindexUploadsOptions{ States: []string{"errored"}, Term: "", RepositoryID: 0, @@ -985,7 +937,7 @@ func TestReindexUploads(t *testing.T) { } // Upload has been marked for reindexing - if upload, exists, err := store.GetUploadByID(context.Background(), 2); err != nil { + if upload, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), 2); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if !exists { t.Fatal("upload missing") @@ -1004,7 +956,7 @@ func TestReindexUploadsWithIndexerKey(t *testing.T) { insertUploads(t, db, shared.Upload{ID: 3, Indexer: "sourcegraph/scip-typescript"}) insertUploads(t, db, shared.Upload{ID: 4, Indexer: "sourcegraph/scip-typescript"}) - if err := store.ReindexUploads(context.Background(), shared.ReindexUploadsOptions{ + if err := store.ReindexUploads(actor.WithInternalActor(context.Background()), shared.ReindexUploadsOptions{ IndexerNames: []string{"scip-go"}, Term: "", RepositoryID: 0, @@ -1017,7 +969,7 @@ func TestReindexUploadsWithIndexerKey(t *testing.T) { 1: true, 2: true, 3: false, 4: false, } { - if upload, exists, err := store.GetUploadByID(context.Background(), id); err != nil { + if upload, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), id); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if !exists { t.Fatal("upload missing") @@ -1040,7 +992,7 @@ func TestReindexUploadByID(t *testing.T) { } // Upload has been marked for reindexing - if upload, exists, err := store.GetUploadByID(context.Background(), 2); err != nil { + if upload, exists, err := store.GetUploadByID(actor.WithInternalActor(context.Background()), 2); err != nil { t.Fatalf("unexpected error getting upload: %s", err) } else if !exists { t.Fatal("upload missing") diff --git a/internal/conf/validation/auth.go b/internal/conf/validation/auth.go index 568431f0af8..41bdf51d7b2 100644 --- a/internal/conf/validation/auth.go +++ b/internal/conf/validation/auth.go @@ -17,7 +17,7 @@ func init() { } func validateAuthzProviders(cfg conftypes.SiteConfigQuerier, db database.DB) (problems conf.Problems) { - _, providers, seriousProblems, warnings, _ := providers.ProvidersFromConfig(context.Background(), cfg, db) + providers, seriousProblems, warnings, _ := providers.ProvidersFromConfig(context.Background(), cfg, db) problems = append(problems, conf.NewExternalServiceProblems(seriousProblems...)...) // Validating the connection may make a cross service call, so we should use an diff --git a/internal/database/dbmocks/mocks_temp.go b/internal/database/dbmocks/mocks_temp.go index 602124d24ad..eeb717fa4e4 100644 --- a/internal/database/dbmocks/mocks_temp.go +++ b/internal/database/dbmocks/mocks_temp.go @@ -52303,9 +52303,6 @@ type MockPermsStore struct { // HandleFunc is an instance of a mock function object controlling the // behavior of the method Handle. HandleFunc *PermsStoreHandleFunc - // IsRepoUnrestrictedFunc is an instance of a mock function object - // controlling the behavior of the method IsRepoUnrestricted. - IsRepoUnrestrictedFunc *PermsStoreIsRepoUnrestrictedFunc // ListPendingUsersFunc is an instance of a mock function object // controlling the behavior of the method ListPendingUsers. ListPendingUsersFunc *PermsStoreListPendingUsersFunc @@ -52425,11 +52422,6 @@ func NewMockPermsStore() *MockPermsStore { return }, }, - IsRepoUnrestrictedFunc: &PermsStoreIsRepoUnrestrictedFunc{ - defaultHook: func(context.Context, api.RepoID) (r0 bool, r1 error) { - return - }, - }, ListPendingUsersFunc: &PermsStoreListPendingUsersFunc{ defaultHook: func(context.Context, string, string) (r0 []string, r1 error) { return @@ -52582,11 +52574,6 @@ func NewStrictMockPermsStore() *MockPermsStore { panic("unexpected invocation of MockPermsStore.Handle") }, }, - IsRepoUnrestrictedFunc: &PermsStoreIsRepoUnrestrictedFunc{ - defaultHook: func(context.Context, api.RepoID) (bool, error) { - panic("unexpected invocation of MockPermsStore.IsRepoUnrestricted") - }, - }, ListPendingUsersFunc: &PermsStoreListPendingUsersFunc{ defaultHook: func(context.Context, string, string) ([]string, error) { panic("unexpected invocation of MockPermsStore.ListPendingUsers") @@ -52717,9 +52704,6 @@ func NewMockPermsStoreFrom(i database.PermsStore) *MockPermsStore { HandleFunc: &PermsStoreHandleFunc{ defaultHook: i.Handle, }, - IsRepoUnrestrictedFunc: &PermsStoreIsRepoUnrestrictedFunc{ - defaultHook: i.IsRepoUnrestricted, - }, ListPendingUsersFunc: &PermsStoreListPendingUsersFunc{ defaultHook: i.ListPendingUsers, }, @@ -53963,115 +53947,6 @@ func (c PermsStoreHandleFuncCall) Results() []interface{} { return []interface{}{c.Result0} } -// PermsStoreIsRepoUnrestrictedFunc describes the behavior when the -// IsRepoUnrestricted method of the parent MockPermsStore instance is -// invoked. -type PermsStoreIsRepoUnrestrictedFunc struct { - defaultHook func(context.Context, api.RepoID) (bool, error) - hooks []func(context.Context, api.RepoID) (bool, error) - history []PermsStoreIsRepoUnrestrictedFuncCall - mutex sync.Mutex -} - -// IsRepoUnrestricted delegates to the next hook function in the queue and -// stores the parameter and result values of this invocation. -func (m *MockPermsStore) IsRepoUnrestricted(v0 context.Context, v1 api.RepoID) (bool, error) { - r0, r1 := m.IsRepoUnrestrictedFunc.nextHook()(v0, v1) - m.IsRepoUnrestrictedFunc.appendCall(PermsStoreIsRepoUnrestrictedFuncCall{v0, v1, r0, r1}) - return r0, r1 -} - -// SetDefaultHook sets function that is called when the IsRepoUnrestricted -// method of the parent MockPermsStore instance is invoked and the hook -// queue is empty. -func (f *PermsStoreIsRepoUnrestrictedFunc) SetDefaultHook(hook func(context.Context, api.RepoID) (bool, error)) { - f.defaultHook = hook -} - -// PushHook adds a function to the end of hook queue. Each invocation of the -// IsRepoUnrestricted method of the parent MockPermsStore 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 *PermsStoreIsRepoUnrestrictedFunc) PushHook(hook func(context.Context, api.RepoID) (bool, error)) { - f.mutex.Lock() - f.hooks = append(f.hooks, hook) - f.mutex.Unlock() -} - -// SetDefaultReturn calls SetDefaultHook with a function that returns the -// given values. -func (f *PermsStoreIsRepoUnrestrictedFunc) SetDefaultReturn(r0 bool, r1 error) { - f.SetDefaultHook(func(context.Context, api.RepoID) (bool, error) { - return r0, r1 - }) -} - -// PushReturn calls PushHook with a function that returns the given values. -func (f *PermsStoreIsRepoUnrestrictedFunc) PushReturn(r0 bool, r1 error) { - f.PushHook(func(context.Context, api.RepoID) (bool, error) { - return r0, r1 - }) -} - -func (f *PermsStoreIsRepoUnrestrictedFunc) nextHook() func(context.Context, api.RepoID) (bool, error) { - f.mutex.Lock() - defer f.mutex.Unlock() - - if len(f.hooks) == 0 { - return f.defaultHook - } - - hook := f.hooks[0] - f.hooks = f.hooks[1:] - return hook -} - -func (f *PermsStoreIsRepoUnrestrictedFunc) appendCall(r0 PermsStoreIsRepoUnrestrictedFuncCall) { - f.mutex.Lock() - f.history = append(f.history, r0) - f.mutex.Unlock() -} - -// History returns a sequence of PermsStoreIsRepoUnrestrictedFuncCall -// objects describing the invocations of this function. -func (f *PermsStoreIsRepoUnrestrictedFunc) History() []PermsStoreIsRepoUnrestrictedFuncCall { - f.mutex.Lock() - history := make([]PermsStoreIsRepoUnrestrictedFuncCall, len(f.history)) - copy(history, f.history) - f.mutex.Unlock() - - return history -} - -// PermsStoreIsRepoUnrestrictedFuncCall is an object that describes an -// invocation of method IsRepoUnrestricted on an instance of MockPermsStore. -type PermsStoreIsRepoUnrestrictedFuncCall struct { - // Arg0 is the value of the 1st argument passed to this method - // invocation. - Arg0 context.Context - // Arg1 is the value of the 2nd argument passed to this method - // invocation. - Arg1 api.RepoID - // Result0 is the value of the 1st result returned from this method - // invocation. - Result0 bool - // Result1 is the value of the 2nd result returned from this method - // invocation. - Result1 error -} - -// Args returns an interface slice containing the arguments of this -// invocation. -func (c PermsStoreIsRepoUnrestrictedFuncCall) Args() []interface{} { - return []interface{}{c.Arg0, c.Arg1} -} - -// Results returns an interface slice containing the results of this -// invocation. -func (c PermsStoreIsRepoUnrestrictedFuncCall) Results() []interface{} { - return []interface{}{c.Result0, c.Result1} -} - // PermsStoreListPendingUsersFunc describes the behavior when the // ListPendingUsers method of the parent MockPermsStore instance is invoked. type PermsStoreListPendingUsersFunc struct { diff --git a/internal/database/perms_store.go b/internal/database/perms_store.go index 6cf32be0d1c..40fb0b5f8ff 100644 --- a/internal/database/perms_store.go +++ b/internal/database/perms_store.go @@ -179,8 +179,6 @@ type PermsStore interface { ListUserPermissions(ctx context.Context, userID int32, args *ListUserPermissionsArgs) (perms []*UserPermission, err error) // ListRepoPermissions returns list of users the repo is accessible to. ListRepoPermissions(ctx context.Context, repoID api.RepoID, args *ListRepoPermissionsArgs) (perms []*RepoPermission, err error) - // IsRepoUnrestructed returns if the repo is unrestricted. - IsRepoUnrestricted(ctx context.Context, repoID api.RepoID) (unrestricted bool, err error) } // It is concurrency-safe and maintains data consistency over the 'user_permissions', @@ -2041,28 +2039,22 @@ func (s *permsStore) ListRepoPermissions(ctx context.Context, repoID api.RepoID, permsQueryConditions := []*sqlf.Query{} unrestricted := false - if authzParams.BypassAuthzReasons.NoAuthzProvider { - // return all users as auth is bypassed for everyone + // find if the repo is unrestricted + unrestricted, err = s.isRepoUnrestricted(ctx, repoID, authzParams) + if err != nil { + return nil, err + } + + if unrestricted { + // return all users as repo is unrestricted permsQueryConditions = append(permsQueryConditions, sqlf.Sprintf("TRUE")) - unrestricted = true } else { - // find if the repo is unrestricted - unrestricted, err = s.isRepoUnrestricted(ctx, repoID, authzParams) - if err != nil { - return nil, err + if !authzParams.AuthzEnforceForSiteAdmins { + // include all site admins + permsQueryConditions = append(permsQueryConditions, sqlf.Sprintf("users.site_admin")) } - if unrestricted { - // return all users as repo is unrestricted - permsQueryConditions = append(permsQueryConditions, sqlf.Sprintf("TRUE")) - } else { - if !authzParams.AuthzEnforceForSiteAdmins { - // include all site admins - permsQueryConditions = append(permsQueryConditions, sqlf.Sprintf("users.site_admin")) - } - - permsQueryConditions = append(permsQueryConditions, sqlf.Sprintf(`urp.repo_id = %d`, repoID)) - } + permsQueryConditions = append(permsQueryConditions, sqlf.Sprintf(`urp.repo_id = %d`, repoID)) } where := []*sqlf.Query{sqlf.Sprintf("(%s)", sqlf.Join(permsQueryConditions, " OR "))} @@ -2183,15 +2175,6 @@ WHERE AND %s ` -func (s *permsStore) IsRepoUnrestricted(ctx context.Context, repoID api.RepoID) (bool, error) { - authzParams, err := GetAuthzQueryParameters(context.Background(), s.db) - if err != nil { - return false, err - } - - return s.isRepoUnrestricted(ctx, repoID, authzParams) -} - func (s *permsStore) isRepoUnrestricted(ctx context.Context, repoID api.RepoID, authzParams *AuthzQueryParameters) (bool, error) { conditions := []*sqlf.Query{GetUnrestrictedReposCond()} diff --git a/internal/database/perms_store_test.go b/internal/database/perms_store_test.go index e1d4ff70aa8..f7886f89a41 100644 --- a/internal/database/perms_store_test.go +++ b/internal/database/perms_store_test.go @@ -30,7 +30,6 @@ import ( "github.com/sourcegraph/sourcegraph/internal/database/dbtest" "github.com/sourcegraph/sourcegraph/internal/database/dbutil" "github.com/sourcegraph/sourcegraph/internal/extsvc" - "github.com/sourcegraph/sourcegraph/internal/types" "github.com/sourcegraph/sourcegraph/lib/errors" "github.com/sourcegraph/sourcegraph/lib/pointers" "github.com/sourcegraph/sourcegraph/schema" @@ -3926,9 +3925,6 @@ func TestPermsStore_ListUserPermissions(t *testing.T) { t.Fatal(err) } }) - // Set fake authz providers otherwise authz is bypassed - authz.SetProviders(false, []authz.Provider{&fakePermsProvider{}}) - defer authz.SetProviders(true, nil) // Set up some repositories and permissions qs := []*sqlf.Query{ sqlf.Sprintf(`INSERT INTO users(id, username, site_admin) VALUES(555, 'user555', FALSE)`), @@ -4149,31 +4145,6 @@ func TestPermsStore_ListRepoPermissions(t *testing.T) { }, }, }, - { - Name: "TestPrivateRepoWithNoAuthzProviders", - RepoID: 1, - Args: nil, - NoAuthzProviders: true, - // all users have access - WantResults: []*listRepoPermissionsResult{ - { - UserID: 999, - Reason: UserRepoPermissionReasonUnrestricted, - }, - { - UserID: 777, - Reason: UserRepoPermissionReasonUnrestricted, - }, - { - UserID: 666, - Reason: UserRepoPermissionReasonUnrestricted, - }, - { - UserID: 555, - Reason: UserRepoPermissionReasonUnrestricted, - }, - }, - }, { Name: "TestPaginationWithPrivateRepo", RepoID: 1, @@ -4272,32 +4243,6 @@ func TestPermsStore_ListRepoPermissions(t *testing.T) { }, }, }, - { - Name: "TestUnrestrictedViaExternalServiceRepoWithoutPermsMapping", - RepoID: 4, - Args: nil, - NoAuthzProviders: true, - UsePermissionsUserMapping: false, - // restricted access - WantResults: []*listRepoPermissionsResult{ - { - UserID: 999, - Reason: UserRepoPermissionReasonUnrestricted, - }, - { - UserID: 777, - Reason: UserRepoPermissionReasonUnrestricted, - }, - { - UserID: 666, - Reason: UserRepoPermissionReasonUnrestricted, - }, - { - UserID: 555, - Reason: UserRepoPermissionReasonUnrestricted, - }, - }, - }, { Name: "TestPrivateRepoWithAuthzEnforceForSiteAdminsEnabled", RepoID: 1, @@ -4325,12 +4270,6 @@ func TestPermsStore_ListRepoPermissions(t *testing.T) { for _, test := range tests { t.Run(test.Name, func(t *testing.T) { - if !test.NoAuthzProviders { - // Set fake authz providers otherwise authz is bypassed - authz.SetProviders(false, []authz.Provider{&fakePermsProvider{}}) - defer authz.SetProviders(true, nil) - } - conf.Mock( &conf.Unified{ SiteConfiguration: schema.SiteConfiguration{ @@ -4372,7 +4311,6 @@ type listRepoPermissionsTest struct { RepoID int Args *ListRepoPermissionsArgs WantResults []*listRepoPermissionsResult - NoAuthzProviders bool UsePermissionsUserMapping bool AuthzEnforceForSiteAdmins bool } @@ -4382,29 +4320,6 @@ type listRepoPermissionsResult struct { Reason UserRepoPermissionReason } -type fakePermsProvider struct { - codeHost *extsvc.CodeHost - extAcct *extsvc.Account -} - -func (p *fakePermsProvider) FetchAccount(context.Context, *types.User, []*extsvc.Account, []string) (mine *extsvc.Account, err error) { - return p.extAcct, nil -} - -func (p *fakePermsProvider) ServiceType() string { return p.codeHost.ServiceType } -func (p *fakePermsProvider) ServiceID() string { return p.codeHost.ServiceID } -func (p *fakePermsProvider) URN() string { return extsvc.URN(p.codeHost.ServiceType, 0) } - -func (p *fakePermsProvider) ValidateConnection(context.Context) error { return nil } - -func (p *fakePermsProvider) FetchUserPerms(context.Context, *extsvc.Account, authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { - return nil, nil -} - -func (p *fakePermsProvider) FetchRepoPerms(context.Context, *extsvc.Repository, authz.FetchPermsOptions) ([]extsvc.AccountID, error) { - return nil, nil -} - func equal(t testing.TB, name string, want, have any) { t.Helper() if diff := cmp.Diff(want, have); diff != "" { diff --git a/internal/database/repos_perm.go b/internal/database/repos_perm.go index f61bbd9d828..de546bf32a4 100644 --- a/internal/database/repos_perm.go +++ b/internal/database/repos_perm.go @@ -6,14 +6,12 @@ import ( "github.com/keegancsmith/sqlf" "github.com/sourcegraph/sourcegraph/internal/actor" - "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/conf" ) type BypassAuthzReasonsMap struct { - SiteAdmin bool - IsInternal bool - NoAuthzProvider bool + SiteAdmin bool + IsInternal bool } type AuthzQueryParameters struct { @@ -30,16 +28,14 @@ func (p *AuthzQueryParameters) ToAuthzQuery() *sqlf.Query { func GetAuthzQueryParameters(ctx context.Context, db DB) (params *AuthzQueryParameters, err error) { params = &AuthzQueryParameters{} - authzAllowByDefault, authzProviders := authz.GetProviders() params.UsePermissionsUserMapping = conf.PermissionsUserMapping().Enabled params.AuthzEnforceForSiteAdmins = conf.Get().AuthzEnforceForSiteAdmins a := actor.FromContext(ctx) - // Authz is bypassed when the request is coming from an internal actor or - // there is no authz provider configured and access to all repositories are - // allowed by default. Authz can be bypassed by site admins unless - // conf.AuthEnforceForSiteAdmins is set to "true". + // Authz is bypassed when the request is coming from an internal actor. + // Authz can be bypassed by site admins unless conf.AuthEnforceForSiteAdmins + // is set to "true". // // 🚨 SECURITY: internal requests bypass authz provider permissions checks, // so correctness is important here. @@ -48,16 +44,8 @@ func GetAuthzQueryParameters(ctx context.Context, db DB) (params *AuthzQueryPara params.BypassAuthzReasons.IsInternal = true } - // 🚨 SECURITY: If explicit permissions API is ON, we want to enforce authz - // even if there are no authz providers configured. - // Otherwise bypass authorization with no authz providers. - if !params.UsePermissionsUserMapping && authzAllowByDefault && len(authzProviders) == 0 { - params.BypassAuthz = true - params.BypassAuthzReasons.NoAuthzProvider = true - } - if a.IsAuthenticated() { - currentUser, err := db.Users().GetByCurrentAuthUser(ctx) + currentUser, err := a.User(ctx, db.Users()) if err != nil { if !params.BypassAuthz { return nil, err diff --git a/internal/database/repos_perm_test.go b/internal/database/repos_perm_test.go index 05791a99e73..0cb1c5bfc23 100644 --- a/internal/database/repos_perm_test.go +++ b/internal/database/repos_perm_test.go @@ -20,7 +20,6 @@ import ( "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" - "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/database/batch" "github.com/sourcegraph/sourcegraph/internal/database/dbtest" @@ -29,33 +28,6 @@ import ( "github.com/sourcegraph/sourcegraph/schema" ) -type fakeProvider struct { - codeHost *extsvc.CodeHost - extAcct *extsvc.Account -} - -func (p *fakeProvider) FetchAccount(context.Context, *types.User, []*extsvc.Account, []string) (mine *extsvc.Account, err error) { - return p.extAcct, nil -} - -func (p *fakeProvider) ServiceType() string { return p.codeHost.ServiceType } -func (p *fakeProvider) ServiceID() string { return p.codeHost.ServiceID } -func (p *fakeProvider) URN() string { return extsvc.URN(p.codeHost.ServiceType, 0) } - -func (p *fakeProvider) ValidateConnection(context.Context) error { return nil } - -func (p *fakeProvider) FetchUserPerms(context.Context, *extsvc.Account, authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { - return nil, nil -} - -func (p *fakeProvider) FetchUserPermsByToken(context.Context, string, authz.FetchPermsOptions) (*authz.ExternalUserPermissions, error) { - return nil, nil -} - -func (p *fakeProvider) FetchRepoPerms(context.Context, *extsvc.Repository, authz.FetchPermsOptions) ([]extsvc.AccountID, error) { - return nil, nil -} - func mockExplicitPermsConfig(enabled bool) func() { conf.Mock(&conf.Unified{ SiteConfiguration: schema.SiteConfiguration{ @@ -76,10 +48,8 @@ func TestAuthzQueryConds(t *testing.T) { db := NewDB(logger, dbtest.NewDB(t)) t.Run("When permissions user mapping is enabled", func(t *testing.T) { - authz.SetProviders(false, []authz.Provider{&fakeProvider{}}) cleanup := mockExplicitPermsConfig(true) t.Cleanup(func() { - authz.SetProviders(true, nil) cleanup() }) @@ -93,10 +63,8 @@ func TestAuthzQueryConds(t *testing.T) { }) t.Run("When permissions user mapping is enabled, unrestricted repos work correctly", func(t *testing.T) { - authz.SetProviders(false, []authz.Provider{&fakeProvider{}}) cleanup := mockExplicitPermsConfig(true) t.Cleanup(func() { - authz.SetProviders(true, nil) cleanup() }) @@ -112,10 +80,9 @@ func TestAuthzQueryConds(t *testing.T) { u, err := db.Users().Create(context.Background(), NewUser{Username: "testuser"}) require.NoError(t, err) tests := []struct { - name string - setup func(t *testing.T) (context.Context, DB) - authzAllowByDefault bool - wantQuery *sqlf.Query + name string + setup func(t *testing.T) (context.Context, DB) + wantQuery *sqlf.Query }{ { name: "internal actor bypass checks", @@ -125,20 +92,12 @@ func TestAuthzQueryConds(t *testing.T) { wantQuery: authzQuery(true, int32(0)), }, { - name: "no authz provider and not allow by default", + name: "no authz provider", setup: func(t *testing.T) (context.Context, DB) { return context.Background(), db }, wantQuery: authzQuery(false, int32(0)), }, - { - name: "no authz provider but allow by default", - setup: func(t *testing.T) (context.Context, DB) { - return context.Background(), db - }, - authzAllowByDefault: true, - wantQuery: authzQuery(true, int32(0)), - }, { name: "authenticated user is a site admin", setup: func(_ *testing.T) (context.Context, DB) { @@ -171,9 +130,6 @@ func TestAuthzQueryConds(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - authz.SetProviders(test.authzAllowByDefault, nil) - defer authz.SetProviders(true, nil) - ctx, mockDB := test.setup(t) q, err := AuthzQueryConds(ctx, mockDB) if err != nil { @@ -294,11 +250,6 @@ func TestRepoStore_userCanSeeUnrestricedRepo(t *testing.T) { ctx := context.Background() alice, unrestrictedRepo := setupUnrestrictedDB(t, ctx, db) - authz.SetProviders(false, []authz.Provider{&fakeProvider{}}) - t.Cleanup(func() { - authz.SetProviders(true, nil) - }) - t.Run("Alice cannot see private repo, but can see unrestricted repo", func(t *testing.T) { aliceCtx := actor.WithActor(ctx, &actor.Actor{UID: alice.ID}) repos, err := db.Repos().List(aliceCtx, ReposListOptions{}) @@ -365,12 +316,10 @@ func TestRepoStore_userCanSeePublicRepo(t *testing.T) { alice, publicRepo := setupPublicRepo(t, db) t.Run("Alice can see public repo with explicit permissions ON", func(t *testing.T) { - authz.SetProviders(false, []authz.Provider{&fakeProvider{}}) cleanup := mockExplicitPermsConfig(true) t.Cleanup(func() { cleanup() - authz.SetProviders(true, nil) }) aliceCtx := actor.WithActor(ctx, &actor.Actor{UID: alice.ID}) @@ -526,9 +475,6 @@ func TestRepoStore_List_checkPermissions(t *testing.T) { admin, alice, bob, cindy := users["admin"], users["alice"], users["bob"], users["cindy"] alicePublicRepo, alicePrivateRepo, bobPublicRepo, bobPrivateRepo, cindyPrivateRepo := repos["alice_public_repo"], repos["alice_private_repo"], repos["bob_public_repo"], repos["bob_private_repo"], repos["cindy_private_repo"] - authz.SetProviders(false, []authz.Provider{&fakeProvider{}}) - defer authz.SetProviders(true, nil) - assertRepos := func(t *testing.T, ctx context.Context, want []*types.Repo) { t.Helper() repos, err := db.Repos().List(ctx, ReposListOptions{OrderBy: []RepoListSort{{Field: RepoListID}}}) diff --git a/internal/database/repos_test.go b/internal/database/repos_test.go index d1cf9d7dc95..8998a53efc3 100644 --- a/internal/database/repos_test.go +++ b/internal/database/repos_test.go @@ -2915,7 +2915,7 @@ func TestRepoStore_Metadata(t *testing.T) { }, } - md, err := r.Metadata(ctx, 1, 2) + md, err := r.Metadata(actor.WithInternalActor(ctx), 1, 2) require.NoError(t, err) require.ElementsMatch(t, expected, md) } diff --git a/internal/extsvc/types.go b/internal/extsvc/types.go index 8ff7cbed5e9..4f10d4bbc76 100644 --- a/internal/extsvc/types.go +++ b/internal/extsvc/types.go @@ -192,9 +192,6 @@ const ( // VariantAzureDevOps is the (api.ExternalRepoSpec).ServiceType value for ADO projects. VariantAzureDevOps - // VariantAzureDevOps is the (api.ExternalRepoSpec).ServiceType value for ADO projects. - VariantSCIM - // VariantNpmPackages is the (api.ExternalRepoSpec).ServiceType value for Npm packages (JavaScript/VariantScript ecosystem libraries). VariantNpmPackages @@ -238,7 +235,6 @@ var variantValuesMap = map[Variant]variantValues{ VariantPythonPackages: {AsKind: "PYTHONPACKAGES", AsType: "pythonPackages", ConfigPrototype: func() any { return &schema.PythonPackagesConnection{} }}, VariantRubyPackages: {AsKind: "RUBYPACKAGES", AsType: "rubyPackages", ConfigPrototype: func() any { return &schema.RubyPackagesConnection{} }}, VariantRustPackages: {AsKind: "RUSTPACKAGES", AsType: "rustPackages", ConfigPrototype: func() any { return &schema.RustPackagesConnection{} }}, - VariantSCIM: {AsKind: "SCIM", AsType: "scim"}, } func (v Variant) AsKind() string { @@ -304,7 +300,6 @@ var ( KindNpmPackages = VariantNpmPackages.AsKind() KindPagure = VariantPagure.AsKind() KindAzureDevOps = VariantAzureDevOps.AsKind() - KindSCIM = VariantSCIM.AsKind() KindOther = VariantOther.AsKind() )