launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code



Diff comments:

> === modified file 'turnip/api/store.py'
> --- turnip/api/store.py	2015-02-26 21:34:02 +0000
> +++ turnip/api/store.py	2015-03-10 01:19:14 +0000
> @@ -3,29 +3,68 @@
>  import os
>  import shutil
>  
> -import pygit2
> -
> -
> -class Store(object):
> -    """Provides methods for manipulating repos on disk with pygit2."""
> -
> -    @staticmethod
> -    def init(repo, is_bare=True):
> -        """Initialise a git repo with pygit2."""
> -        if os.path.exists(repo):
> -            raise Exception("Repository '%s' already exists" % repo)
> -        try:
> -            repo_path = pygit2.init_repository(repo, is_bare)
> -        except pygit2.GitError:
> -            print('Unable to create repository in {}.'.format(repo))
> -            raise
> -        return repo_path
> -
> -    @staticmethod
> -    def delete(repo):
> -        """Permanently delete a git repository from repo store."""
> -        try:
> -            shutil.rmtree(repo)
> -        except (IOError, OSError):
> -            print('Unable to delete repository from {}.'.format(repo))
> -            raise
> +from pygit2 import (
> +    GIT_OBJ_BLOB,
> +    GIT_OBJ_COMMIT,
> +    GIT_OBJ_TREE,
> +    GIT_OBJ_TAG,
> +    init_repository,
> +    Repository,
> +)
> +
> +
> +REF_TYPE_NAME = {GIT_OBJ_COMMIT: 'commit',
> +                 GIT_OBJ_TREE: 'tree',
> +                 GIT_OBJ_BLOB: 'blob',
> +                 GIT_OBJ_TAG: 'tag'}
> +
> +
> +def format_refs(ref, git_object):

This just formats a single ref, not multiple.

> +    return {
> +        ref: {
> +            "object": {'sha1': git_object.oid.hex,
> +                       'type': REF_TYPE_NAME[git_object.type]}
> +        }
> +    }

We usually format dicts/lists/etc. using the first option from PEP 8:

"""
The closing brace/bracket/parenthesis on multi-line constructs may either line up under the first non-whitespace character of the last line of list, as in:

my_list = [
    1, 2, 3,
    4, 5, 6,
    ]
"""

> +
> +
> +def init_repo(repo, is_bare=True):
> +    """Initialise a git repository."""
> +    if os.path.exists(repo):
> +        raise Exception("Repository '%s' already exists" % repo)
> +    repo_path = init_repository(repo, is_bare)
> +    return repo_path
> +
> +
> +def open_repo(repo_path):
> +    """Open an existing git repository."""
> +    repo = Repository(repo_path)
> +    return repo
> +
> +
> +def delete_repo(repo):
> +    """Permanently delete a git repository from repo store."""
> +    shutil.rmtree(repo)
> +
> +
> +def get_refs(repo_path):
> +    """Return all refs for a git repository."""
> +    repo = open_repo(repo_path)
> +    refs = {}
> +    for ref in repo.listall_references():
> +        git_object = repo.lookup_reference(ref).peel()
> +        try:
> +            ref.decode('utf-8')
> +        except UnicodeDecodeError:

I'd add a comment here with rationale. It seems like very weird error handling otherwise!

