launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Pushed the requested changes, and added a comment about `get_snap_privacy_filter` method.

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

Indeed, it doesn't need. Fixing it.

>  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."),

Done!

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

I agree that catching TypeError is a bad choice here, but I would rather change it to `if IPerson.providedBy(user) and IPersonRoles(user).in_[comercial_]admin: return True`.

This method is supposed to build the SQL clauses of visibility for Snap table, depending on the user passed (maybe method documentation above is a bit misleading mentioning "subscription").

Moving the (commercial) admin check somewhere else means repeating the same check everywhere that this `get_snap_privacy_filter` method is called, and today it's like 5 or 6 different places.

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