Merge ~andrey-fedoseev/launchpad:archive-subscriptions-links into launchpad:master

[Andrey Fedoseev]

Archive subscriptions: add links to PPAs

LP: #860268

Add `checkViewPermission` to `ArchiveSet`

Update `ViewArchive` to use `ArchiveSet.checkViewPermission`

Add `result` argument to `precache_permission_for_objects`

I added links to PPAs on the Archive Subscriptions page

For that, I need to check `launchpad.View` permission for each of the archives on that page.

Check permission for each individual archive may be expensive since it needs to run a few database queries per-archive. As a workaround, I added a method the `ArchiveSet.checkViewPermission` which runs the check for multiple archives at once.

This is ready for review, but I am going to add some tests for `ArchiveSet.checkViewPermission` before I merge it.

[Colin Watson (cjwatson)]

Thanks for tackling this! This did indeed turn out to be a bit less trivial than I was expecting, but this is a very creditable first attempt and it should be great experience with Storm and with how authorization works in Launchpad. Let me know if you have any questions about these comments, since some of them are quite complex.

review: Needs Fixing

[Andrey Fedoseev (andrey-fedoseev)]

Colin,

I've made the requested changes, except for changing `archive.owner.id` to `archive.ownerID` (I have issues with that, see my other comment).

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/security.py b/lib/lp/security.py
index c8f518f..4eeacbc 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -232,7 +232,10 @@ from lp.snappy.interfaces.snappyseries import (
     ISnappySeriesSet,
     )
 from lp.snappy.interfaces.snapsubscription import ISnapSubscription