> +            pass
> +        else:
> +            refs.update(format_refs(ref, git_object))
> +    return refs
> +
> +
> +def get_ref(repo_path, ref):
> +    """Return a specific ref for a git repository."""
> +    repo = open_repo(repo_path)
> +    git_object = repo.lookup_reference(ref).peel()
> +    ref_obj = format_refs(ref, git_object)
> +    return ref_obj
> 
> === modified file 'turnip/api/tests/test_api.py'
> --- turnip/api/tests/test_api.py	2015-02-25 07:28:44 +0000
> +++ turnip/api/tests/test_api.py	2015-03-10 01:19:14 +0000
> @@ -1,5 +1,5 @@
>  # Copyright 2015 Canonical Ltd.  All rights reserved.
> -
> +# -*- coding: utf-8 -*-
>  from __future__ import print_function
>  
>  import json
> @@ -15,6 +15,7 @@
>  from webtest import TestApp
>  
>  from turnip import api
> +from turnip.api.tests.test_helpers import RepoFactory
>  
>  
>  class ApiTestCase(TestCase):
> @@ -26,9 +27,17 @@
>          self.app = TestApp(api.main({}))
>          self.repo_path = str(uuid.uuid1())
>          self.repo_store = os.path.join(repo_store, self.repo_path)
> -
> -    def test_repo_post(self):
> -        resp = self.app.post('/repo', json.dumps({'repo_path': self.repo_path}))
> +        self.commit = {'ref': 'refs/heads/master', 'message': 'test commit.'}
> +        self.tag = {'ref': 'refs/tags/tag0', 'message': 'tag message'}
> +
> +    def get_ref(self, ref):
> +        resp = self.app.get('/repo/{}/{}'.format(
> +            self.repo_path, ref))
> +        return json.loads(resp.json_body)
> +
> +    def test_repo_create(self):
> +        resp = self.app.post('/repo', json.dumps(
> +            {'repo_path': self.repo_path}))
>          self.assertEqual(resp.status_code, 200)
>  
>      def test_repo_delete(self):
> @@ -37,6 +46,56 @@
>          self.assertEqual(resp.status_code, 200)
>          self.assertFalse(os.path.exists(self.repo_store))
>  
> +    def test_repo_get_refs(self):
> +        """Ensure expected ref objects are returned and shas match."""
> +        ref = self.commit.get('ref')
> +        repo = RepoFactory(self.repo_store, num_commits=1, num_tags=1).build()
> +        resp = self.app.get('/repo/{}/refs'.format(self.repo_path))
> +        body = json.loads(resp.json_body)
> +
> +        self.assertTrue(ref in body)
> +        self.assertTrue(self.tag.get('ref') in body)
> +
> +        oid = repo.head.get_object().oid.hex  # git object sha
> +        resp_sha = body[ref]['object'].get('sha1')
> +        self.assertEqual(oid, resp_sha)
> +
> +    def test_ignore_non_unicode_refs(self):
> +        """Ensure non-unicode refs are dropped from ref collection."""
> +        factory = RepoFactory(self.repo_store)
> +        commit_oid = factory.add_commit('foo', 'foobar.txt')
> +        tag = '\xe9\xe9\xe9'  # latin-1
> +        tag_message = 'tag message'
> +        factory.add_tag(tag, tag_message, commit_oid)
> +
> +        resp = self.app.get('/repo/{}/refs'.format(self.repo_path))
> +        refs = json.loads(resp.json_body)
> +        self.assertEqual(len(refs.keys()), 1)
> +
> +    def test_allow_unicode_refs(self):
> +        """Ensure unicode refs are included in ref collection."""
> +        factory = RepoFactory(self.repo_store)
> +        commit_oid = factory.add_commit('foo', 'foobar.txt')
> +        tag = u'おいしいイカ'.encode('utf-8')
> +        tag_message = u'かわいい タコ'.encode('utf-8')

I can only read hiragana, not katakana. Critical bug.

> +        factory.add_tag(tag, tag_message, commit_oid)
> +
> +        resp = self.app.get('/repo/{}/refs'.format(self.repo_path))
> +        refs = json.loads(resp.json_body)
> +        self.assertEqual(len(refs.keys()), 2)
> +
> +    def test_repo_get_ref(self):
> +        RepoFactory(self.repo_store, num_commits=1).build()
> +        ref = 'refs/heads/master'
> +        resp = self.get_ref(ref)
> +        self.assertTrue(ref in resp)

We should probably have a Unicode equivalent of this test, as we do for get_refs.

> +
> +    def test_repo_get_tag(self):
> +        RepoFactory(self.repo_store, num_commits=1, num_tags=1).build()
> +        tag = self.tag.get('ref')
> +        resp = self.get_ref(tag)
> +        self.assertTrue(tag in resp)
> +
>  
>  if __name__ == '__main__':
>      unittest.main()
> 
> === added file 'turnip/api/tests/test_helpers.py'
> --- turnip/api/tests/test_helpers.py	1970-01-01 00:00:00 +0000
> +++ turnip/api/tests/test_helpers.py	2015-03-10 01:19:14 +0000
> @@ -0,0 +1,90 @@
> +# Copyright 2015 Canonical Ltd.  All rights reserved.
> +
> +import os
> +
> +from pygit2 import (
> +    init_repository,
> +    GIT_FILEMODE_BLOB,
> +    GIT_OBJ_COMMIT,
> +    GIT_SORT_TIME,
> +    IndexEntry,
> +    Signature,
> +    )
> +
> +
> +class RepoFactory():
> +    """Builds a git repository in a user defined state."""
> +
> +    def __init__(self, repo_store=None, num_commits=None, num_tags=None):
> +        self.author = Signature('Test Author', 'author@xxxxxxx')
> +        self.committer = Signature('Test Commiter', 'committer@xxxxxxx')
> +        self.num_commits = num_commits
> +        self.num_tags = num_tags
> +        self.repo_store = repo_store
> +        self.repo = self.init_repo()

"Repo store" has usually meant the directory that contains all repos, but in this case it's actually the path of the single repo that it's creating.

> +
> +    @property
> +    def commits(self):
> +        """Walk repo from HEAD and returns list of commit objects."""
> +        last = self.repo[self.repo.head.target]
> +        return list(self.repo.walk(last.id, GIT_SORT_TIME))
> +
> +    def add_commit(self, blob_content, file_path, parents=[],
> +                   ref='refs/heads/master'):
> +        """Create a commit from blob_content and file_path."""
> +        repo = self.repo
> +
> +        blob_oid = repo.create_blob(blob_content)
> +        blob_entry = IndexEntry(file_path, blob_oid, GIT_FILEMODE_BLOB)
> +        repo.index.add(blob_entry)
> +        tree_id = repo.index.write_tree()
> +        oid = repo.create_commit(ref,
> +                                 self.author,
> +                                 self.committer,
> +                                 'commit', tree_id, parents)
> +        return oid
> +
> +    def add_tag(self, tag_name, tag_message, oid):
> +        """Create a tag from tag_name and oid."""
> +        repo = self.repo
> +        repo.create_tag(tag_name, oid, GIT_OBJ_COMMIT,
> +                        self.committer, tag_message)
> +
> +    def stage(self, file_path):
> +        """Stage a file and return a tree id."""
> +        repo = self.repo
> +        repo.index.add(file_path)
> +        repo.index.write()
> +        return repo.index.write_tree()
> +
> +    def generate_commits(self):
> +        """Generate n number of commits."""

