Merge ~andrey-fedoseev/launchpad:archive-subscriptions-links into launchpad:master
- Git
- lp:~andrey-fedoseev/launchpad
- archive-subscriptions-links
- Merge into master
Status: | Merged |
---|---|
Approved by: | Andrey Fedoseev |
Approved revision: | c4fbaa3203b2edd29741580a7997c004eb5753c5 |
Merge reported by: | Otto Co-Pilot |
Merged at revision: | not available |
Proposed branch: | ~andrey-fedoseev/launchpad:archive-subscriptions-links |
Merge into: | launchpad:master |
Diff against target: |
459 lines (+275/-40) 9 files modified
lib/lp/security.py (+9/-30) lib/lp/services/webapp/authorization.py (+4/-2) lib/lp/services/webapp/tests/test_authorization.py (+20/-4) lib/lp/soyuz/browser/archivesubscription.py (+23/-0) lib/lp/soyuz/interfaces/archive.py (+18/-0) lib/lp/soyuz/model/archive.py (+48/-0) lib/lp/soyuz/templates/person-archive-subscriptions.pt (+6/-2) lib/lp/soyuz/tests/test_archive.py (+138/-0) lib/lp/soyuz/tests/test_archive_subscriptions.py (+9/-2) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Approve | ||
Review via email: mp+423346@code.launchpad.net |
Commit message
Archive subscriptions: add links to PPAs
LP: #860268
Add `checkViewPermi
Update `ViewArchive` to use `ArchiveSet.
Add `result` argument to `precache_
Description of the change
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.
This is ready for review, but I am going to add some tests for `ArchiveSet.
Andrey Fedoseev (andrey-fedoseev) : | # |
Andrey Fedoseev (andrey-fedoseev) wrote : | # |
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) : | # |
Andrey Fedoseev (andrey-fedoseev) : | # |
Colin Watson (cjwatson) : | # |
Andrey Fedoseev (andrey-fedoseev) : | # |
Preview Diff
1 | diff --git a/lib/lp/security.py b/lib/lp/security.py | |||
2 | index c8f518f..4eeacbc 100644 | |||
3 | --- a/lib/lp/security.py | |||
4 | +++ b/lib/lp/security.py | |||
5 | @@ -232,7 +232,10 @@ from lp.snappy.interfaces.snappyseries import ( | |||
6 | 232 | ISnappySeriesSet, | 232 | ISnappySeriesSet, |
7 | 233 | ) | 233 | ) |
8 | 234 | from lp.snappy.interfaces.snapsubscription import ISnapSubscription | 234 | from lp.snappy.interfaces.snapsubscription import ISnapSubscription |
10 | 235 | from lp.soyuz.interfaces.archive import IArchive | 235 | from lp.soyuz.interfaces.archive import ( |
11 | 236 | IArchive, | ||
12 | 237 | IArchiveSet, | ||
13 | 238 | ) | ||
14 | 236 | from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthToken | 239 | from lp.soyuz.interfaces.archiveauthtoken import IArchiveAuthToken |
15 | 237 | from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet | 240 | from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet |
16 | 238 | from lp.soyuz.interfaces.archivesubscriber import ( | 241 | from lp.soyuz.interfaces.archivesubscriber import ( |
17 | @@ -2784,35 +2787,11 @@ class ViewArchive(AuthorizationBase): | |||
18 | 2784 | usedfor = IArchive | 2787 | usedfor = IArchive |
19 | 2785 | 2788 | ||
20 | 2786 | def checkAuthenticated(self, user): | 2789 | def checkAuthenticated(self, user): |
50 | 2787 | """Verify that the user can view the archive. | 2790 | """Verify that the user can view the archive.""" |
51 | 2788 | 2791 | archive_set = getUtility(IArchiveSet) # type: IArchiveSet | |
52 | 2789 | Anyone can see a public and enabled archive. | 2792 | return archive_set.checkViewPermission( |
53 | 2790 | 2793 | [self.obj], user.person | |
54 | 2791 | Only Launchpad admins and uploaders can view private or disabled | 2794 | )[self.obj] |
26 | 2792 | archives. | ||
27 | 2793 | """ | ||
28 | 2794 | # No further checks are required if the archive is public and | ||
29 | 2795 | # enabled. | ||
30 | 2796 | if not self.obj.private and self.obj.enabled: | ||
31 | 2797 | return True | ||
32 | 2798 | |||
33 | 2799 | # Administrator are allowed to view private archives. | ||
34 | 2800 | if user.in_admin or user.in_commercial_admin: | ||
35 | 2801 | return True | ||
36 | 2802 | |||
37 | 2803 | # Registry experts are allowed to view public but disabled archives | ||
38 | 2804 | # (since they are allowed to disable archives). | ||
39 | 2805 | if (not self.obj.private and not self.obj.enabled and | ||
40 | 2806 | user.in_registry_experts): | ||
41 | 2807 | return True | ||
42 | 2808 | |||
43 | 2809 | # Owners can view the PPA. | ||
44 | 2810 | if user.inTeam(self.obj.owner): | ||
45 | 2811 | return True | ||
46 | 2812 | |||
47 | 2813 | filter = get_enabled_archive_filter(user.person) | ||
48 | 2814 | return not IStore(self.obj).find( | ||
49 | 2815 | Archive.id, And(Archive.id == self.obj.id, filter)).is_empty() | ||
55 | 2816 | 2795 | ||
56 | 2817 | def checkUnauthenticated(self): | 2796 | def checkUnauthenticated(self): |
57 | 2818 | """Unauthenticated users can see the PPA if it's not private.""" | 2797 | """Unauthenticated users can see the PPA if it's not private.""" |
58 | diff --git a/lib/lp/services/webapp/authorization.py b/lib/lp/services/webapp/authorization.py | |||
59 | index 558e3fe..018baa3 100644 | |||
60 | --- a/lib/lp/services/webapp/authorization.py | |||
61 | +++ b/lib/lp/services/webapp/authorization.py | |||
62 | @@ -337,7 +337,9 @@ def iter_authorization(objecttoauthorize, permission, principal, cache, | |||
63 | 337 | yield result | 337 | yield result |
64 | 338 | 338 | ||
65 | 339 | 339 | ||
67 | 340 | def precache_permission_for_objects(participation, permission_name, objects): | 340 | def precache_permission_for_objects( |
68 | 341 | participation, permission_name, objects, result=True | ||
69 | 342 | ): | ||
70 | 341 | """Precaches the permission for the objects into the policy cache.""" | 343 | """Precaches the permission for the objects into the policy cache.""" |
71 | 342 | if participation is None: | 344 | if participation is None: |
72 | 343 | participation = getInteraction().participations[0] | 345 | participation = getInteraction().participations[0] |
73 | @@ -347,7 +349,7 @@ def precache_permission_for_objects(participation, permission_name, objects): | |||
74 | 347 | for obj in objects: | 349 | for obj in objects: |
75 | 348 | naked_obj = removeSecurityProxy(obj) | 350 | naked_obj = removeSecurityProxy(obj) |
76 | 349 | obj_permission_cache = permission_cache.setdefault(naked_obj, {}) | 351 | obj_permission_cache = permission_cache.setdefault(naked_obj, {}) |
78 | 350 | obj_permission_cache[permission_name] = True | 352 | obj_permission_cache[permission_name] = result |
79 | 351 | 353 | ||
80 | 352 | 354 | ||
81 | 353 | def check_permission(permission_name, context): | 355 | def check_permission(permission_name, context): |
82 | diff --git a/lib/lp/services/webapp/tests/test_authorization.py b/lib/lp/services/webapp/tests/test_authorization.py | |||
83 | index 5e31460..ebac5e9 100644 | |||
84 | --- a/lib/lp/services/webapp/tests/test_authorization.py | |||
85 | +++ b/lib/lp/services/webapp/tests/test_authorization.py | |||
86 | @@ -521,13 +521,29 @@ class TestPrecachePermissionForObjects(TestCase): | |||
87 | 521 | # policy cache for the permission specified. | 521 | # policy cache for the permission specified. |
88 | 522 | class Boring: | 522 | class Boring: |
89 | 523 | """A boring, but weakref-able object.""" | 523 | """A boring, but weakref-able object.""" |
91 | 524 | objects = [Boring(), Boring()] | 524 | viewable_objects = [Boring(), Boring()] |
92 | 525 | non_viewable_objects = [Boring(), Boring()] | ||
93 | 525 | request = LaunchpadTestRequest() | 526 | request = LaunchpadTestRequest() |
94 | 526 | login(ANONYMOUS, request) | 527 | login(ANONYMOUS, request) |
96 | 527 | precache_permission_for_objects(request, 'launchpad.View', objects) | 528 | precache_permission_for_objects( |
97 | 529 | request, 'launchpad.View', viewable_objects | ||
98 | 530 | ) | ||
99 | 531 | precache_permission_for_objects( | ||
100 | 532 | request, 'launchpad.View', non_viewable_objects, result=False | ||
101 | 533 | ) | ||
102 | 528 | # Confirm that the objects have the permission set. | 534 | # Confirm that the objects have the permission set. |
105 | 529 | self.assertTrue(check_permission('launchpad.View', objects[0])) | 535 | self.assertTrue( |
106 | 530 | self.assertTrue(check_permission('launchpad.View', objects[1])) | 536 | check_permission('launchpad.View', viewable_objects[0]) |
107 | 537 | ) | ||
108 | 538 | self.assertTrue( | ||
109 | 539 | check_permission('launchpad.View', viewable_objects[1]) | ||
110 | 540 | ) | ||
111 | 541 | self.assertFalse( | ||
112 | 542 | check_permission('launchpad.View', non_viewable_objects[0]) | ||
113 | 543 | ) | ||
114 | 544 | self.assertFalse( | ||
115 | 545 | check_permission('launchpad.View', non_viewable_objects[1]) | ||
116 | 546 | ) | ||
117 | 531 | 547 | ||
118 | 532 | def test_default_request(self): | 548 | def test_default_request(self): |
119 | 533 | # If no request is provided, the current interaction is used. | 549 | # If no request is provided, the current interaction is used. |
120 | diff --git a/lib/lp/soyuz/browser/archivesubscription.py b/lib/lp/soyuz/browser/archivesubscription.py | |||
121 | index 8cff69b..e472bde 100644 | |||
122 | --- a/lib/lp/soyuz/browser/archivesubscription.py | |||
123 | +++ b/lib/lp/soyuz/browser/archivesubscription.py | |||
124 | @@ -336,6 +336,29 @@ class PersonArchiveSubscriptionsView(LaunchpadView): | |||
125 | 336 | for archive in archives: | 336 | for archive in archives: |
126 | 337 | get_property_cache(archive)._known_subscribers = [self.context] | 337 | get_property_cache(archive)._known_subscribers = [self.context] |
127 | 338 | 338 | ||
128 | 339 | # Check if the user can view the archives and cache the permission | ||
129 | 340 | # check results | ||
130 | 341 | viewable_archives = [] | ||
131 | 342 | non_viewable_archives = [] | ||
132 | 343 | archive_set = getUtility(IArchiveSet) # type: IArchiveSet | ||
133 | 344 | for archive, has_view_permission in archive_set.checkViewPermission( | ||
134 | 345 | archives, self.user | ||
135 | 346 | ).items(): | ||
136 | 347 | if has_view_permission: | ||
137 | 348 | viewable_archives.append(archive) | ||
138 | 349 | else: | ||
139 | 350 | non_viewable_archives.append(archive) | ||
140 | 351 | precache_permission_for_objects( | ||
141 | 352 | None, | ||
142 | 353 | 'launchpad.View', | ||
143 | 354 | viewable_archives, result=True | ||
144 | 355 | ) | ||
145 | 356 | precache_permission_for_objects( | ||
146 | 357 | None, | ||
147 | 358 | 'launchpad.View', | ||
148 | 359 | non_viewable_archives, result=False | ||
149 | 360 | ) | ||
150 | 361 | |||
151 | 339 | # Turn the result set into a list of dicts so it can be easily | 362 | # Turn the result set into a list of dicts so it can be easily |
152 | 340 | # accessed in TAL. Note that we need to ensure that only one | 363 | # accessed in TAL. Note that we need to ensure that only one |
153 | 341 | # PersonalArchiveSubscription is included for each archive, | 364 | # PersonalArchiveSubscription is included for each archive, |
154 | diff --git a/lib/lp/soyuz/interfaces/archive.py b/lib/lp/soyuz/interfaces/archive.py | |||
155 | index 5ee645a..fb2ef57 100644 | |||
156 | --- a/lib/lp/soyuz/interfaces/archive.py | |||
157 | +++ b/lib/lp/soyuz/interfaces/archive.py | |||
158 | @@ -54,6 +54,7 @@ __all__ = [ | |||
159 | 54 | 54 | ||
160 | 55 | import http.client | 55 | import http.client |
161 | 56 | import re | 56 | import re |
162 | 57 | import typing | ||
163 | 57 | from urllib.parse import urlparse | 58 | from urllib.parse import urlparse |
164 | 58 | 59 | ||
165 | 59 | from lazr.restful.declarations import ( | 60 | from lazr.restful.declarations import ( |
166 | @@ -2515,6 +2516,23 @@ class IArchiveSet(Interface): | |||
167 | 2515 | that are currently published in the given archives. | 2516 | that are currently published in the given archives. |
168 | 2516 | """ | 2517 | """ |
169 | 2517 | 2518 | ||
170 | 2519 | def checkViewPermission( | ||
171 | 2520 | archives: typing.List[IArchive], user: IPerson | ||
172 | 2521 | ) -> typing.Dict[IArchive, bool]: | ||
173 | 2522 | """ | ||
174 | 2523 | Given a collection of archives, check if the user can view | ||
175 | 2524 | each of them. | ||
176 | 2525 | |||
177 | 2526 | Anyone can see a public and enabled archive. | ||
178 | 2527 | |||
179 | 2528 | Only Launchpad admins and uploaders can view private or disabled | ||
180 | 2529 | archives. | ||
181 | 2530 | |||
182 | 2531 | :param archives: a collection of `IArchive` objects | ||
183 | 2532 | :param user: a user (a `IPerson` object) | ||
184 | 2533 | :return: a mapping of `IArchive` -> `bool`, where the values represent | ||
185 | 2534 | the result of the permission check. | ||
186 | 2535 | """ | ||
187 | 2518 | 2536 | ||
188 | 2519 | default_name_by_purpose = { | 2537 | default_name_by_purpose = { |
189 | 2520 | ArchivePurpose.PRIMARY: 'primary', | 2538 | ArchivePurpose.PRIMARY: 'primary', |
190 | diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py | |||
191 | index 4d1ccb1..76fbadf 100644 | |||
192 | --- a/lib/lp/soyuz/model/archive.py | |||
193 | +++ b/lib/lp/soyuz/model/archive.py | |||
194 | @@ -13,6 +13,7 @@ __all__ = [ | |||
195 | 13 | 13 | ||
196 | 14 | from operator import attrgetter | 14 | from operator import attrgetter |
197 | 15 | import re | 15 | import re |
198 | 16 | import typing | ||
199 | 16 | 17 | ||
200 | 17 | from lazr.lifecycle.event import ObjectCreatedEvent | 18 | from lazr.lifecycle.event import ObjectCreatedEvent |
201 | 18 | import six | 19 | import six |
202 | @@ -82,6 +83,7 @@ from lp.registry.interfaces.distroseries import IDistroSeriesSet | |||
203 | 82 | from lp.registry.interfaces.distroseriesparent import IDistroSeriesParentSet | 83 | from lp.registry.interfaces.distroseriesparent import IDistroSeriesParentSet |
204 | 83 | from lp.registry.interfaces.gpg import IGPGKeySet | 84 | from lp.registry.interfaces.gpg import IGPGKeySet |
205 | 84 | from lp.registry.interfaces.person import ( | 85 | from lp.registry.interfaces.person import ( |
206 | 86 | IPerson, | ||
207 | 85 | IPersonSet, | 87 | IPersonSet, |
208 | 86 | validate_person, | 88 | validate_person, |
209 | 87 | ) | 89 | ) |
210 | @@ -3049,6 +3051,52 @@ class ArchiveSet: | |||
211 | 3049 | 3051 | ||
212 | 3050 | return results.order_by(SourcePackagePublishingHistory.id) | 3052 | return results.order_by(SourcePackagePublishingHistory.id) |
213 | 3051 | 3053 | ||
214 | 3054 | def checkViewPermission( | ||
215 | 3055 | self, archives: typing.List[IArchive], user: IPerson | ||
216 | 3056 | ) -> typing.Dict[IArchive, bool]: | ||
217 | 3057 | """See `IArchiveSet`.""" | ||
218 | 3058 | allowed_ids = set() | ||
219 | 3059 | ids_to_check_in_database = [] | ||
220 | 3060 | roles = IPersonRoles(user) | ||
221 | 3061 | for archive in archives: | ||
222 | 3062 | # No further checks are required if the archive is public and | ||
223 | 3063 | # enabled. | ||
224 | 3064 | if not archive.private and archive.enabled: | ||
225 | 3065 | allowed_ids.add(archive.id) | ||
226 | 3066 | |||
227 | 3067 | # Administrators are allowed to view private archives. | ||
228 | 3068 | elif roles.in_admin or roles.in_commercial_admin: | ||
229 | 3069 | allowed_ids.add(archive.id) | ||
230 | 3070 | |||
231 | 3071 | # Registry experts are allowed to view public but disabled archives | ||
232 | 3072 | # (since they are allowed to disable archives). | ||
233 | 3073 | elif (not archive.private and not archive.enabled and | ||
234 | 3074 | roles.in_registry_experts): | ||
235 | 3075 | allowed_ids.add(archive.id) | ||
236 | 3076 | |||
237 | 3077 | # Users that are direct owners (not through a team) | ||
238 | 3078 | # can view the PPA. | ||
239 | 3079 | # This is an optimization to avoid making a database query | ||
240 | 3080 | # when a user is the direct owner of the PPA. | ||
241 | 3081 | # Team ownership is accounted for in `get_enabled_archive_filter` | ||
242 | 3082 | # below | ||
243 | 3083 | elif user.id == removeSecurityProxy(archive).ownerID: | ||
244 | 3084 | allowed_ids.add(archive.id) | ||
245 | 3085 | |||
246 | 3086 | else: | ||
247 | 3087 | ids_to_check_in_database.append(archive.id) | ||
248 | 3088 | |||
249 | 3089 | if ids_to_check_in_database: | ||
250 | 3090 | store = IStore(Archive) | ||
251 | 3091 | allowed_ids.update( | ||
252 | 3092 | store.find( | ||
253 | 3093 | Archive.id, | ||
254 | 3094 | Archive.id.is_in(ids_to_check_in_database), | ||
255 | 3095 | get_enabled_archive_filter(user), | ||
256 | 3096 | ) | ||
257 | 3097 | ) | ||
258 | 3098 | return {archive: archive.id in allowed_ids for archive in archives} | ||
259 | 3099 | |||
260 | 3052 | def empty_list(self): | 3100 | def empty_list(self): |
261 | 3053 | """See `IArchiveSet.""" | 3101 | """See `IArchiveSet.""" |
262 | 3054 | return [] | 3102 | return [] |
263 | diff --git a/lib/lp/soyuz/templates/person-archive-subscriptions.pt b/lib/lp/soyuz/templates/person-archive-subscriptions.pt | |||
264 | index 8711378..19d39ef 100644 | |||
265 | --- a/lib/lp/soyuz/templates/person-archive-subscriptions.pt | |||
266 | +++ b/lib/lp/soyuz/templates/person-archive-subscriptions.pt | |||
267 | @@ -30,8 +30,12 @@ | |||
268 | 30 | <tal:subscription_and_token | 30 | <tal:subscription_and_token |
269 | 31 | define="subscription subscription_with_token/subscription; | 31 | define="subscription subscription_with_token/subscription; |
270 | 32 | token subscription_with_token/token"> | 32 | token subscription_with_token/token"> |
273 | 33 | <td class="ppa_display_name"> | 33 | <td class="ppa_display_name" |
274 | 34 | <tal:display_name content="subscription/archive/displayname"> | 34 | tal:define="archive_link subscription/archive/fmt:link"> |
275 | 35 | <tal:link condition="archive_link" content="structure archive_link"> | ||
276 | 36 | Private PPA for Celso | ||
277 | 37 | </tal:link> | ||
278 | 38 | <tal:display_name condition="not: archive_link" content="subscription/archive/displayname"> | ||
279 | 35 | Private PPA for Celso | 39 | Private PPA for Celso |
280 | 36 | </tal:display_name> | 40 | </tal:display_name> |
281 | 37 | (<tal:reference content="subscription/archive/fmt:reference"> | 41 | (<tal:reference content="subscription/archive/fmt:reference"> |
282 | diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py | |||
283 | index fd3d254..ae21161 100644 | |||
284 | --- a/lib/lp/soyuz/tests/test_archive.py | |||
285 | +++ b/lib/lp/soyuz/tests/test_archive.py | |||
286 | @@ -4050,6 +4050,144 @@ class TestArchiveSetGetByReference(TestCaseWithFactory): | |||
287 | 4050 | self.assertLookupFails('~enoent/twonoent/threenoent/fournoent') | 4050 | self.assertLookupFails('~enoent/twonoent/threenoent/fournoent') |
288 | 4051 | 4051 | ||
289 | 4052 | 4052 | ||
290 | 4053 | class TestArchiveSetCheckViewPermission(TestCaseWithFactory): | ||
291 | 4054 | |||
292 | 4055 | layer = DatabaseFunctionalLayer | ||
293 | 4056 | |||
294 | 4057 | def setUp(self): | ||
295 | 4058 | super().setUp() | ||
296 | 4059 | self.archive_set = getUtility(IArchiveSet) | ||
297 | 4060 | self.public_archive = self.factory.makeArchive(private=False) | ||
298 | 4061 | self.public_disabled_archive = self.factory.makeArchive( | ||
299 | 4062 | private=False, enabled=False | ||
300 | 4063 | ) | ||
301 | 4064 | self.private_archive = self.factory.makeArchive(private=True) | ||
302 | 4065 | |||
303 | 4066 | def test_public_enabled_archives(self): | ||
304 | 4067 | somebody = self.factory.makePerson() | ||
305 | 4068 | with person_logged_in(somebody): | ||
306 | 4069 | results = self.archive_set.checkViewPermission( | ||
307 | 4070 | [ | ||
308 | 4071 | self.public_archive, | ||
309 | 4072 | self.public_disabled_archive, | ||
310 | 4073 | self.private_archive | ||
311 | 4074 | ], | ||
312 | 4075 | somebody, | ||
313 | 4076 | ) | ||
314 | 4077 | self.assertDictEqual(results, { | ||
315 | 4078 | self.public_archive: True, | ||
316 | 4079 | self.public_disabled_archive: False, | ||
317 | 4080 | self.private_archive: False, | ||
318 | 4081 | }) | ||
319 | 4082 | |||
320 | 4083 | def test_admin_can_view(self): | ||
321 | 4084 | admin = self.factory.makeAdministrator() | ||
322 | 4085 | with person_logged_in(admin): | ||
323 | 4086 | results = self.archive_set.checkViewPermission( | ||
324 | 4087 | [ | ||
325 | 4088 | self.public_archive, | ||
326 | 4089 | self.public_disabled_archive, | ||
327 | 4090 | self.private_archive | ||
328 | 4091 | ], | ||
329 | 4092 | admin, | ||
330 | 4093 | ) | ||
331 | 4094 | self.assertDictEqual(results, { | ||
332 | 4095 | self.public_archive: True, | ||
333 | 4096 | self.public_disabled_archive: True, | ||
334 | 4097 | self.private_archive: True, | ||
335 | 4098 | }) | ||
336 | 4099 | comm_admin = self.factory.makeCommercialAdmin() | ||
337 | 4100 | with person_logged_in(comm_admin): | ||
338 | 4101 | results = self.archive_set.checkViewPermission( | ||
339 | 4102 | [ | ||
340 | 4103 | self.public_archive, | ||
341 | 4104 | self.public_disabled_archive, | ||
342 | 4105 | self.private_archive | ||
343 | 4106 | ], | ||
344 | 4107 | comm_admin, | ||
345 | 4108 | ) | ||
346 | 4109 | self.assertDictEqual(results, { | ||
347 | 4110 | self.public_archive: True, | ||
348 | 4111 | self.public_disabled_archive: True, | ||
349 | 4112 | self.private_archive: True, | ||
350 | 4113 | }) | ||
351 | 4114 | |||
352 | 4115 | def test_registry_experts(self): | ||
353 | 4116 | registry_expert = self.factory.makeRegistryExpert() | ||
354 | 4117 | with person_logged_in(registry_expert): | ||
355 | 4118 | results = self.archive_set.checkViewPermission( | ||
356 | 4119 | [ | ||
357 | 4120 | self.public_archive, | ||
358 | 4121 | self.public_disabled_archive, | ||
359 | 4122 | self.private_archive | ||
360 | 4123 | ], | ||
361 | 4124 | registry_expert, | ||
362 | 4125 | ) | ||
363 | 4126 | self.assertDictEqual(results, { | ||
364 | 4127 | self.public_archive: True, | ||
365 | 4128 | self.public_disabled_archive: True, | ||
366 | 4129 | self.private_archive: False, | ||
367 | 4130 | }) | ||
368 | 4131 | |||
369 | 4132 | def test_owner(self): | ||
370 | 4133 | owner = self.factory.makePerson() | ||
371 | 4134 | enabled_archive = self.factory.makeArchive( | ||
372 | 4135 | owner=owner, private=False, enabled=True | ||
373 | 4136 | ) | ||
374 | 4137 | disabled_archive = self.factory.makeArchive( | ||
375 | 4138 | owner=owner, private=False, enabled=False | ||
376 | 4139 | ) | ||
377 | 4140 | with person_logged_in(owner): | ||
378 | 4141 | results = self.archive_set.checkViewPermission( | ||
379 | 4142 | [ | ||
380 | 4143 | enabled_archive, | ||
381 | 4144 | disabled_archive, | ||
382 | 4145 | ], | ||
383 | 4146 | owner, | ||
384 | 4147 | ) | ||
385 | 4148 | self.assertDictEqual(results, { | ||
386 | 4149 | enabled_archive: True, | ||
387 | 4150 | disabled_archive: True, | ||
388 | 4151 | }) | ||
389 | 4152 | |||
390 | 4153 | def test_team_owner(self): | ||
391 | 4154 | team_member = self.factory.makePerson() | ||
392 | 4155 | team = self.factory.makeTeam(members=[team_member]) | ||
393 | 4156 | enabled_archive = self.factory.makeArchive( | ||
394 | 4157 | owner=team, private=False, enabled=True | ||
395 | 4158 | ) | ||
396 | 4159 | disabled_archive = self.factory.makeArchive( | ||
397 | 4160 | owner=team, private=False, enabled=False | ||
398 | 4161 | ) | ||
399 | 4162 | with person_logged_in(team_member): | ||
400 | 4163 | results = self.archive_set.checkViewPermission( | ||
401 | 4164 | [ | ||
402 | 4165 | enabled_archive, | ||
403 | 4166 | disabled_archive, | ||
404 | 4167 | ], | ||
405 | 4168 | team_member, | ||
406 | 4169 | ) | ||
407 | 4170 | self.assertDictEqual(results, { | ||
408 | 4171 | enabled_archive: True, | ||
409 | 4172 | disabled_archive: True, | ||
410 | 4173 | }) | ||
411 | 4174 | |||
412 | 4175 | def test_query_count(self): | ||
413 | 4176 | archives = [ | ||
414 | 4177 | self.factory.makeArchive(private=False) for _ in range(10) | ||
415 | 4178 | ] | ||
416 | 4179 | somebody = self.factory.makePerson() | ||
417 | 4180 | with StormStatementRecorder() as recorder1: | ||
418 | 4181 | self.archive_set.checkViewPermission( | ||
419 | 4182 | archives[:5], somebody | ||
420 | 4183 | ) | ||
421 | 4184 | with StormStatementRecorder() as recorder2: | ||
422 | 4185 | self.archive_set.checkViewPermission( | ||
423 | 4186 | archives, somebody | ||
424 | 4187 | ) | ||
425 | 4188 | self.assertThat(recorder2, HasQueryCount.byEquality(recorder1)) | ||
426 | 4189 | |||
427 | 4190 | |||
428 | 4053 | class TestDisplayName(TestCaseWithFactory): | 4191 | class TestDisplayName(TestCaseWithFactory): |
429 | 4054 | 4192 | ||
430 | 4055 | layer = DatabaseFunctionalLayer | 4193 | layer = DatabaseFunctionalLayer |
431 | diff --git a/lib/lp/soyuz/tests/test_archive_subscriptions.py b/lib/lp/soyuz/tests/test_archive_subscriptions.py | |||
432 | index 516fcd7..50c8a3b 100644 | |||
433 | --- a/lib/lp/soyuz/tests/test_archive_subscriptions.py | |||
434 | +++ b/lib/lp/soyuz/tests/test_archive_subscriptions.py | |||
435 | @@ -182,7 +182,14 @@ class PersonArchiveSubscriptions(TestCaseWithFactory): | |||
436 | 182 | def test_query_count(self): | 182 | def test_query_count(self): |
437 | 183 | subscriber = self.factory.makePerson() | 183 | subscriber = self.factory.makePerson() |
438 | 184 | for x in range(10): | 184 | for x in range(10): |
440 | 185 | archive = self.factory.makeArchive(private=True) | 185 | archive_owner = self.factory.makePerson() |
441 | 186 | # some archives are owned by a user, others are owned by a team | ||
442 | 187 | archive = self.factory.makeArchive( | ||
443 | 188 | private=True, | ||
444 | 189 | owner=archive_owner if x < 5 else self.factory.makeTeam( | ||
445 | 190 | members=[archive_owner] | ||
446 | 191 | ) | ||
447 | 192 | ) | ||
448 | 186 | with person_logged_in(archive.owner): | 193 | with person_logged_in(archive.owner): |
449 | 187 | if x >= 5: | 194 | if x >= 5: |
450 | 188 | team = self.factory.makeTeam(members=[subscriber]) | 195 | team = self.factory.makeTeam(members=[subscriber]) |
451 | @@ -196,7 +203,7 @@ class PersonArchiveSubscriptions(TestCaseWithFactory): | |||
452 | 196 | view = create_initialized_view( | 203 | view = create_initialized_view( |
453 | 197 | subscriber, '+archivesubscriptions', principal=subscriber) | 204 | subscriber, '+archivesubscriptions', principal=subscriber) |
454 | 198 | view.render() | 205 | view.render() |
456 | 199 | self.assertThat(recorder, HasQueryCount(Equals(12))) | 206 | self.assertThat(recorder, HasQueryCount(Equals(16))) |
457 | 200 | 207 | ||
458 | 201 | def test_getArchiveSubscriptions(self): | 208 | def test_getArchiveSubscriptions(self): |
459 | 202 | # Anyone with 'View' permission on a given person is able to | 209 | # Anyone with 'View' permission on a given person is able to |
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.