launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === modified file 'lib/lp/answers/tests/test_question.py'
> --- lib/lp/answers/tests/test_question.py	2015-01-29 14:14:01 +0000
> +++ lib/lp/answers/tests/test_question.py	2015-12-01 06:43:00 +0000
> @@ -3,12 +3,42 @@
>  
>  __metaclass__ = type
>  
> +from testtools.testcase import ExpectedException
> +from zope.security.interfaces import Unauthorized
>  from zope.security.proxy import removeSecurityProxy
>  
> -from lp.testing import TestCaseWithFactory
> +from lp.testing import (
> +    admin_logged_in,
> +    anonymous_logged_in,
> +    celebrity_logged_in,
> +    person_logged_in,
> +    TestCaseWithFactory,
> +    )
>  from lp.testing.layers import DatabaseFunctionalLayer
>  
>  
> +class TestQuestionSecurity(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_title_and_description_writes(self):

Maybe actually test writing to description here as well, just as a hint to future developers.

> +        question = self.factory.makeQuestion()
> +        with anonymous_logged_in():
> +            with ExpectedException(Unauthorized):
> +                question.title = 'foo anon'
> +        with person_logged_in(self.factory.makePerson()):
> +            with ExpectedException(Unauthorized):
> +                question.title = 'foo random'
> +        with person_logged_in(question.owner):
> +            question.title = 'foo owner'
> +        with person_logged_in(question.target.owner):
> +            question.title = 'foo target owner'
> +        with admin_logged_in():
> +            question.title = 'foo admin'
> +        with celebrity_logged_in('registry_experts'):
> +            question.title = 'foo registry'
> +
> +
>  class TestQuestionSearch(TestCaseWithFactory):
>      layer = DatabaseFunctionalLayer
>  
> 
> === modified file 'lib/lp/security.py'
> --- lib/lp/security.py	2015-07-23 16:02:58 +0000
> +++ lib/lp/security.py	2015-12-01 06:43:00 +0000
> @@ -1977,7 +1977,7 @@
>          """Allow only admins and owners of the question pillar target."""
>          context = self.obj.product or self.obj.distribution
>          return (AdminByAdminsTeam.checkAuthenticated(self, user) or
> -                user.inTeam(context.owner))
> +                user.in_registry_experts or user.inTeam(context.owner))

I'd prefer either "EditByRegistryExpertsOrAdmins.checkAuthenticated(self, user)" or "user.in_admin or user.in_registry_experts" rather than mixing the two styles; probably the latter.

>  
>  
>  class AppendQuestion(AdminQuestion):


-- 
https://code.launchpad.net/~wgrant/launchpad/question-content-lockdown/+merge/279069
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.