These no longer take an "n" argument, so the docstring is a bit confusing.

> +        parents = []
> +        for i in xrange(self.num_commits):
> +            blob_content = b'commit {}'.format(i)
> +            test_file = 'test.txt'
> +            with open(os.path.join(self.repo_store, test_file), 'w') as f:
> +                f.write(blob_content)
> +
> +            self.stage(test_file)
> +
> +            commit_oid = self.add_commit(blob_content, test_file, parents)
> +            parents = [commit_oid]
> +
> +    def generate_tags(self):
> +        """Generate n number of tags."""
> +        repo = self.repo
> +        oid = repo.head.get_object().oid
> +        for i in xrange(self.num_tags):
> +            self.add_tag('tag{}'.format(i), 'tag message {}'.format(i), oid)
> +
> +    def init_repo(self):
> +        return init_repository(self.repo_store)
> +
> +    def build(self):
> +        """Return a repo, optionally with generated commits and tags."""
> +        if self.num_commits:
> +            self.generate_commits()
> +        if self.num_tags:
> +            self.generate_tags()
> +        return self.repo
> 
> === modified file 'turnip/api/views.py'
> --- turnip/api/views.py	2015-03-04 08:39:15 +0000
> +++ turnip/api/views.py	2015-03-10 01:19:14 +0000
> @@ -1,16 +1,30 @@
>  # Copyright 2015 Canonical Ltd.  All rights reserved.
>  
> +import json
>  import os
>  
>  from cornice.resource import resource
>  from cornice.util import extract_json_data
> +from pygit2 import GitError
>  import pyramid.httpexceptions as exc
>  
>  from turnip.config import TurnipConfig
> -from turnip.api.store import Store
> -
> -
> -@resource(collection_path='repo', path='/repo/{name}')
> +from turnip.api import store
> +
> +
> +def repo_path(func):
> +    """Decorator builds repo path from request name and repo_store."""
> +    def repo_path_decorator(self):
> +        name = self.request.matchdict['name']
> +        if not name:
> +            self.request.errors.add('body', 'name', 'repo name is missing')
> +            return
> +        repo_path = os.path.join(self.repo_store, name)

To avoid exploitation through malicious repo names (directory traversal, symlinks, etc.), ensure that os.path.realpath(repo_path).startswith(os.path.realpath(self.repo_store))

> +        return func(self, repo_path)
> +    return repo_path_decorator
> +
> +
> +@resource(collection_path='/repo', path='/repo/{name}')
>  class RepoAPI(object):
>      """Provides HTTP API for repository actions."""
>  
> @@ -28,18 +42,42 @@
>              return
>          repo = os.path.join(self.repo_store, repo_path)
>          try:
> -            Store.init(repo)
> -        except Exception:
> +            store.init_repo(repo)
> +        except GitError:
>              return exc.HTTPConflict()  # 409
>  
> -    def delete(self):
> +    @repo_path
> +    def delete(self, repo_path):
>          """Delete an existing git repository."""
> -        name = self.request.matchdict['name']
> -        if not name:
> -            self.request.errors.add('body', 'name', 'repo name is missing')
> -            return
> -        repo = os.path.join(self.repo_store, name)
> -        try:
> -            Store.delete(repo)
> -        except Exception:
> -            return exc.HTTPNotFound()  # 404
> +        try:
> +            store.delete_repo(repo_path)
> +        except (IOError, OSError):
> +            return exc.HTTPNotFound()  # 404
> +
> +
> +@resource(collection_path='/repo/{name}/refs',
> +          path='/repo/{name}/refs/{ref:.*}')
> +class RefAPI(object):
> +    """Provides HTTP API for git references."""
> +
> +    def __init__(self, request):
> +        config = TurnipConfig()
> +        self.request = request
> +        self.repo_store = config.get('repo_store')
> +
> +    @repo_path
> +    def collection_get(self, repo_path):
> +        try:
> +            refs = store.get_refs(repo_path)
> +        except GitError:
> +            return exc.HTTPNotFound()  # 404
> +        return json.dumps(refs)
> +
> +    @repo_path
> +    def get(self, repo_path):
> +        ref = 'refs/' + self.request.matchdict['ref']
> +        try:
> +            ref = store.get_ref(repo_path, ref)
> +        except GitError:
> +            return exc.HTTPNotFound()
> +        return json.dumps(ref)
> 


-- 
https://code.launchpad.net/~blr/turnip/api-refs/+merge/251394
Your team Launchpad code reviewers is subscribed to branch lp:turnip.