diff --git a/cmd/gitserver/internal/search/BUILD.bazel b/cmd/gitserver/internal/search/BUILD.bazel index de7d0e03f38b7..0bcc29db017ce 100644 --- a/cmd/gitserver/internal/search/BUILD.bazel +++ b/cmd/gitserver/internal/search/BUILD.bazel @@ -20,6 +20,7 @@ go_library( "//internal/authz", "//internal/byteutils", "//internal/gitserver/protocol", + "//internal/requestclient", "//internal/search/casetransform", "//internal/search/result", "//lib/errors", diff --git a/cmd/gitserver/internal/search/search.go b/cmd/gitserver/internal/search/search.go index fee82112208ca..9c33d47afff58 100644 --- a/cmd/gitserver/internal/search/search.go +++ b/cmd/gitserver/internal/search/search.go @@ -16,6 +16,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/gitserver/protocol" + "github.com/sourcegraph/sourcegraph/internal/requestclient" "github.com/sourcegraph/sourcegraph/internal/search/result" "github.com/sourcegraph/sourcegraph/lib/errors" ) @@ -224,8 +225,9 @@ func getSubRepoFilterFunc(ctx context.Context, checker authz.SubRepoPermissionCh return nil } a := actor.FromContext(ctx) + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) return func(filePath string) (bool, error) { - return authz.FilterActorPath(ctx, checker, a, repo, filePath) + return authz.FilterActorPath(ctx, checker, a, ipSource, repo, filePath) } } diff --git a/internal/authz/BUILD.bazel b/internal/authz/BUILD.bazel index c8768bd86b53d..a7dd0c177e6c3 100644 --- a/internal/authz/BUILD.bazel +++ b/internal/authz/BUILD.bazel @@ -22,6 +22,7 @@ go_library( "//internal/collections", "//internal/dotcom", "//internal/extsvc", + "//internal/requestclient", "//internal/types", "//lib/errors", "@com_github_prometheus_client_golang//prometheus", diff --git a/internal/authz/mocks_temp.go b/internal/authz/mocks_temp.go index d5483490994c3..552984d1605f5 100644 --- a/internal/authz/mocks_temp.go +++ b/internal/authz/mocks_temp.go @@ -8,6 +8,7 @@ package authz import ( "context" + "net/netip" "sync" api "github.com/sourcegraph/sourcegraph/internal/api" @@ -60,7 +61,7 @@ func NewMockSubRepoPermissionChecker() *MockSubRepoPermissionChecker { }, }, PermissionsFunc: &SubRepoPermissionCheckerPermissionsFunc{ - defaultHook: func(context.Context, int32, RepoContent) (r0 Perms, r1 error) { + defaultHook: func(context.Context, int32, netip.Addr, RepoContent) (r0 Perms, r1 error) { return }, }, @@ -93,7 +94,7 @@ func NewStrictMockSubRepoPermissionChecker() *MockSubRepoPermissionChecker { }, }, PermissionsFunc: &SubRepoPermissionCheckerPermissionsFunc{ - defaultHook: func(context.Context, int32, RepoContent) (Perms, error) { + defaultHook: func(context.Context, int32, netip.Addr, RepoContent) (Perms, error) { panic("unexpected invocation of MockSubRepoPermissionChecker.Permissions") }, }, @@ -568,24 +569,24 @@ func (c SubRepoPermissionCheckerFilePermissionsFuncFuncCall) Results() []interfa // Permissions method of the parent MockSubRepoPermissionChecker instance is // invoked. type SubRepoPermissionCheckerPermissionsFunc struct { - defaultHook func(context.Context, int32, RepoContent) (Perms, error) - hooks []func(context.Context, int32, RepoContent) (Perms, error) + defaultHook func(context.Context, int32, netip.Addr, RepoContent) (Perms, error) + hooks []func(context.Context, int32, netip.Addr, RepoContent) (Perms, error) history []SubRepoPermissionCheckerPermissionsFuncCall mutex sync.Mutex } // Permissions delegates to the next hook function in the queue and stores // the parameter and result values of this invocation. -func (m *MockSubRepoPermissionChecker) Permissions(v0 context.Context, v1 int32, v2 RepoContent) (Perms, error) { - r0, r1 := m.PermissionsFunc.nextHook()(v0, v1, v2) - m.PermissionsFunc.appendCall(SubRepoPermissionCheckerPermissionsFuncCall{v0, v1, v2, r0, r1}) +func (m *MockSubRepoPermissionChecker) Permissions(v0 context.Context, v1 int32, v2 netip.Addr, v3 RepoContent) (Perms, error) { + r0, r1 := m.PermissionsFunc.nextHook()(v0, v1, v2, v3) + m.PermissionsFunc.appendCall(SubRepoPermissionCheckerPermissionsFuncCall{v0, v1, v2, v3, r0, r1}) return r0, r1 } // SetDefaultHook sets function that is called when the Permissions method // of the parent MockSubRepoPermissionChecker instance is invoked and the // hook queue is empty. -func (f *SubRepoPermissionCheckerPermissionsFunc) SetDefaultHook(hook func(context.Context, int32, RepoContent) (Perms, error)) { +func (f *SubRepoPermissionCheckerPermissionsFunc) SetDefaultHook(hook func(context.Context, int32, netip.Addr, RepoContent) (Perms, error)) { f.defaultHook = hook } @@ -594,7 +595,7 @@ func (f *SubRepoPermissionCheckerPermissionsFunc) SetDefaultHook(hook func(conte // 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 *SubRepoPermissionCheckerPermissionsFunc) PushHook(hook func(context.Context, int32, RepoContent) (Perms, error)) { +func (f *SubRepoPermissionCheckerPermissionsFunc) PushHook(hook func(context.Context, int32, netip.Addr, RepoContent) (Perms, error)) { f.mutex.Lock() f.hooks = append(f.hooks, hook) f.mutex.Unlock() @@ -603,19 +604,19 @@ func (f *SubRepoPermissionCheckerPermissionsFunc) PushHook(hook func(context.Con // SetDefaultReturn calls SetDefaultHook with a function that returns the // given values. func (f *SubRepoPermissionCheckerPermissionsFunc) SetDefaultReturn(r0 Perms, r1 error) { - f.SetDefaultHook(func(context.Context, int32, RepoContent) (Perms, error) { + f.SetDefaultHook(func(context.Context, int32, netip.Addr, RepoContent) (Perms, error) { return r0, r1 }) } // PushReturn calls PushHook with a function that returns the given values. func (f *SubRepoPermissionCheckerPermissionsFunc) PushReturn(r0 Perms, r1 error) { - f.PushHook(func(context.Context, int32, RepoContent) (Perms, error) { + f.PushHook(func(context.Context, int32, netip.Addr, RepoContent) (Perms, error) { return r0, r1 }) } -func (f *SubRepoPermissionCheckerPermissionsFunc) nextHook() func(context.Context, int32, RepoContent) (Perms, error) { +func (f *SubRepoPermissionCheckerPermissionsFunc) nextHook() func(context.Context, int32, netip.Addr, RepoContent) (Perms, error) { f.mutex.Lock() defer f.mutex.Unlock() @@ -657,7 +658,10 @@ type SubRepoPermissionCheckerPermissionsFuncCall struct { Arg1 int32 // Arg2 is the value of the 3rd argument passed to this method // invocation. - Arg2 RepoContent + Arg2 netip.Addr + // Arg3 is the value of the 4th argument passed to this method + // invocation. + Arg3 RepoContent // Result0 is the value of the 1st result returned from this method // invocation. Result0 Perms @@ -669,7 +673,7 @@ type SubRepoPermissionCheckerPermissionsFuncCall struct { // Args returns an interface slice containing the arguments of this // invocation. func (c SubRepoPermissionCheckerPermissionsFuncCall) Args() []interface{} { - return []interface{}{c.Arg0, c.Arg1, c.Arg2} + return []interface{}{c.Arg0, c.Arg1, c.Arg2, c.Arg3} } // Results returns an interface slice containing the results of this diff --git a/internal/authz/sub_repo_perms.go b/internal/authz/sub_repo_perms.go index 20e66c12a52e5..a8b89911fac41 100644 --- a/internal/authz/sub_repo_perms.go +++ b/internal/authz/sub_repo_perms.go @@ -3,6 +3,7 @@ package authz import ( "context" "io/fs" + "net/netip" "strconv" "time" @@ -11,6 +12,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" + "github.com/sourcegraph/sourcegraph/internal/requestclient" "github.com/sourcegraph/sourcegraph/lib/errors" ) @@ -26,7 +28,7 @@ type RepoContent struct { // function is associated with a user and repository and should not be used // beyond the lifetime of a single request. It exists to amortize the costs of // setup when checking many files in a repository. -type FilePermissionFunc func(path string) (Perms, error) +type FilePermissionFunc func(path string, ip netip.Addr) (Perms, error) // SubRepoPermissionChecker is the interface exposed by the SubRepoPermsClient and is // exposed to allow consumers to mock out the client. @@ -35,7 +37,7 @@ type SubRepoPermissionChecker interface { // content. // // If the userID represents an anonymous user, ErrUnauthenticated is returned. - Permissions(ctx context.Context, userID int32, content RepoContent) (Perms, error) + Permissions(ctx context.Context, userID int32, ip netip.Addr, content RepoContent) (Perms, error) // FilePermissionsFunc returns a FilePermissionFunc for userID in repo. // This function should only be used during the lifetime of a request. It @@ -62,12 +64,12 @@ var DefaultSubRepoPermsChecker SubRepoPermissionChecker = &noopPermsChecker{} type noopPermsChecker struct{} -func (*noopPermsChecker) Permissions(_ context.Context, _ int32, _ RepoContent) (Perms, error) { +func (*noopPermsChecker) Permissions(_ context.Context, _ int32, _ netip.Addr, _ RepoContent) (Perms, error) { return None, nil } func (*noopPermsChecker) FilePermissionsFunc(_ context.Context, _ int32, _ api.RepoName) (FilePermissionFunc, error) { - return func(path string) (Perms, error) { + return func(path string, ip netip.Addr) (Perms, error) { return None, nil }, nil } @@ -89,7 +91,7 @@ func (*noopPermsChecker) EnabledForRepo(_ context.Context, _ api.RepoName) (bool // // If the context is unauthenticated, ErrUnauthenticated is returned. If the context is // internal, Read permissions is granted. -func ActorPermissions(ctx context.Context, s SubRepoPermissionChecker, a *actor.Actor, content RepoContent) (Perms, error) { +func ActorPermissions(ctx context.Context, s SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, content RepoContent) (Perms, error) { // Check config here, despite checking again in the s.Permissions implementation, // because we also make some permissions decisions here. if doCheck, err := actorSubRepoEnabled(s, a); err != nil { @@ -98,7 +100,12 @@ func ActorPermissions(ctx context.Context, s SubRepoPermissionChecker, a *actor. return Read, nil } - perms, err := s.Permissions(ctx, a.UID, content) + ip, err := ipSource.GetIP() + if err != nil { + return None, errors.Wrap(err, "getting the IP address for checking permissions") + } + + perms, err := s.Permissions(ctx, a.UID, ip, content) if err != nil { return None, errors.Wrapf(err, "getting actor permissions for actor: %d", a.UID) } @@ -156,14 +163,18 @@ var ( }, []string{"any", "result"}) ) -func canReadPaths(ctx context.Context, checker SubRepoPermissionChecker, repo api.RepoName, paths []string, any bool) (result bool, err error) { - a := actor.FromContext(ctx) +func canReadPaths(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, paths []string, any bool) (result bool, err error) { if doCheck, err := actorSubRepoEnabled(checker, a); err != nil { return false, err } else if !doCheck { return true, nil } + ip, err := ipSource.GetIP() + if err != nil { + return false, errors.Wrap(err, "getting the IP address for checking permissions") + } + start := time.Now() var checkPathPermsCount int defer func() { @@ -181,7 +192,7 @@ func canReadPaths(ctx context.Context, checker SubRepoPermissionChecker, repo ap for _, p := range paths { checkPathPermsCount++ - perms, err := checkPathPerms(p) + perms, err := checkPathPerms(p, ip) if err != nil { return false, err } @@ -196,13 +207,13 @@ func canReadPaths(ctx context.Context, checker SubRepoPermissionChecker, repo ap } // CanReadAllPaths returns true if the actor can read all paths. -func CanReadAllPaths(ctx context.Context, checker SubRepoPermissionChecker, repo api.RepoName, paths []string) (bool, error) { - return canReadPaths(ctx, checker, repo, paths, false) +func CanReadAllPaths(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, paths []string) (bool, error) { + return canReadPaths(ctx, checker, a, ipSource, repo, paths, false) } // CanReadAnyPath returns true if the actor can read any path in the list of paths. -func CanReadAnyPath(ctx context.Context, checker SubRepoPermissionChecker, repo api.RepoName, paths []string) (bool, error) { - return canReadPaths(ctx, checker, repo, paths, true) +func CanReadAnyPath(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, paths []string) (bool, error) { + return canReadPaths(ctx, checker, a, ipSource, repo, paths, true) } var ( @@ -218,13 +229,18 @@ var ( // FilterActorPaths will filter the given list of paths for the given actor // returning on paths they are allowed to read. -func FilterActorPaths(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, repo api.RepoName, paths []string) (_ []string, err error) { +func FilterActorPaths(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, paths []string) (_ []string, err error) { if doCheck, err := actorSubRepoEnabled(checker, a); err != nil { return nil, errors.Wrap(err, "checking sub-repo permissions") } else if !doCheck { return paths, nil } + ip, err := ipSource.GetIP() + if err != nil { + return nil, errors.Wrap(err, "getting IP address for filtering actor paths") + } + start := time.Now() var checkPathPermsCount int defer func() { @@ -240,7 +256,7 @@ func FilterActorPaths(ctx context.Context, checker SubRepoPermissionChecker, a * filtered := make([]string, 0, len(paths)) for _, p := range paths { checkPathPermsCount++ - perms, err := checkPathPerms(p) + perms, err := checkPathPerms(p, ip) if err != nil { return nil, errors.Wrap(err, "checking sub-repo permissions") } @@ -253,27 +269,34 @@ func FilterActorPaths(ctx context.Context, checker SubRepoPermissionChecker, a * // FilterActorPath will filter the given path for the given actor // returning true if the path is allowed to read. -func FilterActorPath(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, repo api.RepoName, path string) (bool, error) { +func FilterActorPath(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, path string) (bool, error) { if !SubRepoEnabled(checker) { return true, nil } - perms, err := ActorPermissions(ctx, checker, a, RepoContent{ - Repo: repo, - Path: path, - }) + + perms, err := ActorPermissions(ctx, checker, a, ipSource, + RepoContent{ + Repo: repo, + Path: path, + }) if err != nil { return false, errors.Wrap(err, "checking sub-repo permissions") } return perms.Include(Read), nil } -func FilterActorFileInfos(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, repo api.RepoName, fis []fs.FileInfo) (_ []fs.FileInfo, err error) { +func FilterActorFileInfos(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, fis []fs.FileInfo) (_ []fs.FileInfo, err error) { if doCheck, err := actorSubRepoEnabled(checker, a); err != nil { return nil, errors.Wrap(err, "checking sub-repo permissions") } else if !doCheck { return fis, nil } + ip, err := ipSource.GetIP() + if err != nil { + return nil, errors.Wrap(err, "getting IP address for filtering actor paths") + } + start := time.Now() var checkPathPermsCount int defer func() { @@ -291,7 +314,7 @@ func FilterActorFileInfos(ctx context.Context, checker SubRepoPermissionChecker, filtered := make([]fs.FileInfo, 0, len(fis)) for _, fi := range fis { checkPathPermsCount++ - perms, err := checkPathPerms(fileInfoPath(fi)) + perms, err := checkPathPerms(fileInfoPath(fi), ip) if err != nil { return nil, err } @@ -302,12 +325,12 @@ func FilterActorFileInfos(ctx context.Context, checker SubRepoPermissionChecker, return filtered, nil } -func FilterActorFileInfo(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, repo api.RepoName, fi fs.FileInfo) (bool, error) { +func FilterActorFileInfo(ctx context.Context, checker SubRepoPermissionChecker, a *actor.Actor, ipSource IPSource, repo api.RepoName, fi fs.FileInfo) (bool, error) { rc := RepoContent{ Repo: repo, Path: fileInfoPath(fi), } - perms, err := ActorPermissions(ctx, checker, a, rc) + perms, err := ActorPermissions(ctx, checker, a, ipSource, rc) if err != nil { return false, errors.Wrap(err, "checking sub-repo permissions") } @@ -322,3 +345,31 @@ func fileInfoPath(fi fs.FileInfo) string { } return fi.Name() } + +type IPSource interface { + GetIP() (netip.Addr, error) +} + +type clientIPSource struct { + client *requestclient.Client +} + +func (c *clientIPSource) GetIP() (netip.Addr, error) { + if c.client == nil { + return netip.Addr{}, errors.New("client is nil") + } + + return fakeIP, nil // TODO@ggilmore: Replace this with the real IP extraction logic from the client +} + +var fakeIP = netip.MustParseAddr("127.0.0.1") // TODO@ggimore: Fake ip address used until we thread the real one through. + +func NewRequestClientIPSource(client *requestclient.Client) IPSource { + return &clientIPSource{client: client} +} + +type IPSourceFunc func() (netip.Addr, error) + +func (f IPSourceFunc) GetIP() (netip.Addr, error) { + return f() +} diff --git a/internal/authz/sub_repo_perms_test.go b/internal/authz/sub_repo_perms_test.go index c71e86a5d84e9..8f3fe546a7a81 100644 --- a/internal/authz/sub_repo_perms_test.go +++ b/internal/authz/sub_repo_perms_test.go @@ -3,112 +3,310 @@ package authz import ( "context" "io/fs" + "net/netip" "testing" "github.com/gobwas/glob" "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/fileutil" + "github.com/sourcegraph/sourcegraph/lib/errors" ) func TestFilterActorPaths(t *testing.T) { - testPaths := []string{"file1", "file2", "file3"} - checker := NewMockSubRepoPermissionChecker() - ctx := context.Background() - a := &actor.Actor{ - UID: 1, + tests := []struct { + name string + paths []string + enabledFunc func() bool + permissionsFunc func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) + ipSource IPSource + + expectedPaths []string + expectedError error + }{ + { + name: "basic filtering", + paths: []string{"file1", "file2", "file3"}, + enabledFunc: func() bool { return true }, + permissionsFunc: func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { + return func(path string, ip netip.Addr) (Perms, error) { + if path == "file1" { + return Read, nil + } + return None, nil + }, nil + }, + ipSource: IPSourceFunc(func() (netip.Addr, error) { + ip := netip.MustParseAddr("127.0.0.1") + return ip, nil + }), + expectedPaths: []string{"file1"}, + expectedError: nil, + }, + { + name: "IP address check", + paths: []string{"file1", "file2", "file3"}, + enabledFunc: func() bool { return true }, + permissionsFunc: func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { + return func(path string, ip netip.Addr) (Perms, error) { + if path == "file2" && ip == netip.MustParseAddr("192.168.1.1") { + return Read, nil + } + return None, nil + }, nil + }, + ipSource: IPSourceFunc(func() (netip.Addr, error) { + ip := netip.MustParseAddr("192.168.1.1") + return ip, nil + }), + + expectedPaths: []string{"file2"}, + expectedError: nil, + }, } - ctx = actor.WithActor(ctx, a) - repo := api.RepoName("foo") - checker.EnabledFunc.SetDefaultHook(func() bool { - return true - }) - checker.FilePermissionsFuncFunc.SetDefaultHook(func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { - return func(path string) (Perms, error) { - if path == "file1" { - return Read, nil + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + checker := NewMockSubRepoPermissionChecker() + ctx := context.Background() + a := &actor.Actor{UID: 1} + ctx = actor.WithActor(ctx, a) + repo := api.RepoName("foo") + + checker.EnabledFunc.SetDefaultHook(tt.enabledFunc) + checker.FilePermissionsFuncFunc.SetDefaultHook(tt.permissionsFunc) + + filtered, err := FilterActorPaths(ctx, checker, a, tt.ipSource, repo, tt.paths) + if tt.expectedError != nil { + require.ErrorIs(t, err, tt.expectedError) + + return } - return None, nil - }, nil - }) - filtered, err := FilterActorPaths(ctx, checker, a, repo, testPaths) - if err != nil { - t.Fatal(err) + require.NoError(t, err) + if diff := cmp.Diff(tt.expectedPaths, filtered); diff != "" { + t.Fatal(diff) + } + }) } - want := []string{"file1"} - if diff := cmp.Diff(want, filtered); diff != "" { - t.Fatal(diff) - } + t.Run("propagates error if ip source function fails", func(t *testing.T) { + checker := NewMockSubRepoPermissionChecker() + checker.EnabledFunc.SetDefaultHook(func() bool { return true }) + checker.FilePermissionsFuncFunc.SetDefaultHook(func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { + return func(path string, ip netip.Addr) (Perms, error) { + return Read, nil + }, nil + }) + + expectedErr := errors.New("getting the IP failed for some unknown reason") + ipSource := IPSourceFunc(func() (netip.Addr, error) { + return netip.Addr{}, errors.Wrap(expectedErr, "hmm...") + }) + + ctx := context.Background() + a := &actor.Actor{UID: 1} + ctx = actor.WithActor(ctx, a) + repo := api.RepoName("foo") + + _, err := FilterActorPaths(ctx, checker, a, ipSource, repo, []string{"file1"}) + require.ErrorIs(t, err, expectedErr) + }) } func TestCanReadAllPaths(t *testing.T) { - testPaths := []string{"file1", "file2", "file3"} - checker := NewMockSubRepoPermissionChecker() - ctx := context.Background() - a := &actor.Actor{ - UID: 1, + tests := []struct { + name string + paths []string + enabledFunc func() bool + permissionsFunc func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) + ipSource IPSource + + expectedCanReadAll bool + expectedCanReadAny bool + }{ + { + name: "can read all paths", + paths: []string{"file1", "file2", "file3"}, + enabledFunc: func() bool { return true }, + permissionsFunc: func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { + return func(path string, ip netip.Addr) (Perms, error) { + return Read, nil + }, nil + }, + ipSource: IPSourceFunc(func() (netip.Addr, error) { + return netip.MustParseAddr("127.0.0.1"), nil + }), + expectedCanReadAll: true, + expectedCanReadAny: true, + }, + { + name: "can read all but one", + paths: []string{"file1", "file2", "file3"}, + enabledFunc: func() bool { return true }, + permissionsFunc: func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { + return func(path string, ip netip.Addr) (Perms, error) { + if path == "file2" { + return None, nil + } + return Read, nil + }, nil + }, + ipSource: IPSourceFunc(func() (netip.Addr, error) { + return netip.MustParseAddr("127.0.0.1"), nil + }), + expectedCanReadAll: false, + expectedCanReadAny: true, + }, + { + name: "can only read one", + paths: []string{"file1", "file2", "file3"}, + enabledFunc: func() bool { return true }, + permissionsFunc: func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { + return func(path string, ip netip.Addr) (Perms, error) { + if path == "file2" { + return Read, nil + } + return None, nil + }, nil + }, + ipSource: IPSourceFunc(func() (netip.Addr, error) { + return netip.MustParseAddr("127.0.0.1"), nil + }), + expectedCanReadAll: false, + expectedCanReadAny: true, + }, + { + name: "IP address check - pass", + paths: []string{"file1", "file2", "file3"}, + enabledFunc: func() bool { return true }, + permissionsFunc: func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { + return func(path string, ip netip.Addr) (Perms, error) { + if path == "file2" && ip == netip.MustParseAddr("192.168.1.1") { + return Read, nil + } + return None, nil + }, nil + }, + ipSource: IPSourceFunc(func() (netip.Addr, error) { + return netip.MustParseAddr("192.168.1.1"), nil + }), + expectedCanReadAll: false, + expectedCanReadAny: true, + }, + { + name: "IP address check - fail", + paths: []string{"file1", "file2", "file3"}, + enabledFunc: func() bool { return true }, + permissionsFunc: func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { + return func(path string, ip netip.Addr) (Perms, error) { + if path == "file2" && ip == netip.MustParseAddr("192.168.1.1") { + return Read, nil + } + return None, nil + }, nil + }, + ipSource: IPSourceFunc(func() (netip.Addr, error) { + return netip.MustParseAddr("127.0.0.1"), nil + }), + expectedCanReadAll: false, + expectedCanReadAny: false, + }, } - ctx = actor.WithActor(ctx, a) - repo := api.RepoName("foo") - checker.EnabledFunc.SetDefaultHook(func() bool { - return true - }) - checker.FilePermissionsFuncFunc.SetDefaultHook(func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { - return func(path string) (Perms, error) { - switch path { - case "file1", "file2", "file3": - return Read, nil - default: - return None, nil + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + checker := NewMockSubRepoPermissionChecker() + checker.EnabledFunc.SetDefaultHook(tt.enabledFunc) + checker.FilePermissionsFuncFunc.SetDefaultHook(tt.permissionsFunc) + + ctx := context.Background() + a := &actor.Actor{UID: 1} + ctx = actor.WithActor(ctx, a) + repo := api.RepoName("foo") + + canReadAll, err := CanReadAllPaths(ctx, checker, a, tt.ipSource, repo, tt.paths) + if err != nil { + t.Fatal(err) } - }, nil - }) - checker.EnabledForRepoFunc.SetDefaultHook(func(ctx context.Context, rn api.RepoName) (bool, error) { - if rn == repo { - return true, nil + if diff := cmp.Diff(tt.expectedCanReadAll, canReadAll); diff != "" { + t.Fatalf("CanReadAllPaths: expected %v, got %v", tt.expectedCanReadAll, canReadAll) + } + + canReadAny, err := CanReadAnyPath(ctx, checker, a, tt.ipSource, repo, tt.paths) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(tt.expectedCanReadAny, canReadAny); diff != "" { + t.Fatalf("CanReadAnyPath: expected %v, got %v", tt.expectedCanReadAny, canReadAny) + } + }) + } + + t.Run("CanReadAllPaths IP source error propagation", func(t *testing.T) { + checker := NewMockSubRepoPermissionChecker() + checker.EnabledFunc.SetDefaultHook(func() bool { return true }) + checker.FilePermissionsFuncFunc.SetDefaultHook(func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { + return func(path string, ip netip.Addr) (Perms, error) { + return Read, nil + }, nil + }) + + ipSource := IPSourceFunc(func() (netip.Addr, error) { + return netip.Addr{}, errors.New("IP source error") + }) + + ctx := context.Background() + a := &actor.Actor{UID: 1} + ctx = actor.WithActor(ctx, a) + repo := api.RepoName("foo") + paths := []string{"file1", "file2", "file3"} + + expectedError := errors.New("getting the IP address for checking permissions: IP source error") + + canReadAll, err := CanReadAllPaths(ctx, checker, a, ipSource, repo, paths) + if err == nil || err.Error() != expectedError.Error() { + t.Errorf("CanReadAllPaths error: expected %v, got %v", expectedError, err) + } + if canReadAll { + t.Errorf("CanReadAllPaths: expected false, got true") } - return false, nil }) - ok, err := CanReadAllPaths(ctx, checker, repo, testPaths) - if err != nil { - t.Fatal(err) - } - if !ok { - t.Fatal("Should be allowed to read all paths") - } - ok, err = CanReadAnyPath(ctx, checker, repo, testPaths) - if err != nil { - t.Fatal(err) - } - if !ok { - t.Fatal("CanReadyAnyPath should've returned true since the user can read all paths") - } + t.Run("CanReadAnyPath IP source error propagation", func(t *testing.T) { + checker := NewMockSubRepoPermissionChecker() + checker.EnabledFunc.SetDefaultHook(func() bool { return true }) + checker.FilePermissionsFuncFunc.SetDefaultHook(func(context.Context, int32, api.RepoName) (FilePermissionFunc, error) { + return func(path string, ip netip.Addr) (Perms, error) { + return Read, nil + }, nil + }) - // Add path we can't read - testPaths = append(testPaths, "file4") + ipSource := IPSourceFunc(func() (netip.Addr, error) { + return netip.Addr{}, errors.New("IP source error") + }) - ok, err = CanReadAllPaths(ctx, checker, repo, testPaths) - if err != nil { - t.Fatal(err) - } - if ok { - t.Fatal("Should fail, not allowed to read file4") - } - ok, err = CanReadAnyPath(ctx, checker, repo, testPaths) - if err != nil { - t.Fatal(err) - } - if !ok { - t.Fatal("user can read some of the testPaths, so CanReadAnyPath should return true") - } + ctx := context.Background() + a := &actor.Actor{UID: 1} + ctx = actor.WithActor(ctx, a) + repo := api.RepoName("foo") + paths := []string{"file1", "file2", "file3"} + + expectedError := errors.New("getting the IP address for checking permissions: IP source error") + + canReadAny, err := CanReadAnyPath(ctx, checker, a, ipSource, repo, paths) + if err == nil || err.Error() != expectedError.Error() { + t.Errorf("CanReadAnyPath error: expected %v, got %v", expectedError, err) + } + if canReadAny { + t.Errorf("CanReadAnyPath: expected false, got true") + } + }) } func TestSubRepoEnabled(t *testing.T) { diff --git a/internal/authz/subrepoperms/sub_repo_perms.go b/internal/authz/subrepoperms/sub_repo_perms.go index 6ec27e2a001da..9daaa65318770 100644 --- a/internal/authz/subrepoperms/sub_repo_perms.go +++ b/internal/authz/subrepoperms/sub_repo_perms.go @@ -3,6 +3,7 @@ package subrepoperms import ( "context" "fmt" + "net/netip" "strconv" "strings" "sync" @@ -41,10 +42,11 @@ type SubRepoPermsClient struct { clock func() time.Time since func(time.Time) time.Duration - group *singleflight.Group - cache *lru.Cache[int32, cachedRules] - enabled *atomic.Bool - repoEnabledCache repoEnabledCache + group *singleflight.Group + cache *lru.Cache[int32, cachedRules] + enabled *atomic.Bool + repoEnabledCache repoEnabledCache + makeIPMatcherFunc func() ipMatcher } const ( @@ -73,17 +75,50 @@ type path struct { } type compiledRules struct { - paths []path + rules []rule +} + +type rule struct { + path path + ip ipMatcher +} + +func (r *rule) Match(path string, ip netip.Addr) bool { + return r.path.globPath.Match(path) && r.ip.Match(ip) +} + +type ipMatcher interface { + Match(ip netip.Addr) bool +} + +type ipMatcherFunc func(ip netip.Addr) bool + +func (f ipMatcherFunc) Match(ip netip.Addr) bool { + return f(ip) +} + +var _ ipMatcher = ipMatcherFunc(func(ip netip.Addr) bool { + return true +}) + +type alwaysTrueMatcher struct{} + +func (m alwaysTrueMatcher) Match(ip netip.Addr) bool { + // TODO@ggilmore: (Once IP configuration option is in place, don't hardcode true) + + return true } // GetPermissionsForPath tries to match a given path to a list of rules. // Since the last applicable rule is the one that applies, the list is // traversed in reverse, and the function returns as soon as a match is found. // If no match is found, None is returned. -func (rules compiledRules) GetPermissionsForPath(path string) authz.Perms { - for i := len(rules.paths) - 1; i >= 0; i-- { - if rules.paths[i].globPath.Match(path) { - if rules.paths[i].exclusion { +func (rules compiledRules) GetPermissionsForPath(path string, ip netip.Addr) authz.Perms { + for i := len(rules.rules) - 1; i >= 0; i-- { + r := rules.rules[i] + + if r.Match(path, ip) { + if r.path.exclusion { return authz.None } return authz.Read @@ -129,8 +164,13 @@ func NewSubRepoPermsClient(permissionsGetter SubRepoPermissionsGetter) *SubRepoP } cache.Resize(cacheSize) enabled.Store(c.ExperimentalFeatures.SubRepoPermissions.Enabled) + // TODO@ggilmore: Add a watch that dumps the existing cache when IP permissions are toggled. }) + makeIPMatcherFunc := func() ipMatcher { + return alwaysTrueMatcher{} + } + return &SubRepoPermsClient{ permissionsGetter: permissionsGetter, clock: time.Now, @@ -139,6 +179,7 @@ func NewSubRepoPermsClient(permissionsGetter SubRepoPermissionsGetter) *SubRepoP cache: cache, enabled: enabled, repoEnabledCache: newRepoEnabledCache(defaultRepoEnabledCacheTTL), + makeIPMatcherFunc: makeIPMatcherFunc, } } @@ -177,7 +218,7 @@ func init() { // Permissions return the current permissions granted to the given user on the // given content. If sub-repo permissions are disabled, it is a no-op that return // Read. -func (s *SubRepoPermsClient) Permissions(ctx context.Context, userID int32, content authz.RepoContent) (perms authz.Perms, err error) { +func (s *SubRepoPermsClient) Permissions(ctx context.Context, userID int32, ip netip.Addr, content authz.RepoContent) (perms authz.Perms, err error) { // Are sub-repo permissions enabled at the site level if !s.Enabled() { return authz.Read, nil @@ -197,12 +238,12 @@ func (s *SubRepoPermsClient) Permissions(ctx context.Context, userID int32, cont if err != nil { return authz.None, err } - return f(content.Path) + return f(content.Path, ip) } // filePermissionsFuncAllRead is a FilePermissionFunc which _always_ returns // Read. Only use in cases that sub repo permission checks should not be done. -func filePermissionsFuncAllRead(_ string) (authz.Perms, error) { +func filePermissionsFuncAllRead(_ string, _ netip.Addr) (authz.Perms, error) { return authz.Read, nil } @@ -234,7 +275,7 @@ func (s *SubRepoPermsClient) FilePermissionsFunc(ctx context.Context, userID int return filePermissionsFuncAllRead, nil } - return func(path string) (authz.Perms, error) { + return func(path string, ip netip.Addr) (authz.Perms, error) { // An empty path is equivalent to repo permissions so we can assume it has // already been checked at that level. if path == "" { @@ -248,7 +289,7 @@ func (s *SubRepoPermsClient) FilePermissionsFunc(ctx context.Context, userID int // Iterate through all rules for the current path, and the final match takes // preference. - return rules.GetPermissionsForPath(path), nil + return rules.GetPermissionsForPath(path, ip), nil }, nil } @@ -280,36 +321,44 @@ func (s *SubRepoPermsClient) getCompiledRules(ctx context.Context, userID int32) rules: make(map[api.RepoName]compiledRules, len(repoPerms)), } for repo, perms := range repoPerms { - paths := make([]path, 0, len(perms.Paths)) + rules := make([]rule, 0, len(perms.Paths)) for _, p := range perms.Paths { - rule := p.Path // TODO@ggilmore: Adapt this logic to thread through ip information - exclusion := strings.HasPrefix(rule, "-") - rule = strings.TrimPrefix(rule, "-") + ipMatcher := s.makeIPMatcherFunc() + + pathRule := p.Path + exclusion := strings.HasPrefix(pathRule, "-") + pathRule = strings.TrimPrefix(pathRule, "-") - if !strings.HasPrefix(rule, "/") { - rule = "/" + rule + if !strings.HasPrefix(pathRule, "/") { + pathRule = "/" + pathRule } - g, err := glob.Compile(rule, '/') + g, err := glob.Compile(pathRule, '/') if err != nil { return nil, errors.Wrap(err, "building include matcher") } - paths = append(paths, path{globPath: g, exclusion: exclusion, original: rule}) + rules = append(rules, rule{ + path: path{globPath: g, exclusion: exclusion, original: pathRule}, + ip: ipMatcher, + }) // Special case. Our glob package does not handle rules starting with a double // wildcard correctly. For example, we would expect `/**/*.java` to match all // java files, but it does not match files at the root, eg `/foo.java`. To get // around this we add an extra rule to cover this case. - if strings.HasPrefix(rule, "/**/") { - trimmed := rule + if strings.HasPrefix(pathRule, "/**/") { + trimmed := pathRule for ; strings.HasPrefix(trimmed, "/**/"); trimmed = strings.TrimPrefix(trimmed, "/**") { } g, err := glob.Compile(trimmed, '/') if err != nil { return nil, errors.Wrap(err, "building include matcher") } - paths = append(paths, path{globPath: g, exclusion: exclusion, original: trimmed}) + rules = append(rules, rule{ + path: path{globPath: g, exclusion: exclusion, original: trimmed}, + ip: ipMatcher, + }) } // We should include all directories above an include rule so that we can browse @@ -319,18 +368,21 @@ func (s *SubRepoPermsClient) getCompiledRules(ctx context.Context, userID int32) continue } - dirs := expandDirs(rule) + dirs := expandDirs(pathRule) for _, dir := range dirs { g, err := glob.Compile(dir, '/') if err != nil { return nil, errors.Wrap(err, "building include matcher for dir") } - paths = append(paths, path{globPath: g, exclusion: false, original: dir}) + rules = append(rules, rule{ + path: path{globPath: g, exclusion: false, original: dir}, + ip: ipMatcher, + }) } } toCache.rules[repo] = compiledRules{ - paths: paths, + rules: rules, } } toCache.timestamp = s.clock() diff --git a/internal/authz/subrepoperms/sub_repo_perms_test.go b/internal/authz/subrepoperms/sub_repo_perms_test.go index b5da502b5cf20..d68aa12ba8330 100644 --- a/internal/authz/subrepoperms/sub_repo_perms_test.go +++ b/internal/authz/subrepoperms/sub_repo_perms_test.go @@ -3,6 +3,7 @@ package subrepoperms import ( "context" "fmt" + "net/netip" "sort" "testing" "time" @@ -31,6 +32,7 @@ func TestSubRepoPermsPermissions(t *testing.T) { testCases := []struct { name string userID int32 + ip netip.Addr content authz.RepoContent clientFn func() *SubRepoPermsClient want authz.Perms @@ -38,6 +40,7 @@ func TestSubRepoPermsPermissions(t *testing.T) { { name: "Empty path", userID: 1, + ip: netip.MustParseAddr("1.2.3.4"), content: authz.RepoContent{ Repo: "sample", Path: "", @@ -50,6 +53,7 @@ func TestSubRepoPermsPermissions(t *testing.T) { { name: "No rules", userID: 1, + ip: netip.MustParseAddr("1.2.3.4"), content: authz.RepoContent{ Repo: "sample", Path: "/dev/thing", @@ -70,6 +74,8 @@ func TestSubRepoPermsPermissions(t *testing.T) { { name: "Exclude", userID: 1, + ip: netip.MustParseAddr("1.2.3.4"), + content: authz.RepoContent{ Repo: "sample", Path: "/dev/thing", @@ -95,6 +101,7 @@ func TestSubRepoPermsPermissions(t *testing.T) { { name: "Include", userID: 1, + ip: netip.MustParseAddr("1.2.3.4"), content: authz.RepoContent{ Repo: "sample", Path: "/dev/thing", @@ -120,6 +127,7 @@ func TestSubRepoPermsPermissions(t *testing.T) { { name: "Last rule takes precedence (exclude)", userID: 1, + ip: netip.MustParseAddr("1.2.3.4"), content: authz.RepoContent{ Repo: "sample", Path: "/dev/thing", @@ -149,6 +157,7 @@ func TestSubRepoPermsPermissions(t *testing.T) { { name: "Last rule takes precedence (include)", userID: 1, + ip: netip.MustParseAddr("1.2.3.4"), content: authz.RepoContent{ Repo: "sample", Path: "/dev/thing", @@ -180,7 +189,7 @@ func TestSubRepoPermsPermissions(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { client := tc.clientFn() - have, err := client.Permissions(context.Background(), tc.userID, tc.content) + have, err := client.Permissions(context.Background(), tc.userID, tc.ip, tc.content) if err != nil { t.Fatal(err) } @@ -270,12 +279,15 @@ func BenchmarkFilterActorPaths(b *testing.B) { UID: 1, } ctx := actor.WithActor(context.Background(), a) + ipSource := authz.IPSourceFunc(func() (netip.Addr, error) { + return netip.MustParseAddr("1.2.3.4"), nil // TODO@ggilmore: replace me when we have real ip tests + }) b.ResetTimer() start := time.Now() for n := 0; n <= b.N; n++ { - filtered, err := authz.FilterActorPaths(ctx, checker, a, repo, paths) + filtered, err := authz.FilterActorPaths(ctx, checker, a, ipSource, repo, paths) if err != nil { b.Fatal(err) } @@ -305,25 +317,30 @@ func TestSubRepoPermissionsCanReadDirectoriesInPath(t *testing.T) { testCases := []struct { paths []string + ip netip.Addr canReadAll []string cannotReadAny []string }{ { paths: []string{"foo/bar/thing.txt"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"foo/", "foo/bar/"}, cannotReadAny: []string{"foo/thing.txt", "foo/bar/other.txt"}, }, { paths: []string{"foo/bar/**"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"foo/", "foo/bar/", "foo/bar/baz/", "foo/bar/baz/fox/"}, }, { paths: []string{"foo/bar/"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"foo/", "foo/bar/"}, cannotReadAny: []string{"foo/thing.txt", "foo/bar/thing.txt"}, }, { paths: []string{"baz/*/foo/bar/thing.txt"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"baz/", "baz/x/", "baz/x/foo/bar/"}, cannotReadAny: []string{"baz/thing.txt"}, }, @@ -331,32 +348,39 @@ func TestSubRepoPermissionsCanReadDirectoriesInPath(t *testing.T) { // explicitly excluded. { paths: []string{"**/foo/bar/thing.txt"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"foo/", "foo/bar/"}, }, { paths: []string{"*/foo/bar/thing.txt"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"foo/", "foo/bar/"}, }, { paths: []string{"/**/foo/bar/thing.txt"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"foo/", "foo/bar/"}, }, { paths: []string{"/*/foo/bar/thing.txt"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"foo/", "foo/bar/"}, }, { paths: []string{"-/**", "/storage/redis/**"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"storage/", "/storage/", "/storage/redis/"}, }, { paths: []string{"-/**", "-/storage/**", "/storage/redis/**"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"storage/", "/storage/", "/storage/redis/"}, }, // Even with a wildcard include rule, we should still exclude directories that // are explicitly excluded later { paths: []string{"/**", "-/storage/**"}, + ip: netip.MustParseAddr("1.2.3.4"), canReadAll: []string{"/foo"}, cannotReadAny: []string{"storage/", "/storage/", "/storage/redis/"}, }, @@ -388,7 +412,7 @@ func TestSubRepoPermissionsCanReadDirectoriesInPath(t *testing.T) { Repo: repoName, Path: path, } - perm, err := client.Permissions(ctx, 1, content) + perm, err := client.Permissions(ctx, 1, tc.ip, content) if err != nil { t.Error(err) } @@ -402,7 +426,7 @@ func TestSubRepoPermissionsCanReadDirectoriesInPath(t *testing.T) { Repo: repoName, Path: path, } - perm, err := client.Permissions(ctx, 1, content) + perm, err := client.Permissions(ctx, 1, tc.ip, content) if err != nil { t.Error(err) } @@ -412,6 +436,56 @@ func TestSubRepoPermissionsCanReadDirectoriesInPath(t *testing.T) { } }) } + + t.Run("uses ip matcher", func(t *testing.T) { + filePath := "foo/bar/thing.txt" + + getter := NewMockSubRepoPermissionsGetter() + getter.GetByUserWithIPsFunc.SetDefaultHook(func(ctx context.Context, i int32, _ bool) (map[api.RepoName]authz.SubRepoPermissionsWithIPs, error) { + var paths = []authz.PathWithIP{ + { + Path: filePath, + IP: "*", + }, + } + + return map[api.RepoName]authz.SubRepoPermissionsWithIPs{ + repoName: { + Paths: paths, + }, + }, nil + }) + + client := NewSubRepoPermsClient(getter) + + var observedIPAddress netip.Addr + + client.makeIPMatcherFunc = func() ipMatcher { + return ipMatcherFunc(func(ip netip.Addr) bool { + observedIPAddress = ip + + return true + }) + } + + inputIP := netip.MustParseAddr("1.2.3.4") + + perm, err := client.Permissions(context.Background(), 1, inputIP, authz.RepoContent{ + Repo: repoName, + Path: filePath, + }) + if err != nil { + t.Error(err) + } + + if !perm.Include(authz.Read) { + t.Errorf("Should be able to read %q, cannot", filePath) + } + + if observedIPAddress != inputIP { + t.Errorf("Should have used the input IP address %q, got %q", inputIP, observedIPAddress) + } + }) } func TestSubRepoPermsPermissionsCache(t *testing.T) { @@ -435,9 +509,11 @@ func TestSubRepoPermsPermissionsCache(t *testing.T) { Path: "/stuff", } + fakeIP := netip.MustParseAddr("1.2.3.4") + // Should hit DB only once for range 3 { - _, err := client.Permissions(ctx, 1, content) + _, err := client.Permissions(ctx, 1, fakeIP, content) if err != nil { t.Fatal(err) } @@ -453,7 +529,7 @@ func TestSubRepoPermsPermissionsCache(t *testing.T) { return defaultCacheTTL + 1 } - _, err := client.Permissions(ctx, 1, content) + _, err := client.Permissions(ctx, 1, fakeIP, content) if err != nil { t.Fatal(err) } diff --git a/internal/codeintel/codenav/BUILD.bazel b/internal/codeintel/codenav/BUILD.bazel index c098861ffd5ca..d24e4e2f6665a 100644 --- a/internal/codeintel/codenav/BUILD.bazel +++ b/internal/codeintel/codenav/BUILD.bazel @@ -38,6 +38,7 @@ go_library( "//internal/gitserver/gitdomain", "//internal/metrics", "//internal/observation", + "//internal/requestclient", "//internal/search", "//internal/search/client", "//internal/search/result", diff --git a/internal/codeintel/codenav/service.go b/internal/codeintel/codenav/service.go index 1db8bcc06570f..e12dbbff07f48 100644 --- a/internal/codeintel/codenav/service.go +++ b/internal/codeintel/codenav/service.go @@ -23,6 +23,7 @@ import ( uploadsshared "github.com/sourcegraph/sourcegraph/internal/codeintel/uploads/shared" "github.com/sourcegraph/sourcegraph/internal/gitserver" "github.com/sourcegraph/sourcegraph/internal/observation" + "github.com/sourcegraph/sourcegraph/internal/requestclient" searcher "github.com/sourcegraph/sourcegraph/internal/search/client" "github.com/sourcegraph/sourcegraph/internal/types" "github.com/sourcegraph/sourcegraph/lib/codeintel/languages" @@ -284,6 +285,7 @@ func (s *Service) getUploadLocations(ctx context.Context, args RequestArgs, requ checkerEnabled := authz.SubRepoEnabled(requestState.authChecker) var a *actor.Actor + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) if checkerEnabled { a = actor.FromContext(ctx) } @@ -305,7 +307,7 @@ func (s *Service) getUploadLocations(ctx context.Context, args RequestArgs, requ uploadLocations = append(uploadLocations, adjustedLocation) } else { repo := api.RepoName(adjustedLocation.Upload.RepositoryName) - if include, err := authz.FilterActorPath(ctx, requestState.authChecker, a, repo, adjustedLocation.Path.RawValue()); err != nil { + if include, err := authz.FilterActorPath(ctx, requestState.authChecker, a, ipSource, repo, adjustedLocation.Path.RawValue()); err != nil { return nil, err } else if include { uploadLocations = append(uploadLocations, adjustedLocation) @@ -405,6 +407,8 @@ func (s *Service) GetDiagnostics(ctx context.Context, args PositionalRequestArgs checkerEnabled := authz.SubRepoEnabled(requestState.authChecker) var a *actor.Actor + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) + if checkerEnabled { a = actor.FromContext(ctx) } @@ -434,7 +438,7 @@ func (s *Service) GetDiagnostics(ctx context.Context, args PositionalRequestArgs } // sub-repo checker is enabled, proceeding with check - if include, err := authz.FilterActorPath(ctx, requestState.authChecker, a, api.RepoName(adjustedDiagnostic.Upload.RepositoryName), adjustedDiagnostic.Path.RawValue()); err != nil { + if include, err := authz.FilterActorPath(ctx, requestState.authChecker, a, ipSource, api.RepoName(adjustedDiagnostic.Upload.RepositoryName), adjustedDiagnostic.Path.RawValue()); err != nil { return nil, 0, err } else if include { diagnosticsAtUploads = append(diagnosticsAtUploads, adjustedDiagnostic) diff --git a/internal/codeintel/codenav/transport/graphql/BUILD.bazel b/internal/codeintel/codenav/transport/graphql/BUILD.bazel index 095963cdc67bc..d822c5ac76943 100644 --- a/internal/codeintel/codenav/transport/graphql/BUILD.bazel +++ b/internal/codeintel/codenav/transport/graphql/BUILD.bazel @@ -42,6 +42,7 @@ go_library( "//internal/gitserver", "//internal/metrics", "//internal/observation", + "//internal/requestclient", "//internal/types", "//lib/errors", "//lib/pointers", diff --git a/internal/codeintel/codenav/transport/graphql/root_resolver.go b/internal/codeintel/codenav/transport/graphql/root_resolver.go index 4596c8cf1c5d9..9626ac8d706f8 100644 --- a/internal/codeintel/codenav/transport/graphql/root_resolver.go +++ b/internal/codeintel/codenav/transport/graphql/root_resolver.go @@ -28,6 +28,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/gitserver" "github.com/sourcegraph/sourcegraph/internal/observation" + "github.com/sourcegraph/sourcegraph/internal/requestclient" "github.com/sourcegraph/sourcegraph/internal/types" sgtypes "github.com/sourcegraph/sourcegraph/internal/types" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -127,8 +128,9 @@ func (r *rootResolver) CodeGraphData(ctx context.Context, opts *resolverstubs.Co } currentActor := actor.FromContext(ctx) + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) hasAccess, err := authz.FilterActorPath(ctx, authz.DefaultSubRepoPermsChecker, - currentActor, opts.Repo.Name, opts.Path.RawValue()) + currentActor, ipSource, opts.Repo.Name, opts.Path.RawValue()) if err != nil { return nil, err } else if !hasAccess { diff --git a/internal/gitserver/BUILD.bazel b/internal/gitserver/BUILD.bazel index fc6068e8bf956..8d2f13012279b 100644 --- a/internal/gitserver/BUILD.bazel +++ b/internal/gitserver/BUILD.bazel @@ -35,6 +35,7 @@ go_library( "//internal/metrics", "//internal/observation", "//internal/perforce", + "//internal/requestclient", "//internal/search/streaming/http", "//lib/errors", "//lib/pointers", diff --git a/internal/gitserver/commands.go b/internal/gitserver/commands.go index 6a74fd1b0dc56..b40f0113afa2b 100644 --- a/internal/gitserver/commands.go +++ b/internal/gitserver/commands.go @@ -23,6 +23,7 @@ import ( proto "github.com/sourcegraph/sourcegraph/internal/gitserver/v1" "github.com/sourcegraph/sourcegraph/internal/grpc/streamio" "github.com/sourcegraph/sourcegraph/internal/observation" + "github.com/sourcegraph/sourcegraph/internal/requestclient" "github.com/sourcegraph/sourcegraph/lib/errors" "github.com/sourcegraph/sourcegraph/lib/pointers" ) @@ -135,8 +136,12 @@ func getFilterFunc(ctx context.Context, checker authz.SubRepoPermissionChecker, if !authz.SubRepoEnabled(checker) { return nil } + + a := actor.FromContext(ctx) + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) + return func(fileName string) (bool, error) { - shouldFilter, err := authz.FilterActorPath(ctx, checker, actor.FromContext(ctx), repo, fileName) + shouldFilter, err := authz.FilterActorPath(ctx, checker, a, ipSource, repo, fileName) if err != nil { return false, err } @@ -430,9 +435,11 @@ func (i *readDirIterator) Next() (fs.FileInfo, error) { for _, f := range chunk.GetFileInfo() { fds = append(fds, gitdomain.ProtoFileInfoToFS(f)) } + if authz.SubRepoEnabled(i.subRepoPermsChecker) { a := actor.FromContext(i.ctx) - filtered, filteringErr := authz.FilterActorFileInfos(i.ctx, i.subRepoPermsChecker, a, i.repo, fds) + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(i.ctx)) + filtered, filteringErr := authz.FilterActorFileInfos(i.ctx, i.subRepoPermsChecker, a, ipSource, i.repo, fds) if filteringErr != nil { return nil, errors.Wrap(err, "filtering paths") } @@ -478,8 +485,11 @@ func (c *clientImplementor) StreamBlameFile(ctx context.Context, repo api.RepoNa }, opt.Attrs()...), }) + a := actor.FromContext(ctx) + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) + // First, verify that the actor has access to the given path. - hasAccess, err := authz.FilterActorPath(ctx, c.subRepoPermsChecker, actor.FromContext(ctx), repo, path) + hasAccess, err := authz.FilterActorPath(ctx, c.subRepoPermsChecker, a, ipSource, repo, path) if err != nil { return nil, err } @@ -781,7 +791,8 @@ func (c *clientImplementor) NewFileReader(ctx context.Context, repo api.RepoName // First, verify the actor can see the path. a := actor.FromContext(ctx) - if hasAccess, err := authz.FilterActorPath(ctx, c.subRepoPermsChecker, a, repo, name); err != nil { + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) + if hasAccess, err := authz.FilterActorPath(ctx, c.subRepoPermsChecker, a, ipSource, repo, name); err != nil { return nil, err } else if !hasAccess { return nil, os.ErrNotExist @@ -913,7 +924,9 @@ func (c *clientImplementor) Stat(ctx context.Context, repo api.RepoName, commit // Applying sub-repo permissions a := actor.FromContext(ctx) - include, filteringErr := authz.FilterActorFileInfo(ctx, c.subRepoPermsChecker, a, repo, fi) + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) + + include, filteringErr := authz.FilterActorFileInfo(ctx, c.subRepoPermsChecker, a, ipSource, repo, fi) if include && filteringErr == nil { return fi, nil } else { @@ -1116,11 +1129,12 @@ func unWrapCommits(wrappedCommits []*wrappedCommit) []*gitdomain.Commit { func hasAccessToCommit(ctx context.Context, commit *wrappedCommit, repoName api.RepoName, checker authz.SubRepoPermissionChecker) (bool, error) { a := actor.FromContext(ctx) + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) if commit.files == nil || len(commit.files) == 0 { return true, nil // If commit has no files, assume user has access to view the commit. } for _, fileName := range commit.files { - if hasAccess, err := authz.FilterActorPath(ctx, checker, a, repoName, fileName); err != nil { + if hasAccess, err := authz.FilterActorPath(ctx, checker, a, ipSource, repoName, fileName); err != nil { return false, err } else if hasAccess { // if the user has access to one file modified in the commit, they have access to view the commit diff --git a/internal/search/job/jobutil/BUILD.bazel b/internal/search/job/jobutil/BUILD.bazel index 14891f43c4eed..0b117895ce988 100644 --- a/internal/search/job/jobutil/BUILD.bazel +++ b/internal/search/job/jobutil/BUILD.bazel @@ -35,6 +35,7 @@ go_library( "//internal/gitserver/gitdomain", "//internal/grpc/defaults", "//internal/own/search", + "//internal/requestclient", "//internal/search", "//internal/search/alert", "//internal/search/codycontext", diff --git a/internal/search/job/jobutil/sub_repo_perms_job.go b/internal/search/job/jobutil/sub_repo_perms_job.go index d00cbc3c35036..7ae29ac2ee9c2 100644 --- a/internal/search/job/jobutil/sub_repo_perms_job.go +++ b/internal/search/job/jobutil/sub_repo_perms_job.go @@ -10,6 +10,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/authz" + "github.com/sourcegraph/sourcegraph/internal/requestclient" "github.com/sourcegraph/sourcegraph/internal/search" "github.com/sourcegraph/sourcegraph/internal/search/job" "github.com/sourcegraph/sourcegraph/internal/search/result" @@ -82,6 +83,8 @@ func applySubRepoFiltering(ctx context.Context, checker authz.SubRepoPermissionC a := actor.FromContext(ctx) var errs error + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) + // Filter matches in place filtered := matches[:0] @@ -114,7 +117,7 @@ func applySubRepoFiltering(ctx context.Context, checker authz.SubRepoPermissionC Repo: repo, Path: matchedPath, } - perms, err := authz.ActorPermissions(ctx, checker, a, content) + perms, err := authz.ActorPermissions(ctx, checker, a, ipSource, content) if err != nil { errs = errors.Append(errs, err) continue @@ -124,7 +127,7 @@ func applySubRepoFiltering(ctx context.Context, checker authz.SubRepoPermissionC filtered = append(filtered, m) } case *result.CommitMatch: - allowed, err := authz.CanReadAnyPath(ctx, checker, mm.Repo.Name, mm.ModifiedFiles) + allowed, err := authz.CanReadAnyPath(ctx, checker, a, ipSource, mm.Repo.Name, mm.ModifiedFiles) if err != nil { errs = errors.Append(errs, err) continue diff --git a/internal/search/symbol/BUILD.bazel b/internal/search/symbol/BUILD.bazel index e46e7664aa50f..571cc95ae1788 100644 --- a/internal/search/symbol/BUILD.bazel +++ b/internal/search/symbol/BUILD.bazel @@ -12,6 +12,7 @@ go_library( "//internal/actor", "//internal/api", "//internal/authz", + "//internal/requestclient", "//internal/search", "//internal/search/result", "//internal/search/zoektquery", diff --git a/internal/search/symbol/symbol.go b/internal/search/symbol/symbol.go index 82c835608298c..9923e9b32ce16 100644 --- a/internal/search/symbol/symbol.go +++ b/internal/search/symbol/symbol.go @@ -14,6 +14,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/authz" + "github.com/sourcegraph/sourcegraph/internal/requestclient" "github.com/sourcegraph/sourcegraph/internal/search" "github.com/sourcegraph/sourcegraph/internal/search/result" "github.com/sourcegraph/sourcegraph/internal/search/zoektquery" @@ -168,9 +169,10 @@ func filterZoektResults(ctx context.Context, checker authz.SubRepoPermissionChec } // Filter out results from files we don't have access to: act := actor.FromContext(ctx) + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) filtered := results[:0] for i, r := range results { - ok, err := authz.FilterActorPath(ctx, checker, act, repo, r.File.Path) + ok, err := authz.FilterActorPath(ctx, checker, act, ipSource, repo, r.File.Path) if err != nil { return nil, errors.Wrap(err, "checking permissions") } diff --git a/internal/symbols/BUILD.bazel b/internal/symbols/BUILD.bazel index a638c20f70efd..a804aba1732d9 100644 --- a/internal/symbols/BUILD.bazel +++ b/internal/symbols/BUILD.bazel @@ -17,6 +17,7 @@ go_library( "//internal/conf/conftypes", "//internal/endpoint", "//internal/grpc/defaults", + "//internal/requestclient", "//internal/search", "//internal/search/result", "//internal/symbols/v1:symbols", diff --git a/internal/symbols/client.go b/internal/symbols/client.go index 3b8138c402854..25bbc319c07c8 100644 --- a/internal/symbols/client.go +++ b/internal/symbols/client.go @@ -15,6 +15,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/conf/conftypes" "github.com/sourcegraph/sourcegraph/internal/endpoint" "github.com/sourcegraph/sourcegraph/internal/grpc/defaults" + "github.com/sourcegraph/sourcegraph/internal/requestclient" "github.com/sourcegraph/sourcegraph/internal/search" "github.com/sourcegraph/sourcegraph/internal/search/result" proto "github.com/sourcegraph/sourcegraph/internal/symbols/v1" @@ -84,6 +85,7 @@ func (c *Client) Search(ctx context.Context, args search.SymbolsParameters) (sym } a := actor.FromContext(ctx) + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) // Filter in place filtered := symbols[:0] for _, r := range symbols { @@ -91,7 +93,7 @@ func (c *Client) Search(ctx context.Context, args search.SymbolsParameters) (sym Repo: args.Repo, Path: r.Path, } - perm, err := authz.ActorPermissions(ctx, checker, a, rc) + perm, err := authz.ActorPermissions(ctx, checker, a, ipSource, rc) if err != nil { return nil, false, errors.Wrap(err, "checking sub-repo permissions") } @@ -186,8 +188,11 @@ func (c *Client) SymbolInfo(ctx context.Context, args types.RepoCommitPathPoint) return nil, errors.Wrap(err, "executing symbol info request") } + a := actor.FromContext(ctx) + ipSource := authz.NewRequestClientIPSource(requestclient.FromContext(ctx)) + // 🚨 SECURITY: We have a valid result, so we need to apply sub-repo permissions filtering. - ok, err := authz.FilterActorPath(ctx, c.SubRepoPermsChecker(), actor.FromContext(ctx), api.RepoName(args.Repo), args.Path) + ok, err := authz.FilterActorPath(ctx, c.SubRepoPermsChecker(), a, ipSource, api.RepoName(args.Repo), args.Path) if err != nil { return nil, errors.Wrap(err, "checking sub-repo permissions") }