Merge lp:~sinzui/launchpad/location-bug-262193 into lp:launchpad

Proposed by Curtis Hovey on 2009-11-15
Status: Merged
Approved by: Eleanor Berger on 2009-11-15
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~sinzui/launchpad/location-bug-262193
Merge into: lp:launchpad
Diff against target: 459 lines (+57/-212)
10 files modified
lib/canonical/launchpad/emailtemplates/person-location-modified.txt (+0/-19)
lib/canonical/launchpad/permissions.zcml (+0/-4)
lib/canonical/launchpad/security.py (+0/-32)
lib/lp/registry/browser/configure.zcml (+1/-1)
lib/lp/registry/browser/person.py (+2/-14)
lib/lp/registry/configure.zcml (+1/-1)
lib/lp/registry/doc/personlocation.txt (+15/-53)
lib/lp/registry/model/person.py (+0/-14)
lib/lp/registry/stories/location/personlocation-edit.txt (+27/-59)
lib/lp/registry/stories/location/personlocation.txt (+11/-15)
To merge this branch: bzr merge lp:~sinzui/launchpad/location-bug-262193
Reviewer Review Type Date Requested Status
Eleanor Berger (community) code 2009-11-15 Approve on 2009-11-15
Review via email: mp+14878@code.launchpad.net
To post a comment you must log in.
Curtis Hovey (sinzui) wrote :
Download full text (3.1 KiB)

This is my branch to only permit users to edit their own location.

    lp:~sinzui/launchpad/location-bug-262193
    Diff size: 460
    Launchpad bug: https://bugs.launchpad.net/bugs/262193
    Test command: ./bin/test -vvt "personlocation"
    Pre-implementation: my conscience
    Target release: 3.1.11

= Only permit users to edit their own location. =

After much debate about allowing users to set someone else's location, it
is clear from the usage statistics that the feature is rarely used.
Removing the feature will not have a significant impact on Launchpad.

== Rules ==

    * Restrict the permissions to edit a location to the owner or admin:
      launchpad.Edit
    * Remove the launchpad.Editlocation permission.
      \o/ one less exception permission in security.py
    * Remove any UI that suggests that a user can edit someone else's
      location. Most, maybe all the UI elements suggesting that a person
      can set another person's location was removed in 3.0.
    * Remove the notification email set when some edits another person's
      location.

== QA ==

    * On staging, verify that the page does not prompt you to set someone's
      location.
    * Verify that you cannot access the user's +editlocation page.

== Lint ==

Linting changed files:
  lib/canonical/launchpad/permissions.zcml
  lib/canonical/launchpad/security.py
  lib/lp/registry/configure.zcml
  lib/lp/registry/browser/configure.zcml
  lib/lp/registry/browser/person.py
  lib/lp/registry/doc/personlocation.txt
  lib/lp/registry/model/person.py
  lib/lp/registry/stories/location/personlocation-edit.txt
  lib/lp/registry/stories/location/personlocation.txt

== Test ==

    * lib/lp/registry/doc/personlocation.txt
      * Revised the test to show that a user's location is only editable
        by himself or an admin.
      * Removed all tests that showed that a user could set someone else's
        location if the location was unset.
    * lib/lp/registry/stories/location/personlocation-edit.txt
      * Removed the tests that demonstrated a user could set another user's
        location if the location was unset.
    * lib/lp/registry/stories/location/personlocation.txt
      * Removed the test that implied a user could set another user's
        location.
      * Revised to the test to show that any user can see another user's
        location if it is public (the previous test implied the user had
        to be logged in).

== Implementation ==

    * lib/canonical/launchpad/emailtemplates/person-location-modified.txt
      * Deleted because it is not possible to set another user's location.

    * lib/canonical/launchpad/permissions.zcml
    * lib/canonical/launchpad/security.py
      * Removed launchpad.EditLocation.

    * lib/lp/registry/configure.zcml
    * lib/lp/registry/browser/configure.zcml
      * Replaced launchpad.EditLocation with launchpad.Edit.

    * lib/lp/registry/browser/person.py
      * Replaced launchpad.EditLocation with launchpad.Edit.
      * Since there is only one permission to access +editlocation, the
        field_names never change.

    * lib/lp/registry/model/person.py
      * Removed the notification about a user setting an...

Read more...

Eleanor Berger (intellectronica) wrote :

