artin/sourcegraph
1
0
Fork 0
mirror of https://github.com/sourcegraph/sourcegraph.git synced 2026-02-06 17:31:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

authz: Don't return error for subrepoperm constructor (#57471)

Browse Source 
See inline comment, there's no good reason to make this so complicated. So simplifying here.
This commit is contained in:
Erik Seliger 2023-10-09 21:30:43 +02:00 committed by GitHub
parent f5c3da8d6a
commit 59af27246b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 53 additions and 98 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
cmd/embeddings/shared/main.go
View File

@ -62,10 +62,7 @@ func Main(ctx context.Context, observationCtx *observation.Context, ready servic
return err
}
authz.DefaultSubRepoPermsChecker, err = srp.NewSubRepoPermsClient(db.SubRepoPerms())
if err != nil {
return errors.Wrap(err, "creating sub-repo client")
}
authz.DefaultSubRepoPermsChecker = srp.NewSubRepoPermsClient(db.SubRepoPerms())
indexGetter, err := NewCachedEmbeddingIndexGetter(
repoStore,
@ -181,7 +178,7 @@ func setAuthzProviders(ctx context.Context, db database.DB) {
globals.WatchPermissionsUserMapping()
for range time.NewTicker(providers.RefreshInterval()).C {
allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db.ExternalServices(), db)
allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db)
authz.SetProviders(allowAccessByDefault, authzProviders)
}
}

1
cmd/frontend/internal/authz/BUILD.bazel
View File

@ -26,7 +26,6 @@ go_library(
"//internal/licensing",
"//internal/observation",
"//internal/timeutil",
"//lib/errors",
"@com_github_sourcegraph_log//:log",
],
)

15
cmd/frontend/internal/authz/init.go
View File

