launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

A quick first-pass review to get you started ...

Diff comments:

> === added file 'database/schema/patch-2209-85-0.sql'
> --- database/schema/patch-2209-85-0.sql	1970-01-01 00:00:00 +0000
> +++ database/schema/patch-2209-85-0.sql	2018-09-26 15:45:21 +0000
> @@ -0,0 +1,68 @@
> +-- Copyright 2018 Canonical Ltd.  This software is licensed under the

This must mean that you merged one of my database branches.  It's important not to do this, as it can cause significant deployment confusion - when you're dealing with unmerged DB branches, the way to do that is just to apply DB patches from a separate checkout.

You'll probably need to (effectively) rebase this branch to get rid of that (given bzr, it might be simplest to just rebranch, recommit each logical commit one by one, push --overwrite).

> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +CREATE TABLE GitRule (
> +    id serial PRIMARY KEY,
> +    repository integer NOT NULL REFERENCES gitrepository,
> +    position integer NOT NULL,
> +    ref_pattern text NOT NULL,
> +    creator integer NOT NULL REFERENCES person,
> +    date_created timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT TIME ZONE 'UTC') NOT NULL,
> +    date_last_modified timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT TIME ZONE 'UTC') NOT NULL,
> +    CONSTRAINT gitrule__repository__position__key UNIQUE (repository, position) DEFERRABLE INITIALLY DEFERRED,
> +    CONSTRAINT gitrule__repository__ref_pattern__key UNIQUE (repository, ref_pattern),
> +    -- Used by repository_matches_rule constraint on GitRuleGrant.
> +    CONSTRAINT gitrule__repository__id__key UNIQUE (repository, id)
> +);
> +
> +COMMENT ON TABLE GitRule IS 'An access rule for a Git repository.';
> +COMMENT ON COLUMN GitRule.repository IS 'The repository that this rule is for.';
> +COMMENT ON COLUMN GitRule.position IS 'The position of this rule in its repository''s rule order.';
> +COMMENT ON COLUMN GitRule.ref_pattern IS 'The pattern of references matched by this rule.';
> +COMMENT ON COLUMN GitRule.creator IS 'The user who created this rule.';
> +COMMENT ON COLUMN GitRule.date_created IS 'The time when this rule was created.';
> +COMMENT ON COLUMN GitRule.date_last_modified IS 'The time when this rule was last modified.';
> +
> +CREATE TABLE GitRuleGrant (
> +    id serial PRIMARY KEY,
> +    repository integer NOT NULL REFERENCES gitrepository,
> +    rule integer NOT NULL REFERENCES gitrule,
> +    grantee_type integer NOT NULL,
> +    grantee integer REFERENCES person,
> +    can_create boolean DEFAULT false NOT NULL,
> +    can_push boolean DEFAULT false NOT NULL,
> +    can_force_push boolean DEFAULT false NOT NULL,
> +    grantor integer NOT NULL REFERENCES person,
> +    date_created timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT TIME ZONE 'UTC') NOT NULL,
> +    date_last_modified timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT TIME ZONE 'UTC') NOT NULL,
> +    CONSTRAINT repository_matches_rule FOREIGN KEY (repository, rule) REFERENCES gitrule (repository, id),
> +    -- 2 == PERSON
> +    CONSTRAINT has_grantee CHECK ((grantee_type = 2) = (grantee IS NOT NULL))
> +);
> +
> +CREATE INDEX gitrulegrant__repository__idx
> +    ON GitRuleGrant(repository);
> +CREATE UNIQUE INDEX gitrulegrant__rule__grantee_type__key
> +    ON GitRuleGrant(rule, grantee_type)
> +    -- 2 == PERSON
> +    WHERE grantee_type != 2;
> +CREATE UNIQUE INDEX gitrulegrant__rule__grantee_type__grantee_key
> +    ON GitRuleGrant(rule, grantee_type, grantee)
> +    -- 2 == PERSON
> +    WHERE grantee_type = 2;
> +
> +COMMENT ON TABLE GitRuleGrant IS 'An access grant for a Git repository rule.';
> +COMMENT ON COLUMN GitRuleGrant.repository IS 'The repository that this grant is for.';
> +COMMENT ON COLUMN GitRuleGrant.rule IS 'The rule that this grant is for.';
> +COMMENT ON COLUMN GitRuleGrant.grantee_type IS 'The type of entity being granted access.';
> +COMMENT ON COLUMN GitRuleGrant.grantee IS 'The person or team being granted access.';
> +COMMENT ON COLUMN GitRuleGrant.can_create IS 'Whether creating references is allowed.';
> +COMMENT ON COLUMN GitRuleGrant.can_push IS 'Whether pushing references is allowed.';
> +COMMENT ON COLUMN GitRuleGrant.can_force_push IS 'Whether force-pushing references is allowed.';
> +COMMENT ON COLUMN GitRuleGrant.grantor IS 'The user who created this grant.';
> +COMMENT ON COLUMN GitRuleGrant.date_created IS 'The time when this grant was created.';
> +COMMENT ON COLUMN GitRuleGrant.date_last_modified IS 'The time when this grant was last modified.';
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2209, 85, 0);
> 
> === modified file 'lib/lp/code/configure.zcml'
> --- lib/lp/code/configure.zcml	2018-09-26 15:45:20 +0000
> +++ lib/lp/code/configure.zcml	2018-09-26 15:45:21 +0000
> @@ -1013,6 +1016,11 @@
>      <allow interface="lp.code.interfaces.gitlookup.IGitLookup" />
>    </securedutility>
>    <securedutility
> +      class="lp.code.model.gitlookup.GitRuleGrantLookup"
> +      provides="lp.code.interfaces.gitlookup.IGitRuleGrantLookup">
> +    <allow interface="lp.code.interfaces.gitlookup.IGitRuleGrantLookup" />
> +  </securedutility>
> +  <securedutility

