launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/code/interfaces/gitrepository.py'
> --- lib/lp/code/interfaces/gitrepository.py	2018-08-23 17:03:05 +0000
> +++ lib/lp/code/interfaces/gitrepository.py	2018-09-25 18:01:53 +0000
> @@ -715,6 +719,25 @@
>          This may be helpful in cases where a previous scan crashed.
>          """
>  
> +    def addRule(ref_pattern, creator, position=None):
> +        """Add an access rule to this repository.
> +
> +        :param ref_pattern: The reference pattern that the new rule should
> +            match.
> +        :param creator: The `IPerson` who is adding the rule.
> +        :param position: The list position at which to insert the rule, or
> +            None to append it.
> +        """
> +
> +    def moveRule(rule, position):
> +        """Move a rule to a new position in its repository's rule order.
> +
> +        :param rule: The `IGitRule` to move.
> +        :param position: The new position.  For example, 0 puts the rule at
> +            the start, while `len(repository.rules)` puts the rule at the
> +            end.
> +        """

Worth explicitly calling out that it shifts the rest as appropriate?

> +
>      @export_read_operation()
>      @operation_for_version("devel")
>      def canBeDeleted():
> 
> === added file 'lib/lp/code/interfaces/gitrule.py'
> --- lib/lp/code/interfaces/gitrule.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/interfaces/gitrule.py	2018-09-25 18:01:53 +0000
> @@ -0,0 +1,178 @@
> +# Copyright 2018 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Git repository access rules."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> +    'IGitRule',
> +    'IGitRuleGrant',
> +    ]
> +
> +from lazr.restful.fields import Reference
> +from zope.interface import (
> +    Attribute,
> +    Interface,
> +    )
> +from zope.schema import (
> +    Bool,
> +    Choice,
> +    Datetime,
> +    Int,
> +    TextLine,
> +    )
> +
> +from lp import _
> +from lp.code.enums import GitGranteeType
> +from lp.code.interfaces.gitrepository import IGitRepository
> +from lp.services.fields import (
> +    PersonChoice,
> +    PublicPersonChoice,
> +    )
> +
> +
> +class IGitRuleView(Interface):
> +    """`IGitRule` attributes that require launchpad.View."""
> +
> +    id = Int(title=_("ID"), readonly=True, required=True)
> +
> +    repository = Reference(
> +        title=_("Repository"), required=True, readonly=True,
> +        schema=IGitRepository,
> +        description=_("The repository that this rule is for."))
> +
> +    position = Int(
> +        title=_("Position"), required=True, readonly=True,
> +        description=_(
> +            "The position of this rule in its repository's rule order."))
> +
> +    creator = PublicPersonChoice(
> +        title=_("Creator"), required=True, readonly=True,
> +        vocabulary="ValidPerson",
> +        description=_("The user who created this rule."))
> +
> +    date_created = Datetime(
> +        title=_("Date created"), required=True, readonly=True,
> +        description=_("The time when this rule was created."))
> +
> +    is_exact = Bool(
> +        title=_("Is this an exact-match rule?"), required=True, readonly=True,
> +        description=_(
> +            "True if this rule is for an exact reference name, or False if "
> +            "it is for a wildcard."))
> +
> +    grants = Attribute("The access grants for this rule.")
> +
> +
> +class IGitRuleEditableAttributes(Interface):
> +    """`IGitRule` attributes that can be edited.
> +
> +    These attributes need launchpad.View to see, and launchpad.Edit to change.
> +    """
> +
> +    ref_pattern = TextLine(
> +        title=_("Pattern"), required=True, readonly=False,
> +        description=_("The pattern of references matched by this rule."))
> +
> +    date_last_modified = Datetime(
> +        title=_("Date last modified"), required=True, readonly=True,
> +        description=_("The time when this rule was last modified."))

Does this need to actually be editable for the subscriber to set it? Same with IGitRuleGrantEditableAttributes['date_last_modified'].

> +
> +
> +class IGitRuleEdit(Interface):
> +    """`IGitRule` attributes that require launchpad.Edit."""
> +
> +    def addGrant(grantee, grantor, can_create=False, can_push=False,
> +                 can_force_push=False):
> +        """Add an access grant to this rule.
> +
> +        :param grantee: The `IPerson` who is being granted permission, or an
> +            item of `GitGranteeType` other than `GitGranteeType.PERSON` to
> +            grant permission to some other kind of entity.
> +        :param grantor: The `IPerson` who is granting permission.
> +        :param can_create: Whether the grantee can create references
> +            matching this rule.
> +        :param can_push: Whether the grantee can push references matching
> +            this rule.
> +        :param can_force_push: Whether the grantee can force-push references
> +            matching this rule.
> +        """
> +
> +    def destroySelf():
> +        """Delete this rule."""
> +
> +
> +class IGitRule(IGitRuleView, IGitRuleEditableAttributes, IGitRuleEdit):
> +    """An access rule for a Git repository."""
> +
> +
> +class IGitRuleGrantView(Interface):
> +    """`IGitRuleGrant` attributes that require launchpad.View."""
> +
> +    id = Int(title=_("ID"), readonly=True, required=True)
> +
> +    repository = Reference(
> +        title=_("Repository"), required=True, readonly=True,
> +        schema=IGitRepository,
> +        description=_("The repository that this grant is for."))
> +
> +    rule = Reference(
> +        title=_("Rule"), required=True, readonly=True,
> +        schema=IGitRule,
> +        description=_("The rule that this grant is for."))
> +
> +    grantor = PublicPersonChoice(
> +        title=_("Grantor"), required=True, readonly=True,
> +        vocabulary="ValidPerson",
> +        description=_("The user who created this grant."))
> +
> +    grantee_type = Choice(
> +        title=_("Grantee type"), required=True, readonly=True,
> +        vocabulary=GitGranteeType,
> +        description=_("The type of grantee for this grant."))
> +
> +    grantee = PersonChoice(
> +        title=_("Grantee"), required=False, readonly=True,
> +        vocabulary="ValidPersonOrTeam",
> +        description=_("The person being granted access."))
> +
> +    date_created = Datetime(
> +        title=_("Date created"), required=True, readonly=True,
> +        description=_("The time when this grant was created."))
> +
> +
> +class IGitRuleGrantEditableAttributes(Interface):
> +    """`IGitRuleGrant` attributes that can be edited.
> +
> +    These attributes need launchpad.View to see, and launchpad.Edit to change.
> +    """
> +
> +    can_create = Bool(
> +        title=_("Can create"), required=True, readonly=False,
> +        description=_("Whether creating references is allowed."))
> +
> +    can_push = Bool(
> +        title=_("Can push"), required=True, readonly=False,
> +        description=_("Whether pushing references is allowed."))
> +
> +    can_force_push = Bool(
> +        title=_("Can force-push"), required=True, readonly=False,
> +        description=_("Whether force-pushing references is allowed."))
> +
> +    date_last_modified = Datetime(
> +        title=_("Date last modified"), required=True, readonly=True,
> +        description=_("The time when this grant was last modified."))
> +
> +
> +class IGitRuleGrantEdit(Interface):
> +    """`IGitRuleGrant` attributes that require launchpad.Edit."""
> +
> +    def destroySelf():
> +        """Delete this access grant."""
> +
> +
> +class IGitRuleGrant(IGitRuleGrantView, IGitRuleGrantEditableAttributes,
> +                    IGitRuleGrantEdit):
> +    """An access grant for a Git repository rule."""
> 
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py	2018-09-06 14:25:46 +0000
> +++ lib/lp/code/model/gitrepository.py	2018-09-25 18:01:53 +0000
> @@ -1121,6 +1125,66 @@
>      def code_import(self):
>          return getUtility(ICodeImportSet).getByGitRepository(self)
>  
> +    @property
> +    def rules(self):
> +        """See `IGitRepository`."""
> +        return Store.of(self).find(
> +            GitRule, GitRule.repository == self).order_by(GitRule.position)
> +
> +    def _syncRulePositions(self, rules):
> +        """Synchronise rule positions with their order in a provided list.
> +
> +        :param rules: A sequence of `IGitRule`s in the desired order.
> +        """
> +        # Canonicalise rule ordering: exact-match rules come first in
> +        # lexicographical order, followed by wildcard rules in the requested
> +        # order.  (Note that `sorted` is guaranteed to be stable.)
> +        rules = sorted(
> +            rules,
> +            key=lambda rule: (0, rule.ref_pattern) if rule.is_exact else (1,))
> +        # Ensure the correct position of all rules, which may involve more
> +        # work than necessary, but is simple and tends to be
> +        # self-correcting.
> +        for position, rule in enumerate(rules):
> +            if rule.repository != self:
> +                raise AssertionError("%r does not belong to %r" % (rule, self))
> +            if rule.position != position:
> +                removeSecurityProxy(rule).position = position

Worth noting that this only works because of the deferred unique constraint?

> +
> +    def addRule(self, ref_pattern, creator, position=None):
> +        """See `IGitRepository`."""
> +        rules = list(self.rules)
> +        rule = GitRule(
> +            repository=self,
> +            # -1 isn't a valid position, but _syncRulePositions will correct
> +            # it in a moment.
> +            position=position if position is not None else -1,
> +            ref_pattern=ref_pattern, creator=creator, date_created=DEFAULT)
> +        if position is None:
> +            rules.append(rule)
> +        else:
> +            rules.insert(position, rule)
> +        self._syncRulePositions(rules)
> +        return rule
> +
> +    def moveRule(self, rule, position):
> +        """See `IGitRepository`."""
> +        if rule.repository != self:
> +            raise ValueError("%r does not belong to %r" % (rule, self))
> +        if position < 0:
> +            raise ValueError("Negative positions are not supported")
> +        if position != rule.position:
> +            rules = list(self.rules)
> +            rules.remove(rule)
> +            rules.insert(position, rule)
> +            self._syncRulePositions(rules)
> +
> +    @property
> +    def grants(self):
> +        """See `IGitRepository`."""
> +        return Store.of(self).find(
> +            GitRuleGrant, GitRuleGrant.repository_id == self.id)
> +
>      def canBeDeleted(self):
>          """See `IGitRepository`."""
>          # Can't delete if the repository is associated with anything.
> @@ -1252,6 +1316,8 @@
>          self._deleteRepositorySubscriptions()
>          self._deleteJobs()
>          getUtility(IWebhookSet).delete(self.webhooks)
> +        self.grants.remove()
> +        self.rules.remove()

Probably deserves a comment that we're deliberately skipping the destructors because all they do is log to the per-repo activity that will also be removed.

>  
>          # Now destroy the repository.
>          repository_name = self.unique_name
> 
> === added file 'lib/lp/code/model/gitrule.py'
> --- lib/lp/code/model/gitrule.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/code/model/gitrule.py	2018-09-25 18:01:53 +0000
> @@ -0,0 +1,206 @@
> +# Copyright 2018 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Git repository access rules."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> +    'GitRule',
> +    'GitRuleGrant',
> +    ]
> +
> +from lazr.enum import DBItem
> +import pytz
> +from storm.locals import (
> +    Bool,
> +    DateTime,
> +    Int,
> +    Reference,
> +    Store,
> +    Unicode,
> +    )
> +from zope.interface import implementer
> +from zope.security.proxy import removeSecurityProxy
> +
> +from lp.code.enums import GitGranteeType
> +from lp.code.interfaces.gitrule import (
> +    IGitRule,
> +    IGitRuleGrant,
> +    )
> +from lp.registry.interfaces.person import (
> +    validate_person,
> +    validate_public_person,
> +    )
> +from lp.services.database.constants import (
> +    DEFAULT,
> +    UTC_NOW,
> +    )
> +from lp.services.database.enumcol import DBEnum
> +from lp.services.database.stormbase import StormBase
> +
> +
> +def git_rule_modified(rule, event):
> +    """Update date_last_modified when a GitRule is modified.
> +
> +    This method is registered as a subscriber to `IObjectModifiedEvent`
> +    events on Git repository rules.
> +    """
> +    if event.edited_fields:
> +        rule.date_last_modified = UTC_NOW
> +
> +
> +@implementer(IGitRule)
> +class GitRule(StormBase):
> +    """See `IGitRule`."""
> +
> +    __storm_table__ = 'GitRule'
> +
> +    id = Int(primary=True)
> +
> +    repository_id = Int(name='repository', allow_none=False)
> +    repository = Reference(repository_id, 'GitRepository.id')
> +
> +    position = Int(name='position', allow_none=False)
> +
> +    ref_pattern = Unicode(name='ref_pattern', allow_none=False)
> +
> +    creator_id = Int(
> +        name='creator', allow_none=False, validator=validate_public_person)
> +    creator = Reference(creator_id, 'Person.id')
> +
> +    date_created = DateTime(
> +        name='date_created', tzinfo=pytz.UTC, allow_none=False)
> +    date_last_modified = DateTime(
> +        name='date_last_modified', tzinfo=pytz.UTC, allow_none=False)
> +
> +    def __init__(self, repository, position, ref_pattern, creator,
> +                 date_created):
> +        super(GitRule, self).__init__()
> +        self.repository = repository
> +        self.position = position
> +        self.ref_pattern = ref_pattern
> +        self.creator = creator
> +        self.date_created = date_created
> +        self.date_last_modified = date_created
> +
> +    def __repr__(self):
> +        return "<GitRule '%s'> for %r" % (self.ref_pattern, self.repository)

Should the > be at the end?

> +
> +    @property
> +    def is_exact(self):
> +        """See `IGitRule`."""
> +        # turnip's glob_to_re only treats * as special, so any rule whose
> +        # pattern does not contain * must be an exact-match rule.
> +        return "*" not in self.ref_pattern
> +
> +    @property
> +    def grants(self):
> +        """See `IGitRule`."""
> +        return Store.of(self).find(
> +            GitRuleGrant, GitRuleGrant.rule_id == self.id)
> +
> +    def addGrant(self, grantee, grantor, can_create=False, can_push=False,
> +                 can_force_push=False):
> +        """See `IGitRule`."""
> +        return GitRuleGrant(
> +            rule=self, grantee=grantee, can_create=can_create,
> +            can_push=can_push, can_force_push=can_force_push, grantor=grantor,
> +            date_created=DEFAULT)
> +
> +    def destroySelf(self):
> +        """See `IGitRule`."""
> +        for grant in self.grants:
> +            grant.destroySelf()
> +        rules = list(self.repository.rules)
> +        Store.of(self).remove(self)
> +        rules.remove(self)
> +        removeSecurityProxy(self.repository)._syncRulePositions(rules)
> +
> +
> +def git_rule_grant_modified(grant, event):
> +    """Update date_last_modified when a GitRuleGrant is modified.
> +
> +    This method is registered as a subscriber to `IObjectModifiedEvent`
> +    events on Git repository grants.
> +    """
> +    if event.edited_fields:
> +        grant.date_last_modified = UTC_NOW
> +
> +
> +@implementer(IGitRuleGrant)
> +class GitRuleGrant(StormBase):
> +    """See `IGitRuleGrant`."""
> +
> +    __storm_table__ = 'GitRuleGrant'
> +
> +    id = Int(primary=True)
> +
> +    repository_id = Int(name='repository', allow_none=False)
> +    repository = Reference(repository_id, 'GitRepository.id')
> +
> +    rule_id = Int(name='rule', allow_none=False)
> +    rule = Reference(rule_id, 'GitRule.id')
> +
> +    grantee_type = DBEnum(
> +        name='grantee_type', enum=GitGranteeType, allow_none=False)
> +
> +    grantee_id = Int(
> +        name='grantee', allow_none=True, validator=validate_person)
> +    grantee = Reference(grantee_id, 'Person.id')
> +
> +    can_create = Bool(name='can_create', allow_none=False)
> +    can_push = Bool(name='can_push', allow_none=False)
> +    can_force_push = Bool(name='can_force_push', allow_none=False)
> +
> +    grantor_id = Int(
> +        name='grantor', allow_none=False, validator=validate_public_person)
> +    grantor = Reference(grantor_id, 'Person.id')
> +
> +    date_created = DateTime(
> +        name='date_created', tzinfo=pytz.UTC, allow_none=False)
> +    date_last_modified = DateTime(
> +        name='date_last_modified', tzinfo=pytz.UTC, allow_none=False)
> +
> +    def __init__(self, rule, grantee, can_create, can_push, can_force_push,
> +                 grantor, date_created):
> +        if isinstance(grantee, DBItem) and grantee.enum == GitGranteeType:
> +            if grantee == GitGranteeType.PERSON:
> +                raise ValueError(
> +                    "grantee may not be GitGranteeType.PERSON; pass a person "
> +                    "object instead")
> +            grantee_type = grantee
> +            grantee = None
> +        else:
> +            grantee_type = GitGranteeType.PERSON
> +
> +        self.repository = rule.repository
> +        self.rule = rule
> +        self.grantee_type = grantee_type
> +        self.grantee = grantee
> +        self.can_create = can_create
> +        self.can_push = can_push
> +        self.can_force_push = can_force_push
> +        self.grantor = grantor
> +        self.date_created = date_created
> +        self.date_last_modified = date_created
> +
> +    def __repr__(self):
> +        permissions = []
> +        if self.can_create:
> +            permissions.append("create")
> +        if self.can_push:
> +            permissions.append("push")
> +        if self.can_force_push:
> +            permissions.append("force-push")
> +        if self.grantee_type == GitGranteeType.PERSON:
> +            grantee_name = "~%s" % self.grantee.name
> +        else:
> +            grantee_name = self.grantee_type.title.lower()
> +        return "<GitRuleGrant [%s] to %s> for %r" % (
> +            ", ".join(permissions), grantee_name, self.rule)

> positioning seems weird here too.

> +
> +    def destroySelf(self):
> +        """See `IGitRuleGrant`."""
> +        Store.of(self).remove(self)
> 
> === modified file 'lib/lp/registry/personmerge.py'
> --- lib/lp/registry/personmerge.py	2015-09-16 13:30:33 +0000
> +++ lib/lp/registry/personmerge.py	2018-09-25 18:01:53 +0000
> @@ -141,6 +141,25 @@
>          ''' % vars())
>  
>  
> +def _mergeGitRuleGrant(cur, from_id, to_id):
> +    # Update only the GitRuleGrants that will not conflict.
> +    cur.execute('''
> +        UPDATE GitRuleGrant
> +        SET grantee=%(to_id)d
> +        WHERE
> +            grantee = %(from_id)d
> +            AND rule NOT IN (
> +                SELECT rule
> +                FROM GitRuleGrant
> +                WHERE grantee = %(to_id)d
> +                )
> +        ''' % vars())
> +    # and delete those left over.
> +    cur.execute('''
> +        DELETE FROM GitRuleGrant WHERE grantee = %(from_id)d
> +        ''' % vars())

Is it possible for this to remove what is in effect a deny grant?

> +
> +
>  def _mergeBranches(from_person, to_person):
>      # This shouldn't use removeSecurityProxy.
>      branches = getUtility(IBranchCollection).ownedBy(from_person)


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-permissions-model/+merge/354201
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.