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

Update commit filtering and diff view for file-level permissions to better reflect P4 behavior (#38673)

Browse Source 
* update listing commits to show any commit modifying a file the user has access to. Filter out files in Diff()

* address PR comments
This commit is contained in:
Molly Weitzel 2022-07-14 15:07:17 -06:00 committed by GitHub
parent 4e3fdb37d5
commit 79d0c6359c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 188 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
cmd/frontend/graphqlbackend/repository_comparison.go
View File

@ -18,6 +18,7 @@ import (
"github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend/graphqlutil"
"github.com/sourcegraph/sourcegraph/cmd/frontend/internal/highlight"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/authz"
"github.com/sourcegraph/sourcegraph/internal/database"
"github.com/sourcegraph/sourcegraph/internal/gitserver"
"github.com/sourcegraph/sourcegraph/lib/errors"
@ -238,7 +239,7 @@ func computeRepositoryComparisonDiff(cmp *RepositoryComparisonResolver) ComputeD
Head: string(cmp.head.OID()),
RangeType: cmp.rangeType,
Paths: paths,
})
}, authz.DefaultSubRepoPermsChecker)
if err != nil {
return
}

3
enterprise/internal/batches/state/state.go
View File

@ -15,6 +15,7 @@ import (
btypes "github.com/sourcegraph/sourcegraph/enterprise/internal/batches/types"
"github.com/sourcegraph/sourcegraph/internal/actor"
"github.com/sourcegraph/sourcegraph/internal/api"
"github.com/sourcegraph/sourcegraph/internal/authz"
"github.com/sourcegraph/sourcegraph/internal/database"
"github.com/sourcegraph/sourcegraph/internal/extsvc"
"github.com/sourcegraph/sourcegraph/internal/extsvc/bitbucketcloud"
@ -618,7 +619,7 @@ func computeDiffStat(ctx context.Context, db database.DB, c *btypes.Changeset, r
Repo: repo,
Base: c.SyncState.BaseRefOid,
Head: c.SyncState.HeadRefOid,
})
}, authz.DefaultSubRepoPermsChecker)
if err != nil {
return nil, err
}

48
internal/gitserver/commands.go
View File

