Merge ~cjwatson/launchpad:private-team-many-archive-subscriptions into launchpad:master

Proposed by Colin Watson
Status: Merged
Approved by: Colin Watson
Approved revision: 49b2e99cb967a3e0a1841d9abbf78068e64d0d52
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~cjwatson/launchpad:private-team-many-archive-subscriptions
Merge into: launchpad:master
Diff against target: 41 lines (+8/-4)
2 files modified
lib/lp/registry/security.py (+5/-3)
lib/lp/registry/tests/test_security.py (+3/-1)
Reviewer Review Type Date Requested Status
Jürgen Gmach Approve
Review via email: mp+434170@code.launchpad.net

Commit message

Fix private team visibility queries for some users

Description of the change

`PublicOrPrivateTeamsExistence.checkAuthenticated` was slow for users with archive subscriptions via many different teams, because it performed security checks on all those teams when computing the set of archive IDs to which the user has access via subscriptions in order to see whether any of them match one of the target team's private PPAs. We can safely skip those security checks, because we know that the user can view all their own subscriptions.

Fixes https://oops.canonical.com/?oopsid=OOPS-7b1698553c348c7b467e93b8cc067f03.

To post a comment you must log in.
Revision history for this message
Jürgen Gmach (jugmac00) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/registry/security.py b/lib/lp/registry/security.py
2index 14866a0..0487001 100644
3--- a/lib/lp/registry/security.py
4+++ b/lib/lp/registry/security.py
5@@ -10,6 +10,7 @@ __all__ = [
6
7 from storm.expr import Select, Union
8 from zope.component import getUtility
9+from zope.security.proxy import removeSecurityProxy
10
11 from lp.app.security import (
12 AnonymousAuthorization,
13@@ -678,9 +679,10 @@ class PublicOrPrivateTeamsExistence(AuthorizationBase):
14 and self.obj.visibility == PersonVisibility.PRIVATE
15 ):
16 # Grant visibility to people with subscriptions on a private
17- # team's private PPA.
18- subscriptions = getUtility(IArchiveSubscriberSet).getBySubscriber(
19- user.person
20+ # team's private PPA. We can safely skip security checks here: the
21+ # user can view all their own subscriptions.
22+ subscriptions = removeSecurityProxy(
23+ getUtility(IArchiveSubscriberSet).getBySubscriber(user.person)
24 )
25 subscriber_archive_ids = {sub.archive_id for sub in subscriptions}
26 team_ppa_ids = {ppa.id for ppa in self.obj.ppas if ppa.private}
27diff --git a/lib/lp/registry/tests/test_security.py b/lib/lp/registry/tests/test_security.py
28index ed8d447..f31563c 100644
29--- a/lib/lp/registry/tests/test_security.py
30+++ b/lib/lp/registry/tests/test_security.py
31@@ -125,7 +125,9 @@ class TestPublicOrPrivateTeamsExistence(TestCaseWithFactory):
32 archive = self.factory.makeArchive(
33 owner=private_team, private=True
34 )
35- archive.newSubscription(person, team_owner)
36+ archive.newSubscription(
37+ self.factory.makeTeam(owner=person), team_owner
38+ )
39
40 def check_team_limited_view():
41 person.clearInTeamCache()

Subscribers

People subscribed via source and target branches

to status/vote changes: