Launchpad itself

Merge lp:~sinzui/launchpad/spam-eggs-bug-495250 into lp:launchpad

  1. spam-eggs-bug-495250
  2. Merge into devel
Proposed by Curtis Hovey
Status: Merged
Merged at revision: not available
Proposed branch: lp:~sinzui/launchpad/spam-eggs-bug-495250
Merge into: lp:launchpad
Diff against target: 358 lines (+183/-15)
9 files modified
lib/lp/registry/browser/person.py (+42/-3)
lib/lp/registry/browser/tests/karmaaction-views.txt (+1/-1)
lib/lp/registry/browser/tests/person-views.txt (+121/-4)
lib/lp/registry/browser/tests/poll-views.txt (+1/-1)
lib/lp/registry/stories/person/xx-admin-person-review.txt (+4/-0)
lib/lp/registry/stories/person/xx-login.txt (+2/-0)
lib/lp/registry/stories/person/xx-reg-with-existing-email.txt (+5/-0)
lib/lp/registry/templates/person-index.pt (+3/-3)
lib/lp/testing/views.py (+4/-3)
To merge this branch: bzr merge lp:~sinzui/launchpad/spam-eggs-bug-495250
Reviewer Review Type Date Requested Status
Данило Шеган (community) release-critical Abstain
Henning Eggers (community) Approve
Review via email: mp+16174@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

This is my branch to discourage spammers.

    lp:~sinzui/launchpad/spam-eggs-bug-495250
    Diff size: 282
    Launchpad bug: https://bugs.launchpad.net/bugs/495250
    Test command: ./bin/test -vvt "person-views"
    Pre-implementation: flacoste, gary
    Target release: 3.1.12

= Discourage spammers =

Do not link the homepage_content of probationary users. Do not let search
engine index probationary user' pages.

== Rules ==

    * All users with karma == 0 are probationary.
    * Set the robots directive to noindex,nofollow
    * Do not linkify the homepage_content.
    * Set the HTTP status code to 410 on profile pages for suspended accounts.

== QA ==

    * Locate a suspended user (search answers)
      * Verify the request response headers are set the status code to 410.
    * Locate an account with zero karma.
      * Verify that the profile page meta robots tag is index,nofollow.
    * Locate an account with zero karma and a description (probably a spammer)
      * Verify the homepage text is not formated as HTML.

== Lint ==

Linting changed files:
  lib/lp/registry/browser/person.py
  lib/lp/registry/browser/tests/person-views.txt
  lib/lp/registry/templates/person-index.pt

== Test ==

    * lib/lp/registry/browser/tests/person-views.txt
      * Added tests to verify is_probationary_or_invalid_user and
        homepage_content properties on the PersonView.
      * Add a test to verify that the HTTP status code is set to 410
        for suspended accounts.

== Implementation ==

    * lib/lp/registry/browser/person.py
      * Added is_probationary_or_invalid_user and homepage_content properties
        to control how to display the profile page for probationary users.
      * Redefined the render() method to set the status code to 410 when the
        account is suspended.
    * lib/lp/registry/templates/person-index.pt
      * Updated the template to use the view instead of the context object
        to show content and set the meta data.
    * lib/lp/testing/views.py
      * Discovered that path_info was not used. I updated the helper to
        use it.

Revision history for this message
Henning Eggers (henninge) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 15.12.2009 00:00, Curtis Hovey schrieb:
> Curtis Hovey has proposed merging lp:~sinzui/launchpad/spam-eggs-bug-495250 into lp:launchpad/devel.

> This is my branch to discourage spammers.

Yeah! Cool stuff! Thanks for the quick and smart fix.

 review approve

All I found was a little glitch in a comment...

> === modified file 'lib/lp/registry/browser/person.py'

Straightforward implementation, clear comments, good code. ;)