@ -57,7 +57,7 @@ type DiffOptions struct {
// Diff returns an iterator that can be used to access the diff between two
// commits on a per-file basis. The iterator must be closed with Close when no
// longer required.
func (c *ClientImplementor) Diff(ctx context.Context, opts DiffOptions) (*DiffFileIterator, error) {
func (c *ClientImplementor) Diff(ctx context.Context, opts DiffOptions, checker authz.SubRepoPermissionChecker) (*DiffFileIterator, error) {
// Rare case: the base is the empty tree, in which case we must use ..
// instead of ... as the latter only works for commits.
if opts.Base == DevNullSHA {
@ -91,14 +91,31 @@ func (c *ClientImplementor) Diff(ctx context.Context, opts DiffOptions) (*DiffFi
}
return &DiffFileIterator{
rdr: rdr,
mfdr: diff.NewMultiFileDiffReader(rdr),
rdr: rdr,
mfdr: diff.NewMultiFileDiffReader(rdr),
fileFilterFunc: getFilterFunc(ctx, checker, opts.Repo),
}, nil
}
type DiffFileIterator struct {
rdr io.ReadCloser
mfdr *diff.MultiFileDiffReader
rdr io.ReadCloser
mfdr *diff.MultiFileDiffReader
fileFilterFunc diffFileIteratorFilter
}
type diffFileIteratorFilter func(fileName string) (bool, error)
func getFilterFunc(ctx context.Context, checker authz.SubRepoPermissionChecker, repo api.RepoName) diffFileIteratorFilter {
if !authz.SubRepoEnabled(checker) {
return nil
}
return func(fileName string) (bool, error) {
shouldFilter, err := authz.FilterActorPath(ctx, checker, actor.FromContext(ctx), repo, fileName)
if err != nil {
return false, err
}
return shouldFilter, nil
}
}
func (i *DiffFileIterator) Close() error {
@ -108,7 +125,19 @@ func (i *DiffFileIterator) Close() error {
// Next returns the next file diff. If no more diffs are available, the diff
// will be nil and the error will be io.EOF.
func (i *DiffFileIterator) Next() (*diff.FileDiff, error) {
return i.mfdr.ReadFile()
fd, err := i.mfdr.ReadFile()
if err != nil {
return fd, err
}
if i.fileFilterFunc != nil {
if canRead, err := i.fileFilterFunc(fd.NewName); err != nil {
return nil, err
} else if !canRead {
// go to next
return i.Next()
}
}
return fd, err
}
// ShortLogOptions contains options for (Repository).ShortLog.
@ -1591,11 +1620,12 @@ func hasAccessToCommit(ctx context.Context, commit *wrappedCommit, repoName api.
for _, fileName := range commit.files {
if hasAccess, err := authz.FilterActorPath(ctx, checker, a, repoName, fileName); err != nil {
return false, err
} else if !hasAccess {
return false, nil
} 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 true, nil
return false, nil
}
// CommitsUniqueToBranch returns a map from commits that exist on a particular

157
internal/gitserver/commands_test.go
View File

@ -15,6 +15,8 @@ import (
"testing"
"time"
"github.com/sourcegraph/go-diff/diff"
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/require"
@ -86,6 +88,111 @@ func TestParseShortLog(t *testing.T) {
}
}
func TestDiffWithSubRepoFiltering(t *testing.T) {
ctx := context.Background()
ctx = actor.WithActor(ctx, &actor.Actor{
UID: 1,
})
db := database.NewMockDB()
ClientMocks.LocalGitserver = true
defer ResetClientMocks()
cmds := getGitCommandsWithFileLists([]string{"file0"}, []string{"file1", "file1.1"}, []string{"file2"}, []string{"file3", "file3.3"})
checker := getTestSubRepoPermsChecker("file1.1", "file2")
testCases := []struct {
label string
extraGitCommands []string
expectedDiffFiles []string
expectedFileStat *diff.Stat
rangeOverAllCommits bool
}{
{
label: "adding files",
expectedDiffFiles: []string{"file1", "file3", "file3.3"},
expectedFileStat: &diff.Stat{Added: 3},
rangeOverAllCommits: true,
},
{
label: "changing filename",
extraGitCommands: []string{
"mv file1.1 file_can_access",
"git add file_can_access",
makeGitCommit("rename", 7),
},
expectedDiffFiles: []string{"file_can_access"},
expectedFileStat: &diff.Stat{Added: 1},
},
{
label: "file modified",
extraGitCommands: []string{
"echo new_file_content > file2",
"echo more_new_file_content > file1",
"git add file2",
"git add file1",
makeGitCommit("edit_files", 7),
},
expectedDiffFiles: []string{"file1"}, // file2 is updated but user doesn't have access
expectedFileStat: &diff.Stat{Changed: 1},
},
{
label: "diff for commit w/ no access returns empty result",
extraGitCommands: []string{
"echo new_file_content > file2",
"git add file2",
makeGitCommit("no_access", 7),
},
expectedDiffFiles: []string{},
expectedFileStat: &diff.Stat{},
},
}
for _, tc := range testCases {
t.Run(tc.label, func(t *testing.T) {
repo := MakeGitRepository(t, append(cmds, tc.extraGitCommands...)...)
c := NewClient(db)
commits, err := c.Commits(ctx, repo, CommitsOptions{}, nil)
if err != nil {
t.Fatalf("err fetching commits: %s", err)
}
baseCommit := commits[1]
headCommit := commits[0]
if tc.rangeOverAllCommits {
baseCommit = commits[len(commits)-1]
}
iter, err := c.Diff(ctx, DiffOptions{Base: string(baseCommit.ID), Head: string(headCommit.ID), Repo: repo}, checker)
if err != nil {
t.Fatalf("error fetching diff: %s", err)
}
defer iter.Close()
stat := &diff.Stat{}
fileNames := make([]string, 0, 3)
for {
file, err := iter.Next()
if err == io.EOF {
break
} else if err != nil {
t.Error(err)
}
fileNames = append(fileNames, file.NewName)
fs := file.Stat()
stat.Added += fs.Added
stat.Changed += fs.Changed
stat.Deleted += fs.Deleted
}
if diff := cmp.Diff(fileNames, tc.expectedDiffFiles); diff != "" {
t.Fatal(diff)
}
if diff := cmp.Diff(stat, tc.expectedFileStat); diff != "" {
t.Fatal(diff)
}
})
}
}
func TestDiff(t *testing.T) {
ctx := context.Background()
db := database.NewMockDB()
@ -97,7 +204,7 @@ func TestDiff(t *testing.T) {
".foo",
} {
t.Run("invalid base: "+input, func(t *testing.T) {
i, err := NewClient(db).Diff(ctx, DiffOptions{Base: input})
i, err := NewClient(db).Diff(ctx, DiffOptions{Base: input}, nil)
if i != nil {
t.Errorf("unexpected non-nil iterator: %+v", i)
}
@ -125,7 +232,7 @@ func TestDiff(t *testing.T) {
return nil, nil
}
t.Cleanup(ResetMocks)
_, _ = c.Diff(ctx, tc.opts)
_, _ = c.Diff(ctx, tc.opts, nil)
})
}
})
@ -137,7 +244,7 @@ func TestDiff(t *testing.T) {
}
t.Cleanup(ResetMocks)
i, err := c.Diff(ctx, DiffOptions{Base: "foo", Head: "bar"})
i, err := c.Diff(ctx, DiffOptions{Base: "foo", Head: "bar"}, nil)
if i != nil {
t.Errorf("unexpected non-nil iterator: %+v", i)
}
@ -218,7 +325,7 @@ index 9bd8209..d2acfa9 100644
}
t.Cleanup(ResetMocks)
i, err := c.Diff(ctx, DiffOptions{Base: "foo", Head: "bar"})
i, err := c.Diff(ctx, DiffOptions{Base: "foo", Head: "bar"}, nil)
if i == nil {
t.Error("unexpected nil iterator")
}
@ -1621,6 +1728,7 @@ func TestCommits_SubRepoPerms(t *testing.T) {
"git add file3",
"GIT_COMMITTER_NAME=c GIT_COMMITTER_EMAIL=c@c.com GIT_COMMITTER_DATE=2006-01-02T15:04:07Z git commit -m commit3 --author='a <a@a.com>' --date 2006-01-02T15:04:07Z",
}
repo := MakeGitRepository(t, gitCommands...)
tests := map[string]struct {
wantCommits []*gitdomain.Commit
@ -1628,9 +1736,16 @@ func TestCommits_SubRepoPerms(t *testing.T) {
wantTotal uint
noAccessPaths []string
}{
"if no read perms on file should filter out commit": {
wantTotal: 1,
"if no read perms on at least one file in the commit should filter out commit": {
wantTotal: 2,
wantCommits: []*gitdomain.Commit{
{
ID: "b96d097108fa49e339ca88bc97ab07f833e62131",
Author: gitdomain.Signature{Name: "a", Email: "a@a.com", Date: MustParseTime(time.RFC3339, "2006-01-02T15:04:06Z")},
Committer: &gitdomain.Signature{Name: "c", Email: "c@c.com", Date: MustParseTime(time.RFC3339, "2006-01-02T15:04:07Z")},
Message: "commit2",
Parents: []api.CommitID{"d38233a79e037d2ab8170b0d0bc0aa438473e6da"},
},
{
ID: "d38233a79e037d2ab8170b0d0bc0aa438473e6da",
Author: gitdomain.Signature{Name: "a", Email: "a@a.com", Date: MustParseTime(time.RFC3339, "2006-01-02T15:04:05Z")},
@ -1667,7 +1782,6 @@ func TestCommits_SubRepoPerms(t *testing.T) {
for label, test := range tests {
t.Run(label, func(t *testing.T) {
repo := MakeGitRepository(t, gitCommands...)
checker := getTestSubRepoPermsChecker(test.noAccessPaths...)
commits, err := NewClient(database.NewMockDB()).Commits(ctx, repo, test.opt, checker)
if err != nil {
@ -1679,7 +1793,7 @@ func TestCommits_SubRepoPerms(t *testing.T) {
t.Errorf("%s: got %d commits, want %d", label, len(commits), len(test.wantCommits))
}
checkCommits(t, label, commits, test.wantCommits)
checkCommits(t, commits, test.wantCommits)
})
}
}
@ -2299,10 +2413,10 @@ func testCommits(ctx context.Context, label string, repo api.RepoName, opt Commi
if len(commits) != len(wantCommits) {
t.Errorf("%s: got %d commits, want %d", label, len(commits), len(wantCommits))
}
checkCommits(t, label, commits, wantCommits)
checkCommits(t, commits, wantCommits)
}
func checkCommits(t *testing.T, label string, commits, wantCommits []*gitdomain.Commit) {
func checkCommits(t *testing.T, commits, wantCommits []*gitdomain.Commit) {
t.Helper()
for i := 0; i < len(commits) || i < len(wantCommits); i++ {
var gotC, wantC *gitdomain.Commit
@ -2312,8 +2426,8 @@ func checkCommits(t *testing.T, label string, commits, wantCommits []*gitdomain.
if i < len(wantCommits) {
wantC = wantCommits[i]
}
if !CommitsEqual(gotC, wantC) {
t.Errorf("%s: got commit %d == %+v, want %+v", label, i, gotC, wantC)
if diff := cmp.Diff(gotC, wantC); diff != "" {
t.Fatal(diff)
}
}
}
@ -2335,6 +2449,25 @@ func getTestSubRepoPermsChecker(noAccessPaths ...string) authz.SubRepoPermission
return checker
}
func getGitCommandsWithFileLists(filenamesPerCommit ...[]string) []string {
cmds := make([]string, 0, len(filenamesPerCommit)*3)
for i, filenames := range filenamesPerCommit {
for _, fn := range filenames {
cmds = append(cmds,
fmt.Sprintf("touch %s", fn),
fmt.Sprintf("echo my_content_%d > %s", i, fn),
fmt.Sprintf("git add %s", fn))
}
cmds = append(cmds,
fmt.Sprintf("GIT_COMMITTER_NAME=a GIT_COMMITTER_EMAIL=a@a.com GIT_COMMITTER_DATE=2006-01-02T15:04:05=%dZ git commit -m commit%d --author='a <a@a.com>' --date 2006-01-02T15:04:0%dZ", i, i, i))
}
return cmds
}
func makeGitCommit(commitMessage string, seconds int) string {
return fmt.Sprintf("GIT_COMMITTER_NAME=a GIT_COMMITTER_EMAIL=a@a.com GIT_COMMITTER_DATE=2006-01-02T15:04:05=%dZ git commit -m %s --author='a <a@a.com>' --date 2006-01-02T15:04:0%dZ", seconds, commitMessage, seconds)
}
func getGitCommandsWithFiles(fileName1, fileName2 string) []string {
return []string{
fmt.Sprintf("touch %s", fileName1),