launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28492
[Merge] ~andrey-fedoseev/launchpad:archive-subscriptions-links into launchpad:master
Andrey Fedoseev has proposed merging ~andrey-fedoseev/launchpad:archive-subscriptions-links into launchpad:master.
Commit message:
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`
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #860268 in Launchpad itself: "+archivesubscriptions doesn't link to the archives"
https://bugs.launchpad.net/launchpad/+bug/860268
For more details, see:
https://code.launchpad.net/~andrey-fedoseev/launchpad/+git/launchpad/+merge/423346
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~andrey-fedoseev/launchpad:archive-subscriptions-links into launchpad:master.
diff --git a/lib/lp/security.py b/lib/lp/security.py
index c8f518f..a3e8a58 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,9 @@ 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: IArchiveSet = getUtility(IArchiveSet)
+ return archive_set.checkViewPermission([self.obj], user)[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..3edcec9 100644
--- a/lib/lp/soyuz/browser/archivesubscription.py
+++ b/lib/lp/soyuz/browser/archivesubscription.py
@@ -36,6 +36,7 @@ from lp.app.browser.launchpadform import (
from lp.app.widgets.date import DateWidget
from lp.app.widgets.popup import PersonPickerWidget
from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.role import IPersonRoles
from lp.services.fields import PersonChoice
from lp.services.propertycache import (
cachedproperty,
@@ -336,6 +337,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: IArchiveSet = getUtility(IArchiveSet)
+ for archive, has_view_permission in archive_set.checkViewPermission(
+ archives, IPersonRoles(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 e011591..30abac0 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 (
@@ -104,7 +105,10 @@ from lp.registry.interfaces.distroseries import IDistroSeries
from lp.registry.interfaces.gpg import IGPGKey
from lp.registry.interfaces.person import IPerson
from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.registry.interfaces.role import IHasOwner
+from lp.registry.interfaces.role import (
+ IHasOwner,
+ IPersonRoles,
+ )
from lp.services.fields import (
PersonChoice,
PublicPersonChoice,
@@ -2503,6 +2507,23 @@ class IArchiveSet(Interface):
that are currently published in the given archives.
"""
+ def checkViewPermission(
+ archives: typing.List[IArchive], user: IPersonRoles
+ ) -> 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 `IPersonRoles` 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 727d8b2..8d6e844 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
@@ -23,6 +24,7 @@ from storm.expr import (
Count,
Desc,
Exists,
+ In,
Join,
Not,
Or,
@@ -3020,6 +3022,54 @@ class ArchiveSet:
return results.order_by(SourcePackagePublishingHistory.id)
+ def checkViewPermission(
+ self, archives: typing.List[IArchive], user: IPersonRoles
+ ) -> typing.Dict[IArchive, bool]:
+ """See `IArchiveSet`."""
+ allowed_ids = set()
+ ids_to_check_in_database = []
+ 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)
+
+ # Administrator are allowed to view private archives.
+ elif user.in_admin or user.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
+ user.in_registry_experts):
+ allowed_ids.add(archive.id)
+
+ # Owners can view the PPA.
+ elif user.id == archive.owner.id:
+ allowed_ids.add(archive.id)
+
+ else:
+ ids_to_check_in_database.append(archive.id)
+
+ if ids_to_check_in_database:
+ filter = get_enabled_archive_filter(user.person)
+ user_teams = Select(
+ TeamParticipation.teamID,
+ where=TeamParticipation.person == user.person
+ )
+ is_owner = Archive.ownerID.is_in(user_teams)
+ store = IStore(Archive)
+ allowed_ids.update(
+ store.find(
+ Archive.id,
+ And(
+ In(Archive.id, ids_to_check_in_database),
+ Or(filter, is_owner)
+ )
+ ).values(Archive.id)
+ )
+ 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_subscriptions.py b/lib/lp/soyuz/tests/test_archive_subscriptions.py
index 516fcd7..60f36d8 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 owner 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
