Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

gitserver: Move subrepoperms checks back to client #62022

Merged
merged 1 commit into from
Apr 24, 2024
Merged

gitserver: Move subrepoperms checks back to client #62022

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions cmd/gitserver/internal/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ go_library(
"//cmd/gitserver/internal/vcssyncer",
"//internal/actor",
"//internal/api",
"//internal/authz",
"//internal/conf",
"//internal/database",
"//internal/diskusage",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -113,7 +111,6 @@ go_test(
"//cmd/gitserver/internal/vcssyncer",
"//internal/actor",
"//internal/api",
"//internal/authz",
"//internal/conf",
"//internal/database",
"//internal/database/dbmocks",
Expand Down
105 changes: 6 additions & 99 deletions cmd/gitserver/internal/server_grpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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 {
Expand All @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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
}

Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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,
Expand Down
Loading