Code review comment for lp:~edwin-grubbs/launchpad/bug-451208-subscribing-reveals-email

> === modified file 'lib/canonical/launchpad/webapp/configure.zcml'
> --- lib/canonical/launchpad/webapp/configure.zcml 2009-11-18 00:22:41 +0000
> +++ lib/canonical/launchpad/webapp/configure.zcml 2010-01-12 22:36:26 +0000
> @@ -809,6 +809,7 @@
>
> <class class="canonical.launchpad.webapp.vocabulary.CountableIterator">
> <allow interface="canonical.launchpad.webapp.vocabulary.ICountableIterator" />
> + <allow attributes="__getslice__"/>

Wouldn't it make sense to add __getslice__ to CountableIterator?

> </class>
>
> <class class="canonical.launchpad.webapp.vocabulary.BatchedCountableIterator">
>
> === modified file 'lib/lp/registry/doc/vocabularies.txt'
> --- lib/lp/registry/doc/vocabularies.txt 2009-12-24 01:41:54 +0000
> +++ lib/lp/registry/doc/vocabularies.txt 2010-01-12 22:36:26 +0000
> @@ -20,7 +20,9 @@
> The active mailing lists vocabulary matches and returns only those mailing
> lists which are active.
>
> - >>> list_vocabulary = vocabulary_registry.get(None, 'ActiveMailingList')
> + >>> from zope.security.proxy import removeSecurityProxy
> + >>> list_vocabulary = removeSecurityProxy(
> + ... vocabulary_registry.get(None, 'ActiveMailingList'))
> >>> from canonical.launchpad.webapp.testing import verifyObject
> >>> from canonical.launchpad.webapp.vocabulary import IHugeVocabulary
> >>> verifyObject(IHugeVocabulary, list_vocabulary)
> @@ -203,6 +205,7 @@
> u'The Hoary Hedgehog Release'
>
> >>> def getTerms(vocab, search_text):
> + ... vocab = removeSecurityProxy(vocab)
> ... [vocab.toTerm(item) for item in vocab.search(search_text)]
>
> >>> getTerms(distroseries_vocabulary, 'woody')
> @@ -508,7 +511,8 @@
>
> The list of selectable projects. The results are ordered by displayname.
>
> - >>> project_vocabulary = vocabulary_registry.get(None, "Project")
> + >>> project_vocabulary = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "Project"))

I think we wought to have a wrapper around vocabulary_registry.get() to remove
the security proxy of the vocab before returning. Something like

    def get_naked_vocab(context, name):
        return removeSecurityProxy(vocab_registry.get(context, name))

That way we don't have to repeat the removeSecurityProxy() call in all the
places we call vocabulary_registry.get.

