From 26718a785ac49f17eab51ad0f5324d036b810f73 Mon Sep 17 00:00:00 2001 From: Martin Scholz Date: Fri, 11 Feb 2022 13:47:22 +0100 Subject: [PATCH 01/11] Change git.cmd to RunWithContext (#18693) Change all `cmd...Pipeline` commands to `cmd.RunWithContext`. #18553 Co-authored-by: Martin Scholz --- integrations/pull_merge_test.go | 17 +++- modules/git/batch_reader.go | 22 ++++- modules/git/commit.go | 7 +- modules/git/diff.go | 12 ++- modules/git/log_name_status.go | 7 +- modules/git/pipeline/catfile.go | 23 ++++- modules/git/pipeline/lfs_nogogit.go | 7 +- modules/git/pipeline/namerev.go | 8 +- modules/git/pipeline/revlist.go | 14 ++- modules/git/repo.go | 8 +- modules/git/repo_archive.go | 7 +- modules/git/repo_attribute.go | 22 ++++- modules/git/repo_branch_nogogit.go | 7 +- modules/git/repo_commit.go | 7 +- modules/git/repo_compare.go | 55 ++++++++--- modules/git/repo_index.go | 8 +- modules/git/repo_object.go | 8 +- modules/git/repo_ref_nogogit.go | 7 +- modules/git/repo_stats.go | 24 ++--- modules/git/repo_tree.go | 9 +- modules/gitgraph/graph.go | 90 +++++++++-------- routers/private/hook_verification.go | 24 +++-- services/mirror/mirror_pull.go | 35 ++++++- services/pull/merge.go | 131 ++++++++++++++++++++++--- services/pull/patch.go | 12 ++- services/pull/patch_unmerged.go | 14 +-- services/pull/temp_repo.go | 40 +++++++- services/repository/files/temp_repo.go | 60 ++++++++--- 28 files changed, 530 insertions(+), 155 deletions(-) diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index 57a9868678e9..8aded910d4b2 100644 --- a/integrations/pull_merge_test.go +++ b/integrations/pull_merge_test.go @@ -274,7 +274,13 @@ func TestCantMergeUnrelated(t *testing.T) { stdin := bytes.NewBufferString("Unrelated File") var stdout strings.Builder - err = git.NewCommand(git.DefaultContext, "hash-object", "-w", "--stdin").RunInDirFullPipeline(path, &stdout, nil, stdin) + err = git.NewCommand(git.DefaultContext, "hash-object", "-w", "--stdin").RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: path, + Stdin: stdin, + Stdout: &stdout, + }) + assert.NoError(t, err) sha := strings.TrimSpace(stdout.String()) @@ -301,7 +307,14 @@ func TestCantMergeUnrelated(t *testing.T) { _, _ = messageBytes.WriteString("\n") stdout.Reset() - err = git.NewCommand(git.DefaultContext, "commit-tree", treeSha).RunInDirTimeoutEnvFullPipeline(env, -1, path, &stdout, nil, messageBytes) + err = git.NewCommand(git.DefaultContext, "commit-tree", treeSha). + RunWithContext(&git.RunContext{ + Env: env, + Timeout: -1, + Dir: path, + Stdin: messageBytes, + Stdout: &stdout, + }) assert.NoError(t, err) commitSha := strings.TrimSpace(stdout.String()) diff --git a/modules/git/batch_reader.go b/modules/git/batch_reader.go index 4cd6cb121771..66ca118de5a4 100644 --- a/modules/git/batch_reader.go +++ b/modules/git/batch_reader.go @@ -34,7 +34,11 @@ func EnsureValidGitRepository(ctx context.Context, repoPath string) error { stderr := strings.Builder{} err := NewCommand(ctx, "rev-parse"). SetDescription(fmt.Sprintf("%s rev-parse [repo_path: %s]", GitExecutable, repoPath)). - RunInDirFullPipeline(repoPath, nil, &stderr, nil) + RunWithContext(&RunContext{ + Timeout: -1, + Dir: repoPath, + Stderr: &stderr, + }) if err != nil { return ConcatenateError(err, (&stderr).String()) } @@ -61,7 +65,13 @@ func CatFileBatchCheck(ctx context.Context, repoPath string) (WriteCloserError, stderr := strings.Builder{} err := NewCommand(ctx, "cat-file", "--batch-check"). SetDescription(fmt.Sprintf("%s cat-file --batch-check [repo_path: %s] (%s:%d)", GitExecutable, repoPath, filename, line)). - RunInDirFullPipeline(repoPath, batchStdoutWriter, &stderr, batchStdinReader) + RunWithContext(&RunContext{ + Timeout: -1, + Dir: repoPath, + Stdin: batchStdinReader, + Stdout: batchStdoutWriter, + Stderr: &stderr, + }) if err != nil { _ = batchStdoutWriter.CloseWithError(ConcatenateError(err, (&stderr).String())) _ = batchStdinReader.CloseWithError(ConcatenateError(err, (&stderr).String())) @@ -100,7 +110,13 @@ func CatFileBatch(ctx context.Context, repoPath string) (WriteCloserError, *bufi stderr := strings.Builder{} err := NewCommand(ctx, "cat-file", "--batch"). SetDescription(fmt.Sprintf("%s cat-file --batch [repo_path: %s] (%s:%d)", GitExecutable, repoPath, filename, line)). - RunInDirFullPipeline(repoPath, batchStdoutWriter, &stderr, batchStdinReader) + RunWithContext(&RunContext{ + Timeout: -1, + Dir: repoPath, + Stdin: batchStdinReader, + Stdout: batchStdoutWriter, + Stderr: &stderr, + }) if err != nil { _ = batchStdoutWriter.CloseWithError(ConcatenateError(err, (&stderr).String())) _ = batchStdinReader.CloseWithError(ConcatenateError(err, (&stderr).String())) diff --git a/modules/git/commit.go b/modules/git/commit.go index 77ba3c0eb22b..340a7e21dd64 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -486,7 +486,12 @@ func GetCommitFileStatus(ctx context.Context, repoPath, commitID string) (*Commi stderr := new(bytes.Buffer) args := []string{"log", "--name-status", "-c", "--pretty=format:", "--parents", "--no-renames", "-z", "-1", commitID} - err := NewCommand(ctx, args...).RunInDirPipeline(repoPath, w, stderr) + err := NewCommand(ctx, args...).RunWithContext(&RunContext{ + Timeout: -1, + Dir: repoPath, + Stdout: w, + Stderr: stderr, + }) w.Close() // Close writer to exit parsing goroutine if err != nil { return nil, ConcatenateError(err, stderr.String()) diff --git a/modules/git/diff.go b/modules/git/diff.go index 2d85db475396..621878f62040 100644 --- a/modules/git/diff.go +++ b/modules/git/diff.go @@ -301,9 +301,12 @@ func GetAffectedFiles(repo *Repository, oldCommitID, newCommitID string, env []s // Run `git diff --name-only` to get the names of the changed files err = NewCommand(repo.Ctx, "diff", "--name-only", oldCommitID, newCommitID). - RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, - stdoutWriter, nil, nil, - func(ctx context.Context, cancel context.CancelFunc) error { + RunWithContext(&RunContext{ + Env: env, + Timeout: -1, + Dir: repo.Path, + Stdout: stdoutWriter, + PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { // Close the writer end of the pipe to begin processing _ = stdoutWriter.Close() defer func() { @@ -320,7 +323,8 @@ func GetAffectedFiles(repo *Repository, oldCommitID, newCommitID string, env []s affectedFiles = append(affectedFiles, path) } return scanner.Err() - }) + }, + }) if err != nil { log.Error("Unable to get affected files for commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err) } diff --git a/modules/git/log_name_status.go b/modules/git/log_name_status.go index 7720d53db19c..0571a4dd20c6 100644 --- a/modules/git/log_name_status.go +++ b/modules/git/log_name_status.go @@ -55,7 +55,12 @@ func LogNameStatusRepo(ctx context.Context, repository, head, treepath string, p go func() { stderr := strings.Builder{} - err := NewCommand(ctx, args...).RunInDirFullPipeline(repository, stdoutWriter, &stderr, nil) + err := NewCommand(ctx, args...).RunWithContext(&RunContext{ + Timeout: -1, + Dir: repository, + Stdout: stdoutWriter, + Stderr: &stderr, + }) if err != nil { _ = stdoutWriter.CloseWithError(ConcatenateError(err, (&stderr).String())) } else { diff --git a/modules/git/pipeline/catfile.go b/modules/git/pipeline/catfile.go index 15c3396ff5fa..6948131e460d 100644 --- a/modules/git/pipeline/catfile.go +++ b/modules/git/pipeline/catfile.go @@ -27,7 +27,13 @@ func CatFileBatchCheck(ctx context.Context, shasToCheckReader *io.PipeReader, ca stderr := new(bytes.Buffer) var errbuf strings.Builder cmd := git.NewCommand(ctx, "cat-file", "--batch-check") - if err := cmd.RunInDirFullPipeline(tmpBasePath, catFileCheckWriter, stderr, shasToCheckReader); err != nil { + if err := cmd.RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdin: shasToCheckReader, + Stdout: catFileCheckWriter, + Stderr: stderr, + }); err != nil { _ = catFileCheckWriter.CloseWithError(fmt.Errorf("git cat-file --batch-check [%s]: %v - %s", tmpBasePath, err, errbuf.String())) } } @@ -40,7 +46,12 @@ func CatFileBatchCheckAllObjects(ctx context.Context, catFileCheckWriter *io.Pip stderr := new(bytes.Buffer) var errbuf strings.Builder cmd := git.NewCommand(ctx, "cat-file", "--batch-check", "--batch-all-objects") - if err := cmd.RunInDirPipeline(tmpBasePath, catFileCheckWriter, stderr); err != nil { + if err := cmd.RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: catFileCheckWriter, + Stderr: stderr, + }); err != nil { log.Error("git cat-file --batch-check --batch-all-object [%s]: %v - %s", tmpBasePath, err, errbuf.String()) err = fmt.Errorf("git cat-file --batch-check --batch-all-object [%s]: %v - %s", tmpBasePath, err, errbuf.String()) _ = catFileCheckWriter.CloseWithError(err) @@ -56,7 +67,13 @@ func CatFileBatch(ctx context.Context, shasToBatchReader *io.PipeReader, catFile stderr := new(bytes.Buffer) var errbuf strings.Builder - if err := git.NewCommand(ctx, "cat-file", "--batch").RunInDirFullPipeline(tmpBasePath, catFileBatchWriter, stderr, shasToBatchReader); err != nil { + if err := git.NewCommand(ctx, "cat-file", "--batch").RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: catFileBatchWriter, + Stdin: shasToBatchReader, + Stderr: stderr, + }); err != nil { _ = shasToBatchReader.CloseWithError(fmt.Errorf("git rev-list [%s]: %v - %s", tmpBasePath, err, errbuf.String())) } } diff --git a/modules/git/pipeline/lfs_nogogit.go b/modules/git/pipeline/lfs_nogogit.go index 90ffef16bbe5..1d43080a5a01 100644 --- a/modules/git/pipeline/lfs_nogogit.go +++ b/modules/git/pipeline/lfs_nogogit.go @@ -53,7 +53,12 @@ func FindLFSFile(repo *git.Repository, hash git.SHA1) ([]*LFSResult, error) { go func() { stderr := strings.Builder{} - err := git.NewCommand(repo.Ctx, "rev-list", "--all").RunInDirPipeline(repo.Path, revListWriter, &stderr) + err := git.NewCommand(repo.Ctx, "rev-list", "--all").RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: revListWriter, + Stderr: &stderr, + }) if err != nil { _ = revListWriter.CloseWithError(git.ConcatenateError(err, (&stderr).String())) } else { diff --git a/modules/git/pipeline/namerev.go b/modules/git/pipeline/namerev.go index 84006e9005d8..357322070ece 100644 --- a/modules/git/pipeline/namerev.go +++ b/modules/git/pipeline/namerev.go @@ -23,7 +23,13 @@ func NameRevStdin(ctx context.Context, shasToNameReader *io.PipeReader, nameRevS stderr := new(bytes.Buffer) var errbuf strings.Builder - if err := git.NewCommand(ctx, "name-rev", "--stdin", "--name-only", "--always").RunInDirFullPipeline(tmpBasePath, nameRevStdinWriter, stderr, shasToNameReader); err != nil { + if err := git.NewCommand(ctx, "name-rev", "--stdin", "--name-only", "--always").RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: nameRevStdinWriter, + Stdin: shasToNameReader, + Stderr: stderr, + }); err != nil { _ = shasToNameReader.CloseWithError(fmt.Errorf("git name-rev [%s]: %v - %s", tmpBasePath, err, errbuf.String())) } } diff --git a/modules/git/pipeline/revlist.go b/modules/git/pipeline/revlist.go index 75dc676f367b..a1f8f079f959 100644 --- a/modules/git/pipeline/revlist.go +++ b/modules/git/pipeline/revlist.go @@ -25,7 +25,12 @@ func RevListAllObjects(ctx context.Context, revListWriter *io.PipeWriter, wg *sy stderr := new(bytes.Buffer) var errbuf strings.Builder cmd := git.NewCommand(ctx, "rev-list", "--objects", "--all") - if err := cmd.RunInDirPipeline(basePath, revListWriter, stderr); err != nil { + if err := cmd.RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: basePath, + Stdout: revListWriter, + Stderr: stderr, + }); err != nil { log.Error("git rev-list --objects --all [%s]: %v - %s", basePath, err, errbuf.String()) err = fmt.Errorf("git rev-list --objects --all [%s]: %v - %s", basePath, err, errbuf.String()) _ = revListWriter.CloseWithError(err) @@ -40,7 +45,12 @@ func RevListObjects(ctx context.Context, revListWriter *io.PipeWriter, wg *sync. stderr := new(bytes.Buffer) var errbuf strings.Builder cmd := git.NewCommand(ctx, "rev-list", "--objects", headSHA, "--not", baseSHA) - if err := cmd.RunInDirPipeline(tmpBasePath, revListWriter, stderr); err != nil { + if err := cmd.RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: revListWriter, + Stderr: stderr, + }); err != nil { log.Error("git rev-list [%s]: %v - %s", tmpBasePath, err, errbuf.String()) errChan <- fmt.Errorf("git rev-list [%s]: %v - %s", tmpBasePath, err, errbuf.String()) } diff --git a/modules/git/repo.go b/modules/git/repo.go index ff704138a7b1..79a540209c5d 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -204,7 +204,13 @@ func Push(ctx context.Context, repoPath string, opts PushOptions) error { opts.Timeout = -1 } - err := cmd.RunInDirTimeoutEnvPipeline(opts.Env, opts.Timeout, repoPath, &outbuf, &errbuf) + err := cmd.RunWithContext(&RunContext{ + Env: opts.Env, + Timeout: opts.Timeout, + Dir: repoPath, + Stdout: &outbuf, + Stderr: &errbuf, + }) if err != nil { if strings.Contains(errbuf.String(), "non-fast-forward") { return &ErrPushOutOfDate{ diff --git a/modules/git/repo_archive.go b/modules/git/repo_archive.go index cf09bba0db34..b7c339c271fd 100644 --- a/modules/git/repo_archive.go +++ b/modules/git/repo_archive.go @@ -57,7 +57,12 @@ func (repo *Repository) CreateArchive(ctx context.Context, format ArchiveType, t ) var stderr strings.Builder - err := NewCommand(ctx, args...).RunInDirPipeline(repo.Path, target, &stderr) + err := NewCommand(ctx, args...).RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: target, + Stderr: &stderr, + }) if err != nil { return ConcatenateError(err, stderr.String()) } diff --git a/modules/git/repo_attribute.go b/modules/git/repo_attribute.go index c63dfacdfc2b..d31203aabd04 100644 --- a/modules/git/repo_attribute.go +++ b/modules/git/repo_attribute.go @@ -76,7 +76,13 @@ func (repo *Repository) CheckAttribute(opts CheckAttributeOpts) (map[string]map[ cmd := NewCommand(repo.Ctx, cmdArgs...) - if err := cmd.RunInDirTimeoutEnvPipeline(env, -1, repo.Path, stdOut, stdErr); err != nil { + if err := cmd.RunWithContext(&RunContext{ + Env: env, + Timeout: -1, + Dir: repo.Path, + Stdout: stdOut, + Stderr: stdErr, + }); err != nil { return nil, fmt.Errorf("failed to run check-attr: %v\n%s\n%s", err, stdOut.String(), stdErr.String()) } @@ -182,9 +188,17 @@ func (c *CheckAttributeReader) Run() error { _ = c.Close() }() stdErr := new(bytes.Buffer) - err := c.cmd.RunInDirTimeoutEnvFullPipelineFunc(c.env, -1, c.Repo.Path, c.stdOut, stdErr, c.stdinReader, func(_ context.Context, _ context.CancelFunc) error { - close(c.running) - return nil + err := c.cmd.RunWithContext(&RunContext{ + Env: c.env, + Timeout: -1, + Dir: c.Repo.Path, + Stdin: c.stdinReader, + Stdout: c.stdOut, + Stderr: stdErr, + PipelineFunc: func(_ context.Context, _ context.CancelFunc) error { + close(c.running) + return nil + }, }) if err != nil && c.ctx.Err() != nil && err.Error() != "signal: killed" { return fmt.Errorf("failed to run attr-check. Error: %w\nStderr: %s", err, stdErr.String()) diff --git a/modules/git/repo_branch_nogogit.go b/modules/git/repo_branch_nogogit.go index 2e9335a31636..66990add6fd9 100644 --- a/modules/git/repo_branch_nogogit.go +++ b/modules/git/repo_branch_nogogit.go @@ -96,7 +96,12 @@ func walkShowRef(ctx context.Context, repoPath, arg string, skip, limit int, wal if arg != "" { args = append(args, arg) } - err := NewCommand(ctx, args...).RunInDirPipeline(repoPath, stdoutWriter, stderrBuilder) + err := NewCommand(ctx, args...).RunWithContext(&RunContext{ + Timeout: -1, + Dir: repoPath, + Stdout: stdoutWriter, + Stderr: stderrBuilder, + }) if err != nil { if stderrBuilder.Len() == 0 { _ = stdoutWriter.Close() diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 5ccc42a383eb..8e059ce0ea25 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -211,7 +211,12 @@ func (repo *Repository) CommitsByFileAndRange(revision, file string, page int) ( err := NewCommand(repo.Ctx, "log", revision, "--follow", "--max-count="+strconv.Itoa(setting.Git.CommitsRangeSize*page), prettyLogFormat, "--", file). - RunInDirPipeline(repo.Path, stdoutWriter, &stderr) + RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: stdoutWriter, + Stderr: &stderr, + }) if err != nil { _ = stdoutWriter.CloseWithError(ConcatenateError(err, (&stderr).String())) } else { diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index dddc158dcc45..aa8015af14e5 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -147,13 +147,23 @@ func (repo *Repository) GetDiffNumChangedFiles(base, head string, directComparis } if err := NewCommand(repo.Ctx, "diff", "-z", "--name-only", base+separator+head). - RunInDirPipeline(repo.Path, w, stderr); err != nil { + RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: w, + Stderr: stderr, + }); err != nil { if strings.Contains(stderr.String(), "no merge base") { // git >= 2.28 now returns an error if base and head have become unrelated. // previously it would return the results of git diff -z --name-only base head so let's try that... w = &lineCountWriter{} stderr.Reset() - if err = NewCommand(repo.Ctx, "diff", "-z", "--name-only", base, head).RunInDirPipeline(repo.Path, w, stderr); err == nil { + if err = NewCommand(repo.Ctx, "diff", "-z", "--name-only", base, head).RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: w, + Stderr: stderr, + }); err == nil { return w.numLines, nil } } @@ -238,28 +248,46 @@ func (repo *Repository) GetDiffOrPatch(base, head string, w io.Writer, patch, bi // GetDiff generates and returns patch data between given revisions, optimized for human readability func (repo *Repository) GetDiff(base, head string, w io.Writer) error { - return NewCommand(repo.Ctx, "diff", "-p", base, head). - RunInDirPipeline(repo.Path, w, nil) + return NewCommand(repo.Ctx, "diff", "-p", base, head).RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: w, + }) } // GetDiffBinary generates and returns patch data between given revisions, including binary diffs. func (repo *Repository) GetDiffBinary(base, head string, w io.Writer) error { if CheckGitVersionAtLeast("1.7.7") == nil { - return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram", base, head). - RunInDirPipeline(repo.Path, w, nil) + return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram", base, head).RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: w, + }) } - return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--patience", base, head). - RunInDirPipeline(repo.Path, w, nil) + return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--patience", base, head).RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: w, + }) } // GetPatch generates and returns format-patch data between given revisions, able to be used with `git apply` func (repo *Repository) GetPatch(base, head string, w io.Writer) error { stderr := new(bytes.Buffer) err := NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout", base+"..."+head). - RunInDirPipeline(repo.Path, w, stderr) + RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: w, + Stderr: stderr, + }) if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { return NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout", base, head). - RunInDirPipeline(repo.Path, w, nil) + RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: w, + }) } return err } @@ -268,7 +296,12 @@ func (repo *Repository) GetPatch(base, head string, w io.Writer) error { func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) error { stderr := new(bytes.Buffer) err := NewCommand(repo.Ctx, "diff", "-p", "--binary", base+"..."+head). - RunInDirPipeline(repo.Path, w, stderr) + RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: w, + Stderr: stderr, + }) if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) { return repo.GetDiffBinary(base, head, w) } diff --git a/modules/git/repo_index.go b/modules/git/repo_index.go index 8e76c5e46609..53de0f1cb8ec 100644 --- a/modules/git/repo_index.go +++ b/modules/git/repo_index.go @@ -106,7 +106,13 @@ func (repo *Repository) RemoveFilesFromIndex(filenames ...string) error { buffer.WriteByte('\000') } } - return cmd.RunInDirFullPipeline(repo.Path, stdout, stderr, bytes.NewReader(buffer.Bytes())) + return cmd.RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdin: bytes.NewReader(buffer.Bytes()), + Stdout: stdout, + Stderr: stderr, + }) } // AddObjectToIndex adds the provided object hash to the index at the provided filename diff --git a/modules/git/repo_object.go b/modules/git/repo_object.go index a9ab66b28f17..378e657ce4f4 100644 --- a/modules/git/repo_object.go +++ b/modules/git/repo_object.go @@ -45,7 +45,13 @@ func (repo *Repository) hashObject(reader io.Reader) (string, error) { cmd := NewCommand(repo.Ctx, "hash-object", "-w", "--stdin") stdout := new(bytes.Buffer) stderr := new(bytes.Buffer) - err := cmd.RunInDirFullPipeline(repo.Path, stdout, stderr, reader) + err := cmd.RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdin: reader, + Stdout: stdout, + Stderr: stderr, + }) if err != nil { return "", err } diff --git a/modules/git/repo_ref_nogogit.go b/modules/git/repo_ref_nogogit.go index e17d23eb9c41..42295e43ac58 100644 --- a/modules/git/repo_ref_nogogit.go +++ b/modules/git/repo_ref_nogogit.go @@ -23,7 +23,12 @@ func (repo *Repository) GetRefsFiltered(pattern string) ([]*Reference, error) { go func() { stderrBuilder := &strings.Builder{} - err := NewCommand(repo.Ctx, "for-each-ref").RunInDirPipeline(repo.Path, stdoutWriter, stderrBuilder) + err := NewCommand(repo.Ctx, "for-each-ref").RunWithContext(&RunContext{ + Timeout: -1, + Dir: repo.Path, + Stdout: stdoutWriter, + Stderr: stderrBuilder, + }) if err != nil { _ = stdoutWriter.CloseWithError(ConcatenateError(err, stderrBuilder.String())) } else { diff --git a/modules/git/repo_stats.go b/modules/git/repo_stats.go index 6f5973ebe5f2..598ec37a2c61 100644 --- a/modules/git/repo_stats.go +++ b/modules/git/repo_stats.go @@ -67,12 +67,14 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) } stderr := new(strings.Builder) - err = NewCommand(repo.Ctx, args...).RunInDirTimeoutEnvFullPipelineFunc( - nil, -1, repo.Path, - stdoutWriter, stderr, nil, - func(ctx context.Context, cancel context.CancelFunc) error { + err = NewCommand(repo.Ctx, args...).RunWithContext(&RunContext{ + Env: []string{}, + Timeout: -1, + Dir: repo.Path, + Stdout: stdoutWriter, + Stderr: stderr, + PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() - scanner := bufio.NewScanner(stdoutReader) scanner.Split(bufio.ScanLines) stats.CommitCount = 0 @@ -103,11 +105,7 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) case 4: // E-mail email := strings.ToLower(l) if _, ok := authors[email]; !ok { - authors[email] = &CodeActivityAuthor{ - Name: author, - Email: email, - Commits: 0, - } + authors[email] = &CodeActivityAuthor{Name: author, Email: email, Commits: 0} } authors[email].Commits++ default: // Changed file @@ -128,7 +126,6 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) } } } - a := make([]*CodeActivityAuthor, 0, len(authors)) for _, v := range authors { a = append(a, v) @@ -137,14 +134,13 @@ func (repo *Repository) GetCodeActivityStats(fromTime time.Time, branch string) sort.Slice(a, func(i, j int) bool { return a[i].Commits > a[j].Commits }) - stats.AuthorCount = int64(len(authors)) stats.ChangedFiles = int64(len(files)) stats.Authors = a - _ = stdoutReader.Close() return nil - }) + }, + }) if err != nil { return nil, fmt.Errorf("Failed to get GetCodeActivityStats for repository.\nError: %w\nStderr: %s", err, stderr) } diff --git a/modules/git/repo_tree.go b/modules/git/repo_tree.go index 9efacc8dfe00..3219b569a53d 100644 --- a/modules/git/repo_tree.go +++ b/modules/git/repo_tree.go @@ -60,7 +60,14 @@ func (repo *Repository) CommitTree(author, committer *Signature, tree *Tree, opt stdout := new(bytes.Buffer) stderr := new(bytes.Buffer) - err = cmd.RunInDirTimeoutEnvFullPipeline(env, -1, repo.Path, stdout, stderr, messageBytes) + err = cmd.RunWithContext(&RunContext{ + Env: env, + Timeout: -1, + Dir: repo.Path, + Stdin: messageBytes, + Stdout: stdout, + Stderr: stderr, + }) if err != nil { return SHA1{}, ConcatenateError(err, stderr.String()) diff --git a/modules/gitgraph/graph.go b/modules/gitgraph/graph.go index c0618152d8a8..e15441b8831d 100644 --- a/modules/gitgraph/graph.go +++ b/modules/gitgraph/graph.go @@ -64,57 +64,63 @@ func GetCommitGraph(r *git.Repository, page, maxAllowedColors int, hidePRRefs bo scanner := bufio.NewScanner(stdoutReader) - if err := graphCmd.RunInDirTimeoutEnvFullPipelineFunc(nil, -1, r.Path, stdoutWriter, stderr, nil, func(ctx context.Context, cancel context.CancelFunc) error { - _ = stdoutWriter.Close() - defer stdoutReader.Close() - parser := &Parser{} - parser.firstInUse = -1 - parser.maxAllowedColors = maxAllowedColors - if maxAllowedColors > 0 { - parser.availableColors = make([]int, maxAllowedColors) - for i := range parser.availableColors { - parser.availableColors[i] = i + 1 - } - } else { - parser.availableColors = []int{1, 2} - } - for commitsToSkip > 0 && scanner.Scan() { - line := scanner.Bytes() - dataIdx := bytes.Index(line, []byte("DATA:")) - if dataIdx < 0 { - dataIdx = len(line) + if err := graphCmd.RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: r.Path, + Stdout: stdoutWriter, + Stderr: stderr, + PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { + _ = stdoutWriter.Close() + defer stdoutReader.Close() + parser := &Parser{} + parser.firstInUse = -1 + parser.maxAllowedColors = maxAllowedColors + if maxAllowedColors > 0 { + parser.availableColors = make([]int, maxAllowedColors) + for i := range parser.availableColors { + parser.availableColors[i] = i + 1 + } + } else { + parser.availableColors = []int{1, 2} } - starIdx := bytes.IndexByte(line, '*') - if starIdx >= 0 && starIdx < dataIdx { - commitsToSkip-- + for commitsToSkip > 0 && scanner.Scan() { + line := scanner.Bytes() + dataIdx := bytes.Index(line, []byte("DATA:")) + if dataIdx < 0 { + dataIdx = len(line) + } + starIdx := bytes.IndexByte(line, '*') + if starIdx >= 0 && starIdx < dataIdx { + commitsToSkip-- + } + parser.ParseGlyphs(line[:dataIdx]) } - parser.ParseGlyphs(line[:dataIdx]) - } - row := 0 + row := 0 + + // Skip initial non-commit lines + for scanner.Scan() { + line := scanner.Bytes() + if bytes.IndexByte(line, '*') >= 0 { + if err := parser.AddLineToGraph(graph, row, line); err != nil { + cancel() + return err + } + break + } + parser.ParseGlyphs(line) + } - // Skip initial non-commit lines - for scanner.Scan() { - line := scanner.Bytes() - if bytes.IndexByte(line, '*') >= 0 { + for scanner.Scan() { + row++ + line := scanner.Bytes() if err := parser.AddLineToGraph(graph, row, line); err != nil { cancel() return err } - break - } - parser.ParseGlyphs(line) - } - - for scanner.Scan() { - row++ - line := scanner.Bytes() - if err := parser.AddLineToGraph(graph, row, line); err != nil { - cancel() - return err } - } - return scanner.Err() + return scanner.Err() + }, }); err != nil { return graph, err } diff --git a/routers/private/hook_verification.go b/routers/private/hook_verification.go index 565cb273e790..683ed8d071c6 100644 --- a/routers/private/hook_verification.go +++ b/routers/private/hook_verification.go @@ -45,9 +45,12 @@ func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env [] // This is safe as force pushes are already forbidden err = git.NewCommand(repo.Ctx, "rev-list", oldCommitID+"..."+newCommitID). - RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, - stdoutWriter, nil, nil, - func(ctx context.Context, cancel context.CancelFunc) error { + RunWithContext(&git.RunContext{ + Env: env, + Timeout: -1, + Dir: repo.Path, + Stdout: stdoutWriter, + PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() err := readAndVerifyCommitsFromShaReader(stdoutReader, repo, env) if err != nil { @@ -56,7 +59,8 @@ func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env [] } _ = stdoutReader.Close() return err - }) + }, + }) if err != nil && !isErrUnverifiedCommit(err) { log.Error("Unable to check commits from %s to %s in %s: %v", oldCommitID, newCommitID, repo.Path, err) } @@ -89,9 +93,12 @@ func readAndVerifyCommit(sha string, repo *git.Repository, env []string) error { hash := git.MustIDFromString(sha) return git.NewCommand(repo.Ctx, "cat-file", "commit", sha). - RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path, - stdoutWriter, nil, nil, - func(ctx context.Context, cancel context.CancelFunc) error { + RunWithContext(&git.RunContext{ + Env: env, + Timeout: -1, + Dir: repo.Path, + Stdout: stdoutWriter, + PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { _ = stdoutWriter.Close() commit, err := git.CommitFromReader(repo, hash, stdoutReader) if err != nil { @@ -105,7 +112,8 @@ func readAndVerifyCommit(sha string, repo *git.Repository, env []string) error { } } return nil - }) + }, + }) } type errUnverifiedCommit struct { diff --git a/services/mirror/mirror_pull.go b/services/mirror/mirror_pull.go index 4710f9642da3..c86f15efc021 100644 --- a/services/mirror/mirror_pull.go +++ b/services/mirror/mirror_pull.go @@ -161,7 +161,12 @@ func pruneBrokenReferences(ctx context.Context, stdoutBuilder.Reset() pruneErr := git.NewCommand(ctx, "remote", "prune", m.GetRemoteName()). SetDescription(fmt.Sprintf("Mirror.runSync %ssPrune references: %s ", wiki, m.Repo.FullName())). - RunInDirTimeoutPipeline(timeout, repoPath, stdoutBuilder, stderrBuilder) + RunWithContext(&git.RunContext{ + Timeout: timeout, + Dir: repoPath, + Stdout: stdoutBuilder, + Stderr: stderrBuilder, + }) if pruneErr != nil { stdout := stdoutBuilder.String() stderr := stderrBuilder.String() @@ -203,7 +208,12 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo stderrBuilder := strings.Builder{} if err := git.NewCommand(ctx, gitArgs...). SetDescription(fmt.Sprintf("Mirror.runSync: %s", m.Repo.FullName())). - RunInDirTimeoutPipeline(timeout, repoPath, &stdoutBuilder, &stderrBuilder); err != nil { + RunWithContext(&git.RunContext{ + Timeout: timeout, + Dir: repoPath, + Stdout: &stdoutBuilder, + Stderr: &stderrBuilder, + }); err != nil { stdout := stdoutBuilder.String() stderr := stderrBuilder.String() @@ -226,7 +236,12 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo stdoutBuilder.Reset() if err = git.NewCommand(ctx, gitArgs...). SetDescription(fmt.Sprintf("Mirror.runSync: %s", m.Repo.FullName())). - RunInDirTimeoutPipeline(timeout, repoPath, &stdoutBuilder, &stderrBuilder); err != nil { + RunWithContext(&git.RunContext{ + Timeout: timeout, + Dir: repoPath, + Stdout: &stdoutBuilder, + Stderr: &stderrBuilder, + }); err != nil { stdout := stdoutBuilder.String() stderr := stderrBuilder.String() @@ -282,7 +297,12 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo stdoutBuilder.Reset() if err := git.NewCommand(ctx, "remote", "update", "--prune", m.GetRemoteName()). SetDescription(fmt.Sprintf("Mirror.runSync Wiki: %s ", m.Repo.FullName())). - RunInDirTimeoutPipeline(timeout, wikiPath, &stdoutBuilder, &stderrBuilder); err != nil { + RunWithContext(&git.RunContext{ + Timeout: timeout, + Dir: wikiPath, + Stdout: &stdoutBuilder, + Stderr: &stderrBuilder, + }); err != nil { stdout := stdoutBuilder.String() stderr := stderrBuilder.String() @@ -314,7 +334,12 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo if err = git.NewCommand(ctx, "remote", "update", "--prune", m.GetRemoteName()). SetDescription(fmt.Sprintf("Mirror.runSync Wiki: %s ", m.Repo.FullName())). - RunInDirTimeoutPipeline(timeout, wikiPath, &stdoutBuilder, &stderrBuilder); err != nil { + RunWithContext(&git.RunContext{ + Timeout: timeout, + Dir: wikiPath, + Stdout: &stdoutBuilder, + Stderr: &stderrBuilder, + }); err != nil { stdout := stdoutBuilder.String() stderr := stderrBuilder.String() stderrMessage = sanitizer.Replace(stderr) diff --git a/services/pull/merge.go b/services/pull/merge.go index 62c502011a32..cb857cc60dbc 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -188,35 +188,65 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User } // Switch off LFS process (set required, clean and smudge here also) - if err := gitConfigCommand().AddArguments("filter.lfs.process", "").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := gitConfigCommand().AddArguments("filter.lfs.process", ""). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git config [filter.lfs.process -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("git config [filter.lfs.process -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } outbuf.Reset() errbuf.Reset() - if err := gitConfigCommand().AddArguments("filter.lfs.required", "false").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := gitConfigCommand().AddArguments("filter.lfs.required", "false"). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git config [filter.lfs.required -> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("git config [filter.lfs.required -> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } outbuf.Reset() errbuf.Reset() - if err := gitConfigCommand().AddArguments("filter.lfs.clean", "").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := gitConfigCommand().AddArguments("filter.lfs.clean", ""). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git config [filter.lfs.clean -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("git config [filter.lfs.clean -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } outbuf.Reset() errbuf.Reset() - if err := gitConfigCommand().AddArguments("filter.lfs.smudge", "").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := gitConfigCommand().AddArguments("filter.lfs.smudge", ""). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git config [filter.lfs.smudge -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("git config [filter.lfs.smudge -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } outbuf.Reset() errbuf.Reset() - if err := gitConfigCommand().AddArguments("core.sparseCheckout", "true").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := gitConfigCommand().AddArguments("core.sparseCheckout", "true"). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git config [core.sparseCheckout -> true ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("git config [core.sparsecheckout -> true]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } @@ -224,7 +254,13 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User errbuf.Reset() // Read base branch index - if err := git.NewCommand(ctx, "read-tree", "HEAD").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "read-tree", "HEAD"). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git read-tree HEAD: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("Unable to read base branch in to the index: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } @@ -279,7 +315,13 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User fallthrough case repo_model.MergeStyleRebaseMerge: // Checkout head branch - if err := git.NewCommand(ctx, "checkout", "-b", stagingBranch, trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "checkout", "-b", stagingBranch, trackingBranch). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git checkout base prior to merge post staging rebase [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("git checkout base prior to merge post staging rebase [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } @@ -287,7 +329,13 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User errbuf.Reset() // Rebase before merging - if err := git.NewCommand(ctx, "rebase", baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "rebase", baseBranch). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { // Rebase will leave a REBASE_HEAD file in .git if there is a conflict if _, statErr := os.Stat(filepath.Join(tmpBasePath, ".git", "REBASE_HEAD")); statErr == nil { var commitSha string @@ -335,7 +383,13 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User } // Checkout base branch again - if err := git.NewCommand(ctx, "checkout", baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "checkout", baseBranch). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git checkout base prior to merge post staging rebase [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("git checkout base prior to merge post staging rebase [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } @@ -375,7 +429,14 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User } sig := pr.Issue.Poster.NewGitSig() if signArg == "" { - if err := git.NewCommand(ctx, "commit", fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email), "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "commit", fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email), "-m", message). + RunWithContext(&git.RunContext{ + Env: env, + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } @@ -384,7 +445,14 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User // add trailer message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String()) } - if err := git.NewCommand(ctx, "commit", signArg, fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email), "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "commit", signArg, fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email), "-m", message). + RunWithContext(&git.RunContext{ + Env: env, + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) return "", fmt.Errorf("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } @@ -448,7 +516,13 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User } // Push back to upstream. - if err := pushCmd.RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + if err := pushCmd.RunWithContext(&git.RunContext{ + Env: env, + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { if strings.Contains(errbuf.String(), "non-fast-forward") { return "", &git.ErrPushOutOfDate{ StdOut: outbuf.String(), @@ -475,12 +549,26 @@ func rawMerge(ctx context.Context, pr *models.PullRequest, doer *user_model.User func commitAndSignNoAuthor(ctx context.Context, pr *models.PullRequest, message, signArg, tmpBasePath string, env []string) error { var outbuf, errbuf strings.Builder if signArg == "" { - if err := git.NewCommand(ctx, "commit", "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "commit", "-m", message). + RunWithContext(&git.RunContext{ + Env: env, + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) return fmt.Errorf("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } } else { - if err := git.NewCommand(ctx, "commit", signArg, "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "commit", signArg, "-m", message). + RunWithContext(&git.RunContext{ + Env: env, + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) return fmt.Errorf("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } @@ -490,7 +578,12 @@ func commitAndSignNoAuthor(ctx context.Context, pr *models.PullRequest, message, func runMergeCommand(pr *models.PullRequest, mergeStyle repo_model.MergeStyle, cmd *git.Command, tmpBasePath string) error { var outbuf, errbuf strings.Builder - if err := cmd.RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := cmd.RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { // Merge will leave a MERGE_HEAD file in the .git folder if there is a conflict if _, statErr := os.Stat(filepath.Join(tmpBasePath, ".git", "MERGE_HEAD")); statErr == nil { // We have a merge conflict error @@ -523,7 +616,13 @@ func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string) ( getDiffTreeFromBranch := func(repoPath, baseBranch, headBranch string) (string, error) { var outbuf, errbuf strings.Builder // Compute the diff-tree for sparse-checkout - if err := git.NewCommand(ctx, "diff-tree", "--no-commit-id", "--name-only", "-r", "-z", "--root", baseBranch, headBranch, "--").RunInDirPipeline(repoPath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "diff-tree", "--no-commit-id", "--name-only", "-r", "-z", "--root", baseBranch, headBranch, "--"). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: repoPath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { return "", fmt.Errorf("git diff-tree [%s base:%s head:%s]: %s", repoPath, baseBranch, headBranch, errbuf.String()) } return outbuf.String(), nil diff --git a/services/pull/patch.go b/services/pull/patch.go index a2c8345326f0..f401b85345eb 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -383,10 +383,11 @@ func checkConflicts(ctx context.Context, pr *models.PullRequest, gitRepo *git.Re // 7. Run the check command conflict = false err = git.NewCommand(gitRepo.Ctx, args...). - RunInDirTimeoutEnvFullPipelineFunc( - nil, -1, tmpBasePath, - nil, stderrWriter, nil, - func(ctx context.Context, cancel context.CancelFunc) error { + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stderr: stderrWriter, + PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { // Close the writer end of the pipe to begin processing _ = stderrWriter.Close() defer func() { @@ -444,7 +445,8 @@ func checkConflicts(ctx context.Context, pr *models.PullRequest, gitRepo *git.Re } return nil - }) + }, + }) // 8. If there is a conflict the `git apply` command will return a non-zero error code - so there will be a positive error. if err != nil { diff --git a/services/pull/patch_unmerged.go b/services/pull/patch_unmerged.go index 65264f9865ae..abd54b07cf12 100644 --- a/services/pull/patch_unmerged.go +++ b/services/pull/patch_unmerged.go @@ -63,10 +63,12 @@ func readUnmergedLsFileLines(ctx context.Context, tmpBasePath string, outputChan stderr := &strings.Builder{} err = git.NewCommand(ctx, "ls-files", "-u", "-z"). - RunInDirTimeoutEnvFullPipelineFunc( - nil, -1, tmpBasePath, - lsFilesWriter, stderr, nil, - func(_ context.Context, _ context.CancelFunc) error { + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: lsFilesWriter, + Stderr: stderr, + PipelineFunc: func(_ context.Context, _ context.CancelFunc) error { _ = lsFilesWriter.Close() defer func() { _ = lsFilesReader.Close() @@ -102,8 +104,8 @@ func readUnmergedLsFileLines(ctx context.Context, tmpBasePath string, outputChan toemit.path = split[2][2 : len(split[2])-1] outputChan <- toemit } - }) - + }, + }) if err != nil { outputChan <- &lsFileLine{err: fmt.Errorf("git ls-files -u -z: %v", git.ConcatenateError(err, stderr.String()))} } diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index e9c227d79422..831d98745e4c 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -93,7 +93,13 @@ func createTemporaryRepo(ctx context.Context, pr *models.PullRequest) (string, e } var outbuf, errbuf strings.Builder - if err := git.NewCommand(ctx, "remote", "add", "-t", pr.BaseBranch, "-m", pr.BaseBranch, "origin", baseRepoPath).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "remote", "add", "-t", pr.BaseBranch, "-m", pr.BaseBranch, "origin", baseRepoPath). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("Unable to add base repository as origin [%s -> %s]: %v\n%s\n%s", pr.BaseRepo.FullName(), tmpBasePath, err, outbuf.String(), errbuf.String()) if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) @@ -103,7 +109,13 @@ func createTemporaryRepo(ctx context.Context, pr *models.PullRequest) (string, e outbuf.Reset() errbuf.Reset() - if err := git.NewCommand(ctx, "fetch", "origin", "--no-tags", "--", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "fetch", "origin", "--no-tags", "--", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("Unable to fetch origin base branch [%s:%s -> base, original_base in %s]: %v:\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, tmpBasePath, err, outbuf.String(), errbuf.String()) if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) @@ -113,7 +125,13 @@ func createTemporaryRepo(ctx context.Context, pr *models.PullRequest) (string, e outbuf.Reset() errbuf.Reset() - if err := git.NewCommand(ctx, "symbolic-ref", "HEAD", git.BranchPrefix+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "symbolic-ref", "HEAD", git.BranchPrefix+baseBranch). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("Unable to set HEAD as base branch [%s]: %v\n%s\n%s", tmpBasePath, err, outbuf.String(), errbuf.String()) if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) @@ -131,7 +149,13 @@ func createTemporaryRepo(ctx context.Context, pr *models.PullRequest) (string, e return "", fmt.Errorf("Unable to head base repository to temporary repo [%s -> tmpBasePath]: %v", pr.HeadRepo.FullName(), err) } - if err := git.NewCommand(ctx, "remote", "add", remoteRepoName, headRepoPath).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "remote", "add", remoteRepoName, headRepoPath). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { log.Error("Unable to add head repository as head_repo [%s -> %s]: %v\n%s\n%s", pr.HeadRepo.FullName(), tmpBasePath, err, outbuf.String(), errbuf.String()) if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) @@ -151,7 +175,13 @@ func createTemporaryRepo(ctx context.Context, pr *models.PullRequest) (string, e } else { headBranch = pr.GetGitRefName() } - if err := git.NewCommand(ctx, "fetch", "--no-tags", remoteRepoName, headBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + if err := git.NewCommand(ctx, "fetch", "--no-tags", remoteRepoName, headBranch+":"+trackingBranch). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: tmpBasePath, + Stdout: &outbuf, + Stderr: &errbuf, + }); err != nil { if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { log.Error("CreateTempRepo: RemoveTemporaryPath: %s", err) } diff --git a/services/repository/files/temp_repo.go b/services/repository/files/temp_repo.go index b89d51601afe..2223e1c8fda3 100644 --- a/services/repository/files/temp_repo.go +++ b/services/repository/files/temp_repo.go @@ -97,7 +97,13 @@ func (t *TemporaryUploadRepository) LsFiles(filenames ...string) ([]string, erro } } - if err := git.NewCommand(t.ctx, cmdArgs...).RunInDirPipeline(t.basePath, stdOut, stdErr); err != nil { + if err := git.NewCommand(t.ctx, cmdArgs...). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: t.basePath, + Stdout: stdOut, + Stderr: stdErr, + }); err != nil { log.Error("Unable to run git ls-files for temporary repo: %s (%s) Error: %v\nstdout: %s\nstderr: %s", t.repo.FullName(), t.basePath, err, stdOut.String(), stdErr.String()) err = fmt.Errorf("Unable to run git ls-files for temporary repo of: %s Error: %v\nstdout: %s\nstderr: %s", t.repo.FullName(), err, stdOut.String(), stdErr.String()) return nil, err @@ -124,7 +130,14 @@ func (t *TemporaryUploadRepository) RemoveFilesFromIndex(filenames ...string) er } } - if err := git.NewCommand(t.ctx, "update-index", "--remove", "-z", "--index-info").RunInDirFullPipeline(t.basePath, stdOut, stdErr, stdIn); err != nil { + if err := git.NewCommand(t.ctx, "update-index", "--remove", "-z", "--index-info"). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: t.basePath, + Stdin: stdIn, + Stdout: stdOut, + Stderr: stdErr, + }); err != nil { log.Error("Unable to update-index for temporary repo: %s (%s) Error: %v\nstdout: %s\nstderr: %s", t.repo.FullName(), t.basePath, err, stdOut.String(), stdErr.String()) return fmt.Errorf("Unable to update-index for temporary repo: %s Error: %v\nstdout: %s\nstderr: %s", t.repo.FullName(), err, stdOut.String(), stdErr.String()) } @@ -136,7 +149,14 @@ func (t *TemporaryUploadRepository) HashObject(content io.Reader) (string, error stdOut := new(bytes.Buffer) stdErr := new(bytes.Buffer) - if err := git.NewCommand(t.ctx, "hash-object", "-w", "--stdin").RunInDirFullPipeline(t.basePath, stdOut, stdErr, content); err != nil { + if err := git.NewCommand(t.ctx, "hash-object", "-w", "--stdin"). + RunWithContext(&git.RunContext{ + Timeout: -1, + Dir: t.basePath, + Stdin: content, + Stdout: stdOut, + Stderr: stdErr, + }); err != nil { log.Error("Unable to hash-object to temporary repo: %s (%s) Error: %v\nstdout: %s\nstderr: %s", t.repo.FullName(), t.basePath, err, stdOut.String(), stdErr.String()) return "", fmt.Errorf("Unable to hash-object to temporary repo: %s Error: %v\nstdout: %s\nstderr: %s", t.repo.FullName(), err, stdOut.String(), stdErr.String()) } @@ -254,7 +274,15 @@ func (t *TemporaryUploadRepository) CommitTreeWithDate(author, committer *user_m stdout := new(bytes.Buffer) stderr := new(bytes.Buffer) - if err := git.NewCommand(t.ctx, args...).RunInDirTimeoutEnvFullPipeline(env, -1, t.basePath, stdout, stderr, messageBytes); err != nil { + if err := git.NewCommand(t.ctx, args...). + RunWithContext(&git.RunContext{ + Env: env, + Timeout: -1, + Dir: t.basePath, + Stdin: messageBytes, + Stdout: stdout, + Stderr: stderr, + }); err != nil { log.Error("Unable to commit-tree in temporary repo: %s (%s) Error: %v\nStdout: %s\nStderr: %s", t.repo.FullName(), t.basePath, err, stdout, stderr) return "", fmt.Errorf("Unable to commit-tree in temporary repo: %s Error: %v\nStdout: %s\nStderr: %s", @@ -304,15 +332,21 @@ func (t *TemporaryUploadRepository) DiffIndex() (*gitdiff.Diff, error) { var finalErr error if err := git.NewCommand(t.ctx, "diff-index", "--src-prefix=\\a/", "--dst-prefix=\\b/", "--cached", "-p", "HEAD"). - RunInDirTimeoutEnvFullPipelineFunc(nil, 30*time.Second, t.basePath, stdoutWriter, stderr, nil, func(ctx context.Context, cancel context.CancelFunc) error { - _ = stdoutWriter.Close() - diff, finalErr = gitdiff.ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "") - if finalErr != nil { - log.Error("ParsePatch: %v", finalErr) - cancel() - } - _ = stdoutReader.Close() - return finalErr + RunWithContext(&git.RunContext{ + Timeout: 30 * time.Second, + Dir: t.basePath, + Stdout: stdoutWriter, + Stderr: stderr, + PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error { + _ = stdoutWriter.Close() + diff, finalErr = gitdiff.ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, stdoutReader, "") + if finalErr != nil { + log.Error("ParsePatch: %v", finalErr) + cancel() + } + _ = stdoutReader.Close() + return finalErr + }, }); err != nil { if finalErr != nil { log.Error("Unable to ParsePatch in temporary repo %s (%s). Error: %v", t.repo.FullName(), t.basePath, finalErr) From 832ce406aefed0cceb30d42d1435f425a9aba279 Mon Sep 17 00:00:00 2001 From: Sven Seeberg Date: Fri, 11 Feb 2022 15:24:58 +0100 Subject: [PATCH 02/11] Add LDAP group sync to Teams, fixes #1395 (#16299) * Add setting for a JSON that maps LDAP groups to Org Teams. * Add log when removing or adding team members. * Sync is being run on login and periodically. * Existing group filter settings are reused. * Adding and removing team members. * Sync not existing LDAP group. * Login with broken group map JSON. --- cmd/admin_auth_ldap.go | 1 - integrations/auth_ldap_test.go | 119 ++++++++++++++- options/locale/locale_en-US.ini | 6 +- routers/web/admin/auths.go | 2 + services/auth/source/ldap/README.md | 8 + services/auth/source/ldap/source.go | 2 + .../auth/source/ldap/source_authenticate.go | 13 +- .../auth/source/ldap/source_group_sync.go | 100 +++++++++++++ services/auth/source/ldap/source_search.go | 141 ++++++++++++++---- services/auth/source/ldap/source_sync.go | 7 + services/forms/auth_form.go | 2 + templates/admin/auth/edit.tmpl | 36 +++-- templates/admin/auth/source/ldap.tmpl | 35 +++-- web_src/js/features/admin-common.js | 16 +- 14 files changed, 423 insertions(+), 65 deletions(-) create mode 100644 services/auth/source/ldap/source_group_sync.go diff --git a/cmd/admin_auth_ldap.go b/cmd/admin_auth_ldap.go index 06f9244d5009..ec86b2c671d4 100644 --- a/cmd/admin_auth_ldap.go +++ b/cmd/admin_auth_ldap.go @@ -260,7 +260,6 @@ func parseLdapConfig(c *cli.Context, config *ldap.Source) error { if c.IsSet("skip-local-2fa") { config.SkipLocalTwoFA = c.Bool("skip-local-2fa") } - return nil } diff --git a/integrations/auth_ldap_test.go b/integrations/auth_ldap_test.go index 6eb017017f8d..ef0fafc93de4 100644 --- a/integrations/auth_ldap_test.go +++ b/integrations/auth_ldap_test.go @@ -11,6 +11,9 @@ import ( "strings" "testing" + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/services/auth" "github.com/stretchr/testify/assert" @@ -97,7 +100,13 @@ func getLDAPServerHost() string { return host } -func addAuthSourceLDAP(t *testing.T, sshKeyAttribute string) { +func addAuthSourceLDAP(t *testing.T, sshKeyAttribute string, groupMapParams ...string) { + groupTeamMapRemoval := "off" + groupTeamMap := "" + if len(groupMapParams) == 2 { + groupTeamMapRemoval = groupMapParams[0] + groupTeamMap = groupMapParams[1] + } session := loginUser(t, "user1") csrf := GetCSRF(t, session, "/admin/auths/new") req := NewRequestWithValues(t, "POST", "/admin/auths/new", map[string]string{ @@ -119,6 +128,12 @@ func addAuthSourceLDAP(t *testing.T, sshKeyAttribute string) { "attribute_ssh_public_key": sshKeyAttribute, "is_sync_enabled": "on", "is_active": "on", + "groups_enabled": "on", + "group_dn": "ou=people,dc=planetexpress,dc=com", + "group_member_uid": "member", + "group_team_map": groupTeamMap, + "group_team_map_removal": groupTeamMapRemoval, + "user_uid": "DN", }) session.MakeRequest(t, req, http.StatusFound) } @@ -294,3 +309,105 @@ func TestLDAPUserSSHKeySync(t *testing.T) { assert.ElementsMatch(t, u.SSHKeys, syncedKeys, "Unequal number of keys synchronized for user: %s", u.UserName) } } + +func TestLDAPGroupTeamSyncAddMember(t *testing.T) { + if skipLDAPTests() { + t.Skip() + return + } + defer prepareTestEnv(t)() + addAuthSourceLDAP(t, "", "on", `{"cn=ship_crew,ou=people,dc=planetexpress,dc=com":{"org26": ["team11"]},"cn=admin_staff,ou=people,dc=planetexpress,dc=com": {"non-existent": ["non-existent"]}}`) + org, err := models.GetOrgByName("org26") + assert.NoError(t, err) + team, err := models.GetTeam(org.ID, "team11") + assert.NoError(t, err) + auth.SyncExternalUsers(context.Background(), true) + for _, gitLDAPUser := range gitLDAPUsers { + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ + Name: gitLDAPUser.UserName, + }).(*user_model.User) + usersOrgs, err := models.FindOrgs(models.FindOrgOptions{ + UserID: user.ID, + IncludePrivate: true, + }) + assert.NoError(t, err) + allOrgTeams, err := models.GetUserOrgTeams(org.ID, user.ID) + assert.NoError(t, err) + if user.Name == "fry" || user.Name == "leela" || user.Name == "bender" { + // assert members of LDAP group "cn=ship_crew" are added to mapped teams + assert.Equal(t, len(usersOrgs), 1, "User [%s] should be member of one organization", user.Name) + assert.Equal(t, usersOrgs[0].Name, "org26", "Membership should be added to the right organization") + isMember, err := models.IsTeamMember(usersOrgs[0].ID, team.ID, user.ID) + assert.NoError(t, err) + assert.True(t, isMember, "Membership should be added to the right team") + err = team.RemoveMember(user.ID) + assert.NoError(t, err) + err = usersOrgs[0].RemoveMember(user.ID) + assert.NoError(t, err) + } else { + // assert members of LDAP group "cn=admin_staff" keep initial team membership since mapped team does not exist + assert.Empty(t, usersOrgs, "User should be member of no organization") + isMember, err := models.IsTeamMember(org.ID, team.ID, user.ID) + assert.NoError(t, err) + assert.False(t, isMember, "User should no be added to this team") + assert.Empty(t, allOrgTeams, "User should not be added to any team") + } + } +} + +func TestLDAPGroupTeamSyncRemoveMember(t *testing.T) { + if skipLDAPTests() { + t.Skip() + return + } + defer prepareTestEnv(t)() + addAuthSourceLDAP(t, "", "on", `{"cn=dispatch,ou=people,dc=planetexpress,dc=com": {"org26": ["team11"]}}`) + org, err := models.GetOrgByName("org26") + assert.NoError(t, err) + team, err := models.GetTeam(org.ID, "team11") + assert.NoError(t, err) + loginUserWithPassword(t, gitLDAPUsers[0].UserName, gitLDAPUsers[0].Password) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ + Name: gitLDAPUsers[0].UserName, + }).(*user_model.User) + err = org.AddMember(user.ID) + assert.NoError(t, err) + err = team.AddMember(user.ID) + assert.NoError(t, err) + isMember, err := models.IsOrganizationMember(org.ID, user.ID) + assert.NoError(t, err) + assert.True(t, isMember, "User should be member of this organization") + isMember, err = models.IsTeamMember(org.ID, team.ID, user.ID) + assert.NoError(t, err) + assert.True(t, isMember, "User should be member of this team") + // assert team member "professor" gets removed from org26 team11 + loginUserWithPassword(t, gitLDAPUsers[0].UserName, gitLDAPUsers[0].Password) + isMember, err = models.IsOrganizationMember(org.ID, user.ID) + assert.NoError(t, err) + assert.False(t, isMember, "User membership should have been removed from organization") + isMember, err = models.IsTeamMember(org.ID, team.ID, user.ID) + assert.NoError(t, err) + assert.False(t, isMember, "User membership should have been removed from team") +} + +// Login should work even if Team Group Map contains a broken JSON +func TestBrokenLDAPMapUserSignin(t *testing.T) { + if skipLDAPTests() { + t.Skip() + return + } + defer prepareTestEnv(t)() + addAuthSourceLDAP(t, "", "on", `{"NOT_A_VALID_JSON"["MISSING_DOUBLE_POINT"]}`) + + u := gitLDAPUsers[0] + + session := loginUserWithPassword(t, u.UserName, u.Password) + req := NewRequest(t, "GET", "/user/settings") + resp := session.MakeRequest(t, req, http.StatusOK) + + htmlDoc := NewHTMLParser(t, resp.Body) + + assert.Equal(t, u.UserName, htmlDoc.GetInputValueByName("name")) + assert.Equal(t, u.FullName, htmlDoc.GetInputValueByName("full_name")) + assert.Equal(t, u.Email, htmlDoc.Find(`label[for="email"]`).Siblings().First().Text()) +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 1c4313247871..722cade0eba6 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2581,11 +2581,13 @@ auths.filter = User Filter auths.admin_filter = Admin Filter auths.restricted_filter = Restricted Filter auths.restricted_filter_helper = Leave empty to not set any users as restricted. Use an asterisk ('*') to set all users that do not match Admin Filter as restricted. -auths.verify_group_membership = Verify group membership in LDAP +auths.verify_group_membership = Verify group membership in LDAP (leave the filter empty to skip) auths.group_search_base = Group Search Base DN -auths.valid_groups_filter = Valid Groups Filter auths.group_attribute_list_users = Group Attribute Containing List Of Users auths.user_attribute_in_group = User Attribute Listed In Group +auths.map_group_to_team = Map LDAP groups to Organization teams (leave the field empty to skip) +auths.map_group_to_team_removal = Remove users from synchronized teams if user does not belong to corresponding LDAP group +auths.enable_ldap_groups = Enable LDAP groups auths.ms_ad_sa = MS AD Search Attributes auths.smtp_auth = SMTP Authentication Type auths.smtphost = SMTP Host diff --git a/routers/web/admin/auths.go b/routers/web/admin/auths.go index 20f364739e12..748f2e7a8b8b 100644 --- a/routers/web/admin/auths.go +++ b/routers/web/admin/auths.go @@ -145,6 +145,8 @@ func parseLDAPConfig(form forms.AuthenticationForm) *ldap.Source { GroupDN: form.GroupDN, GroupFilter: form.GroupFilter, GroupMemberUID: form.GroupMemberUID, + GroupTeamMap: form.GroupTeamMap, + GroupTeamMapRemoval: form.GroupTeamMapRemoval, UserUID: form.UserUID, AdminFilter: form.AdminFilter, RestrictedFilter: form.RestrictedFilter, diff --git a/services/auth/source/ldap/README.md b/services/auth/source/ldap/README.md index 3a839fa3142a..59fc5cabad75 100644 --- a/services/auth/source/ldap/README.md +++ b/services/auth/source/ldap/README.md @@ -120,3 +120,11 @@ share the following fields: * Group Attribute for User (optional) * Which group LDAP attribute contains an array above user attribute names. * Example: memberUid + +* Team group map (optional) + * Automatically add users to Organization teams, depending on LDAP group memberships. + * Note: this function only adds users to teams, it never removes users. + * Example: {"cn=MyGroup,cn=groups,dc=example,dc=org": {"MyGiteaOrganization": ["MyGiteaTeam1", "MyGiteaTeam2", ...], ...}, ...} + +* Team group map removal (optional) + * If set to true, users will be removed from teams if they are not members of the corresponding group. diff --git a/services/auth/source/ldap/source.go b/services/auth/source/ldap/source.go index fc778b0114d7..ad97e2dd499b 100644 --- a/services/auth/source/ldap/source.go +++ b/services/auth/source/ldap/source.go @@ -52,6 +52,8 @@ type Source struct { GroupDN string // Group Search Base GroupFilter string // Group Name Filter GroupMemberUID string // Group Attribute containing array of UserUID + GroupTeamMap string // Map LDAP groups to teams + GroupTeamMapRemoval bool // Remove user from teams which are synchronized and user is not a member of the corresponding LDAP group UserUID string // User Attribute listed in Group SkipLocalTwoFA bool `json:",omitempty"` // Skip Local 2fa for users authenticated with this source diff --git a/services/auth/source/ldap/source_authenticate.go b/services/auth/source/ldap/source_authenticate.go index 52971bb87e58..e804e32e845d 100644 --- a/services/auth/source/ldap/source_authenticate.go +++ b/services/auth/source/ldap/source_authenticate.go @@ -8,6 +8,7 @@ import ( "fmt" "strings" + "code.gitea.io/gitea/models" asymkey_model "code.gitea.io/gitea/models/asymkey" "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" @@ -59,10 +60,14 @@ func (source *Source) Authenticate(user *user_model.User, userName, password str } if user != nil { + if source.GroupsEnabled && (source.GroupTeamMap != "" || source.GroupTeamMapRemoval) { + orgCache := make(map[string]*models.Organization) + teamCache := make(map[string]*models.Team) + source.SyncLdapGroupsToTeams(user, sr.LdapTeamAdd, sr.LdapTeamRemove, orgCache, teamCache) + } if isAttributeSSHPublicKeySet && asymkey_model.SynchronizePublicKeys(user, source.authSource, sr.SSHPublicKey) { return user, asymkey_model.RewriteAllPublicKeys() } - return user, nil } @@ -98,10 +103,14 @@ func (source *Source) Authenticate(user *user_model.User, userName, password str if isAttributeSSHPublicKeySet && asymkey_model.AddPublicKeysBySource(user, source.authSource, sr.SSHPublicKey) { err = asymkey_model.RewriteAllPublicKeys() } - if err == nil && len(source.AttributeAvatar) > 0 { _ = user_service.UploadAvatar(user, sr.Avatar) } + if source.GroupsEnabled && (source.GroupTeamMap != "" || source.GroupTeamMapRemoval) { + orgCache := make(map[string]*models.Organization) + teamCache := make(map[string]*models.Team) + source.SyncLdapGroupsToTeams(user, sr.LdapTeamAdd, sr.LdapTeamRemove, orgCache, teamCache) + } return user, err } diff --git a/services/auth/source/ldap/source_group_sync.go b/services/auth/source/ldap/source_group_sync.go new file mode 100644 index 000000000000..7c62af705e04 --- /dev/null +++ b/services/auth/source/ldap/source_group_sync.go @@ -0,0 +1,100 @@ +// Copyright 2021 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package ldap + +import ( + "code.gitea.io/gitea/models" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/log" +) + +// SyncLdapGroupsToTeams maps LDAP groups to organization and team memberships +func (source *Source) SyncLdapGroupsToTeams(user *user_model.User, ldapTeamAdd, ldapTeamRemove map[string][]string, orgCache map[string]*models.Organization, teamCache map[string]*models.Team) { + var err error + if source.GroupsEnabled && source.GroupTeamMapRemoval { + // when the user is not a member of configs LDAP group, remove mapped organizations/teams memberships + removeMappedMemberships(user, ldapTeamRemove, orgCache, teamCache) + } + for orgName, teamNames := range ldapTeamAdd { + org, ok := orgCache[orgName] + if !ok { + org, err = models.GetOrgByName(orgName) + if err != nil { + // organization must be created before LDAP group sync + log.Warn("LDAP group sync: Could not find organisation %s: %v", orgName, err) + continue + } + orgCache[orgName] = org + } + if isMember, err := models.IsOrganizationMember(org.ID, user.ID); !isMember && err == nil { + log.Trace("LDAP group sync: adding user [%s] to organization [%s]", user.Name, org.Name) + err = org.AddMember(user.ID) + if err != nil { + log.Error("LDAP group sync: Could not add user to organization: %v", err) + continue + } + } + for _, teamName := range teamNames { + team, ok := teamCache[orgName+teamName] + if !ok { + team, err = org.GetTeam(teamName) + if err != nil { + // team must be created before LDAP group sync + log.Warn("LDAP group sync: Could not find team %s: %v", teamName, err) + continue + } + teamCache[orgName+teamName] = team + } + if isMember, err := models.IsTeamMember(org.ID, team.ID, user.ID); !isMember && err == nil { + log.Trace("LDAP group sync: adding user [%s] to team [%s]", user.Name, org.Name) + } else { + continue + } + err := team.AddMember(user.ID) + if err != nil { + log.Error("LDAP group sync: Could not add user to team: %v", err) + } + } + } +} + +// remove membership to organizations/teams if user is not member of corresponding LDAP group +// e.g. lets assume user is member of LDAP group "x", but LDAP group team map contains LDAP groups "x" and "y" +// then users membership gets removed for all organizations/teams mapped by LDAP group "y" +func removeMappedMemberships(user *user_model.User, ldapTeamRemove map[string][]string, orgCache map[string]*models.Organization, teamCache map[string]*models.Team) { + var err error + for orgName, teamNames := range ldapTeamRemove { + org, ok := orgCache[orgName] + if !ok { + org, err = models.GetOrgByName(orgName) + if err != nil { + // organization must be created before LDAP group sync + log.Warn("LDAP group sync: Could not find organisation %s: %v", orgName, err) + continue + } + orgCache[orgName] = org + } + for _, teamName := range teamNames { + team, ok := teamCache[orgName+teamName] + if !ok { + team, err = org.GetTeam(teamName) + if err != nil { + // team must must be created before LDAP group sync + log.Warn("LDAP group sync: Could not find team %s: %v", teamName, err) + continue + } + } + if isMember, err := models.IsTeamMember(org.ID, team.ID, user.ID); isMember && err == nil { + log.Trace("LDAP group sync: removing user [%s] from team [%s]", user.Name, org.Name) + } else { + continue + } + err = team.RemoveMember(user.ID) + if err != nil { + log.Error("LDAP group sync: Could not remove user from team: %v", err) + } + } + } +} diff --git a/services/auth/source/ldap/source_search.go b/services/auth/source/ldap/source_search.go index 1f1cca270d40..f2b940cabe02 100644 --- a/services/auth/source/ldap/source_search.go +++ b/services/auth/source/ldap/source_search.go @@ -12,22 +12,26 @@ import ( "strconv" "strings" + "code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/util" "github.com/go-ldap/ldap/v3" ) // SearchResult : user data type SearchResult struct { - Username string // Username - Name string // Name - Surname string // Surname - Mail string // E-mail address - SSHPublicKey []string // SSH Public Key - IsAdmin bool // if user is administrator - IsRestricted bool // if user is restricted - LowerName string // Lowername - Avatar []byte + Username string // Username + Name string // Name + Surname string // Surname + Mail string // E-mail address + SSHPublicKey []string // SSH Public Key + IsAdmin bool // if user is administrator + IsRestricted bool // if user is restricted + LowerName string // LowerName + Avatar []byte + LdapTeamAdd map[string][]string // organizations teams to add + LdapTeamRemove map[string][]string // organizations teams to remove } func (ls *Source) sanitizedUserQuery(username string) (string, bool) { @@ -192,6 +196,71 @@ func checkRestricted(l *ldap.Conn, ls *Source, userDN string) bool { return false } +// List all group memberships of a user +func (ls *Source) listLdapGroupMemberships(l *ldap.Conn, uid string) []string { + var ldapGroups []string + groupFilter := fmt.Sprintf("(%s=%s)", ls.GroupMemberUID, uid) + result, err := l.Search(ldap.NewSearchRequest( + ls.GroupDN, + ldap.ScopeWholeSubtree, + ldap.NeverDerefAliases, + 0, + 0, + false, + groupFilter, + []string{}, + nil, + )) + if err != nil { + log.Error("Failed group search using filter[%s]: %v", groupFilter, err) + return ldapGroups + } + + for _, entry := range result.Entries { + if entry.DN == "" { + log.Error("LDAP search was successful, but found no DN!") + continue + } + ldapGroups = append(ldapGroups, entry.DN) + } + + return ldapGroups +} + +// parse LDAP groups and return map of ldap groups to organizations teams +func (ls *Source) mapLdapGroupsToTeams() map[string]map[string][]string { + ldapGroupsToTeams := make(map[string]map[string][]string) + err := json.Unmarshal([]byte(ls.GroupTeamMap), &ldapGroupsToTeams) + if err != nil { + log.Error("Failed to unmarshall LDAP teams map: %v", err) + return ldapGroupsToTeams + } + return ldapGroupsToTeams +} + +// getMappedMemberships : returns the organizations and teams to modify the users membership +func (ls *Source) getMappedMemberships(l *ldap.Conn, uid string) (map[string][]string, map[string][]string) { + // get all LDAP group memberships for user + usersLdapGroups := ls.listLdapGroupMemberships(l, uid) + // unmarshall LDAP group team map from configs + ldapGroupsToTeams := ls.mapLdapGroupsToTeams() + membershipsToAdd := map[string][]string{} + membershipsToRemove := map[string][]string{} + for group, memberships := range ldapGroupsToTeams { + isUserInGroup := util.IsStringInSlice(group, usersLdapGroups) + if isUserInGroup { + for org, teams := range memberships { + membershipsToAdd[org] = teams + } + } else if !isUserInGroup { + for org, teams := range memberships { + membershipsToRemove[org] = teams + } + } + } + return membershipsToAdd, membershipsToRemove +} + // SearchEntry : search an LDAP source if an entry (name, passwd) is valid and in the specific filter func (ls *Source) SearchEntry(name, passwd string, directBind bool) *SearchResult { // See https://tools.ietf.org/search/rfc4513#section-5.1.2 @@ -308,9 +377,12 @@ func (ls *Source) SearchEntry(name, passwd string, directBind bool) *SearchResul surname := sr.Entries[0].GetAttributeValue(ls.AttributeSurname) mail := sr.Entries[0].GetAttributeValue(ls.AttributeMail) uid := sr.Entries[0].GetAttributeValue(ls.UserUID) + if ls.UserUID == "dn" || ls.UserUID == "DN" { + uid = sr.Entries[0].DN + } // Check group membership - if ls.GroupsEnabled { + if ls.GroupsEnabled && ls.GroupFilter != "" { groupFilter, ok := ls.sanitizedGroupFilter(ls.GroupFilter) if !ok { return nil @@ -373,16 +445,24 @@ func (ls *Source) SearchEntry(name, passwd string, directBind bool) *SearchResul Avatar = sr.Entries[0].GetRawAttributeValue(ls.AttributeAvatar) } + teamsToAdd := make(map[string][]string) + teamsToRemove := make(map[string][]string) + if ls.GroupsEnabled && (ls.GroupTeamMap != "" || ls.GroupTeamMapRemoval) { + teamsToAdd, teamsToRemove = ls.getMappedMemberships(l, uid) + } + return &SearchResult{ - LowerName: strings.ToLower(username), - Username: username, - Name: firstname, - Surname: surname, - Mail: mail, - SSHPublicKey: sshPublicKey, - IsAdmin: isAdmin, - IsRestricted: isRestricted, - Avatar: Avatar, + LowerName: strings.ToLower(username), + Username: username, + Name: firstname, + Surname: surname, + Mail: mail, + SSHPublicKey: sshPublicKey, + IsAdmin: isAdmin, + IsRestricted: isRestricted, + Avatar: Avatar, + LdapTeamAdd: teamsToAdd, + LdapTeamRemove: teamsToRemove, } } @@ -417,7 +497,7 @@ func (ls *Source) SearchEntries() ([]*SearchResult, error) { isAttributeSSHPublicKeySet := len(strings.TrimSpace(ls.AttributeSSHPublicKey)) > 0 isAtributeAvatarSet := len(strings.TrimSpace(ls.AttributeAvatar)) > 0 - attribs := []string{ls.AttributeUsername, ls.AttributeName, ls.AttributeSurname, ls.AttributeMail} + attribs := []string{ls.AttributeUsername, ls.AttributeName, ls.AttributeSurname, ls.AttributeMail, ls.UserUID} if isAttributeSSHPublicKeySet { attribs = append(attribs, ls.AttributeSSHPublicKey) } @@ -444,12 +524,23 @@ func (ls *Source) SearchEntries() ([]*SearchResult, error) { result := make([]*SearchResult, len(sr.Entries)) for i, v := range sr.Entries { + teamsToAdd := make(map[string][]string) + teamsToRemove := make(map[string][]string) + if ls.GroupsEnabled && (ls.GroupTeamMap != "" || ls.GroupTeamMapRemoval) { + userAttributeListedInGroup := v.GetAttributeValue(ls.UserUID) + if ls.UserUID == "dn" || ls.UserUID == "DN" { + userAttributeListedInGroup = v.DN + } + teamsToAdd, teamsToRemove = ls.getMappedMemberships(l, userAttributeListedInGroup) + } result[i] = &SearchResult{ - Username: v.GetAttributeValue(ls.AttributeUsername), - Name: v.GetAttributeValue(ls.AttributeName), - Surname: v.GetAttributeValue(ls.AttributeSurname), - Mail: v.GetAttributeValue(ls.AttributeMail), - IsAdmin: checkAdmin(l, ls, v.DN), + Username: v.GetAttributeValue(ls.AttributeUsername), + Name: v.GetAttributeValue(ls.AttributeName), + Surname: v.GetAttributeValue(ls.AttributeSurname), + Mail: v.GetAttributeValue(ls.AttributeMail), + IsAdmin: checkAdmin(l, ls, v.DN), + LdapTeamAdd: teamsToAdd, + LdapTeamRemove: teamsToRemove, } if !result[i].IsAdmin { result[i].IsRestricted = checkRestricted(l, ls, v.DN) diff --git a/services/auth/source/ldap/source_sync.go b/services/auth/source/ldap/source_sync.go index 398d9ef79883..74a62ce4e72d 100644 --- a/services/auth/source/ldap/source_sync.go +++ b/services/auth/source/ldap/source_sync.go @@ -10,6 +10,7 @@ import ( "sort" "strings" + "code.gitea.io/gitea/models" asymkey_model "code.gitea.io/gitea/models/asymkey" "code.gitea.io/gitea/models/db" user_model "code.gitea.io/gitea/models/user" @@ -61,6 +62,8 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { }) userPos := 0 + orgCache := make(map[string]*models.Organization) + teamCache := make(map[string]*models.Team) for _, su := range sr { select { @@ -166,6 +169,10 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { } } } + // Synchronize LDAP groups with organization and team memberships + if source.GroupsEnabled && (source.GroupTeamMap != "" || source.GroupTeamMapRemoval) { + source.SyncLdapGroupsToTeams(usr, su.LdapTeamAdd, su.LdapTeamRemove, orgCache, teamCache) + } } // Rewrite authorized_keys file if LDAP Public SSH Key attribute is set and any key was added or removed diff --git a/services/forms/auth_form.go b/services/forms/auth_form.go index d0962926014b..7e7c75675299 100644 --- a/services/forms/auth_form.go +++ b/services/forms/auth_form.go @@ -79,6 +79,8 @@ type AuthenticationForm struct { SSPIStripDomainNames bool SSPISeparatorReplacement string `binding:"AlphaDashDot;MaxSize(5)"` SSPIDefaultLanguage string + GroupTeamMap string + GroupTeamMapRemoval bool } // Validate validates fields diff --git a/templates/admin/auth/edit.tmpl b/templates/admin/auth/edit.tmpl index efa440ff33ef..31c87597f055 100644 --- a/templates/admin/auth/edit.tmpl +++ b/templates/admin/auth/edit.tmpl @@ -108,31 +108,43 @@ + + +
- - + +
-
+
+
+ + +
- - + +
- - + +
- - + +
- - + + +
+
+ +
-
+ + {{if .Source.IsLDAP}}
diff --git a/templates/admin/auth/source/ldap.tmpl b/templates/admin/auth/source/ldap.tmpl index 9ea0fdf8c060..afdfbadd6518 100644 --- a/templates/admin/auth/source/ldap.tmpl +++ b/templates/admin/auth/source/ldap.tmpl @@ -79,31 +79,42 @@
+ +
- - + +
-
+
- - + +
- - + +
- - + +
- - + + +
+
+ + +
+
+ +
-
+ +
diff --git a/web_src/js/features/admin-common.js b/web_src/js/features/admin-common.js index d2021c45aa27..2438fcf62b46 100644 --- a/web_src/js/features/admin-common.js +++ b/web_src/js/features/admin-common.js @@ -91,12 +91,8 @@ export function initAdminCommon() { } } - function onVerifyGroupMembershipChange() { - if ($('#groups_enabled').is(':checked')) { - $('#groups_enabled_change').show(); - } else { - $('#groups_enabled_change').hide(); - } + function onEnableLdapGroupsChange() { + $('#ldap-group-options').toggle($('.js-ldap-group-toggle').is(':checked')); } // New authentication @@ -139,7 +135,7 @@ export function initAdminCommon() { } if (authType === '2' || authType === '5') { onSecurityProtocolChange(); - onVerifyGroupMembershipChange(); + onEnableLdapGroupsChange(); } if (authType === '2') { onUsePagedSearchChange(); @@ -150,15 +146,15 @@ export function initAdminCommon() { $('#use_paged_search').on('change', onUsePagedSearchChange); $('#oauth2_provider').on('change', () => onOAuth2Change(true)); $('#oauth2_use_custom_url').on('change', () => onOAuth2UseCustomURLChange(true)); - $('#groups_enabled').on('change', onVerifyGroupMembershipChange); + $('.js-ldap-group-toggle').on('change', onEnableLdapGroupsChange); } // Edit authentication if ($('.admin.edit.authentication').length > 0) { const authType = $('#auth_type').val(); if (authType === '2' || authType === '5') { $('#security_protocol').on('change', onSecurityProtocolChange); - $('#groups_enabled').on('change', onVerifyGroupMembershipChange); - onVerifyGroupMembershipChange(); + $('.js-ldap-group-toggle').on('change', onEnableLdapGroupsChange); + onEnableLdapGroupsChange(); if (authType === '2') { $('#use_paged_search').on('change', onUsePagedSearchChange); } From c86ecaebae5d4db60edaf3ffdc0ee8ce40707e67 Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 11 Feb 2022 15:29:58 +0000 Subject: [PATCH 03/11] Separate the details links of commit-statuses in headers (#18661) --- templates/repo/commit_statuses.tmpl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/templates/repo/commit_statuses.tmpl b/templates/repo/commit_statuses.tmpl index d2e9f0bd16d9..f33635abff13 100644 --- a/templates/repo/commit_statuses.tmpl +++ b/templates/repo/commit_statuses.tmpl @@ -2,11 +2,11 @@