gitserver: Move subrepoperms checks back to client

While starting this migration, it felt nice to move sub repo permission checks to the server side, because that means we can make certain optimizations and none of the clients need to reimplement the SRP checker implementation.

However, this moves sub repo authz over the wire, but doesn't move repo perms generally here, so this was a weird split.

This also unnecessarily couples Sourcegraph and gitserver, so for now this is going back into the client layer, like all the auth checks.

Please make sure to pay good attention to the backend and client implementation and help me validate that those are indeed equal.

Test plan:

Added tests, E2E perforce subrepo perms tests are still passing.

Author: eseliger

cmd/gitserver/internal/BUILD.bazel

@@ -39,7 +39,6 @@ go_library(
         "//cmd/gitserver/internal/vcssyncer",
         "//internal/actor",
         "//internal/api",
-        "//internal/authz",
         "//internal/conf",
         "//internal/database",
         "//internal/diskusage",
@@ -70,7 +69,6 @@ go_library(
         "//internal/wrexec",
         "//lib/errors",
         "//lib/gitservice",
-        "//lib/pointers",
         "@com_github_mxk_go_flowrate//flowrate",
         "@com_github_prometheus_client_golang//prometheus",
         "@com_github_prometheus_client_golang//prometheus/promauto",
@@ -113,7 +111,6 @@ go_test(
         "//cmd/gitserver/internal/vcssyncer",
         "//internal/actor",
         "//internal/api",
-        "//internal/authz",
         "//internal/conf",
         "//internal/database",
         "//internal/database/dbmocks",

cmd/gitserver/internal/server_grpc.go

@@ -18,9 +18,7 @@ import (
 	"github.com/sourcegraph/sourcegraph/cmd/gitserver/internal/git/gitcli"
 	"github.com/sourcegraph/sourcegraph/cmd/gitserver/internal/gitserverfs"
 	"github.com/sourcegraph/sourcegraph/cmd/gitserver/internal/perforce"
-	"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"
 	"github.com/sourcegraph/sourcegraph/internal/dotcom"
@@ -32,7 +30,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/repoupdater"
 	"github.com/sourcegraph/sourcegraph/internal/trace"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
-	"github.com/sourcegraph/sourcegraph/lib/pointers"
 )
 
 type service interface {
@@ -48,7 +45,6 @@ func NewGRPCServer(server *Server) proto.GitserverServiceServer {
 		logger:         server.logger,
 		db:             server.db,
 		hostname:       server.hostname,
-		subRepoChecker: authz.DefaultSubRepoPermsChecker,
 		locker:         server.locker,
 		getBackendFunc: server.getBackendFunc,
 		svc:            server,
@@ -60,7 +56,6 @@ type grpcServer struct {
 	logger         log.Logger
 	db             database.DB
 	hostname       string
-	subRepoChecker authz.SubRepoPermissionChecker
 	locker         RepositoryLocker
 	getBackendFunc Backender
 	fs             gitserverfs.FS
@@ -269,15 +264,6 @@ func (gs *grpcServer) Archive(req *proto.ArchiveRequest, ss proto.GitserverServi
 		return err
 	}
 
-	if !actor.FromContext(ctx).IsInternal() {
-		if enabled, err := gs.subRepoChecker.EnabledForRepo(ctx, repoName); err != nil {
-			return errors.Wrap(err, "sub-repo permissions check")
-		} else if enabled {
-			s := status.New(codes.Unimplemented, "archiveReader invoked for a repo with sub-repo permissions")
-			return s.Err()
-		}
-	}
-
 	// This is a long time, but this never blocks a user request for this
 	// long. Even repos that are not that large can take a long time, for
 	// example a search over all repos in an organization may have several
@@ -788,14 +774,9 @@ func (gs *grpcServer) GetCommit(ctx context.Context, req *proto.GetCommitRequest
 		return nil, err
 	}
 
-	subRepoPermsEnabled, err := authz.SubRepoEnabledForRepo(ctx, gs.subRepoChecker, repoName)
-	if err != nil {
-		return nil, err
-	}
-
 	backend := gs.getBackendFunc(repoDir, repoName)
 
-	commit, err := backend.GetCommit(ctx, api.CommitID(req.GetCommit()), subRepoPermsEnabled)
+	commit, err := backend.GetCommit(ctx, api.CommitID(req.GetCommit()), req.GetIncludeModifiedFiles())
 	if err != nil {
 		var e *gitdomain.RevisionNotFoundError
 		if errors.As(err, &e) {
@@ -813,24 +794,14 @@ func (gs *grpcServer) GetCommit(ctx context.Context, req *proto.GetCommitRequest
 		return nil, err
 	}
 
-	hasAccess, err := hasAccessToCommit(ctx, repoName, commit.ModifiedFiles, gs.subRepoChecker)
-	if err != nil {
-		return nil, err
-	}
-
-	if !hasAccess {
-		s, err := status.New(codes.NotFound, "revision not found").WithDetails(&proto.RevisionNotFoundPayload{
-			Repo: req.GetRepoName(),
-			Spec: req.GetCommit(),
-		})
-		if err != nil {
-			return nil, err
-		}
-		return nil, s.Err()
+	modifiedFiles := make([][]byte, len(commit.ModifiedFiles))
+	for i, f := range commit.ModifiedFiles {
+		modifiedFiles[i] = []byte(f)
 	}
 
 	return &proto.GetCommitResponse{
-		Commit: commit.ToProto(),
+		Commit:        commit.ToProto(),
+		ModifiedFiles: modifiedFiles,
 	}, nil
 }
 
@@ -863,26 +834,6 @@ func (gs *grpcServer) Blame(req *proto.BlameRequest, ss proto.GitserverService_B
 		return err
 	}
 
-	// First, verify that the actor has access to the given path.
-	hasAccess, err := authz.FilterActorPath(ctx, gs.subRepoChecker, actor.FromContext(ctx), repoName, req.GetPath())
-	if err != nil {
-		return err
-	}
-	if !hasAccess {
-		up := &proto.UnauthorizedPayload{
-			RepoName: req.GetRepoName(),
-			Commit:   pointers.Ptr(req.GetCommit()),
-			Path:     pointers.Ptr(req.GetPath()),
-		}
-
-		s, marshalErr := status.New(codes.PermissionDenied, "no access to path").WithDetails(up)
-		if marshalErr != nil {
-			gs.logger.Error("failed to marshal error", log.Error(marshalErr))
-			return err
-		}
-		return s.Err()
-	}
-
 	backend := gs.getBackendFunc(repoDir, repoName)
 
 	opts := git.BlameOptions{
@@ -1023,27 +974,6 @@ func (gs *grpcServer) ReadFile(req *proto.ReadFileRequest, ss proto.GitserverSer
 		return err
 	}
 
-	// First, verify that the actor has access to the given path.
-	hasAccess, err := authz.FilterActorPath(ctx, gs.subRepoChecker, actor.FromContext(ctx), repoName, req.GetPath())
-	if err != nil {
-		return err
-	}
-	if !hasAccess {
-		up := &proto.UnauthorizedPayload{
-			RepoName: req.GetRepoName(),
-			Path:     pointers.Ptr(req.GetPath()),
-		}
-		if c := req.GetCommit(); c != "" {
-			up.Commit = &c
-		}
-		s, marshalErr := status.New(codes.PermissionDenied, "no access to path").WithDetails(up)
-		if marshalErr != nil {
-			gs.logger.Error("failed to marshal error", log.Error(marshalErr))
-			return err
-		}
-		return s.Err()
-	}
-
 	backend := gs.getBackendFunc(repoDir, repoName)
 
 	r, err := backend.ReadFile(ctx, api.CommitID(req.GetCommit()), req.GetPath())
@@ -1430,29 +1360,6 @@ func (gs *grpcServer) checkRepoExists(ctx context.Context, repo api.RepoName) er
 	return newRepoNotFoundError(repo, cloneInProgress, cloneProgress)
 }
 
-func hasAccessToCommit(ctx context.Context, repoName api.RepoName, files []string, checker authz.SubRepoPermissionChecker) (bool, error) {
-	if len(files) == 0 {
-		return true, nil // If commit has no files, assume user has access to view the commit.
-	}
-
-	if enabled, err := authz.SubRepoEnabledForRepo(ctx, checker, repoName); err != nil {
-		return false, err
-	} else if !enabled {
-		return true, nil
-	}
-
-	a := actor.FromContext(ctx)
-	for _, fileName := range files {
-		if hasAccess, err := authz.FilterActorPath(ctx, checker, a, 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
-			return true, nil
-		}
-	}
-	return false, nil
-}
-
 func newRepoNotFoundError(repo api.RepoName, cloneInProgress bool, cloneProgress string) error {
 	s, err := status.New(codes.NotFound, "repo not found").WithDetails(&proto.RepoNotFoundPayload{
 		CloneInProgress: cloneInProgress,