-from lp.soyuz.interfaces.archive import IArchive
+from lp.soyuz.interfaces.archive import (
+    IArchive,
+    IArchiveSet,
+    )
 from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthToken
 from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.interfaces.archivesubscriber import (
@@ -2784,35 +2787,11 @@ class ViewArchive(AuthorizationBase):
     usedfor = IArchive
 
     def checkAuthenticated(self, user):
-        """Verify that the user can view the archive.
-
-        Anyone can see a public and enabled archive.
-
-        Only Launchpad admins and uploaders can view private or disabled
-        archives.
-        """
-        # No further checks are required if the archive is public and
-        # enabled.
-        if not self.obj.private and self.obj.enabled:
-            return True
-
-        # Administrator are allowed to view private archives.
-        if user.in_admin or user.in_commercial_admin:
-            return True
-
-        # Registry experts are allowed to view public but disabled archives
-        # (since they are allowed to disable archives).
-        if (not self.obj.private and not self.obj.enabled and
-                user.in_registry_experts):
-            return True
-
-        # Owners can view the PPA.
-        if user.inTeam(self.obj.owner):
-            return True
-
-        filter = get_enabled_archive_filter(user.person)
-        return not IStore(self.obj).find(
-            Archive.id, And(Archive.id == self.obj.id, filter)).is_empty()
+        """Verify that the user can view the archive."""
+        archive_set = getUtility(IArchiveSet) # type: IArchiveSet
+        return archive_set.checkViewPermission(
+            [self.obj], user.person
+        )[self.obj]
 
     def checkUnauthenticated(self):
         """Unauthenticated users can see the PPA if it's not private."""
diff --git a/lib/lp/services/webapp/authorization.py b/lib/lp/services/webapp/authorization.py
index 558e3fe..018baa3 100644
--- a/lib/lp/services/webapp/authorization.py
+++ b/lib/lp/services/webapp/authorization.py
@@ -337,7 +337,9 @@ def iter_authorization(objecttoauthorize, permission, principal, cache,
             yield result
 
 
-def precache_permission_for_objects(participation, permission_name, objects):
+def precache_permission_for_objects(
+    participation, permission_name, objects, result=True
+):
     """Precaches the permission for the objects into the policy cache."""
     if participation is None:
         participation = getInteraction().participations[0]
@@ -347,7 +349,7 @@ def precache_permission_for_objects(participation, permission_name, objects):
     for obj in objects:
         naked_obj = removeSecurityProxy(obj)
         obj_permission_cache = permission_cache.setdefault(naked_obj, {})
-        obj_permission_cache[permission_name] = True
+        obj_permission_cache[permission_name] = result
 
 
 def check_permission(permission_name, context):
diff --git a/lib/lp/services/webapp/tests/test_authorization.py b/lib/lp/services/webapp/tests/test_authorization.py
index 5e31460..ebac5e9 100644
--- a/lib/lp/services/webapp/tests/test_authorization.py
+++ b/lib/lp/services/webapp/tests/test_authorization.py
@@ -521,13 +521,29 @@ class TestPrecachePermissionForObjects(TestCase):
         # policy cache for the permission specified.
         class Boring:
             """A boring, but weakref-able object."""
-        objects = [Boring(), Boring()]
+        viewable_objects = [Boring(), Boring()]
+        non_viewable_objects = [Boring(), Boring()]
         request = LaunchpadTestRequest()
         login(ANONYMOUS, request)
-        precache_permission_for_objects(request, 'launchpad.View', objects)
+        precache_permission_for_objects(
+            request, 'launchpad.View', viewable_objects
+        )
+        precache_permission_for_objects(
+            request, 'launchpad.View', non_viewable_objects, result=False
+        )
         # Confirm that the objects have the permission set.
-        self.assertTrue(check_permission('launchpad.View', objects[0]))
-        self.assertTrue(check_permission('launchpad.View', objects[1]))
+        self.assertTrue(
+            check_permission('launchpad.View', viewable_objects[0])
+        )
+        self.assertTrue(
+            check_permission('launchpad.View', viewable_objects[1])
+        )
+        self.assertFalse(
+            check_permission('launchpad.View', non_viewable_objects[0])
+        )
+        self.assertFalse(
+            check_permission('launchpad.View', non_viewable_objects[1])
+        )
 
     def test_default_request(self):
         # If no request is provided, the current interaction is used.
diff --git a/lib/lp/soyuz/browser/archivesubscription.py b/lib/lp/soyuz/browser/archivesubscription.py
index 8cff69b..e472bde 100644
--- a/lib/lp/soyuz/browser/archivesubscription.py
+++ b/lib/lp/soyuz/browser/archivesubscription.py
@@ -336,6 +336,29 @@ class PersonArchiveSubscriptionsView(LaunchpadView):
         for archive in archives:
             get_property_cache(archive)._known_subscribers = [self.context]
 
+        # Check if the user can view the archives and cache the permission
+        # check results
+        viewable_archives = []
+        non_viewable_archives = []
+        archive_set = getUtility(IArchiveSet) # type: IArchiveSet
+        for archive, has_view_permission in archive_set.checkViewPermission(
+            archives, self.user
+        ).items():
+            if has_view_permission:
+                viewable_archives.append(archive)
+            else:
+                non_viewable_archives.append(archive)
+        precache_permission_for_objects(
+            None,
+            'launchpad.View',
+            viewable_archives, result=True
+        )
+        precache_permission_for_objects(
+            None,
+            'launchpad.View',
+            non_viewable_archives, result=False
+        )
+
         # Turn the result set into a list of dicts so it can be easily
         # accessed in TAL. Note that we need to ensure that only one
         # PersonalArchiveSubscription is included for each archive,
diff --git a/lib/lp/soyuz/interfaces/archive.py b/lib/lp/soyuz/interfaces/archive.py
index 5ee645a..fb2ef57 100644
--- a/lib/lp/soyuz/interfaces/archive.py
+++ b/lib/lp/soyuz/interfaces/archive.py
@@ -54,6 +54,7 @@ __all__ = [
 
 import http.client
 import re
+import typing
 from urllib.parse import urlparse
 
 from lazr.restful.declarations import (
@@ -2515,6 +2516,23 @@ class IArchiveSet(Interface):
             that are currently published in the given archives.
         """
 
+    def checkViewPermission(
+        archives: typing.List[IArchive], user: IPerson
+    ) -> typing.Dict[IArchive, bool]:
+        """
+        Given a collection of archives, check if the user can view
+        each of them.
+
+        Anyone can see a public and enabled archive.
+
+        Only Launchpad admins and uploaders can view private or disabled
+        archives.
+
+        :param archives: a collection of `IArchive` objects
+        :param user: a user (a `IPerson` object)
+        :return: a mapping of `IArchive` -> `bool`, where the values represent
+            the result of the permission check.
+        """
 
 default_name_by_purpose = {
     ArchivePurpose.PRIMARY: 'primary',
diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
index 4d1ccb1..76fbadf 100644
--- a/lib/lp/soyuz/model/archive.py
+++ b/lib/lp/soyuz/model/archive.py
@@ -13,6 +13,7 @@ __all__ = [
 
 from operator import attrgetter
 import re
+import typing
 
 from lazr.lifecycle.event import ObjectCreatedEvent
 import six
@@ -82,6 +83,7 @@ from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.interfaces.distroseriesparent import IDistroSeriesParentSet
 from lp.registry.interfaces.gpg import IGPGKeySet
 from lp.registry.interfaces.person import (
+    IPerson,
     IPersonSet,
     validate_person,
     )
@@ -3049,6 +3051,52 @@ class ArchiveSet:
 
         return results.order_by(SourcePackagePublishingHistory.id)
 
+    def checkViewPermission(
+        self, archives: typing.List[IArchive], user: IPerson
+    ) -> typing.Dict[IArchive, bool]:
+        """See `IArchiveSet`."""
+        allowed_ids = set()
+        ids_to_check_in_database = []
+        roles = IPersonRoles(user)
+        for archive in archives:
+            # No further checks are required if the archive is public and
+            # enabled.
+            if not archive.private and archive.enabled:
+                allowed_ids.add(archive.id)
+
+            # Administrators are allowed to view private archives.
+            elif roles.in_admin or roles.in_commercial_admin:
+                allowed_ids.add(archive.id)
+
+            # Registry experts are allowed to view public but disabled archives
+            # (since they are allowed to disable archives).
+            elif (not archive.private and not archive.enabled and
+                    roles.in_registry_experts):
+                allowed_ids.add(archive.id)
+
+            # Users that are direct owners (not through a team)
+            # can view the PPA.
+            # This is an optimization to avoid making a database query
+            # when a user is the direct owner of the PPA.
+            # Team ownership is accounted for in `get_enabled_archive_filter`
+            # below
+            elif user.id == removeSecurityProxy(archive).ownerID:
+                allowed_ids.add(archive.id)
+
+            else:
+                ids_to_check_in_database.append(archive.id)
+
+        if ids_to_check_in_database:
+            store = IStore(Archive)
+            allowed_ids.update(
+                store.find(
+                    Archive.id,
+                    Archive.id.is_in(ids_to_check_in_database),
+                    get_enabled_archive_filter(user),
+                )
+            )
+        return {archive: archive.id in allowed_ids for archive in archives}
+
     def empty_list(self):
         """See `IArchiveSet."""
         return []
diff --git a/lib/lp/soyuz/templates/person-archive-subscriptions.pt b/lib/lp/soyuz/templates/person-archive-subscriptions.pt
index 8711378..19d39ef 100644
--- a/lib/lp/soyuz/templates/person-archive-subscriptions.pt
+++ b/lib/lp/soyuz/templates/person-archive-subscriptions.pt
@@ -30,8 +30,12 @@
                 <tal:subscription_and_token
                     define="subscription subscription_with_token/subscription;
                             token subscription_with_token/token">
-                <td class="ppa_display_name">
-                  <tal:display_name content="subscription/archive/displayname">
+                <td class="ppa_display_name"
+                    tal:define="archive_link subscription/archive/fmt:link">
+                  <tal:link condition="archive_link" content="structure archive_link">
+                    Private PPA for Celso
+                  </tal:link>
+                  <tal:display_name condition="not: archive_link" content="subscription/archive/displayname">
                     Private PPA for Celso
                   </tal:display_name>
                   (<tal:reference content="subscription/archive/fmt:reference">
diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py
index fd3d254..ae21161 100644
--- a/lib/lp/soyuz/tests/test_archive.py
+++ b/lib/lp/soyuz/tests/test_archive.py
@@ -4050,6 +4050,144 @@ class TestArchiveSetGetByReference(TestCaseWithFactory):
         self.assertLookupFails('~enoent/twonoent/threenoent/fournoent')
 
 
+class TestArchiveSetCheckViewPermission(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super().setUp()
+        self.archive_set = getUtility(IArchiveSet)
+        self.public_archive = self.factory.makeArchive(private=False)
+        self.public_disabled_archive = self.factory.makeArchive(
+            private=False, enabled=False
+        )
+        self.private_archive = self.factory.makeArchive(private=True)
+
+    def test_public_enabled_archives(self):
+        somebody = self.factory.makePerson()
+        with person_logged_in(somebody):
+            results = self.archive_set.checkViewPermission(
+                [
+                    self.public_archive,
+                    self.public_disabled_archive,
+                    self.private_archive
+                ],
+                somebody,
+            )
+        self.assertDictEqual(results, {
+            self.public_archive: True,
+            self.public_disabled_archive: False,
+            self.private_archive: False,
+        })
+
+    def test_admin_can_view(self):
+        admin = self.factory.makeAdministrator()
+        with person_logged_in(admin):
+            results = self.archive_set.checkViewPermission(
+                [
+                    self.public_archive,
+                    self.public_disabled_archive,
+                    self.private_archive
+                ],
+                admin,
+            )
+        self.assertDictEqual(results, {
+            self.public_archive: True,
+            self.public_disabled_archive: True,
+            self.private_archive: True,
+        })
+        comm_admin = self.factory.makeCommercialAdmin()
+        with person_logged_in(comm_admin):
+            results = self.archive_set.checkViewPermission(
+                [
+                    self.public_archive,
+                    self.public_disabled_archive,
+                    self.private_archive
+                ],
+                comm_admin,
+            )
+        self.assertDictEqual(results, {
+            self.public_archive: True,
+            self.public_disabled_archive: True,
+            self.private_archive: True,
+        })
+
+    def test_registry_experts(self):
+        registry_expert = self.factory.makeRegistryExpert()
+        with person_logged_in(registry_expert):
+            results = self.archive_set.checkViewPermission(
+                [
+                    self.public_archive,
+                    self.public_disabled_archive,
+                    self.private_archive
+                ],
+                registry_expert,
+            )
+        self.assertDictEqual(results, {
+            self.public_archive: True,
+            self.public_disabled_archive: True,
+            self.private_archive: False,
+        })
+
+    def test_owner(self):
+        owner = self.factory.makePerson()
+        enabled_archive = self.factory.makeArchive(
+            owner=owner, private=False, enabled=True
+        )
+        disabled_archive = self.factory.makeArchive(
+            owner=owner, private=False, enabled=False
+        )
+        with person_logged_in(owner):
+            results = self.archive_set.checkViewPermission(
+                [
+                    enabled_archive,
+                    disabled_archive,
+                ],
+                owner,
+            )
+        self.assertDictEqual(results, {
+            enabled_archive: True,
+            disabled_archive: True,
+        })
+
+    def test_team_owner(self):
+        team_member = self.factory.makePerson()
+        team = self.factory.makeTeam(members=[team_member])
+        enabled_archive = self.factory.makeArchive(
+            owner=team, private=False, enabled=True
+        )
+        disabled_archive = self.factory.makeArchive(
+            owner=team, private=False, enabled=False
+        )
+        with person_logged_in(team_member):
+            results = self.archive_set.checkViewPermission(
+                [
+                    enabled_archive,
+                    disabled_archive,
+                ],
+                team_member,
+            )
+        self.assertDictEqual(results, {
+            enabled_archive: True,
+            disabled_archive: True,
+        })
+
+    def test_query_count(self):
+        archives = [
+            self.factory.makeArchive(private=False) for _ in range(10)
+        ]
+        somebody = self.factory.makePerson()
+        with StormStatementRecorder() as recorder1:
+            self.archive_set.checkViewPermission(
+                archives[:5], somebody
+            )
+        with StormStatementRecorder() as recorder2:
+            self.archive_set.checkViewPermission(
+                archives, somebody
+            )
+        self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
+
 class TestDisplayName(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
diff --git a/lib/lp/soyuz/tests/test_archive_subscriptions.py b/lib/lp/soyuz/tests/test_archive_subscriptions.py
index 516fcd7..50c8a3b 100644
--- a/lib/lp/soyuz/tests/test_archive_subscriptions.py
+++ b/lib/lp/soyuz/tests/test_archive_subscriptions.py
@@ -182,7 +182,14 @@ class PersonArchiveSubscriptions(TestCaseWithFactory):
     def test_query_count(self):
         subscriber = self.factory.makePerson()
         for x in range(10):
-            archive = self.factory.makeArchive(private=True)
+            archive_owner = self.factory.makePerson()
+            # some archives are owned by a user, others are owned by a team
+            archive = self.factory.makeArchive(
+                private=True,
+                owner=archive_owner if x < 5 else self.factory.makeTeam(
+                    members=[archive_owner]
+                )
+            )
             with person_logged_in(archive.owner):
                 if x >= 5:
                     team = self.factory.makeTeam(members=[subscriber])
@@ -196,7 +203,7 @@ class PersonArchiveSubscriptions(TestCaseWithFactory):
                 view = create_initialized_view(
                     subscriber, '+archivesubscriptions', principal=subscriber)
                 view.render()
-        self.assertThat(recorder, HasQueryCount(Equals(12)))
+        self.assertThat(recorder, HasQueryCount(Equals(16)))
 
     def test_getArchiveSubscriptions(self):
         # Anyone with 'View' permission on a given person is able to