launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/registry/model/accesspolicy.py b/lib/lp/registry/model/accesspolicy.py
> index 2e8fddf..472509f 100644
> --- a/lib/lp/registry/model/accesspolicy.py
> +++ b/lib/lp/registry/model/accesspolicy.py
> @@ -156,18 +162,20 @@ class AccessArtifact(StormBase):
>          insert_values = []
>          for concrete in needed:
>              if IBug.providedBy(concrete):
> -                insert_values.append((concrete, None, None, None))
> +                insert_values.append((concrete, None, None, None, None))
>              elif IBranch.providedBy(concrete):
> -                insert_values.append((None, concrete, None, None))
> +                insert_values.append((None, concrete, None, None, None))
>              elif IGitRepository.providedBy(concrete):
> -                insert_values.append((None, None, concrete, None))
> +                insert_values.append((None, None, concrete, None, None))
>              elif ISpecification.providedBy(concrete):
> -                insert_values.append((None, None, None, concrete))
> +                insert_values.append((None, None, None, concrete, None))
> +            elif ISnap.providedBy(concrete):
> +                insert_values.append((None, None, None, None, concrete))

It might be less confusing if bug, branch, gitrepository, specification, snap were in the same order in all of their occurrences in this MP (I don't really mind which way round that is, but at the moment it's sometimes bug, branch, gitrepository, snap, specification instead).

>              else:
>                  raise ValueError("%r is not a supported artifact" % concrete)
> -        new = create(
> -            (cls.bug, cls.branch, cls.gitrepository, cls.specification),
> -            insert_values, get_objects=True)
> +        columns = (cls.bug, cls.branch, cls.gitrepository, cls.specification,
> +                   cls.snap)
> +        new = create(columns, insert_values, get_objects=True)
>          return list(existing) + new
>  
>      @classmethod
> diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
> index ce14933..c3ba48a 100644
> --- a/lib/lp/snappy/model/snap.py
> +++ b/lib/lp/snappy/model/snap.py
> @@ -1350,7 +1374,8 @@ class SnapSet:
>                      private_snap == False,
>                      Snap.owner_id.is_in(Select(
>                          TeamParticipation.teamID,
> -                        TeamParticipation.person == visible_by_user)))
> +                        TeamParticipation.person == visible_by_user)),
> +                    *get_private_snap_subscriber_filter(visible_by_user))

The owner should be given an access grant, so it shouldn't be necessary to consider that case separately.

I'd suggest pushing the remainder of `_findSnapVisibilityClause` into the new helper function below, and just calling that helper directly everywhere that currently calls `_findSnapVisibilityClause`.  It seems less likely to go wrong that way.

>  
>      def findByURL(self, url, owner=None, visible_by_user=None):
>          """See `ISnapSet`."""
> @@ -1547,3 +1572,31 @@ class SnapStoreSecretsEncryptedContainer(NaClEncryptedContainerBase):
>                  config.snappy.store_secrets_private_key.encode("UTF-8"))
>          else:
>              return None
> +
> +
> +def get_private_snap_subscriber_filter(user):

`get_snap_privacy_filter`, for symmetry with other similar functions?  Especially if you move the rest of `_findSnapVisibilityClause` here as suggested above; compare `get_git_repository_privacy_filter`.

> +    """Returns the filter for all private Snaps that the given user is
> +    subscribed to (that is, has access without being directly an owner).
> +    """
> +    artifact_grant_query = Coalesce(
> +        ArrayIntersects(
> +            SQL("%s.access_grants" % Snap.__storm_table__),
> +            Select(
> +                ArrayAgg(TeamParticipation.teamID),
> +                tables=TeamParticipation,
> +                where=(TeamParticipation.person == user)
> +            )), False)
> +
> +    policy_grant_query = Coalesce(
> +        ArrayIntersects(
> +            Array(SQL("%s.access_policy" % Snap.__storm_table__)),
> +            Select(
> +                ArrayAgg(AccessPolicyGrant.policy_id),
> +                tables=(AccessPolicyGrant,
> +                        Join(TeamParticipation,
> +                             TeamParticipation.teamID ==
> +                             AccessPolicyGrant.grantee_id)),
> +                where=(TeamParticipation.person == user)
> +            )), False)
> +
> +    return [Or(artifact_grant_query, policy_grant_query)]


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/397692
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:snap-pillar-ui.