Repository: repobee/repobee

Issue:
Using git clone poses a security risk
As it writes the token to every single git repo that is cloned (see https://github.blog/2012-09-21-easier-builds-and-deployments-using-git-over-https-and-oauth/).

Should use git pull instead.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/repobee/repobee/pull/176?src=pr&el=h1) Report
> Merging [#176](https://codecov.io/gh/repobee/repobee/pull/176?src=pr&el=desc) into [master](https://codecov.io/gh/repobee/repobee/commit/abc907306dded023c522f5d7d314dfe4e08fc160?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/repobee/repobee/pull/176/graphs/tree.svg?width=650&token=5vOKgkphZe&height=150&src=pr)](https://codecov.io/gh/repobee/repobee/pull/176?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
- Coverage   92.83%   92.81%   -0.02%     
==========================================
  Files          17       17              
  Lines        1116     1128      +12     
  Branches      200      197       -3     
==========================================
+ Hits         1036     1047      +11     
- Misses         65       67       +2     
+ Partials       15       14       -1
```


| [Impacted Files](https://codecov.io/gh/repobee/repobee/pull/176?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [repobee/\_\_version.py](https://codecov.io/gh/repobee/repobee/pull/176/diff?src=pr&el=tree#diff-cmVwb2JlZS9fX3ZlcnNpb24ucHk=) | `100% <100%> (ø)` | :arrow_up: |
| [repobee/git.py](https://codecov.io/gh/repobee/repobee/pull/176/diff?src=pr&el=tree#diff-cmVwb2JlZS9naXQucHk=) | `97.72% <100%> (+1.67%)` | :arrow_up: |
| [repobee/cli.py](https://codecov.io/gh/repobee/repobee/pull/176/diff?src=pr&el=tree#diff-cmVwb2JlZS9jbGkucHk=) | `97.37% <0%> (-0.88%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/repobee/repobee/pull/176?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/repobee/repobee/pull/176?src=pr&el=footer). Last update [abc9073...f1b601d](https://codecov.io/gh/repobee/repobee/pull/176?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/docs/clone.rst b/docs/clone.rst
index ec5f924..7444888 100644
--- a/docs/clone.rst
+++ b/docs/clone.rst
@@ -1,7 +1,7 @@
 Cloning Repos in Bulk (the ``clone`` command)
 *********************************************
 It can at times be beneficial to be able to clone a bunch of student repos
-at the same time. It could for example be prudent to do this slighly after
+at the same time. It could for example be prudent to do this slightly after
 a deadline, as timestamps in a ``git`` commit can easily be altered (and are
 therefore not particularly trustworthy). Whatever your reason may be, it's
 very simple using the ``clone`` command. Again, assume that we have the
@@ -24,3 +24,13 @@ should now be in your current working directory. There is also a possibility to
 run automated tasks on cloned repos, such as running test suites or linters. If
 you're not satisfied with the tasks on offer, you can define your own. Read more
 about it in the :ref:`plugins` section.
+
+.. note::
+
+   `For security reasons
+   <https://github.blog/2012-09-21-easier-builds-and-deployments-using-git-over-https-and-oauth/>`_,
+   RepoBee doesn't actually use ``git clone`` to clone repositories. Instead,
+   RepoBee clones by initializing the repository and running ``git pull``. The
+   practical implication is that you can't simply enter a repository that's
+   been cloned with RepoBee and run ``git pull`` to fetch updates, as there
+   will be no remote set. Run ``repobee clone`` again instead.
diff --git a/docs/conf.py b/docs/conf.py
index 1ae05f7..a537a40 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -72,7 +72,7 @@ author = "Simon Larsén"
 # The short X.Y version.
 version = "1.3"
 # The full version, including alpha/beta/rc tags.
-release = "1.3.0"
+release = "1.3.2"
 
 # The language for content autogenerated by Sphinx. Refer to documentation
 # for a list of supported languages.
diff --git a/pyproject.toml b/pyproject.toml
index a8f43fe..f2f60f2 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,2 +1,3 @@
 [tool.black]
 line-length = 79
+target-version = ['py34']
diff --git a/repobee/__version.py b/repobee/__version.py
index 9c73af2..f708a9b 100644
--- a/repobee/__version.py
+++ b/repobee/__version.py
@@ -1,1 +1,1 @@
-__version__ = "1.3.1"
+__version__ = "1.3.2"
diff --git a/repobee/git.py b/repobee/git.py
index b416db5..bafb749 100644
--- a/repobee/git.py
+++ b/repobee/git.py
@@ -9,10 +9,12 @@ import asyncio
 import os
 import subprocess
 import collections
+import pathlib
 import daiquiri
 from typing import Iterable, List, Any, Callable
 
 from repobee import exception
+from repobee import util
 
 CONCURRENT_TASKS = 20
 
@@ -21,6 +23,56 @@ LOGGER = daiquiri.getLogger(__file__)
 Push = collections.namedtuple("Push", ("local_path", "repo_url", "branch"))
 
 
+def _ensure_repo_dir_exists(repo_url: str, cwd: str) -> pathlib.Path:
+    """Checks if a dir for the repo url exists, and if it does not, creates it.
+    Also initializez (or reinitializes, if it alrady exists) as a git repo.
+    """
+    repo_name = util.repo_name(repo_url)
+    dirpath = pathlib.Path(cwd) / repo_name
+    if not dirpath.exists():
+        dirpath.mkdir()
+    _git_init(dirpath)
+    return dirpath
+
+
+def _git_init(dirpath):
+    captured_run(["git", "init"], cwd=str(dirpath))
+
+
+def _pull_clone(repo_url: str, token: str, branch: str = "", cwd: str = "."):
+    """Simulate a clone with a pull to avoid writing the token to disk."""
+    dirpath = _ensure_repo_dir_exists(repo_url, cwd)
+
+    url_with_token = _insert_token(repo_url, token)
+    pull_command = (
+        "git pull {} {}".format(url_with_token, branch).strip().split()
+    )
+
+    rc, _, stderr = captured_run(pull_command, cwd=str(dirpath))
+    return rc, stderr
+
+
+async def _pull_clone_async(
+    repo_url: str, token: str, branch: str = "", cwd: str = "."
+):
+    """Same as _pull_clone, but asynchronously."""
+    dirpath = _ensure_repo_dir_exists(repo_url, cwd)
+
+    url_with_token = _insert_token(repo_url, token)
+    pull_command = (
+        "git pull {} {}".format(url_with_token, branch).strip().split()
+    )
+
+    proc = await asyncio.create_subprocess_exec(
+        *pull_command,
+        cwd=str(dirpath),
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE
+    )
+    _, stderr = await proc.communicate()
+    return proc.returncode, stderr
+
+
 def _insert_token(url: str, token: str) -> str:
     """Insert a token into the url as described here:
         https://blog.github.com/2012-09-21-easier-builds-and-deployments-using-git-over-https-and-oauth/
@@ -61,69 +113,38 @@ def captured_run(*args, **kwargs):
     return proc.returncode, proc.stdout, proc.stderr
 
 
-def clone_single(
-    repo_url: str,
-    token: str,
-    single_branch: bool = True,
-    branch: str = None,
-    cwd: str = ".",
-):
+def clone_single(repo_url: str, token: str, branch: str = "", cwd: str = "."):
     """Clone a git repository.
 
     Args:
         repo_url: HTTPS url to repository on the form
             https://<host>/<owner>/<repo>.
-        single_branch: Whether or not to clone a single branch.
         branch: The branch to clone.
         cwd: Working directory. Defaults to the current directory.
         token: A GitHub OAUTH token.
     """
-    if isinstance(branch, str) and not branch:
-        raise ValueError("branch must not be empty")
-
-    options = []
-    if single_branch:
-        options.append("--single-branch")
-    if branch is not None:
-        options += ["-b", branch]
-
-    clone_command = ["git", "clone", _insert_token(repo_url, token), *options]
-    rc, _, stderr = captured_run(clone_command, cwd=cwd)
-
+    rc, stderr = _pull_clone(repo_url, token, branch, cwd)
     if rc != 0:
         raise exception.CloneFailedError(
             "Failed to clone", rc, stderr, repo_url
         )
 
 
-async def _clone_async(
-    repo_url: str,
-    token: str,
-    single_branch: bool = True,
-    branch: str = None,
-    cwd=".",
-):
+async def _clone_async(repo_url: str, token: str, branch: str = "", cwd="."):
     """Clone git repositories asynchronously.
 
     Args:
         repo_url: A url to clone.
-        single_branch: Whether to clone a single branch or not.
         branch: Which branch to clone.
         cwd: Working directory.
         token: A GitHub OAUTH token.
     """
-    command = ["git", "clone", _insert_token(repo_url, token)]
-    if single_branch:
-        command.append("--single-branch")
-    proc = await asyncio.create_subprocess_exec(
-        *command, cwd=cwd, stdout=subprocess.PIPE, stderr=subprocess.PIPE
-    )
-    _, stderr = await proc.communicate()
+    rc, stderr = await _pull_clone_async(repo_url, token, branch, cwd)
 
-    if proc.returncode != 0:
+    if rc != 0:
         raise exception.CloneFailedError(
             "Failed to clone {}".format(repo_url),
-            returncode=proc.returncode,
+            returncode=rc,
             stderr=stderr,
             url=repo_url,
         )
@@ -132,16 +153,12 @@ async def _clone_async(
 
 
 def clone(
-    repo_urls: Iterable[str],
-    token: str,
-    single_branch: bool = True,
-    cwd: str = ".",
+    repo_urls: Iterable[str], token: str, cwd: str = "."
 ) -> List[Exception]:
     """Clone all repos asynchronously.
 
     Args:
         repo_urls: URLs to repos to clone.
-        single_branch: Whether or not to clone only the default branch.
         cwd: Working directory. Defaults to the current directory.
         token: A GitHub OAUTH token.
 
@@ -151,9 +168,7 @@ def clone(
     # TODO valdate repo_urls
     return [
         exc.url
-        for exc in _batch_execution(
-            _clone_async, repo_urls, token, single_branch, cwd=cwd
-        )
+        for exc in _batch_execution(_clone_async, repo_urls, token, cwd=cwd)
         if isinstance(exc, exception.CloneFailedError)
     ]
 


Test patch:
diff --git a/tests/conftest.py b/tests/conftest.py
index ed4bdc9..ccaea1d 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -71,7 +71,9 @@ def students_file(empty_students_file):
     """A fixture with a temporary file containt the students in
     constants.STUDENTS.
     """
-    empty_students_file.write(os.linesep.join([str(s) for s in constants.STUDENTS]))
+    empty_students_file.write(
+        os.linesep.join([str(s) for s in constants.STUDENTS])
+    )
     yield empty_students_file
 
 
diff --git a/tests/test_git.py b/tests/test_git.py
index 166db57..7d849dd 100644
--- a/tests/test_git.py
+++ b/tests/test_git.py
@@ -1,17 +1,21 @@
 import os
 import subprocess
+from subprocess import run
 from unittest.mock import call
 from collections import namedtuple
+import pathlib
 
 import pytest
 
 from repobee import git
 from repobee import exception
+from repobee import util
 
 import constants
 from constants import TOKEN
 
 URL_TEMPLATE = "https://{}github.com/slarse/clanim"
+REPO_NAME = "clanim"
 USER = constants.USER
 
 Env = namedtuple("Env", ("expected_url", "expected_url_with_username"))
@@ -20,6 +24,16 @@ RunTuple = namedtuple("RunTuple", ("returncode", "stdout", "stderr"))
 AioSubproc = namedtuple("AioSubproc", ("create_subprocess", "process"))
 
 
+@pytest.fixture(autouse=True)
+def mock_ensure_repo_dir_exists(mocker, request):
+    """Mocked out to not accidentally create directories all over the place."""
+    if "no_ensure_repo_dir_mock" in request.keywords:
+        return
+
+    mocker.patch("pathlib.Path.mkdir", autospec=True)
+    mocker.patch("repobee.git._git_init", autospec=True)
+
+
 @pytest.fixture(scope="function")
 def env_setup(mocker):
     mocker.patch(
@@ -110,15 +124,7 @@ def test_insert_empty_token_raises():
     assert "empty token" in str(exc)
 
 
-def test_clone_single_raises_on_empty_branch(env_setup):
-    with pytest.raises(ValueError) as exc:
-        git.clone_single(URL_TEMPLATE.format(""), TOKEN, branch="")
-    assert "branch must not be empty" in str(exc)
-
-
-def test_clone_single_raises_on_non_zero_exit_from_git_clone(
-    env_setup, mocker
-):
+def test_clone_single_raises_on_non_zero_exit_from_git_pull(env_setup, mocker):
     stderr = b"This is pretty bad!"
     # already patched in env_setup fixture
     subprocess.run.return_value = RunTuple(1, "", stderr)
@@ -129,45 +135,28 @@ def test_clone_single_raises_on_non_zero_exit_from_git_clone(
 
 
 def test_clone_single_issues_correct_command_with_defaults(env_setup):
-    expected_command = "git clone {} --single-branch".format(
-        env_setup.expected_url
-    ).split()
+    expected_command = "git pull {}".format(env_setup.expected_url).split()
 
     git.clone_single(URL_TEMPLATE.format(""), TOKEN)
-    subprocess.run.assert_called_once_with(
+    subprocess.run.assert_any_call(
         expected_command,
-        cwd=".",
+        cwd=str(pathlib.Path(".") / REPO_NAME),
         stderr=subprocess.PIPE,
         stdout=subprocess.PIPE,
     )
 
 
-def test_clone_single_issues_correct_command_without_single_branch(env_setup):
-    expected_command = "git clone {}".format(env_setup.expected_url).split()
-
-    git.clone_single(URL_TEMPLATE.format(""), TOKEN, single_branch=False)
-    subprocess.run.assert_called_once_with(
-        expected_command,
-        cwd=".",
-        stderr=subprocess.PIPE,
-        stdout=subprocess.PIPE,
-    )
-
-
-def test_clone_single_issues_correct_command_with_single_other_branch(
-    env_setup
-):
+def test_clone_single_issues_correct_command_non_default_branch(env_setup):
     branch = "other-branch"
-    expected_command = "git clone {} --single-branch -b {}".format(
+    expected_command = "git pull {} {}".format(
         env_setup.expected_url, branch
     ).split()
 
-    git.clone_single(
-        URL_TEMPLATE.format(""), TOKEN, single_branch=True, branch=branch
-    )
-    subprocess.run.assert_called_once_with(
+    git.clone_single(URL_TEMPLATE.format(""), TOKEN, branch=branch)
+
+    subprocess.run.assert_any_call(
         expected_command,
-        cwd=".",
+        cwd=str(pathlib.Path(".") / REPO_NAME),
         stderr=subprocess.PIPE,
         stdout=subprocess.PIPE,
     )
@@ -176,20 +165,16 @@ def test_clone_single_issues_correct_command_with_single_other_branch(
 def test_clone_single_issues_correct_command_with_cwd(env_setup):
     working_dir = "some/working/dir"
     branch = "other-branch"
-    expected_command = "git clone {} --single-branch -b {}".format(
+    expected_command = "git pull {} {}".format(
         env_setup.expected_url, branch
     ).split()
 
     git.clone_single(
-        URL_TEMPLATE.format(""),
-        TOKEN,
-        single_branch=True,
-        branch=branch,
-        cwd=working_dir,
+        URL_TEMPLATE.format(""), TOKEN, branch=branch, cwd=working_dir
     )
     subprocess.run.assert_called_once_with(
         expected_command,
-        cwd=working_dir,
+        cwd=str(pathlib.Path(working_dir) / REPO_NAME),
         stderr=subprocess.PIPE,
         stdout=subprocess.PIPE,
     )
@@ -309,15 +294,13 @@ class TestClone:
     """Tests for clone."""
 
     @pytest.mark.parametrize("token", (TOKEN, "something-else-entirely"))
-    def test_happy_path(env_setup, push_tuples, aio_subproc, token):
+    def test_happy_path(self, env_setup, push_tuples, aio_subproc, token):
         urls = [pt.repo_url for pt in push_tuples]
         working_dir = "some/working/dir"
         expected_subproc_calls = [
             call(
-                *"git clone {} --single-branch".format(
-                    git._insert_token(url, token)
-                ).split(),
-                cwd=working_dir,
+                *"git pull {}".format(git._insert_token(url, token)).split(),
+                cwd=str(pathlib.Path(working_dir) / util.repo_name(url)),
                 stdout=subprocess.PIPE,
                 stderr=subprocess.PIPE
             )
@@ -330,12 +313,12 @@ class TestClone:
         aio_subproc.create_subprocess.assert_has_calls(expected_subproc_calls)
 
     def test_tries_all_calls_despite_exceptions(
-        env_setup, push_tuples, mocker
+        self, env_setup, push_tuples, mocker
     ):
         urls = [pt.repo_url for pt in push_tuples]
         fail_urls = [urls[0], urls[-1]]
 
-        expected_calls = [call(url, TOKEN, True, cwd=".") for url in urls]
+        expected_calls = [call(url, TOKEN, cwd=".") for url in urls]
 
         async def raise_(repo_url, *args, **kwargs):
             if repo_url in fail_urls:
@@ -356,7 +339,7 @@ class TestClone:
         clone_mock.assert_has_calls(expected_calls)
 
     def test_tries_all_calls_despite_exceptions_lower_level(
-        env_setup, push_tuples, mocker, non_zero_aio_subproc
+        self, env_setup, push_tuples, mocker, non_zero_aio_subproc
     ):
         """Same test as test_tries_all_calls_desipite_exception, but
         asyncio.create_subprocess_exec is mocked out instead of
@@ -366,10 +349,8 @@ class TestClone:
 
         expected_calls = [
             call(
-                *"git clone {} --single-branch".format(
-                    git._insert_token(url, TOKEN)
-                ).split(),
-                cwd=".",
+                *"git pull {}".format(git._insert_token(url, TOKEN)).split(),
+                cwd=str(pathlib.Path(".") / util.repo_name(url)),
                 stdout=subprocess.PIPE,
                 stderr=subprocess.PIPE
             )
@@ -380,3 +361,39 @@ class TestClone:
         non_zero_aio_subproc.create_subprocess.assert_has_calls(expected_calls)
 
         assert failed_urls == urls
+
+
+class TestEnsureRepoDirExists:
+    @pytest.mark.no_ensure_repo_dir_mock
+    def test_functions_when_dir_does_not_exist(self, tmpdir):
+        expected_git_repo = tmpdir.join(REPO_NAME)
+        assert (
+            not expected_git_repo.check()
+        ), "directory should not exist before test"
+
+        git._ensure_repo_dir_exists(URL_TEMPLATE.format(""), cwd=str(tmpdir))
+
+        assert util.is_git_repo(str(expected_git_repo))
+
+    @pytest.mark.no_ensure_repo_dir_mock
+    def test_functions_when_dir_exists(self, tmpdir):
+        expected_git_repo = tmpdir.join(REPO_NAME).mkdir()
+        assert expected_git_repo.check(
+            dir=1
+        ), "directory should exist before test"
+
+        git._ensure_repo_dir_exists(URL_TEMPLATE.format(""), cwd=str(tmpdir))
+
+        assert util.is_git_repo(str(expected_git_repo))
+
+    @pytest.mark.no_ensure_repo_dir_mock
+    def test_functions_when_git_repo_exists(self, tmpdir):
+        expected_git_repo = tmpdir.join(REPO_NAME).mkdir()
+        run(["git", "init"], cwd=str(expected_git_repo))
+        assert util.is_git_repo(
+            str(expected_git_repo)
+        ), "directory should be a git repo before test"
+
+        git._ensure_repo_dir_exists(URL_TEMPLATE.format(""), cwd=str(tmpdir))
+
+        assert util.is_git_repo(str(expected_git_repo))