simplicity rocks!

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== removed file 'lib/canonical/launchpad/emailtemplates/person-location-modified.txt'
2--- lib/canonical/launchpad/emailtemplates/person-location-modified.txt 2008-06-18 13:06:54 +0000
3+++ lib/canonical/launchpad/emailtemplates/person-location-modified.txt 1970-01-01 00:00:00 +0000
4@@ -1,19 +0,0 @@
5-
6-%(actor_browsername)s has updated your location and time zone information in
7-Launchpad. This lets us present dates and times in your local time, and
8-helps to coordinate team events that will span multiple time zones.
9-
10-Your information can be updated by any other user until you provide
11-definitive data yourself, at which point the information is locked
12-so that only you can edit it.
13-
14-Your details (now with map!): https://launchpad.net/~%(person)s
15-%(actor_browsername)s's details: https://launchpad.net/~%(actor)s
16-
17-You can edit the location and time zone information for yourself at:
18-
19- https://launchpad.net/~%(person)s/+editlocation
20-
21-Regards,
22-
23-The Launchpad team
24
25=== modified file 'lib/canonical/launchpad/permissions.zcml'
26--- lib/canonical/launchpad/permissions.zcml 2009-08-13 15:12:16 +0000
27+++ lib/canonical/launchpad/permissions.zcml 2009-11-15 01:25:23 +0000
28@@ -16,10 +16,6 @@
29 <permission
30 id="launchpad.Edit" title="Editing something" access_level="write" />
31
32- <permission
33- id="launchpad.EditLocation" title="Editing an object's location"
34- access_level="write" />
35-
36 <!-- Request large downloads, or other heavyweight jobs that are not
37 semantically like "editing" and are not restricted to administrators.
38 -->
39
40=== modified file 'lib/canonical/launchpad/security.py'
41--- lib/canonical/launchpad/security.py 2009-11-13 05:13:07 +0000
42+++ lib/canonical/launchpad/security.py 2009-11-15 01:25:23 +0000
43@@ -624,38 +624,6 @@
44 return person == user or user.inTeam(admins)
45
46
47-class EditPersonLocation(AuthorizationBase):
48- permission = 'launchpad.EditLocation'
49- usedfor = IPerson
50-
51- def checkAuthenticated(self, user):
52- """Anybody can edit a person's location until that person sets it.
53-
54- Once a person sets his own location that information can only be
55- changed by the person himself or admins.
56- """
57- location = self.obj.location
58- if location is None:
59- # No PersonLocation entry exists for this person, so anybody can
60- # change this person's location.
61- return True
62-
63- # There is a PersonLocation entry for this person, so we'll check its
64- # details to find out whether or not the user can edit them.
65- if (location.visible
66- and (location.latitude is None
67- or location.last_modified_by != self.obj)):
68- # No location has been specified yet or it has been specified
69- # by a non-authoritative source (not the person himself), so
70- # anybody can change it.
71- return True
72- else:
73- admins = getUtility(ILaunchpadCelebrities).admin
74- # The person himself and LP admins can always change that person's
75- # location.
76- return user == self.obj or user.inTeam(admins)
77-
78-
79 class ViewPersonLocation(AuthorizationBase):
80 permission = 'launchpad.View'
81 usedfor = IPersonLocation
82
83=== modified file 'lib/lp/registry/browser/configure.zcml'
84--- lib/lp/registry/browser/configure.zcml 2009-11-11 10:30:23 +0000
85+++ lib/lp/registry/browser/configure.zcml 2009-11-15 01:25:23 +0000
86@@ -830,7 +830,7 @@
87 name="+editlocation"
88 for="lp.registry.interfaces.person.IPerson"
89 class="lp.registry.browser.person.PersonEditLocationView"
90- permission="launchpad.EditLocation"
91+ permission="launchpad.Edit"
92 template="../templates/person-editlocation.pt"/>
93 <browser:page
94 name="+contactuser"
95
96=== modified file 'lib/lp/registry/browser/person.py'
97--- lib/lp/registry/browser/person.py 2009-11-09 04:24:09 +0000
98+++ lib/lp/registry/browser/person.py 2009-11-15 01:25:23 +0000
99@@ -954,7 +954,7 @@
100 text = 'Update Jabber IDs'
101 return Link(target, text, icon='edit')
102
103- @enabled_with_permission('launchpad.EditLocation')
104+ @enabled_with_permission('launchpad.Edit')
105 def editlocation(self):
106 target = '+editlocation'
107 text = 'Set location and time zone'
108@@ -5333,6 +5333,7 @@
109 """Edit a person's location."""
110
111 schema = PersonLocationForm
112+ field_names = ['location', 'hide']
113 custom_widget('location', LocationWidget)
114
115 @property
116@@ -5343,19 +5344,6 @@
117 label = page_title
118
119 @property
120- def field_names(self):
121- """See `LaunchpadFormView`.
122-
123- If the user has launchpad.Edit on this context, then allow him to set
124- whether or not the location should be visible. The field for setting
125- the person's location is always shown.
126- """
127- if check_permission('launchpad.Edit', self.context):
128- return ['location', 'hide']
129- else:
130- return ['location']
131-
132- @property
133 def initial_values(self):
134 """See `LaunchpadFormView`.
135
136
137=== modified file 'lib/lp/registry/configure.zcml'
138--- lib/lp/registry/configure.zcml 2009-11-12 12:01:22 +0000
139+++ lib/lp/registry/configure.zcml 2009-11-15 01:25:23 +0000
140@@ -805,7 +805,7 @@
141 permission="launchpad.View"
142 interface="lp.registry.interfaces.person.IPersonViewRestricted"/>
143 <require
144- permission="launchpad.EditLocation"
145+ permission="launchpad.Edit"
146 interface="lp.registry.interfaces.location.ISetLocation"/>
147 <require
148 permission="launchpad.Edit"
149
150=== modified file 'lib/lp/registry/doc/personlocation.txt'
151--- lib/lp/registry/doc/personlocation.txt 2009-04-17 10:32:16 +0000
152+++ lib/lp/registry/doc/personlocation.txt 2009-11-15 01:25:23 +0000
153@@ -38,53 +38,22 @@
154 requires that the user providing the information is passed as a
155 parameter.
156
157-We'll use jdub as a prolific source of location information for
158-community members.
159+A user cannot set another user's location.
160
161 >>> jdub = personset.getByName('jdub')
162 >>> login_person(jdub)
163-
164-First, jdub will provide information about cprov, who doesn't have any
165-location data in the sampledata.
166-
167 >>> cprov = personset.getByName('cprov')
168- >>> print cprov.latitude
169- None
170- >>> print cprov.time_zone
171- None
172 >>> cprov.setLocation(-43.0, -62.1, 'America/Sao_Paulo', jdub)
173- >>> print cprov.time_zone
174- America/Sao_Paulo
175-
176-When a person's location is changed by somebody else, we notify the
177-person through email.
178-
179- >>> from canonical.launchpad.database import PersonNotification
180- >>> notification = PersonNotification.selectOneBy(person=cprov)
181- >>> print notification.subject
182- Jeff Waugh updated your location and time zone
183-
184- # Delete the notification so that it doesn't interfere in other tests.
185- >>> notification.destroySelf()
186-
187-And once cprov provides data for himself, jdub won't be able to edit it
188-any more.
189+ Traceback (most recent call last):
190+ ...
191+ Unauthorized:...
192+
193+A user can set his own location.
194
195 >>> login_person(cprov)
196 >>> cprov.setLocation(-43.2, -61.93, 'America/Sao_Paulo', cprov)
197
198- >>> login_person(jdub)
199- >>> cprov.setLocation(-43.0, -62.1, 'America/Sao_Paulo', jdub)
200- Traceback (most recent call last):
201- ...
202- Unauthorized:...
203-
204-When a user changes his own location, no notification is sent, though.
205-
206- >>> PersonNotification.selectBy(person=cprov).count()
207- 0
208-
209-But cprov can obviously still edit his own information. We need to deal
210+cprov can change his location information. We need to deal
211 with some floating point precision issues here, hence the rounding.
212
213 >>> login_person(cprov)
214@@ -92,7 +61,7 @@
215 >>> abs(cprov.latitude + 43.52) < 0.001
216 True
217
218-And admins can, too.
219+Admins can set a user's location.
220
221 >>> admin = personset.getByName('name16')
222 >>> login_person(admin)
223@@ -100,20 +69,6 @@
224 >>> abs(cprov.longitude + 62.1) < 0.001
225 True
226
227-If a user had provided only his time zone but not his location, other
228-people would still be able to set his location. That's the case of
229-most our existing users, who set their time zone but hadn't set their
230-locations (since there was no UI for that).
231-
232- >>> salgado = personset.getByName('salgado')
233- >>> login_person(salgado)
234- >>> salgado.setLocation(None, None, 'America/Sao_Paulo', salgado)
235-
236- >>> login_person(jdub)
237- >>> salgado.setLocation(-43.0, -62.1, 'America/Sao_Paulo', jdub)
238- >>> print salgado.latitude
239- -43.0
240-
241 We cannot store a location for a team, though.
242
243 >>> guadamen = personset.getByName('guadamen')
244@@ -214,11 +169,18 @@
245 we provide a way for them to hide their location from other users. By
246 default a person's location is visible.
247
248+ >>> salgado = personset.getByName('salgado')
249+ >>> login_person(salgado)
250+ >>> salgado.setLocation(-43.0, -62.1, 'America/Sao_Paulo', salgado)
251 >>> salgado.location.visible
252 True
253 >>> salgado.location.latitude
254 -43...
255
256+ >>> login_person(jdub)
257+ >>> salgado.location.latitude
258+ -43...
259+
260 But it can be changed through the setLocationVisibility() method. If the
261 visibility is set to False, only the person himself will be able to see
262 the location data except for time zone.
263
264=== modified file 'lib/lp/registry/model/person.py'
265--- lib/lp/registry/model/person.py 2009-11-12 20:55:43 +0000
266+++ lib/lp/registry/model/person.py 2009-11-15 01:25:23 +0000
267@@ -551,20 +551,6 @@
268 person=self, time_zone=time_zone, latitude=latitude,
269 longitude=longitude, last_modified_by=user)
270
271- # Make a note that we need to tell this person that their
272- # information was updated by the user. We can only do this if we
273- # have a validated email address for this person.
274- if user != self and self.preferredemail is not None:
275- mail_text = get_email_template('person-location-modified.txt')
276- mail_text = mail_text % {
277- 'actor': user.name,
278- 'actor_browsername': user.displayname,
279- 'person': self.name}
280- subject = '%s updated your location and time zone' % (
281- user.displayname)
282- getUtility(IPersonNotificationSet).addNotification(
283- self, subject, mail_text)
284-
285 # specification-related joins
286 @property
287 def assigned_specs(self):
288
289=== modified file 'lib/lp/registry/stories/location/personlocation-edit.txt'
290--- lib/lp/registry/stories/location/personlocation-edit.txt 2009-03-24 12:43:49 +0000
291+++ lib/lp/registry/stories/location/personlocation-edit.txt 2009-11-15 01:25:23 +0000
292@@ -1,9 +1,7 @@
293 == Edit person location information ==
294
295-The location information is editable by anybody (like a wiki), until the
296-person provides location data for themselves. At that point it is only
297-editable by people who have launchpad.Edit on the person, which is that
298-person and admins.
299+A person's location is only editable by people who have launchpad.Edit on
300+the person, which is that person and admins.
301
302 >>> login('test@canonical.com')
303 >>> zzz = factory.makePerson(
304@@ -11,37 +9,15 @@
305 ... latitude=None, longitude=None, password='test')
306 >>> logout()
307
308-Anybody logged in can see the Location widget for a person who has no
309-location set.
310+A user cannot set another user's location.
311
312 >>> nopriv_browser = setupBrowser(auth="Basic no-priv@canonical.com:test")
313 >>> nopriv_browser.open('http://launchpad.dev/~zzz/+editlocation')
314- >>> find_tag_by_id(nopriv_browser.contents, "field.location.latitude")
315- <input...name="field.location.latitude" type="hidden"...
316-
317-Not only can we see the widgets, but anybody logged in can actually provide
318-new location information. Here we need to provide the location and time
319-zone, but when accessed through a real browser the page will use Javascript
320-to infer the time zone based on the location given.
321-
322- >>> nopriv_browser.getControl(name='field.location.latitude').value = (
323- ... '13.0')
324- >>> nopriv_browser.getControl(name='field.location.longitude').value = (
325- ... '17.0')
326- >>> nopriv_browser.getControl(name='field.location.time_zone').value = [
327- ... 'Europe/Madrid']
328- >>> nopriv_browser.getControl('Update').click()
329- >>> login('test@canonical.com')
330- >>> zzz.latitude
331- 13.0
332- >>> zzz.longitude
333- 17.0
334- >>> zzz.time_zone
335- u'Europe/Madrid'
336- >>> logout()
337-
338-If the person provides his own location information, then the data is
339-locked, and random people can no longer edit it.
340+ Traceback (most recent call last):
341+ ...
342+ Unauthorized:...
343+
344+A user can set his own location:
345
346 >>> self_browser = setupBrowser(auth="Basic zzz@foo.com:test")
347 >>> self_browser.open('http://launchpad.dev/~zzz/+editlocation')
348@@ -53,12 +29,16 @@
349 ... 'Europe/Madrid']
350 >>> self_browser.getControl('Update').click()
351
352- >>> nopriv_browser.open('http://launchpad.dev/~zzz/+editlocation')
353- Traceback (most recent call last):
354- ...
355- Unauthorized:...
356+ >>> login('zzz@foo.com')
357+ >>> zzz.latitude
358+ 39.4...
359+ >>> zzz.longitude
360+ -0.4...
361+ >>> zzz.time_zone
362+ u'Europe/Madrid'
363+ >>> logout()
364
365-Of course, the person himself can continue to edit it:
366+The user can change his location:
367
368 >>> self_browser.open('http://launchpad.dev/~zzz/+editlocation')
369 >>> self_browser.getControl(name='field.location.latitude').value
370@@ -71,33 +51,21 @@
371 '39.48'
372
373 The +editlocation page also allows a person to change his location
374-visibility, that is, whether or not others can see it. Unlike the actual
375-location (which can be changed by anybody until the person himself sets
376-it), the visibility can only be changed by the person himself.
377+visibility, that is, whether or not others can see it.
378+
379+ >>> nopriv_browser.open('http://launchpad.dev/~no-priv/+editlocation')
380+ >>> nopriv_browser.getControl(
381+ ... 'Hide my location details from others.').selected = True
382+ >>> nopriv_browser.getControl('Update').click()
383+ >>> nopriv_browser.url
384+ 'http://launchpad.dev/~no-priv'
385+
386+Once hidden, other users can't see it.
387
388 >>> name12_browser = setupBrowser(auth="Basic test@canonical.com:test")
389- >>> name12_browser.open('http://launchpad.dev/~no-priv/+editlocation')
390- >>> name12_browser.getControl('Hide my location details from others.')
391- Traceback (most recent call last):
392- ...
393- LookupError:...
394-
395- >>> nopriv_browser.open('http://launchpad.dev/~no-priv/+editlocation')
396- >>> nopriv_browser.getControl(
397- ... 'Hide my location details from others.').selected = True
398- >>> nopriv_browser.getControl('Update').click()
399- >>> nopriv_browser.url
400- 'http://launchpad.dev/~no-priv'
401-
402-Once hidden, other users can't see nor change it.
403-
404 >>> name12_browser.open('http://launchpad.dev/~no-priv')
405 >>> print str(find_tag_by_id(name12_browser.contents, 'person_map_div'))
406 None
407- >>> name12_browser.open('http://launchpad.dev/~no-priv/+editlocation')
408- Traceback (most recent call last):
409- ...
410- Unauthorized:...
411
412 The person himself can still see and change it, though.
413
414
415=== modified file 'lib/lp/registry/stories/location/personlocation.txt'
416--- lib/lp/registry/stories/location/personlocation.txt 2009-09-16 17:56:17 +0000
417+++ lib/lp/registry/stories/location/personlocation.txt 2009-11-15 01:25:23 +0000
418@@ -8,30 +8,26 @@
419 >>> zzz = factory.makePerson(name='zzz', time_zone='Africa/Maseru',
420 ... latitude=None, longitude=None)
421 >>> logout()
422- >>> nopriv_browser = setupBrowser(auth='Basic no-priv@canonical.com:test')
423- >>> nopriv_browser.open('http://launchpad.dev/~zzz')
424-
425-They should see a link to the +editlocation page (around the map image)
426-and the person's time zone.
427-
428- >>> editlink = nopriv_browser.getLink(url='+editlocation', index=1)
429- >>> editlink.text
430- '[IMG]'
431- >>> print extract_text(find_tag_by_id(nopriv_browser.contents, 'portlet-map'))
432+
433+Any person can see another person's time zone.
434+
435+ >>> anon_browser.open('http://launchpad.dev/~zzz')
436+ >>> print extract_text(
437+ ... find_tag_by_id(anon_browser.contents, 'portlet-map'))
438 Location
439 Time zone: Africa/Maseru
440
441-If a person has a location, they should have a little map portlet in their
442-home page. We can't test all the google javascript, but we can make sure
443+If a person has a location, there is a little map portlet in their
444+profile page. We can't test all the google javascript, but we can make sure
445 there's a map, and the scripts are loaded.
446
447 >>> login('test@canonical.com')
448 >>> yyy = factory.makePerson(name='yyy', time_zone='Europe/London',
449 ... latitude=52.2, longitude=0.3)
450 >>> logout()
451- >>> nopriv_browser = setupBrowser(auth='Basic no-priv@canonical.com:test')
452- >>> nopriv_browser.open('http://launchpad.dev/~yyy')
453- >>> markup = str(nopriv_browser.contents)
454+
455+ >>> anon_browser.open('http://launchpad.dev/~yyy')
456+ >>> markup = str(anon_browser.contents)
457 >>> print extract_text(find_tag_by_id(markup, 'portlet-map'), skip_tags=[])
458 Location
459 Time zone: Europe/London...