Re: [Launchpad-reviewers] [Merge] lp:~twom/launchpad/rework-git-permissions-for-shadowing into lp:launchpad
Review: Needs Fixing
Diff comments:
>
> === modified file 'lib/lp/code/interfaces/gitrule.py'
> --- lib/lp/code/interfaces/gitrule.py 2018-10-16 15:29:37 +
> +++ lib/lp/code/interfaces/gitrule.py 2018-10-18 13:49:45 +
> @@ -71,6 +71,16 @@
>
> grants = Attribute("The access grants for this rule.")
>
> +def findRuleGrantsByGrantee(grantee):
> +"""Find the grants for a grantee applied to this repository.
> +
> +:param grantee: The Person affected
> +"""
> +
> +def findRuleGrantsForRepositoryOwner():
> +"""Find the grants of type REPOSITORY_OWNER applied to this
> repository.
> +"""
s/this repository/this rule/ on both these docstrings, if you're leaving them
here. (But see below; I think it might be better to move this back to
IGitRepository.)
> +
>
> class IGitRuleEditableAttributes(Interface):
> """`IGitRule` attributes that can be edited.
>
> === modified file 'lib/lp/code/xmlrpc/git.py'
> --- lib/lp/code/xmlrpc/git.py 2018-10-17 10:09:33 +
> +++ lib/lp/code/xmlrpc/git.py 2018-10-18 13:49:45 +
> @@ -414,3 +418,70 @@
> self._listRefRules,
> translated_path,
> )
> +
> +def make_regex(self, pattern):
> +return re.compile(self.glob_to_re(pattern.rstrip(b'\n')))
> +
> +def glob_to_re(self, s):
> +"""Convert a glob to a regular expression.
> +
> +The only wildcard supported is "*", to match any path segment.
This is now any number of characters, rather than just any path segment.
However, should we consider just using the standard library's `fnmatch`
instead? That's what I'd specified before I realised that turnip had its own
implementation, and I think it probably still makes sense: it's easy enough to
explain, it's a bit more useful, and all its metacharacters are invalid in ref
names.
> +"""
> +return b'^%s\Z' % (
> +b''.join(b'.*' if c == b'*' else re.escape(c) for c in s))
> +
> +def _find_matching_rules(self, repository, ref_path):
> +"""Find all matching rules for a given ref path"""
> +
Extra blank line.
> +matching_rules = []
> +for rule in repository.rules:
> +regex = self.make_regex(rule.ref_pattern)
> +if regex.match(ref_path):
> +matching_rules.append(rule)
> +return matching_rules
> +
> +def _checkRefPermissions(self, requester, translated_path, ref_paths):
> +repository = removeSecurityProxy(
> +getUtility(IGitLookup).getByHostingPath(translated_path))
> +is_owner = requester.inTeam(repository.owner)
> +result = {}
> +for ref in ref_paths:
> +matching_rules = self._find_matching_rules(repository, ref)
> +if is_owner and not matching_rules:
> +result[ref] = ['create', 'push', 'force_push']
> +continue
> +seen_grantees = []
This should be a set, so that membership checks are cheap.
> +union_permissions = set()
> +for rule in matching_rules:
> +grants = rule.findRuleGrantsByGrantee(requester)
> +if is_owner:
> +owner_grants = rule.findRuleGrantsForRepositoryOwner()
> +grants = grants.union(owner_grants)
This is potentially quite a lot of database queries: it's one or two per
matching rule per ref being pushed. Instead, could you collect all the grants
across the whole repository up-front, organise them into a dict indexed by
rule, and look them up in that dict as you go? The DB indexes are arranged
with the intent of making this an efficient operation.
> +
> +for grant in grants:
> +if (grant.grantee, grant.grantee_type) in seen_grantees:
> +continue
> +permissions = self._buildPermissions(grant)
> +union_permissions.update(permissions)
> +seen_grantees.append(
> +(grant.grantee, grant.grantee_type)
> +)
Could be on one line.
> +
> +owner_grantees = any(x[1] == GitGranteeType.REPOSITORY_OWNER
> + for x in seen_grantees)
> +if is_owner and not owner_grantees:
if is_owner and (None, GitGranteeType.REPOSITORY_OWNER) not in seen_grantees:
> +union_permissions.update(['create', 'push'])
> +
> +sorted_permissions = self._sortPermissions(union_permissions)
> +result[ref] = sorted_permissions
> +return result
> +
> +def checkRefPermissions(self, translated_path, ref_paths, auth_params):
> +""" See `IGitAPI`"""
> +requester_id = auth_params.get("uid")
> +return run_with_login(
> +requester_id,
> +self._checkRefPermissions,
> +translated_path,
> +ref_paths
> +
[Launchpad-reviewers] [Merge] lp:~wgrant/launchpad/genericise-cron.publish-ppa into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/genericise-cron.publish-ppa into lp:launchpad. Commit message: Make cron.publish-ppa take distro selection criteria as arguments. Requested reviews: Launchpad code reviewers (launchpad-reviewers) For more details, see: https://code.launchpad.net/~wgrant/launchpad/genericise-cron.publish-ppa/+merge/357123 This is live on production already. -- Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/genericise-cron.publish-ppa into lp:launchpad. === modified file 'cronscripts/publishing/cron.publish-ppa' --- cronscripts/publishing/cron.publish-ppa 2014-08-05 10:37:07 + +++ cronscripts/publishing/cron.publish-ppa 2018-10-19 05:27:09 + @@ -10,12 +10,8 @@ LPCURRENT=`dirname $0`/../.. -# Publish Ubuntu PPAs. -$LPCURRENT/scripts/process-accepted.py -v --ppa -d ubuntu -$LPCURRENT/scripts/publish-distro.py -v --ppa -d ubuntu -$LPCURRENT/scripts/publish-distro.py -v --private-ppa -d ubuntu - -# Publish PPAs for other distros. -$LPCURRENT/scripts/process-accepted.py -v --ppa --all-derived -$LPCURRENT/scripts/publish-distro.py -v --ppa --all-derived -$LPCURRENT/scripts/publish-distro.py -v --private-ppa --all-derived +for DISTRO_ARG in "$@"; do + $LPCURRENT/scripts/process-accepted.py -v --ppa $DISTRO_ARG + $LPCURRENT/scripts/publish-distro.py -v --ppa $DISTRO_ARG + $LPCURRENT/scripts/publish-distro.py -v --private-ppa $DISTRO_ARG +done ___ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
[Launchpad-reviewers] [Merge] lp:~twom/launchpad/rework-git-permissions-for-shadowing into lp:launchpad
Tom Wardill has proposed merging
lp:~twom/launchpad/rework-git-permissions-for-shadowing into lp:launchpad.
Commit message:
Rework git branch permissions to improve shadowing for multiple grants
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~twom/launchpad/rework-git-permissions-for-shadowing/+merge/357091
--
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~twom/launchpad/rework-git-permissions-for-shadowing into lp:launchpad.
=== modified file 'lib/lp/code/interfaces/gitapi.py'
--- lib/lp/code/interfaces/gitapi.py 2018-09-28 13:53:41 +
+++ lib/lp/code/interfaces/gitapi.py 2018-10-18 13:49:45 +
@@ -73,3 +73,10 @@
:returns: A list of rules for the user in the specified repository
"""
+
+def checkRefPermissions(self, repository, ref_paths, user):
+"""Return a list of ref rules for a `user` in a `repository` that
+match the input refs.
+
+:returns: A list of rules for the user in the specified repository
+"""
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2018-10-16 10:10:10 +
+++ lib/lp/code/interfaces/gitrepository.py 2018-10-18 13:49:45 +
@@ -748,16 +748,6 @@
:param user: The `IPerson` who is moving the rule.
"""
-def findRuleGrantsByGrantee(grantee):
-"""Find the grants for a grantee applied to this repository.
-
-:param grantee: The Person affected
-"""
-
-def findRuleGrantsForRepositoryOwner():
-"""Find the grants of type REPOSITORY_OWNER applied to this repository.
-"""
-
@export_read_operation()
@operation_for_version("devel")
def canBeDeleted():
=== modified file 'lib/lp/code/interfaces/gitrule.py'
--- lib/lp/code/interfaces/gitrule.py 2018-10-16 15:29:37 +
+++ lib/lp/code/interfaces/gitrule.py 2018-10-18 13:49:45 +
@@ -71,6 +71,16 @@
grants = Attribute("The access grants for this rule.")
+def findRuleGrantsByGrantee(grantee):
+"""Find the grants for a grantee applied to this repository.
+
+:param grantee: The Person affected
+"""
+
+def findRuleGrantsForRepositoryOwner():
+"""Find the grants of type REPOSITORY_OWNER applied to this repository.
+"""
+
class IGitRuleEditableAttributes(Interface):
"""`IGitRule` attributes that can be edited.
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2018-10-16 10:10:10 +
+++ lib/lp/code/model/gitrepository.py 2018-10-18 13:49:45 +
@@ -72,7 +72,6 @@
from lp.app.interfaces.services import IService
from lp.code.enums import (
BranchMergeProposalStatus,
-GitGranteeType,
GitObjectType,
GitRepositoryType,
)
@@ -1194,20 +1193,6 @@
return Store.of(self).find(
GitRuleGrant, GitRuleGrant.repository_id == self.id)
-def findRuleGrantsByGrantee(self, grantee):
-"""See `IGitRepository`."""
-clauses = [
-GitRuleGrant.grantee_type == GitGranteeType.PERSON,
-TeamParticipation.person == grantee,
-GitRuleGrant.grantee == TeamParticipation.teamID
-]
-return self.grants.find(*clauses).config(distinct=True)
-
-def findRuleGrantsForRepositoryOwner(self):
-"""See `IGitRepository`."""
-return self.grants.find(
-GitRuleGrant.grantee_type == GitGranteeType.REPOSITORY_OWNER)
-
def getActivity(self, changed_after=None):
"""See `IGitRepository`."""
clauses = [GitActivity.repository_id == self.id]
=== modified file 'lib/lp/code/model/gitrule.py'
--- lib/lp/code/model/gitrule.py 2018-10-16 15:29:37 +
+++ lib/lp/code/model/gitrule.py 2018-10-18 13:49:45 +
@@ -54,6 +54,7 @@
validate_person,
validate_public_person,
)
+from lp.registry.model.teammembership import TeamParticipation
from lp.services.database.constants import (
DEFAULT,
UTC_NOW,
@@ -127,6 +128,20 @@
return Store.of(self).find(
GitRuleGrant, GitRuleGrant.rule_id == self.id)
+def findRuleGrantsByGrantee(self, grantee):
+"""See `IGitRule`."""
+clauses = [
+GitRuleGrant.grantee_type == GitGranteeType.PERSON,
+TeamParticipation.person == grantee,
+GitRuleGrant.grantee == TeamParticipation.teamID
+]
+return self.grants.find(*clauses).config(distinct=True)
+
+def findRuleGrantsForRepositoryOwner(self):
+"""See `IGitRule`."""
+return self.grants.find(
+GitRuleGrant.grantee_type == GitGranteeType.REPOSITORY_OWNER)
+
def addGrant(self, grantee, grantor, can_create=False, can_push=False,
can_force_push=False):
"""See `IGitRule`."""
=== modified file 'lib/lp/code/xmlrpc/git.py'
--- lib/lp/code/xmlrpc/git.py 2018-10-17
