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
diff --git a/lib/lp/registry/security.py b/lib/lp/registry/security.py
index 14866a0..0487001 100644
--- a/lib/lp/registry/security.py
+++ b/lib/lp/registry/security.py
@@ -10,6 +10,7 @@ __all__ = [
1010
11from storm.expr import Select, Union11from storm.expr import Select, Union
12from zope.component import getUtility12from zope.component import getUtility
13from zope.security.proxy import removeSecurityProxy
1314
14from lp.app.security import (15from lp.app.security import (
15 AnonymousAuthorization,16 AnonymousAuthorization,
@@ -678,9 +679,10 @@ class PublicOrPrivateTeamsExistence(AuthorizationBase):
678 and self.obj.visibility == PersonVisibility.PRIVATE679 and self.obj.visibility == PersonVisibility.PRIVATE
679 ):680 ):
680 # Grant visibility to people with subscriptions on a private681 # Grant visibility to people with subscriptions on a private
681 # team's private PPA.682 # team's private PPA. We can safely skip security checks here: the
682 subscriptions = getUtility(IArchiveSubscriberSet).getBySubscriber(683 # user can view all their own subscriptions.
683 user.person684 subscriptions = removeSecurityProxy(
685 getUtility(IArchiveSubscriberSet).getBySubscriber(user.person)
684 )686 )
685 subscriber_archive_ids = {sub.archive_id for sub in subscriptions}687 subscriber_archive_ids = {sub.archive_id for sub in subscriptions}
686 team_ppa_ids = {ppa.id for ppa in self.obj.ppas if ppa.private}688 team_ppa_ids = {ppa.id for ppa in self.obj.ppas if ppa.private}
diff --git a/lib/lp/registry/tests/test_security.py b/lib/lp/registry/tests/test_security.py
index ed8d447..f31563c 100644
--- a/lib/lp/registry/tests/test_security.py
+++ b/lib/lp/registry/tests/test_security.py
@@ -125,7 +125,9 @@ class TestPublicOrPrivateTeamsExistence(TestCaseWithFactory):
125 archive = self.factory.makeArchive(125 archive = self.factory.makeArchive(
126 owner=private_team, private=True126 owner=private_team, private=True
127 )127 )
128 archive.newSubscription(person, team_owner)128 archive.newSubscription(
129 self.factory.makeTeam(owner=person), team_owner
130 )
129131
130 def check_team_limited_view():132 def check_team_limited_view():
131 person.clearInTeamCache()133 person.clearInTeamCache()

Subscribers

People subscribed via source and target branches

to status/vote changes: