launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> 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

You're probably right.  I think I may be getting confused with the discrepancy between e.g. bug and Git repository privacy filters.  Bugs allow admin visibility in the privacy filter, while Git repositories (and indeed Bazaar branches) don't.

I think we should escalate the question of whether snaps should permit (commercial) admin visibility in the first place to wgrant, given that this isn't uniform across privacy-capable artifact types at the moment.

If we need admin visibility here, then consider using something like the kind of team membership check that's used in `get_bug_bulk_privacy_filter_terms` instead of `IPersonRoles`.  That approach should work with either a Storm object or something like `SnapSubscription.person_id`.

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