Right.  This sort of thing makes sense when the objects you're searching for aren't clearly subsidiary to anything else, or when you might want to look things up without starting from some kind of context.  But in this case, a grant is clearly subsidiary to a rule which in turn is subsidiary to a repository, and in this case you just want to look up a grant for a particular repository with some constraints.  The proper approach would be to just add a lookup method to GitRepository, e.g. findGrantsByGrantee(grantee).

>        class="lp.code.model.gitlookup.GitTraverser"
>        provides="lp.code.interfaces.gitlookup.IGitTraverser">
>      <allow interface="lp.code.interfaces.gitlookup.IGitTraverser" />
> 
> === modified file 'lib/lp/code/interfaces/gitapi.py'
> --- lib/lp/code/interfaces/gitapi.py	2015-03-31 04:18:22 +0000
> +++ lib/lp/code/interfaces/gitapi.py	2018-09-26 15:45:21 +0000
> @@ -67,3 +67,9 @@
>          :returns: An `Unauthorized` fault, as password authentication is
>              not yet supported.
>          """
> +
> +    def listRefRules(self, repository, user):
> +        """Return the list of RefRules for `user` in `repository`
> +
> +        :returns: A List of rules for the user in the specified repository

No need to capitalise list here - that suggests it's a zope.schema.List or something, but it isn't.