> >>> project_vocabulary.displayname
> 'Select a project group'
>
> @@ -542,7 +546,8 @@
>
> The list of selectable products. Results are ordered by displayname.
>
> - >>> product_vocabulary = vocabulary_registry.get(None, "Product")
> + >>> product_vocabulary = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "Product"))
> >>> product_vocabulary.displayname
> 'Select a project'
>
> @@ -583,8 +588,8 @@
>
> The list of selectable products releases.
>
> - >>> productrelease_vocabulary = vocabulary_registry.get(None,
> - ... "ProductRelease")
> + >>> productrelease_vocabulary = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "ProductRelease"))
> >>> productrelease_vocabulary.displayname
> 'Select a Product Release'
>
> @@ -603,7 +608,8 @@
> All non-merged people with at least one email address. This vocabulary is
> meant to be used only in the people merge form.
>
> - >>> vocab = vocabulary_registry.get(None, "PersonAccountToMerge")
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "PersonAccountToMerge"))
> >>> vocab.displayname
> 'Select a Person to Merge'
>
> @@ -693,7 +699,8 @@
>
> The set of non-merged people.
>
> - >>> vocab = vocabulary_registry.get(None, "AdminMergeablePerson")
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "AdminMergeablePerson"))
> >>> vocab.displayname
> 'Select a Person to Merge'
>
> @@ -712,7 +719,8 @@
>
> All non-merged people and teams.
>
> - >>> vocab = vocabulary_registry.get(None, "NonMergedPeopleAndTeams")
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "NonMergedPeopleAndTeams"))
> >>> vocab.displayname
> 'Select a Person or Team'
>
> @@ -746,7 +754,8 @@
> None). It also includes all public teams and private teams the
> user has permission to view.
>
> - >>> vocab = vocabulary_registry.get(None, "ValidPersonOrTeam")
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "ValidPersonOrTeam"))
> >>> vocab.displayname
> 'Select a Person or Team'
>
> @@ -794,7 +803,8 @@
> A PRIVATE team is displayed when the logged in user is a member of the team.
>
> >>> commercial = person_set.getByEmail('<email address hidden>')
> - >>> vocab = vocabulary_registry.get(commercial, "ValidPersonOrTeam")
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(commercial, "ValidPersonOrTeam"))
> >>> login('<email address hidden>')
> >>> priv_team = factory.makeTeam(
> ... name='private-team',
> @@ -861,7 +871,8 @@
> one of its email addresses.
>
> >>> login('<email address hidden>')
> - >>> vocab = vocabulary_registry.get(None, "ValidPersonOrTeam")
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "ValidPersonOrTeam"))
> >>> sorted(person.name for person in vocab.search('support'))
> [u'ubuntu-team']
>
> @@ -932,7 +943,8 @@
> All valid persons and teams are also valid owners.
>
> >>> login(ANONYMOUS)
> - >>> vocab = vocabulary_registry.get(None, "ValidOwner")
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "ValidOwner"))
> >>> vocab.displayname
> 'Select a Person or Team'
>
> @@ -955,7 +967,8 @@
> except that its terms are limited only to teams. No non-team Persons will be
> returned.
>
> - >>> vocab = vocabulary_registry.get(None, 'ValidTeam')
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(None, 'ValidTeam'))
> >>> vocab.displayname
> 'Select a Team'
> >>> sorted((team.displayname, team.teamowner.displayname)
> @@ -1076,7 +1089,8 @@
>
> 'name16' is a valid member for 'ubuntu-team':
>
> - >>> vocab = vocabulary_registry.get(team, "ValidTeamMember")
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(team, "ValidTeamMember"))
> >>> vocab.displayname
> 'Select a Person or Team'
>
> @@ -1126,7 +1140,8 @@
>
> 'ubuntu-team' is not a valid owner for itself.
>
> - >>> vocab = vocabulary_registry.get(team, "ValidTeamOwner")
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(team, "ValidTeamOwner"))
> >>> vocab.displayname
> 'Select a Person or Team'
>
> @@ -1147,7 +1162,8 @@
>
> All 'valid' persons who are not a team.
>
> - >>> vocab = vocabulary_registry.get(None, "ValidPerson")
> + >>> vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "ValidPerson"))
> >>> vocab.displayname
> 'Select a Person'
> >>> people = vocab.search(None)
> @@ -1172,8 +1188,8 @@
> >>> carlos = getUtility(IPersonSet).getByName('carlos')
> >>> carlos_team = factory.makeTeam(
> ... owner=carlos, name='carlos-team')
> - >>> person_or_team_vocab = vocabulary_registry.get(
> - ... None, "ValidPersonOrTeam")
> + >>> person_or_team_vocab = removeSecurityProxy(
> + ... vocabulary_registry.get(None, "ValidPersonOrTeam"))
> >>> carlos_people_or_team = person_or_team_vocab.search('carlos')
> >>> # The people or team search yields our one Carlos person and
> >>> # the new team.
>
> === added file 'lib/lp/services/tests/test_vocabularies.py'
> --- lib/lp/services/tests/test_vocabularies.py 1970-01-01 00:00:00 +0000
> +++ lib/lp/services/tests/test_vocabularies.py 2010-01-12 22:36:26 +0000
> @@ -0,0 +1,33 @@
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Test registered vocabularies."""
> +
> +__metaclass__ = type
> +__all__ = []
> +
> +import unittest
> +
> +from zope.component import getUtilitiesFor
> +from zope.schema.interfaces import IVocabularyFactory
> +from zope.security._proxy import _Proxy
> +
> +from canonical.testing.layers import FunctionalLayer
> +from lp.testing import TestCase
> +
> +
> +class TestVocabularies(TestCase):
> + layer = FunctionalLayer
> +
> + def test_security_proxy(self):
> + """Our vocabularies should be registered with <securedutility>."""
> + vocabularies = getUtilitiesFor(IVocabularyFactory)
> + for name, vocab in vocabularies:
> + if type(vocab) != _Proxy and vocab.__module__[:5] != 'zope.':
> + raise AssertionError(
> + '%s.%s vocabulary is not wrapped in a security proxy.' % (
> + vocab.__module__, name))

Isn't there a simpler way to check that a given object is wrapped in a
security proxy? Let's check with Gary...

> +
> +
> +def test_suite():
> + return unittest.TestLoader().loadTestsFromName(__name__)
>