> === modified file 'lib/lp/registry/browser/tests/person-views.txt'
> --- lib/lp/registry/browser/tests/person-views.txt 2009-11-20 18:44:57 +0000
> +++ lib/lp/registry/browser/tests/person-views.txt 2009-12-14 23:00:33 +0000
> @@ -4,7 +4,127 @@
> person's information.
>
>
> -== Email address disclosure ==
> +Probationary and invalid users
> +------------------------------
> +
> +The person +index view provides the is_probationary_or_invalid_user so that
> +page features can be disabled because the user may abuse them. Active
> +users with karma are not on probation. The user's homepage_content formatted
> +as HTML

Add an "is" and a ".", please. ;)

[...]

> === modified file 'lib/lp/registry/templates/person-index.pt'
As expected.

> === modified file 'lib/lp/testing/views.py'
Thanks for the drive-by fix.

Cheers,
Henning

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAksnW2cACgkQBT3oW1L17iigywCcDytaSGCqRg0vliB5hMTqAOT0
x7IAn2Pb+6N0J+kAeoBbYvvltNGPfxnV
=qBEM
-----END PGP SIGNATURE-----

review: Approve
Revision history for this message
Данило Шеган (danilo) wrote :

As agreed on IRC, let's have this QAd a bit first (perhaps by cowboying, or landing right after the rollout as an RC=danilo) and then include it in the re-roll or CP it if there's no re-roll.

