launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04269
[Merge] lp:~henninge/launchpad/bug-740208-ws-regression into lp:launchpad
Henning Eggers has proposed merging lp:~henninge/launchpad/bug-740208-ws-regression into lp:launchpad with lp:~launchpad-orange-squad/launchpad/bug-740208-obfuscate-ws as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~henninge/launchpad/bug-740208-ws-regression/+merge/67937
= Summary =
This branch fixes the IDiff interface to not call something a text
which is really implemented as a dict nowadays. The "diffstat" attribute
is stored in the database as a JSON string but the Diff model implements
a getter and setter that deserialize it into a dict. I guess this was
added later but the interface was never updated.
== Pre-implementation notes ==
At first I was not sure if changing the interface would have a negative
impact on the web API as it breaks its consistency. Wgrant and I
agreed that obviously nobody is using this as a text field because
otherwise it would have been noticed much earlier. Therefore the change
should not cause any regressions.
== Implementation details ==
I used zope.schema.Dict but have since found that it is not used
elsewhere. I guess we'd normally use Attribute but as this attribute
*is* a dict, I find that fitting ...
== Tests ==
No new test. AFAIK there is no way to test if the implementation of an
attribute fits its interface. I ran the following to check for
regressions. It took about 15 minutes.
bin/test -vvct diff
== Demo and Q/A ==
No Q/A possible AFAICT.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/browser/stringformatter.py
lib/lp/app/webservice/__init__.py
lib/lp/app/webservice/tests/__init__.py
zcml/override-includes/ws-marshaller-configure.zcml
lib/lp/app/webservice/tests/test_marshallers.py
lib/lp/app/browser/tests/test_webservice.py
lib/lp/app/webservice/marshallers.py
lib/lp/app/browser/webservice.py
lib/lp/code/interfaces/diff.py
lib/lp/services/utils.py
./lib/lp/app/browser/stringformatter.py
557: Line exceeds 78 characters.
557: E501 line too long (86 characters)
--
https://code.launchpad.net/~henninge/launchpad/bug-740208-ws-regression/+merge/67937
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/bug-740208-ws-regression into lp:launchpad.
=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py 2011-07-07 10:14:55 +0000
+++ lib/lp/app/browser/stringformatter.py 2011-07-14 10:07:24 +0000
@@ -34,6 +34,10 @@
from canonical.launchpad.webapp.interfaces import ILaunchBag
from lp.answers.interfaces.faq import IFAQSet
from lp.registry.interfaces.person import IPersonSet
+from lp.services.utils import (
+ re_email_address,
+ obfuscate_email,
+ )
def escape(text, quote=True):
@@ -237,7 +241,7 @@
def extract_email_addresses(text):
'''Unique email addresses in the text.'''
- matches = re.finditer(FormattersAPI._re_email, text)
+ matches = re.finditer(re_email_address, text)
return list(set([match.group() for match in matches]))
@@ -361,12 +365,13 @@
# devaluing the return on effort for spammers that consider
# using Launchpad.
if not FormattersAPI._linkify_url_should_be_ignored(url):
- link_string = ('<a rel="nofollow" '
- 'href="%(url)s">%(linked_text)s</a>%(trailers)s' % {
- 'url': cgi.escape(url, quote=True),
- 'linked_text': add_word_breaks(cgi.escape(url)),
- 'trailers': cgi.escape(trailers)
- })
+ link_string = (
+ '<a rel="nofollow" '
+ 'href="%(url)s">%(linked_text)s</a>%(trailers)s' % {
+ 'url': cgi.escape(url, quote=True),
+ 'linked_text': add_word_breaks(cgi.escape(url)),
+ 'trailers': cgi.escape(trailers)
+ })
return link_string
else:
return full_url
@@ -776,25 +781,6 @@
output.append(line)
return '\n'.join(output)
- # This is a regular expression that matches email address embedded in
- # text. It is not RFC 2821 compliant, nor does it need to be. This
- # expression strives to identify probable email addresses so that they
- # can be obfuscated when viewed by unauthenticated users. See
- # http://www.email-unlimited.com/stuff/email_address_validator.htm
-
- # localnames do not have [&?%!@<>,;:`|{}()#*^~ ] in practice
- # (regardless of RFC 2821) because they conflict with other systems.
- # See https://lists.ubuntu.com
- # /mailman/private/launchpad-reviews/2007-June/006081.html
-
- # This verson of the re is more than 5x faster that the orginal
- # version used in ftest/test_tales.testObfuscateEmail.
- _re_email = re.compile(r"""
- \b[a-zA-Z0-9._/="'+-]{1,64}@ # The localname.
- [a-zA-Z][a-zA-Z0-9-]{1,63} # The hostname.
- \.[a-zA-Z0-9.-]{1,251}\b # Dot starts one or more domains.
- """, re.VERBOSE) # ' <- font-lock turd
-
def obfuscate_email(self):
"""Obfuscate an email address if there's no authenticated user.
@@ -813,11 +799,7 @@
"""
if getUtility(ILaunchBag).user is not None:
return self._stringtoformat
- text = self._re_email.sub(
- r'<email address hidden>', self._stringtoformat)
- text = text.replace(
- "<<email address hidden>>", "<email address hidden>")
- return text
+ return obfuscate_email(self._stringtoformat)
def linkify_email(self, preloaded_person_data=None):
"""Linkify any email address recognised in Launchpad.
@@ -831,7 +813,7 @@
"""
text = self._stringtoformat
- matches = re.finditer(self._re_email, text)
+ matches = re.finditer(re_email_address, text)
for match in matches:
address = match.group()
person = None
=== modified file 'lib/lp/app/browser/tests/test_webservice.py'
--- lib/lp/app/browser/tests/test_webservice.py 2011-07-07 10:14:55 +0000
+++ lib/lp/app/browser/tests/test_webservice.py 2011-07-14 10:07:24 +0000
@@ -30,7 +30,8 @@
request = get_current_web_service_request()
renderer = getMultiAdapter(
(product, field, request), IFieldHTMLRenderer)
- # The representation of a person is the same as a tales PersonFormatter.
+ # The representation of a person is the same as a tales
+ # PersonFormatter.
self.assertEqual(format_link(eric), renderer(eric))
def test_text(self):
@@ -42,9 +43,9 @@
request = get_current_web_service_request()
renderer = getMultiAdapter(
(product, field, request), IFieldHTMLRenderer)
- # The representation is linkified html with hidden email.
+ # The representation is linkified html.
self.assertEqual(
- u'<p>\N{SNOWMAN} <email address hidden> '
+ u'<p>\N{SNOWMAN} snowman@xxxxxxxxxxx '
'<a href="/bugs/1">bug 1</a></p>',
renderer(text))
=== modified file 'lib/lp/app/browser/webservice.py'
--- lib/lp/app/browser/webservice.py 2011-07-07 10:14:55 +0000
+++ lib/lp/app/browser/webservice.py 2011-07-14 10:07:24 +0000
@@ -18,7 +18,7 @@
)
from zope.schema.interfaces import IText
-from lp.app.browser.lazrjs import standard_text_html_representation
+from lp.app.browser.stringformatter import FormattersAPI
from lp.app.browser.tales import format_link
@@ -41,4 +41,6 @@
@implementer(IFieldHTMLRenderer)
def text_xhtml_representation(context, field, request):
"""Render text as XHTML using the webservice."""
- return standard_text_html_representation
+ return lambda text: (
+ '' if text is None
+ else FormattersAPI(text).text_to_html(linkify_text=True))
=== added directory 'lib/lp/app/webservice'
=== added file 'lib/lp/app/webservice/__init__.py'
--- lib/lp/app/webservice/__init__.py 1970-01-01 00:00:00 +0000
+++ lib/lp/app/webservice/__init__.py 2011-07-14 10:07:24 +0000
@@ -0,0 +1,4 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""This package contains code for the Launchpad webservice."""
=== added file 'lib/lp/app/webservice/marshallers.py'
--- lib/lp/app/webservice/marshallers.py 1970-01-01 00:00:00 +0000
+++ lib/lp/app/webservice/marshallers.py 2011-07-14 10:07:24 +0000
@@ -0,0 +1,32 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Launchpad-specific field marshallers for the web service."""
+
+__metaclass__ = type
+
+__all__ = [
+ 'TextFieldMarshaller',
+ ]
+
+
+from lazr.restful.marshallers import (
+ TextFieldMarshaller as LazrTextFieldMarshaller,
+ )
+from zope.app.security.interfaces import IUnauthenticatedPrincipal
+
+from lp.services.utils import obfuscate_email
+
+
+class TextFieldMarshaller(LazrTextFieldMarshaller):
+ """Do not expose email addresses for anonymous users."""
+
+ def unmarshall(self, entry, value):
+ """See `IFieldMarshaller`.
+
+ Return the value as is.
+ """
+ if (value is not None and
+ IUnauthenticatedPrincipal.providedBy(self.request.principal)):
+ return obfuscate_email(value)
+ return value
=== added directory 'lib/lp/app/webservice/tests'
=== added file 'lib/lp/app/webservice/tests/__init__.py'
--- lib/lp/app/webservice/tests/__init__.py 1970-01-01 00:00:00 +0000
+++ lib/lp/app/webservice/tests/__init__.py 2011-07-14 10:07:24 +0000
@@ -0,0 +1,4 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for code for the Launchpad webservice."""
=== added file 'lib/lp/app/webservice/tests/test_marshallers.py'
--- lib/lp/app/webservice/tests/test_marshallers.py 1970-01-01 00:00:00 +0000
+++ lib/lp/app/webservice/tests/test_marshallers.py 2011-07-14 10:07:24 +0000
@@ -0,0 +1,139 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for the webservice marshallers."""
+
+__metaclass__ = type
+
+import transaction
+from zope.component import getUtility
+
+from canonical.launchpad.testing.pages import (
+ LaunchpadWebServiceCaller,
+ webservice_for_person,
+ )
+from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
+from canonical.launchpad.webapp.servers import WebServiceTestRequest
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.webservice.marshallers import TextFieldMarshaller
+from lp.testing import logout, TestCaseWithFactory
+
+
+def ws_url(bug):
+ url = "/bugs/%d" % bug.id
+ return url
+
+
+class TestTextFieldMarshaller(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def _makeRequest(self, is_anonymous):
+ """Create either an anonymous or authenticated request."""
+ request = WebServiceTestRequest()
+ if is_anonymous:
+ request.setPrincipal(
+ getUtility(IPlacelessAuthUtility).unauthenticatedPrincipal())
+ else:
+ request.setPrincipal(self.factory.makePerson())
+ return request
+
+ def test_unmarshall_obfuscated(self):
+ # Data is obfuccated if the request is anonynous.
+ request = self._makeRequest(is_anonymous=True)
+ marshaller = TextFieldMarshaller(None, request)
+ result = marshaller.unmarshall(None, u"foo@xxxxxxxxxxx")
+ self.assertEqual(u"<email address hidden>", result)
+
+ def test_unmarshall_not_obfuscated(self):
+ # Data is not obfuccated if the request is authenticated.
+ request = self._makeRequest(is_anonymous=False)
+ marshaller = TextFieldMarshaller(None, request)
+ result = marshaller.unmarshall(None, u"foo@xxxxxxxxxxx")
+ self.assertEqual(u"foo@xxxxxxxxxxx", result)
+
+
+class TestWebServiceObfuscation(TestCaseWithFactory):
+ """Integration test for obfuscation marshaller.
+
+ Not using WebServiceTestCase because that assumes too much about users
+ """
+
+ layer = DatabaseFunctionalLayer
+
+ email_address = "joe@xxxxxxxxxxx"
+ email_address_obfuscated = "<email address hidden>"
+ email_address_obfuscated_escaped = "<email address hidden>"
+ bug_title = "Title with address %s in it"
+ bug_description = "Description with address %s in it"
+
+ def _makeBug(self):
+ """Create a bug with an email address in title and description."""
+ bug = self.factory.makeBug(
+ title=self.bug_title % self.email_address,
+ description=self.bug_description % self.email_address)
+ transaction.commit()
+ return bug
+
+ def test_email_address_obfuscated(self):
+ # Email addresses are obfuscated for anonymous users.
+ bug = self._makeBug()
+ logout()
+ webservice = LaunchpadWebServiceCaller()
+ result = webservice(ws_url(bug)).jsonBody()
+ self.assertEqual(
+ self.bug_title % self.email_address_obfuscated,
+ result['title'])
+ self.assertEqual(
+ self.bug_description % self.email_address_obfuscated,
+ result['description'])
+
+ def test_email_address_not_obfuscated(self):
+ # Email addresses are not obfuscated for authenticated users.
+ bug = self._makeBug()
+ user = self.factory.makePerson()
+ webservice = webservice_for_person(user)
+ result = webservice(ws_url(bug)).jsonBody()
+ self.assertEqual(self.bug_title % self.email_address, result['title'])
+ self.assertEqual(
+ self.bug_description % self.email_address, result['description'])
+
+ def test_xhtml_email_address_not_obfuscated(self):
+ # Email addresses are not obfuscated for authenticated users.
+ bug = self._makeBug()
+ user = self.factory.makePerson()
+ webservice = webservice_for_person(user)
+ result = webservice(
+ ws_url(bug), headers={'Accept': 'application/xhtml+xml'})
+ self.assertIn(self.email_address, result.body)
+ self.assertNotIn(
+ self.email_address_obfuscated_escaped, result.body)
+
+ def test_xhtml_email_address_obfuscated(self):
+ # Email addresses are obfuscated in the XML representation for
+ # anonymous users.
+ bug = self._makeBug()
+ logout()
+ webservice = LaunchpadWebServiceCaller()
+ result = webservice(
+ ws_url(bug), headers={'Accept': 'application/xhtml+xml'})
+ self.assertNotIn(self.email_address, result.body)
+ self.assertIn(self.email_address_obfuscated_escaped, result.body)
+
+ def test_etags_differ_for_anon_and_non_anon_represetations(self):
+ # When a webservice client retrieves data anonymously, this
+ # data should not be used in later write requests, if the
+ # text fields contain obfuscated email addresses. The etag
+ # for a GET request is calculated after the email address
+ # obfuscation and thus differs from the etag returned for
+ # not obfuscated data, so clients usings etags to check if the
+ # cached data is up to date will not use the obfuscated data
+ # in PATCH or PUT requests.
+ bug = self._makeBug()
+ user = self.factory.makePerson()
+ webservice = webservice_for_person(user)
+ etag_logged_in = webservice(ws_url(bug)).getheader('etag')
+ logout()
+ webservice = LaunchpadWebServiceCaller()
+ etag_logged_out = webservice(ws_url(bug)).getheader('etag')
+ self.assertNotEqual(etag_logged_in, etag_logged_out)
=== modified file 'lib/lp/code/interfaces/diff.py'
--- lib/lp/code/interfaces/diff.py 2011-01-31 17:08:35 +0000
+++ lib/lp/code/interfaces/diff.py 2011-07-14 10:07:24 +0000
@@ -24,6 +24,7 @@
from zope.schema import (
Bool,
Bytes,
+ Dict,
Int,
Text,
TextLine,
@@ -53,7 +54,7 @@
Int(title=_('The number of lines in this diff.'), readonly=True))
diffstat = exported(
- Text(title=_('Statistics about this diff'), readonly=True))
+ Dict(title=_('Statistics about this diff'), readonly=True))
added_lines_count = exported(
Int(title=_('The number of lines added in this diff.'),
=== modified file 'lib/lp/services/utils.py'
--- lib/lp/services/utils.py 2011-07-11 17:56:22 +0000
+++ lib/lp/services/utils.py 2011-07-14 10:07:24 +0000
@@ -18,6 +18,8 @@
'file_exists',
'iter_list_chunks',
'iter_split',
+ 'obfuscate_email',
+ 're_email_address',
'RegisteredSubclass',
'run_capturing_output',
'synchronize',
@@ -30,6 +32,7 @@
from datetime import datetime
from itertools import tee
import os
+import re
from StringIO import StringIO
import string
import sys
@@ -311,3 +314,40 @@
def utc_now():
"""Return a timezone-aware timestamp for the current time."""
return datetime.now(tz=pytz.UTC)
+
+
+# This is a regular expression that matches email address embedded in
+# text. It is not RFC 2821 compliant, nor does it need to be. This
+# expression strives to identify probable email addresses so that they
+# can be obfuscated when viewed by unauthenticated users. See
+# http://www.email-unlimited.com/stuff/email_address_validator.htm
+
+# localnames do not have [&?%!@<>,;:`|{}()#*^~ ] in practice
+# (regardless of RFC 2821) because they conflict with other systems.
+# See https://lists.ubuntu.com
+# /mailman/private/launchpad-reviews/2007-June/006081.html
+
+# This verson of the re is more than 5x faster that the orginal
+# version used in ftest/test_tales.testObfuscateEmail.
+re_email_address = re.compile(r"""
+ \b[a-zA-Z0-9._/="'+-]{1,64}@ # The localname.
+ [a-zA-Z][a-zA-Z0-9-]{1,63} # The hostname.
+ \.[a-zA-Z0-9.-]{1,251}\b # Dot starts one or more domains.
+ """, re.VERBOSE) # ' <- font-lock turd
+
+
+def obfuscate_email(text_to_obfuscate):
+ """Obfuscate an email address.
+
+ The email address is obfuscated as <email address hidden>.
+
+ The pattern used to identify an email address is not 2822. It strives
+ to match any possible email address embedded in the text. For example,
+ mailto:person@xxxxxxxxxx and http://person:password@xxxxxxxxxx both
+ match, though the http match is in fact not an email address.
+ """
+ text = re_email_address.sub(
+ r'<email address hidden>', text_to_obfuscate)
+ text = text.replace(
+ "<<email address hidden>>", "<email address hidden>")
+ return text
=== added file 'zcml/override-includes/ws-marshaller-configure.zcml'
--- zcml/override-includes/ws-marshaller-configure.zcml 1970-01-01 00:00:00 +0000
+++ zcml/override-includes/ws-marshaller-configure.zcml 2011-07-14 10:07:24 +0000
@@ -0,0 +1,23 @@
+<!-- Copyright 2011 Canonical Ltd. This software is licensed under the
+ GNU Affero General Public License version 3 (see the file LICENSE).
+-->
+
+<configure xmlns="http://namespaces.zope.org/zope";>
+ <!-- Override lazr-restful marshallers for test field to enable
+ automatic obfuscation of email addresses for anonymous users. -->
+
+ <adapter
+ for="zope.schema.interfaces.IASCII
+ zope.publisher.interfaces.http.IHTTPRequest"
+ provides="lazr.restful.interfaces.IFieldMarshaller"
+ factory="lp.app.webservice.marshallers.TextFieldMarshaller"
+ />
+
+ <adapter
+ for="zope.schema.interfaces.IText
+ zope.publisher.interfaces.http.IHTTPRequest"
+ provides="lazr.restful.interfaces.IFieldMarshaller"
+ factory="lp.app.webservice.marshallers.TextFieldMarshaller"
+ />
+
+</configure>
\ No newline at end of file
Follow ups
