launchpad-reviewers team mailing list archive

[Tom Wardill <tom.wardill@xxxxxxxxxxxxx>]

I'm unsure on the use of IGitLookup to add an interface for doing the GitRuleGrant lookups, it seemed logical at the time but on reflection it doesn't quite feel to be in the right place/correct structure

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

Not quite sure how this ended up in my diff, I think it's a pre-requisite branch that hasn't been picked up as such

> +-- 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

See top level comment, I'm not sure if this is the best approach.

>        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/gitlookup.py'
> --- lib/lp/code/interfaces/gitlookup.py	2015-03-30 14:47:22 +0000
> +++ lib/lp/code/interfaces/gitlookup.py	2018-09-26 15:45:21 +0000
> @@ -145,3 +146,14 @@
>              leading part of a path as a repository, such as external code
>              browsers.
>          """
> +
> +
> +class IGitRuleGrantLookup(Interface):
> +    """Utility for looking up a GitRuleGrant by properties"""
> +
> +    def getByRulesAffectingPerson(repository, grantee_id):
> +        """Find all the rules for a repository that affect a Person.
> +
> +        :param repository: An instance of a GitRepository
> +        :param granteed_id: An integer of the id of the Person

This isn't an ID anymore, will fix.

> +        """
> 
> === 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)]

I couldn't come up with a `.find()` compatible method of achieving this. Went with the iterator as I assumed the scale would be relatively small, but I'm fairly sure there is a better way.

> +        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:
> +            permissions = []
> +            if grant.can_create:

This will indeed crash if you are attempting to look up a grant for a user that has absolutely no permissions to the repository, leaving it as unfixed per suggestion to implement that in subsequent branch.

> +                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})
> +
> +        if self._isRepositoryOwner(requester, repository):
> +            lines.append({
> +                'ref_pattern': '*',
> +                'permissions': ['create', 'push', 'force-push']})
> +        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
> +
> +        return run_with_login(
> +            requester_id,
> +            self._listRefRules,
> +            translated_path,
> +        )

Blergh, indentation, will fix.



-- 
https://code.launchpad.net/~twom/launchpad/branch-permissions-for-gitapi/+merge/355716
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~twom/launchpad/branch-permissions-for-gitapi into lp:launchpad.