launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/database/schema/security.cfg b/database/schema/security.cfg
> index e9dcc62..09c4fb4 100644
> --- a/database/schema/security.cfg
> +++ b/database/schema/security.cfg
> @@ -2111,6 +2111,8 @@ public.job                              = SELECT, INSERT, UPDATE
>  public.person                           = SELECT
>  public.product                          = SELECT
>  public.sharingjob                       = SELECT, INSERT, UPDATE
> +public.snap                             = SELECT
> +public.snapsubscription                 = SELECT, INSERT, UPDATE, DELETE

I'm a little surprised that `INSERT` is needed here.

>  public.specification                    = SELECT
>  public.specificationsubscription        = SELECT, DELETE
>  public.teamparticipation                = SELECT
> diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
> index 31708c3..b423263 100644
> --- a/lib/lp/snappy/interfaces/snap.py
> +++ b/lib/lp/snappy/interfaces/snap.py
> @@ -571,6 +571,10 @@ class ISnapView(Interface):
>          # Really ISnapBuild, patched in lp.snappy.interfaces.webservice.
>          value_type=Reference(schema=Interface), readonly=True)))
>  
> +    subscribers = CollectionField(
> +        title=_("Persons subscribed to this repository."),

"subscribed to this snap recipe"

> +        readonly=True, value_type=Reference(IPerson))
> +
>      def visibleByUser(user):
>          """Can the specified user see this snap recipe?"""
>  
> diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
> index 1b98ad5..5c53da2 100644
> --- a/lib/lp/snappy/model/snap.py
> +++ b/lib/lp/snappy/model/snap.py
> @@ -1698,8 +1713,25 @@ def get_snap_privacy_filter(user):
>      """Returns the filter for all private Snaps that the given user is
>      subscribed to (that is, has access without being directly an owner).
>  
> +    :param user: An IPerson, or a class attribute that references an IPerson
> +                 in the database.
>      :return: A storm condition.
>      """
> +    try:
> +        roles = IPersonRoles(user)
> +    except TypeError:
> +        # If we cannot adapt `user` to IPersonRoles, continue with creating
> +        # the clause and skip the check for commercial admins.
> +        # By doing this, we keep this function compatible with
> +        # `user` as a class property. For example we can use it like
> +        # get_snap_privacy_filter(SnapSubscription.person_id) and use the
> +        # resulting clause to filter SnapSubscription objects based on its
> +        # snap user's grants.
> +        pass

This is pretty strange, and I don't think other privacy filters have anything similar.  Catching TypeError is generally pretty risky.

I think it might be better to lift the admin/commercial_admin checks out to somewhere else.  (Commercial) admins should be able to see the objects in question by way of the ViewSnap security adapter, but they shouldn't be treated as if they had an actual subscription.

> +    else:
> +        if roles.in_admin or roles.in_commercial_admin:
> +            return True
> +
>      # XXX pappacena 2021-02-12: Once we do the migration to back fill
>      # information_type, we should be able to change this.
>      private_snap = SQL(


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