@ -29,7 +29,6 @@ import (
"github.com/sourcegraph/sourcegraph/internal/licensing"
"github.com/sourcegraph/sourcegraph/internal/observation"
"github.com/sourcegraph/sourcegraph/internal/timeutil"
"github.com/sourcegraph/sourcegraph/lib/errors"
)
var clock = timeutil.Now
@ -47,12 +46,10 @@ func Init(
return database.NewAuthzStore(observationCtx.Logger, db, clock)
}
extsvcStore := db.ExternalServices()
// TODO(nsc): use c
// Report any authz provider problems in external configs.
conf.ContributeWarning(func(cfg conftypes.SiteConfigQuerier) (problems conf.Problems) {
_, providers, seriousProblems, warnings, _ := providers.ProvidersFromConfig(ctx, cfg, extsvcStore, db)
_, providers, seriousProblems, warnings, _ := providers.ProvidersFromConfig(ctx, cfg, db)
problems = append(problems, conf.NewExternalServiceProblems(seriousProblems...)...)
// Validating the connection may make a cross service call, so we should use an
@ -72,11 +69,7 @@ func Init(
enterpriseServices.PermissionsGitHubWebhook = webhooks.NewGitHubWebhook(log.Scoped("PermissionsGitHubWebhook", "permissions sync webhook handler for GitHub webhooks"))
var err error
authz.DefaultSubRepoPermsChecker, err = srp.NewSubRepoPermsClient(db.SubRepoPerms())
if err != nil {
return errors.Wrap(err, "Failed to createe sub-repo client")
}
authz.DefaultSubRepoPermsChecker = srp.NewSubRepoPermsClient(db.SubRepoPerms())
graphqlbackend.AlertFuncs = append(graphqlbackend.AlertFuncs, func(args graphqlbackend.AlertFuncArgs) []*graphqlbackend.Alert {
if licensing.IsLicenseValid() {
@ -102,7 +95,7 @@ func Init(
return nil
}
_, _, _, _, invalidConnections := providers.ProvidersFromConfig(ctx, conf.Get(), extsvcStore, 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)
@ -219,7 +212,7 @@ func Init(
go func() {
t := time.NewTicker(5 * time.Second)
for range t.C {
allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), extsvcStore, db)
allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db)
authz.SetProviders(allowAccessByDefault, authzProviders)
}
}()

5
cmd/gitserver/internal/integration_tests/tree_test.go
View File

@ -409,10 +409,7 @@ func TestReadDir_SubRepoFiltering(t *testing.T) {
},
}
srpGetter.GetByUserFunc.SetDefaultReturn(testSubRepoPerms, nil)
checker, err := srp.NewSubRepoPermsClient(srpGetter)
if err != nil {
t.Fatalf("unexpected error creating sub-repo perms client: %s", err)
}
checker := srp.NewSubRepoPermsClient(srpGetter)
source := gitserver.NewTestClientSource(t, GitserverAddresses)
client := gitserver.NewTestClient(t).WithClientSource(source).WithChecker(checker)

5
cmd/gitserver/shared/shared.go
View File

@ -76,10 +76,7 @@ func Main(ctx context.Context, observationCtx *observation.Context, ready servic
return errors.Wrap(err, "initializing keyring")
}
authz.DefaultSubRepoPermsChecker, err = subrepoperms.NewSubRepoPermsClient(db.SubRepoPerms())
if err != nil {
return errors.Wrap(err, "failed to create sub-repo client")
}
authz.DefaultSubRepoPermsChecker = subrepoperms.NewSubRepoPermsClient(db.SubRepoPerms())
// Setup our server megastruct.
recordingCommandFactory := wrexec.NewRecordingCommandFactory(nil, 0)

8
cmd/precise-code-intel-worker/shared/shared.go
View File

@ -54,11 +54,7 @@ func Main(ctx context.Context, observationCtx *observation.Context, ready servic
ready()
// Initialize sub-repo permissions client
var err error
authz.DefaultSubRepoPermsChecker, err = srp.NewSubRepoPermsClient(db.SubRepoPerms())
if err != nil {
return errors.Wrap(err, "creating sub-repo client")
}
authz.DefaultSubRepoPermsChecker = srp.NewSubRepoPermsClient(db.SubRepoPerms())
services, err := codeintel.NewServices(codeintel.ServiceDependencies{
DB: db,
@ -119,7 +115,7 @@ func mustInitializeDB(observationCtx *observation.Context) *sql.DB {
db := database.NewDB(observationCtx.Logger, sqlDB)
go func() {
for range time.NewTicker(providers.RefreshInterval()).C {
allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db.ExternalServices(), db)
allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db)
authz.SetProviders(allowAccessByDefault, authzProviders)
}
}()

1
cmd/repo-updater/shared/main.go
View File

@ -495,7 +495,6 @@ func watchAuthzProviders(ctx context.Context, db database.DB) {
allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(
ctx,
conf.Get(),
db.ExternalServices(),
db,
)
ossAuthz.SetProviders(allowAccessByDefault, authzProviders)

7
cmd/worker/shared/main.go
View File

@ -158,10 +158,7 @@ func Start(ctx context.Context, observationCtx *observation.Context, ready servi
return errors.Wrap(err, "failed to create database connection")
}
authz.DefaultSubRepoPermsChecker, err = srp.NewSubRepoPermsClient(db.SubRepoPerms())
if err != nil {
return errors.Wrap(err, "failed to create sub-repo client")
}
authz.DefaultSubRepoPermsChecker = srp.NewSubRepoPermsClient(db.SubRepoPerms())
// Emit metrics to help site admins detect instances that accidentally
// omit a job from from the instance's deployment configuration.
@ -395,7 +392,7 @@ func setAuthzProviders(ctx context.Context, observationCtx *observation.Context)
globals.WatchPermissionsUserMapping()
for range time.NewTicker(providers.RefreshInterval()).C {
allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db.ExternalServices(), db)
allowAccessByDefault, authzProviders, _, _, _ := providers.ProvidersFromConfig(ctx, conf.Get(), db)
authz.SetProviders(allowAccessByDefault, authzProviders)
}
}

17
internal/authz/providers/authz.go
View File

@ -38,7 +38,6 @@ import (
func ProvidersFromConfig(
ctx context.Context,
cfg conftypes.SiteConfigQuerier,
store database.ExternalServiceStore,
db database.DB,
) (
allowAccessByDefault bool,
@ -59,13 +58,13 @@ func ProvidersFromConfig(
opt := database.ExternalServicesListOptions{
Kinds: []string{
extsvc.KindAzureDevOps,
extsvc.KindBitbucketCloud,
extsvc.KindBitbucketServer,
extsvc.KindGerrit,
extsvc.KindGitHub,
extsvc.KindGitLab,
extsvc.KindPerforce,
extsvc.VariantAzureDevOps.AsKind(),
extsvc.VariantBitbucketCloud.AsKind(),
extsvc.VariantBitbucketServer.AsKind(),
extsvc.VariantGerrit.AsKind(),
extsvc.VariantGitHub.AsKind(),
extsvc.VariantGitLab.AsKind(),
extsvc.VariantPerforce.AsKind(),
},
LimitOffset: &database.LimitOffset{
Limit: 500, // The number is randomly chosen
@ -82,7 +81,7 @@ func ProvidersFromConfig(
azuredevopsConns []*types.AzureDevOpsConnection
)
for {
svcs, err := store.List(ctx, opt)
svcs, err := db.ExternalServices().List(ctx, opt)
if err != nil {
seriousProblems = append(seriousProblems, fmt.Sprintf("Could not list external services: %v", err))
break

12
internal/authz/providers/authz_test.go
View File

@ -70,8 +70,6 @@ func (m gitlabAuthzProviderParams) FetchRepoPerms(context.Context, *extsvc.Repos
panic("should never be called")
}
var errPermissionsUserMappingConflict = errors.New("The explicit permissions API (site configuration `permissions.userMapping`) cannot be enabled when bitbucketServer authorization provider is in use. Blocking access to all repositories until the conflict is resolved.")
func TestAuthzProvidersFromConfig(t *testing.T) {
t.Cleanup(licensing.TestingSkipFeatureChecks())
gitlab.NewOAuthProvider = func(op gitlab.OAuthProviderOp) authz.Provider {
@ -450,7 +448,9 @@ func TestAuthzProvidersFromConfig(t *testing.T) {
for _, test := range tests {
t.Run(test.description, func(t *testing.T) {
db := dbmocks.NewMockDB()
externalServices := dbmocks.NewMockExternalServiceStore()
db.ExternalServicesFunc.SetDefaultReturn(externalServices)
externalServices.ListFunc.SetDefaultHook(func(ctx context.Context, opt database.ExternalServicesListOptions) ([]*types.ExternalService, error) {
mustMarshalJSONString := func(v any) string {
str, err := jsoniter.MarshalToString(v)
@ -485,8 +485,7 @@ func TestAuthzProvidersFromConfig(t *testing.T) {
allowAccessByDefault, authzProviders, seriousProblems, _, _ := ProvidersFromConfig(
context.Background(),
staticConfig(test.cfg.SiteConfiguration),
externalServices,
dbmocks.NewMockDB(),
db,
)
assert.Equal(t, test.expAuthzAllowAccessByDefault, allowAccessByDefault)
if test.expAuthzProviders != nil {
@ -662,7 +661,9 @@ func TestAuthzProvidersEnabledACLsDisabled(t *testing.T) {
for _, test := range tests {
t.Run(test.description, func(t *testing.T) {
db := dbmocks.NewMockDB()
externalServices := dbmocks.NewMockExternalServiceStore()
db.ExternalServicesFunc.SetDefaultReturn(externalServices)
externalServices.ListFunc.SetDefaultHook(func(ctx context.Context, opt database.ExternalServicesListOptions) ([]*types.ExternalService, error) {
mustMarshalJSONString := func(v any) string {
str, err := jsoniter.MarshalToString(v)
@ -730,8 +731,7 @@ func TestAuthzProvidersEnabledACLsDisabled(t *testing.T) {
_, _, seriousProblems, _, invalidConnections := ProvidersFromConfig(
context.Background(),
staticConfig(test.cfg.SiteConfiguration),
externalServices,
dbmocks.NewMockDB(),
db,
)
assert.Equal(t, test.expSeriousProblems, seriousProblems)

8
internal/authz/providers/bitbucketcloud/provider_test.go
View File

@ -22,14 +22,6 @@ import (
"github.com/sourcegraph/sourcegraph/schema"
)
type mockDoer struct {
do func(*http.Request) (*http.Response, error)
}
func (c *mockDoer) Do(r *http.Request) (*http.Response, error) {
return c.do(r)
}
func mustURL(t *testing.T, u string) *url.URL {
parsed, err := url.Parse(u)
if err != nil {

6
internal/authz/providers/perforce/protects_test.go
View File

@ -628,10 +628,8 @@ read group Dev1 * //depot/main/.../*.go
} else if ok && tc.noRules {
t.Fatal("expected no rules")
}
checker, err := srp.NewSimpleChecker(api.RepoName(tc.depot), rules.Paths)
if err != nil {
t.Fatal(err)
}
checker := srp.NewSimpleChecker(api.RepoName(tc.depot), rules.Paths)
if len(tc.canReadAll) > 0 {
ok, err = authz.CanReadAllPaths(ctx, checker, api.RepoName(tc.depot), tc.canReadAll)
if err != nil {

11
internal/authz/subrepoperms/sub_repo_perms.go
View File

@ -2,6 +2,7 @@ package subrepoperms
import (
"context"
"fmt"
"strconv"
"strings"
"time"
@ -95,10 +96,12 @@ func (rules compiledRules) GetPermissionsForPath(path string) authz.Perms {
//
// Note that sub-repo permissions are currently opt-in via the
// experimentalFeatures.enableSubRepoPermissions option.
func NewSubRepoPermsClient(permissionsGetter SubRepoPermissionsGetter) (*SubRepoPermsClient, error) {
func NewSubRepoPermsClient(permissionsGetter SubRepoPermissionsGetter) *SubRepoPermsClient {
cache, err := lru.New[int32, cachedRules](defaultCacheSize)
if err != nil {
return nil, errors.Wrap(err, "creating LRU cache")
// Errors should only ever occur if we change the value of defaultCacheSize
// to be negative.
panic(fmt.Sprintf("failed to create LRU cache for sub repo perms client: %v", err))
}
enabled := atomic.NewBool(false)
@ -125,7 +128,7 @@ func NewSubRepoPermsClient(permissionsGetter SubRepoPermissionsGetter) (*SubRepo
group: &singleflight.Group{},
cache: cache,
enabled: enabled,
}, nil
}
}
var (
@ -385,7 +388,7 @@ func expandDirs(rule string) []string {
// NewSimpleChecker is exposed for testing and allows creation of a simple
// checker based on the rules provided. The rules are expected to be in glob
// format.
func NewSimpleChecker(repo api.RepoName, paths []string) (authz.SubRepoPermissionChecker, error) {
func NewSimpleChecker(repo api.RepoName, paths []string) authz.SubRepoPermissionChecker {
getter := NewMockSubRepoPermissionsGetter()
getter.GetByUserFunc.SetDefaultHook(func(ctx context.Context, i int32) (map[api.RepoName]authz.SubRepoPermissions, error) {
return map[api.RepoName]authz.SubRepoPermissions{

39
internal/authz/subrepoperms/sub_repo_perms_test.go
View File

@ -30,7 +30,7 @@ func TestSubRepoPermsPermissions(t *testing.T) {
name string
userID int32
content authz.RepoContent
clientFn func() (*SubRepoPermsClient, error)
clientFn func() *SubRepoPermsClient
want authz.Perms
}{
{
@ -40,7 +40,7 @@ func TestSubRepoPermsPermissions(t *testing.T) {
Repo: "sample",
Path: "",
},
clientFn: func() (*SubRepoPermsClient, error) {
clientFn: func() *SubRepoPermsClient {
return NewSubRepoPermsClient(NewMockSubRepoPermissionsGetter())
},
want: authz.Read,
@ -52,7 +52,7 @@ func TestSubRepoPermsPermissions(t *testing.T) {
Repo: "sample",
Path: "/dev/thing",
},
clientFn: func() (*SubRepoPermsClient, error) {
clientFn: func() *SubRepoPermsClient {
getter := NewMockSubRepoPermissionsGetter()
getter.GetByUserFunc.SetDefaultHook(func(ctx context.Context, i int32) (map[api.RepoName]authz.SubRepoPermissions, error) {
return map[api.RepoName]authz.SubRepoPermissions{
@ -72,7 +72,7 @@ func TestSubRepoPermsPermissions(t *testing.T) {
Repo: "sample",
Path: "/dev/thing",
},
clientFn: func() (*SubRepoPermsClient, error) {
clientFn: func() *SubRepoPermsClient {
getter := NewMockSubRepoPermissionsGetter()
getter.GetByUserFunc.SetDefaultHook(func(ctx context.Context, i int32) (map[api.RepoName]authz.SubRepoPermissions, error) {
return map[api.RepoName]authz.SubRepoPermissions{
@ -92,7 +92,7 @@ func TestSubRepoPermsPermissions(t *testing.T) {
Repo: "sample",
Path: "/dev/thing",
},
clientFn: func() (*SubRepoPermsClient, error) {
clientFn: func() *SubRepoPermsClient {
getter := NewMockSubRepoPermissionsGetter()
getter.GetByUserFunc.SetDefaultHook(func(ctx context.Context, i int32) (map[api.RepoName]authz.SubRepoPermissions, error) {
return map[api.RepoName]authz.SubRepoPermissions{
@ -112,7 +112,7 @@ func TestSubRepoPermsPermissions(t *testing.T) {
Repo: "sample",
Path: "/dev/thing",
},
clientFn: func() (*SubRepoPermsClient, error) {
clientFn: func() *SubRepoPermsClient {
getter := NewMockSubRepoPermissionsGetter()
getter.GetByUserFunc.SetDefaultHook(func(ctx context.Context, i int32) (map[api.RepoName]authz.SubRepoPermissions, error) {
return map[api.RepoName]authz.SubRepoPermissions{
@ -132,7 +132,7 @@ func TestSubRepoPermsPermissions(t *testing.T) {
Repo: "sample",
Path: "/dev/thing",
},
clientFn: func() (*SubRepoPermsClient, error) {
clientFn: func() *SubRepoPermsClient {
getter := NewMockSubRepoPermissionsGetter()
getter.GetByUserFunc.SetDefaultHook(func(ctx context.Context, i int32) (map[api.RepoName]authz.SubRepoPermissions, error) {
return map[api.RepoName]authz.SubRepoPermissions{
@ -149,10 +149,7 @@ func TestSubRepoPermsPermissions(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
client, err := tc.clientFn()
if err != nil {
t.Fatal(err)
}
client := tc.clientFn()
have, err := client.Permissions(context.Background(), tc.userID, tc.content)
if err != nil {
t.Fatal(err)
@ -228,10 +225,8 @@ func BenchmarkFilterActorPaths(b *testing.B) {
},
}, nil
})
checker, err := NewSubRepoPermsClient(getter)
if err != nil {
b.Fatal(err)
}
checker := NewSubRepoPermsClient(getter)
a := &actor.Actor{
UID: 1,
}
@ -338,10 +333,7 @@ func TestSubRepoPermissionsCanReadDirectoriesInPath(t *testing.T) {
},
}, nil
})
client, err := NewSubRepoPermsClient(getter)
if err != nil {
t.Fatal(err)
}
client := NewSubRepoPermsClient(getter)
ctx := context.Background()
@ -389,10 +381,7 @@ func TestSubRepoPermsPermissionsCache(t *testing.T) {
t.Cleanup(func() { conf.Mock(nil) })
getter := NewMockSubRepoPermissionsGetter()
client, err := NewSubRepoPermsClient(getter)
if err != nil {
t.Fatal(err)
}
client := NewSubRepoPermsClient(getter)
ctx := context.Background()
content := authz.RepoContent{
@ -402,7 +391,7 @@ func TestSubRepoPermsPermissionsCache(t *testing.T) {
// Should hit DB only once
for i := 0; i < 3; i++ {
_, err = client.Permissions(ctx, 1, content)
_, err := client.Permissions(ctx, 1, content)
if err != nil {
t.Fatal(err)
}
@ -418,7 +407,7 @@ func TestSubRepoPermsPermissionsCache(t *testing.T) {
return defaultCacheTTL + 1
}
_, err = client.Permissions(ctx, 1, content)
_, err := client.Permissions(ctx, 1, content)
if err != nil {
t.Fatal(err)
}

9
internal/search/symbol/symbol_test.go
View File

@ -4,6 +4,8 @@ import (
"context"
"testing"
"github.com/stretchr/testify/assert"
"github.com/sourcegraph/sourcegraph/internal/actor"
"github.com/sourcegraph/sourcegraph/internal/api"
srp "github.com/sourcegraph/sourcegraph/internal/authz/subrepoperms"
@ -13,7 +15,6 @@ import (
"github.com/sourcegraph/sourcegraph/internal/types"
"github.com/sourcegraph/sourcegraph/lib/errors"
"github.com/sourcegraph/sourcegraph/schema"
"github.com/stretchr/testify/assert"
)
func TestSearchZoektDoesntPanicWithNilQuery(t *testing.T) {
@ -48,10 +49,8 @@ func TestFilterZoektResults(t *testing.T) {
ctx = actor.WithActor(ctx, &actor.Actor{
UID: 1,
})
checker, err := srp.NewSimpleChecker(repoName, []string{"/**", "-/*_test.go"})
if err != nil {
t.Fatal(err)
}
checker := srp.NewSimpleChecker(repoName, []string{"/**", "-/*_test.go"})
results := []*result.SymbolMatch{
{
Symbol: result.Symbol{},