Merge lp:~sinzui/launchpad/vocab-storm-bug-413287 into lp:launchpad

[Curtis Hovey]

[Curtis Hovey (sinzui)]

This is my branch to prevent oopses search or validating persons with '%'
in their names.

    lp:~sinzui/launchpad/vocab-storm-bug-413287
    Diff size: 96
    Launchpad bug: https://bugs.launchpad.net/bugs/413287
    Test command: ./bin/test -vvt "registry.*doc/vocab"
    Pre-implementation: no one
    Target release: 3.1.11

= Prevent oopses search or validating persons with '%' in their names =

The '%' is just not escaped, so psycopg's variable substitution blows up.
The ValidPersonOrTeamVocabulary uses SQL() to add hand-coded sql to a storm
query. When it does this, it uses python string substitutions. Instead, we
should use the optional second argument to SQL() to pass in the variables,
which will escape them properly.
For example:
   SQL('SELECT name FROM person WHERE id = ? AND displayname = ?', (33, 'foo'))

This should also eliminate the need to use quote() and quote_like() on the
parameters.

== Rules ==

    * Revise the query to be a Storm expression.

== QA ==

    * Try to reassign an project or team to another user.
    * Place a percent in the user's name.
    * Verify that the screen explains that there are no matching users.

    * Using the person picker
    * Search for a name with a percent in it
    * Verify you do not get an error.

== Lint ==

Linting changed files:
  lib/lp/registry/vocabularies.py
  lib/lp/registry/doc/vocabularies.txt

== Test ==

    * lib/lp/registry/doc/vocabularies.txt
      * Added a test to verify that searching for % and ? work.

== Implementation ==

    * lib/lp/registry/vocabularies.py
      * Revised the two parts of the SQL to use a storm expression

[Eleanor Berger (intellectronica)]

review: Approve

=== modified file 'lib/lp/registry/doc/vocabularies.txt'
--- lib/lp/registry/doc/vocabularies.txt	2009-10-26 18:40:04 +0000
+++ lib/lp/registry/doc/vocabularies.txt	2009-11-21 19:05:35 +0000
@@ -906,8 +906,29 @@
     >>> [person.displayname for person in vocab.search('team')]
     [u'HWDB Team', u'No Team Memberships', u'testing Spanish team']
 
-
-=== ValidOwner ===
+Search for names with '%' and '?' is supported.
+
+    >>> from lp.registry.interfaces.irc import IIrcIDSet
+
+    >>> symbolic_person = factory.makePerson(name='symbolic')
+    >>> ircid_set = getUtility(IIrcIDSet)
+    >>> irc_id = ircid_set.new(
+    ...     symbolic_person, 'irc.freenode.net', '%percent')
+    >>> irc_id =ircid_set.new(
+    ...     symbolic_person, 'irc.fnord.net', 'question?')
+    >>> transaction.commit()
+
+    >>> for person in vocab.search('%percent'):
+    ...     print person.name
+    symbolic
+
+    >>> for person in vocab.search('question?'):
+    ...     print person.name
+    symbolic
+
+
+ValidOwner
+..........
 
 All valid persons and teams are also valid owners.
 

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2009-10-26 18:40:04 +0000
+++ lib/lp/registry/vocabularies.py	2009-11-21 19:05:35 +0000
@@ -504,33 +504,31 @@
             # the best results. The fti rank will be between 0 and 1.
             # Note we use lower() instead of the non-standard ILIKE because
             # ILIKE doesn't hit the indexes.
-            # The '%%%%' is necessary, since the first variable substitution
-            # converts it to '%%' and the storm variable substitution converts
-            # it to '%'.
+            # The '%%' is necessary because storm variable substitution
+            # converts it to '%'.
             public_inner_textual_select = SQL("""
                 SELECT id FROM (
                     SELECT Person.id, 100 AS rank
                     FROM Person
-                    WHERE name = %(text)s
+                    WHERE name = ?
                     UNION ALL
-                    SELECT Person.id, rank(fti, ftq(%(text)s))
+                    SELECT Person.id, rank(fti, ftq(?))
                     FROM Person
-                    WHERE Person.fti @@ ftq(%(text)s)
+                    WHERE Person.fti @@ ftq(?)
                     UNION ALL
                     SELECT Person.id, 10 AS rank
                     FROM Person, IrcId
                     WHERE IrcId.person = Person.id
-                        AND lower(IrcId.nickname) = %(text)s
+                        AND lower(IrcId.nickname) = ?
                     UNION ALL
                     SELECT Person.id, 1 AS rank
                     FROM Person, EmailAddress
                     WHERE EmailAddress.person = Person.id
-                        AND lower(email) LIKE %(like_text)s || '%%%%'
+                        AND lower(email) LIKE ? || '%%'
                     ) AS public_subquery
                 ORDER BY rank DESC
-                LIMIT %(limit)d
-                """ % dict(text=quote(text), like_text=quote_like(text),
-                           limit=self.LIMIT))
+                LIMIT ?
+                """, (text, text, text, text, text, self.LIMIT))
 
             public_result = self.store.using(*public_tables).find(
                 Person,
@@ -564,9 +562,9 @@
             private_inner_select = SQL("""
                 SELECT Person.id
                 FROM Person
-                WHERE Person.fti @@ ftq(%s)
-                LIMIT %d
-                """ % (quote(text), self.LIMIT))
+                WHERE Person.fti @@ ftq(?)
+                LIMIT ?
+                """, (text, self.LIMIT))
             private_result = self.store.using(*private_tables).find(
                 Person,
                 And(