review: Abstain (release-critical)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/registry/browser/person.py'
2--- lib/lp/registry/browser/person.py 2009-12-08 10:20:37 +0000
3+++ lib/lp/registry/browser/person.py 2009-12-15 20:35:30 +0000
4@@ -226,7 +226,8 @@
5 logoutPerson, allowUnauthenticatedSession)
6 from canonical.launchpad.webapp.menu import get_current_view
7 from canonical.launchpad.webapp.publisher import LaunchpadView
8-from canonical.launchpad.webapp.tales import DateTimeFormatterAPI
9+from canonical.launchpad.webapp.tales import (
10+ DateTimeFormatterAPI, FormattersAPI)
11 from lazr.uri import URI, InvalidURIError
12
13 from canonical.launchpad import _
14@@ -2460,7 +2461,7 @@
15
16 @property
17 def next_url(self):
18- """Redirect back to the +languages page if request originated there."""
19+ """Redirect to the +languages page if request originated there."""
20 redirection_url = self.request.get('redirection_url')
21 if redirection_url:
22 return redirection_url
23@@ -2468,7 +2469,7 @@
24
25 @property
26 def cancel_url(self):
27- """Redirect back to the +languages page if request originated there."""
28+ """Redirect to the +languages page if request originated there."""
29 redirection_url = self.getRedirectionURL()
30 if redirection_url:
31 return redirection_url
32@@ -2663,6 +2664,38 @@
33 check_permission('launchpad.Edit', self.context))
34
35 @cachedproperty
36+ def is_probationary_or_invalid_user(self):
37+ """True when the user is not active or does not have karma.
38+
39+ Some content should not be rendered when the context is not a an
40+ established user. For example, probationary and invalid user pages
41+ must not be indexed by search engines and their narrative linkified.
42+ """
43+ user = self.context
44+ if user.isTeam():
45+ # Teams are always valid and do not have probationary rules.
46+ return False
47+ else:
48+ return user.karma == 0 or not user.is_valid_person
49+
50+ @cachedproperty
51+ def homepage_content(self):
52+ """The user's HTML formatted homepage content.
53+
54+ The markup is simply escaped for probationary or invalid users.
55+ The homepage content is reformatted as HTML and linkified if the user
56+ is active.
57+ """
58+ content = self.context.homepage_content
59+ if content is None:
60+ return None
61+ elif self.is_probationary_or_invalid_user:
62+ return cgi.escape(content)
63+ else:
64+ formatter = FormattersAPI
65+ return formatter(content).text_to_html()
66+
67+ @cachedproperty
68 def recently_approved_members(self):
69 members = self.context.getMembersByStatus(
70 TeamMembershipStatus.APPROVED,
71@@ -3137,6 +3170,12 @@
72 if self.request.method == "POST":
73 self.processForm()
74
75+ def render(self):
76+ """See `LaunchpadView`."""
77+ if self.context.account_status == AccountStatus.SUSPENDED:
78+ self.request.response.setStatus(410)
79+ return super(PersonIndexView, self).render()
80+
81 @property
82 def page_title(self):
83 context = self.context
84
85=== modified file 'lib/lp/registry/browser/tests/karmaaction-views.txt'
86--- lib/lp/registry/browser/tests/karmaaction-views.txt 2009-08-17 16:18:51 +0000
87+++ lib/lp/registry/browser/tests/karmaaction-views.txt 2009-12-15 20:35:30 +0000
88@@ -17,7 +17,7 @@
89
90 >>> user = getUtility(ILaunchBag).user
91 >>> view = create_view(karmaactionset, '+index', principal=user,
92- ... PATH_INFO='/karmaaction')
93+ ... path_info='/karmaaction')
94 >>> print extract_text(find_tag_by_id(view(), 'karmas'))
95 Category Action Points
96 bugs Bug Accepted 5
97
98=== modified file 'lib/lp/registry/browser/tests/person-views.txt'
99--- lib/lp/registry/browser/tests/person-views.txt 2009-11-20 18:44:57 +0000
100+++ lib/lp/registry/browser/tests/person-views.txt 2009-12-15 20:35:30 +0000
101@@ -4,7 +4,127 @@
102 person's information.
103
104
105-== Email address disclosure ==
106+Probationary and invalid users
107+------------------------------
108+
109+The person +index view provides the is_probationary_or_invalid_user so that
110+page features can be disabled because the user may abuse them. Active
111+users with karma are not on probation; the user's homepage_content is
112+formatted as HTML.
113+
114+ >>> from lp.registry.interfaces.person import IPersonSet
115+
116+ >>> homepage_content = "line one <script>\n\nhttp://aa.aa/"
117+ >>> person_set = getUtility(IPersonSet)
118+ >>> active_user = person_set.getByName('name12')
119+ >>> login_person(active_user)
120+ >>> active_user.homepage_content = homepage_content
121+ >>> login(ANONYMOUS)
122+ >>> view = create_initialized_view(active_user, '+index')
123+ >>> view.is_probationary_or_invalid_user
124+ False
125+
126+ >>> print view.homepage_content
127+ <p>line one &lt;script&gt;</p>
128+ <BLANKLINE>
129+ <p><a rel="nofollow" href="http://aa.aa/">http://<wbr></wbr>aa.aa/</a></p>
130+
131+Teams are always valid and do not have probation rules; the homepage content
132+is formatted HTML.
133+
134+ >>> team = factory.makeTeam()
135+ >>> login_person(team.teamowner)
136+ >>> team.homepage_content = homepage_content
137+ >>> login(ANONYMOUS)
138+ >>> view = create_initialized_view(team, '+index')
139+ >>> view.is_probationary_or_invalid_user
140+ False
141+
142+ >>> print view.homepage_content
143+ <p>line one &lt;script&gt;</p>
144+ <BLANKLINE>
145+ <p><a rel="nofollow" href="http://aa.aa/">http://<wbr></wbr>aa.aa/</a></p>
146+
147+New users (those without karma) are on probation; the homepage content is
148+escaped HTML.
149+
150+ >>> new_user = factory.makePerson()
151+ >>> new_user.homepage_content = homepage_content
152+ >>> new_user.karma
153+ 0
154+
155+ >>> view = create_initialized_view(new_user, '+index')
156+ >>> view.is_probationary_or_invalid_user
157+ True
158+
159+ >>> print view.homepage_content
160+ line one &lt;script&gt;
161+ <BLANKLINE>
162+ http://aa.aa/
163+
164+Inactive and suspended users are invalid; the homepage content is escaped
165+HTML.
166+
167+ >>> from canonical.launchpad.interfaces.account import AccountStatus
168+ >>> from canonical.launchpad.interfaces import IMasterObject
169+
170+ # Only admins can change an account.
171+ >>> admin_user = person_set.getByName('name16')
172+ >>> login_person(admin_user)
173+ >>> invalid_user = factory.makePerson(name="ugh")
174+ >>> invalid_user.homepage_content = homepage_content
175+ >>> IMasterObject(invalid_user.account).status = AccountStatus.NOACCOUNT
176+ >>> view = create_initialized_view(invalid_user, '+index')
177+ >>> view.is_probationary_or_invalid_user
178+ True
179+
180+ >>> print view.homepage_content
181+ line one &lt;script&gt;
182+ <BLANKLINE>
183+ http://aa.aa/
184+
185+ >>> login(ANONYMOUS)
186+
187+If the user has no homepage content, the view's value is None.
188+
189+ >>> new_user.homepage_content = None
190+ >>> view = create_initialized_view(new_user, '+index')
191+ >>> print view.homepage_content
192+ None
193+
194+
195+Account status and profile page status codes
196+--------------------------------------------
197+
198+The PersonIndexView sets the HTTP status code is 410 when the profile is for
199+a suspended account.
200+
201+ # Only admins can change an account.
202+ >>> login_person(active_user)
203+ >>> IMasterObject(invalid_user.account).status = AccountStatus.SUSPENDED
204+ >>> login(ANONYMOUS)
205+ >>> view = create_initialized_view(
206+ ... invalid_user, '+index', principal=ANONYMOUS,
207+ ... server_url='http://launchpad.dev',
208+ ... path_info="/~%s" % invalid_user.name)
209+
210+ >>> content = view()
211+ >>> view.request.response.getStatus()
212+ 410
213+
214+Otherwise it does nothing (which in Zope's case the response is 599).
215+
216+ >>> view = create_initialized_view(
217+ ... active_user, '+index', principal=ANONYMOUS,
218+ ... server_url='http://launchpad.dev',
219+ ... path_info="/~%s" % active_user.name)
220+ >>> content = view()
221+ >>> view.request.response.getStatus()
222+ 599
223+
224+
225+Email address disclosure
226+------------------------
227
228 PersonView is the base for many views for Person objects, including the
229 default view. It provides several properties to help display email
230@@ -19,9 +139,6 @@
231 Mark has a registered email address, and he has chosen to disclose it to
232 the world.
233
234- >>> from lp.registry.interfaces.person import IPersonSet
235-
236- >>> person_set = getUtility(IPersonSet)
237 >>> mark = person_set.getByEmail('mark@example.com')
238 >>> mark.preferredemail.email
239 u'mark@example.com'
240
241=== modified file 'lib/lp/registry/browser/tests/poll-views.txt'
242--- lib/lp/registry/browser/tests/poll-views.txt 2009-11-20 14:06:04 +0000
243+++ lib/lp/registry/browser/tests/poll-views.txt 2009-12-15 20:35:30 +0000
244@@ -16,7 +16,7 @@
245 ... server_url = 'http://launchpad.dev'
246 ... view = create_view(
247 ... team, name=name, principal=principal,
248- ... server_url=server_url, PATH_INFO=path_info)
249+ ... server_url=server_url, path_info=path_info)
250 ... view.initialize()
251 ... return view
252
253
254=== modified file 'lib/lp/registry/stories/person/xx-admin-person-review.txt'
255--- lib/lp/registry/stories/person/xx-admin-person-review.txt 2009-09-18 15:24:30 +0000
256+++ lib/lp/registry/stories/person/xx-admin-person-review.txt 2009-12-15 20:35:30 +0000
257@@ -58,6 +58,10 @@
258 >>> admin_browser.getControl(
259 ... name='field.status_comment').value = 'Bad boy.'
260 >>> admin_browser.getControl('Change').click()
261+ Traceback (most recent call last):
262+ ...
263+ HTTPError: HTTP Error 410: Gone
264+
265 >>> print admin_browser.title
266 The one and only Salgado does not use Launchpad
267
268
269=== modified file 'lib/lp/registry/stories/person/xx-login.txt'
270--- lib/lp/registry/stories/person/xx-login.txt 2009-09-11 20:12:19 +0000
271+++ lib/lp/registry/stories/person/xx-login.txt 2009-12-15 20:35:30 +0000
272@@ -57,6 +57,8 @@
273 when he visits his profile page.
274
275 >>> browser = setupBrowser()
276+ >>> browser.handleErrors = True
277+ >>> browser.raiseHttpErrors = False
278 >>> browser.open('http://launchpad.dev/~bad-user')
279 >>> print browser.title
280 Bad-user does not use Launchpad
281
282=== modified file 'lib/lp/registry/stories/person/xx-reg-with-existing-email.txt'
283--- lib/lp/registry/stories/person/xx-reg-with-existing-email.txt 2009-10-16 16:13:00 +0000
284+++ lib/lp/registry/stories/person/xx-reg-with-existing-email.txt 2009-12-15 20:35:30 +0000
285@@ -134,6 +134,8 @@
286 when he visits his profile page.
287
288 >>> browser = setupBrowser()
289+ >>> browser.handleErrors = True
290+ >>> browser.raiseHttpErrors = False
291 >>> browser.open('http://launchpad.dev/~bad-user')
292 >>> browser.title
293 'Bad-user does not use Launchpad'
294@@ -177,6 +179,9 @@
295 >>> browser.getControl(name='field.password').value = 'test'
296 >>> browser.getControl(name='field.password_dupe').value = 'test'
297 >>> browser.getControl('Continue').click()
298+ Traceback (most recent call last):
299+ ...
300+ HTTPError: HTTP Error 410: Gone
301
302 >>> for tag in find_tags_by_class(
303 ... browser.contents, 'warning'):
304
305=== modified file 'lib/lp/registry/templates/person-index.pt'
306--- lib/lp/registry/templates/person-index.pt 2009-11-20 16:37:51 +0000
307+++ lib/lp/registry/templates/person-index.pt 2009-12-15 20:35:30 +0000
308@@ -16,7 +16,7 @@
309 title="FOAF" href="+rdf"
310 />
311 </tal:valid_person_or_team>
312- <meta tal:condition="not: context/is_valid_person_or_team"
313+ <meta tal:condition="view/is_probationary_or_invalid_user"
314 name="robots" content="noindex,nofollow" />
315 <tal:openid_delegation condition="view/is_delegated_identity">
316 <link rel="openid.server"
317@@ -84,8 +84,8 @@
318
319 <div
320 class="description"
321- tal:condition="context/homepage_content"
322- tal:content="structure context/homepage_content/fmt:text-to-html"
323+ tal:condition="view/homepage_content"
324+ tal:content="structure view/homepage_content"
325 />
326 <ul class="horizontal">
327 <tal:comment condition="nothing">
328
329=== modified file 'lib/lp/testing/views.py'
330--- lib/lp/testing/views.py 2009-10-05 20:31:28 +0000
331+++ lib/lp/testing/views.py 2009-12-15 20:35:30 +0000
332@@ -43,7 +43,7 @@
333 if request is None:
334 request = LaunchpadTestRequest(
335 form=form, SERVER_URL=server_url, QUERY_STRING=query_string,
336- HTTP_COOKIE=cookie, method=method, **kwargs)
337+ HTTP_COOKIE=cookie, method=method, PATH_INFO=path_info, **kwargs)
338 if principal is not None:
339 request.setPrincipal(principal)
340 else:
341@@ -59,7 +59,8 @@
342
343 def create_initialized_view(context, name, form=None, layer=None,
344 server_url=None, method=None, principal=None,
345- query_string=None, cookie=None, request=None):
346+ query_string=None, cookie=None, request=None,
347+ path_info='/'):
348 """Return a view that has already been initialized."""
349 if method is None:
350 if form is None:
351@@ -68,6 +69,6 @@
352 method = 'POST'
353 view = create_view(
354 context, name, form, layer, server_url, method, principal,
355- query_string, cookie, request)
356+ query_string, cookie, request, path_info)
357 view.initialize()
358 return view