> +        """
> 
> === modified file 'lib/lp/code/model/gitlookup.py'
> --- lib/lp/code/model/gitlookup.py	2018-07-23 10:28:33 +0000
> +++ lib/lp/code/model/gitlookup.py	2018-09-26 15:45:21 +0000
> @@ -372,3 +374,14 @@
>          if trailing:
>              trailing_segments.insert(0, trailing)
>          return repository, "/".join(trailing_segments)
> +
> +
> +@implementer(IGitRuleGrantLookup)
> +class GitRuleGrantLookup:
> +
> +    def getByRulesAffectingPerson(self, repository, grantee):
> +        grants = IStore(GitRuleGrant).find(
> +            GitRuleGrant,
> +            GitRuleGrant.repository == repository)
> +        grants = [grant for grant in grants if grantee.inTeam(grant.grantee)]

We usually add a TeamParticipation join for this kind of thing.  It's definitely worth avoiding doing the checks one by one; that can quickly get expensive.

> +        return grants
> 
> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py	2018-08-28 13:58:37 +0000
> +++ lib/lp/code/xmlrpc/git.py	2018-09-26 15:45:21 +0000
> @@ -325,3 +326,45 @@
>          else:
>              # Only macaroons are supported for password authentication.
>              return faults.Unauthorized()
> +
> +    def _isRepositoryOwner(self, requester, repository):
> +        try:
> +            return requester.inTeam(repository.owner)
> +        except Unauthorized:
> +            return False
> +
> +    def _listRefRules(self, requester, translated_path):
> +        repository = getUtility(IGitLookup).getByHostingPath(translated_path)
> +        grants = getUtility(IGitRuleGrantLookup).getByRulesAffectingPerson(
> +            repository, requester)
> +
> +        lines = []
> +        for grant in grants:

We need to talk about the case where multiple grants for a single rule (remember that rules are ordered, but grants for a given rule aren't) match the requester.

We may also want to think about how this overlaps with GitRef.checkPermissions, since they'll certainly need to share some logic; it may be possible to extract some of this into a method on GitRepository which can then be unit-tested separately, although I haven't worked out the details.

> +            permissions = []
> +            if grant.can_create:

On reflection I think we should probably use removeSecurityProxy to avoid that problem, independent of the other discussion about access grants.  It seems moderately legitimate here, since this is effectively a security checker.

> +                permissions.append("create")
> +            if grant.can_push:
> +                permissions.append("push")
> +            if grant.can_force_push:
> +                permissions.append("force-push")
> +            lines.append(
> +                {'ref_pattern': grant.rule.ref_pattern,
> +                'permissions': permissions})

Somewhat unconventional indentation - I'd put the opening brace just after the opening parenthesis, line up the quotes, and then move the closing brace to the next line.

> +
> +        if self._isRepositoryOwner(requester, repository):
> +            lines.append({
> +                'ref_pattern': '*',
> +                'permissions': ['create', 'push', 'force-push']})

Closing brace on the next line, for the usual diff-minimisation reasons.

> +        return lines
> +
> +    def listRefRules(self, translated_path, auth_params):
> +        """See `IGitAPI`"""
> +        requester_id = auth_params.get("uid")
> +        if requester_id is None:
> +            requester_id = LAUNCHPAD_ANONYMOUS

I guess this is for completeness, but it seems odd since this API is only used for writes.

> +
> +        return run_with_login(
> +            requester_id,
> +            self._listRefRules,
> +            translated_path,
> +        )
> 
> === modified file 'lib/lp/code/xmlrpc/tests/test_git.py'
> --- lib/lp/code/xmlrpc/tests/test_git.py	2018-08-28 14:07:38 +0000
> +++ lib/lp/code/xmlrpc/tests/test_git.py	2018-09-26 15:45:21 +0000
> @@ -260,6 +260,125 @@
>          self.assertEqual(
>              initial_count, getUtility(IAllGitRepositories).count())
>  
> +    def test_listRefRules(self):
> +        # Test that GitGrantRule (ref rule) can be retrieved for a user
> +        requester = self.factory.makePerson()
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=requester, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(repository)
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=requester, can_push=True, can_create=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': requester.id})
> +        self.assertEqual(len(results), 2)
> +        self.assertEqual(results[0]['ref_pattern'], 'refs/heads/*')
> +        self.assertEqual(results[0]['permissions'], ['create', 'push'])

I'd use testtools matchers for this kind of thing so that you can assert the structure of the whole response in one go, and thus get more informative failure messages if it's wrong.

Also note that if you're using assertEqual and friends then the Launchpad style is assertEqual(expected, observed).

> +
> +    def test_listRefRules_no_grants(self):
> +        # User that has no grants and is not the owner
> +        requester = self.factory.makePerson()
> +        owner = self.factory.makePerson()
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(repository)
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=owner, can_push=True, can_create=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': requester.id})
> +        self.assertEqual(len(results), 0)
> +
> +    def test_listRefRules_owner_has_default(self):
> +        owner = self.factory.makePerson()
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(
> +            repository=repository, ref_pattern=u'refs/heads/master')
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=owner, can_push=True, can_create=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': owner.id})
> +        self.assertEqual(len(results), 2)
> +        # Default grant should be last in pattern
> +        self.assertEqual(results[-1]['ref_pattern'], '*')
> +
> +    def test_listRefRules_owner_is_team(self):
> +        member = self.factory.makePerson()
> +        owner = self.factory.makeTeam(members=[member])
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': member.id})
> +
> +        # Should have default grant as member of owning team
> +        self.assertEqual(len(results), 1)
> +        self.assertEqual(results[-1]['ref_pattern'], '*')
> +
> +    def test_listRefRules_owner_is_team_with_grants(self):
> +        member = self.factory.makePerson()
> +        owner = self.factory.makeTeam(members=[member])
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(
> +            repository=repository, ref_pattern=u'refs/heads/master')
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=owner, can_push=True, can_create=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': member.id})
> +
> +        # Should have default grant as member of owning team
> +        self.assertEqual(len(results), 2)
> +        self.assertEqual(results[-1]['ref_pattern'], '*')
> +
> +    def test_listRefRules_owner_is_team_with_grants_to_person(self):
> +        member = self.factory.makePerson()
> +        other_member = self.factory.makePerson()
> +        owner = self.factory.makeTeam(members=[member, other_member])
> +        repository = removeSecurityProxy(
> +            self.factory.makeGitRepository(
> +                owner=owner, information_type=InformationType.USERDATA))
> +
> +        rule = self.factory.makeGitRule(
> +            repository=repository, ref_pattern=u'refs/heads/master')
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=owner, can_push=True, can_create=True)
> +
> +        rule = self.factory.makeGitRule(
> +            repository=repository, ref_pattern=u'refs/heads/tags')
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=member, can_create=True)
> +
> +        # This should not appear
> +        self.factory.makeGitRuleGrant(
> +            rule=rule, grantee=other_member, can_push=True)
> +
> +        results = self.git_api.listRefRules(
> +            repository.getInternalPath(),
> +            {'uid': member.id})
> +
> +        # Should have default grant as member of owning team
> +        self.assertEqual(len(results), 3)
> +        self.assertEqual(results[-1]['ref_pattern'], '*')
> +        tags_rule = results[1]
> +        self.assertEqual(tags_rule['permissions'], ['create'])
>  
>  class TestGitAPI(TestGitAPIMixin, TestCaseWithFactory):
>      """Tests for the implementation of `IGitAPI`."""


-- 
https://code.launchpad.net/~twom/launchpad/branch-permissions-for-gitapi/+merge/355716
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.