Merge lp:~sinzui/launchpad/faq-mailing-list-permissions-0 into lp:launchpad

[Curtis Hovey]

Allow answer contacts for packages to create/edit FAQs.

    Launchpad bug: https://bugs.launchpad.net/bugs/682135
    Pre-implementation: jcsackett
    Test command: ./bin/test -vv -t test_faqtarget -t test_faq
      or to test all changes are kosher: -m lp.answers

Answer contacts for Ubuntu packages cannot create FAQs. There are cases where
the link is shown but the user gets an error when creating the FAQ. This
looked like a conflicting permission at first because the permissions were
changed a few months ago. Examining the history of the code and tests shows
that the permission has aways been broken.

The problem is in the security checker. The checker is based on the question
checker (answer_contacts), but the set of permitted users is larger.
Distribution users who can edit an FAQ are comprised of distro answer_contacts
+ the answer_contacts of all the packages.

That is a lot of inTeam() checks across a lot of packages. There is an
alternate way to solve this that will involve exactly two queries regardless
of the number of packages and answer_contacts. The checker can get the user's
direct and indirect questiontargets from the user and compare them to
the context.

--------------------------------------------------------------------

RULES

    * Update the AppendFAQTarget method use the direct and team answer
      contact methods to get all the question targets. Return True if any
      are the context faq target.
    * Update the AppendQuestion method to use the same principle to make
      answers permission checks faster for large projects.

QA

    * As an answer contact for a package, but not ubuntu. Verify you can
      create an FAQ from a question.
    * Verify you can edit that FAQ.

LINT

    lib/canonical/launchpad/security.py
    lib/lp/answers/doc/faq.txt
    lib/lp/answers/tests/test_faq.py
    lib/lp/answers/tests/test_faqtarget.py

IMPLEMENTATION

Added tests to verify the expect permissions for IFAQTarget and IFAQ. The
format is similar to the Question target tests that verify that all the
implementations have the same behaviour. The test confirmed the permissions
were broken. The change in the security check addressed the issue.
    lib/canonical/launchpad/security.py
    lib/lp/answers/tests/test_faq.py
    lib/lp/answers/tests/test_faqtarget.py

Removed the redundant and incomplete permissions check.
    lib/lp/answers/doc/faq.txt

Update the Question permission rules to reduce the number of queries called
in answers for large projects. test_questiontarget is the specific test that
covers permission, but I ran all lp.answers to ensure that there were no
behavioral changes.
    lib/canonical/launchpad/security.py

[Graham Binns (gmb)]

Hi Curtis,

Two comments, one small, one big:

Small:

> 19 + # object is being examined; the implementers are no synonymous.

s/no/not/

Big:

Also, your tests need comments or docstrings explaining what they're
testing.

review: Needs Fixing (code)

[Curtis Hovey (sinzui)]

On Tue, 2010-12-07 at 11:01 +0000, Graham Binns wrote:
> Review: Needs Fixing code
> Hi Curtis,
>
> Two comments, one small, one big:
>
> Small:
>
> > 19 + # object is being examined; the implementers are no synonymous.
>
> s/no/not/

Fixed.

> Big:
>
> Also, your tests need comments or docstrings explaining what they're
> testing.

I added docstrings to the methods that users might want to reuse. I
added comments to the test methods.

{{{
=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py 2010-12-06 23:13:44 +0000
+++ lib/canonical/launchpad/security.py 2010-12-07 13:45:19 +0000
@@ -1684,7 +1684,7 @@
             return True
         if IQuestionTarget.providedBy(self.obj):
             # Adapt QuestionTargets to FAQTargets to ensure the correct
- # object is being examined; the implementers are no synonymous.
+ # object is being examined; the implementers are not synonymous.
             faq_target = IFAQTarget(self.obj)
             questions_person = IQuestionsPerson(user.person)
             for target in questions_person.getDirectAnswerQuestionTargets():

=== modified file 'lib/lp/answers/tests/test_faq.py'
--- lib/lp/answers/tests/test_faq.py 2010-12-06 22:54:05 +0000
+++ lib/lp/answers/tests/test_faq.py 2010-12-07 14:13:42 +0000
@@ -18,6 +18,7 @@

 class TestFAQPermissions(TestCaseWithFactory):
+ """Test who can edit FAQs."""

     layer = DatabaseFunctionalLayer

@@ -29,29 +30,36 @@
             self.faq = self.factory.makeFAQ(target=target)

     def addAnswerContact(self, answer_contact):
+ """Add the test person to the faq target's answer contacts."""
         language_set = getUtility(ILanguageSet)
         answer_contact.addLanguage(language_set['en'])
         self.faq.target.addAnswerContact(answer_contact)

     def assertCanEdit(self, user, faq):
+ """Assert that the user can edit an FAQ."""
         can_edit = check_permission('launchpad.Edit', faq)
         self.assertTrue(can_edit, 'User cannot edit %s' % faq)

     def assertCannotEdit(self, user, faq):
+ """Assert that the user cannot edit an FAQ."""
         can_edit = check_permission('launchpad.Edit', faq)
         self.assertFalse(can_edit, 'User can edit edit %s' % faq)

     def test_owner_can_edit(self):
+ # The owner of an FAQ target can edit its FAQs.
         login_person(self.owner)
         self.assertCanEdit(self.owner, self.faq)

     def test_direct_answer_contact_can_edit(self):
+ # A direct answer contact for an FAQ target can edit its FAQs.
         direct_answer_contact = self.factory.makePerson()
         login_person(direct_answer_contact)
         self.addAnswerContact(direct_answer_contact)
         self.assertCanEdit(direct_answer_contact, self.faq)

     def test_indirect_answer_contact_can_edit(self):
+ # A indirect answer contact (a member of a team that is an answer
+ # contact) for an FAQ target can edit its FAQs.
         indirect_answer_contact = self.factory.makePerson()
         direct_answer_contact = self.factory.makeTeam()
         with person_logged_in(direct_answer_contact.teamowner):
@@ -...

[Graham Binns (gmb)]

review: Approve (code)

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2010-12-02 00:47:21 +0000
+++ lib/canonical/launchpad/security.py	2010-12-07 14:17:15 +0000
@@ -43,6 +43,7 @@
 from lp.answers.interfaces.faq import IFAQ
 from lp.answers.interfaces.faqtarget import IFAQTarget
 from lp.answers.interfaces.question import IQuestion
+from lp.answers.interfaces.questionsperson import IQuestionsPerson
 from lp.answers.interfaces.questiontarget import IQuestionTarget
 from lp.blueprints.interfaces.specification import (
     ISpecification,
@@ -1653,8 +1654,13 @@
         """Allow user who can administer the question and answer contacts."""
         if AdminQuestion.checkAuthenticated(self, user):
             return True
-        for answer_contact in self.obj.target.answer_contacts:
-            if user.inTeam(answer_contact):
+        question_target = self.obj.target
+        questions_person = IQuestionsPerson(user.person)
+        for target in questions_person.getDirectAnswerQuestionTargets():
+            if target == question_target:
+                return True
+        for target in questions_person.getTeamAnswerQuestionTargets():
+            if target == question_target:
                 return True
         return False
 
@@ -1677,8 +1683,15 @@
         if EditByOwnersOrAdmins.checkAuthenticated(self, user):
             return True
         if IQuestionTarget.providedBy(self.obj):
-            for answer_contact in self.obj.answer_contacts:
-                if user.inTeam(answer_contact):
+            # Adapt QuestionTargets to FAQTargets to ensure the correct
+            # object is being examined; the implementers are not synonymous.
+            faq_target = IFAQTarget(self.obj)
+            questions_person = IQuestionsPerson(user.person)
+            for target in questions_person.getDirectAnswerQuestionTargets():
+                if IFAQTarget(target) == faq_target:
+                    return True
+            for target in questions_person.getTeamAnswerQuestionTargets():
+                if IFAQTarget(target) == faq_target:
                     return True
         return False
 

=== modified file 'lib/lp/answers/doc/faq.txt'
--- lib/lp/answers/doc/faq.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/answers/doc/faq.txt	2010-12-07 14:17:15 +0000
@@ -192,13 +192,7 @@
     >>> check_permission('launchpad.Edit', firefox_faq)
     True
 
-But No Privileges Person doesn't:
-
-    >>> login('[e-mail]')
-    >>> check_permission('launchpad.Edit', firefox_faq)
-    False
-
-But if we make him an answer contact, he will:
+Answer contacts can also edit FAQs:
 
     # An answer contact needs a preferred language.
 

=== added file 'lib/lp/answers/tests/test_faq.py'
--- lib/lp/answers/tests/test_faq.py	1970-01-01 00:00:00 +0000
+++ lib/lp/answers/tests/test_faq.py	2010-12-07 14:17:15 +0000
@@ -0,0 +1,77 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for IFAQ"""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.launchpad.webapp.authorization import check_permission
+from lp.services.worlddata.interfaces.language import ILanguageSet
+from lp.testing import (
+    login_person,
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+
+
+class TestFAQPermissions(TestCaseWithFactory):
+    """Test who can edit FAQs."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestFAQPermissions, self).setUp()
+        target = self.factory.makeProduct()
+        self.owner = target.owner
+        with person_logged_in(self.owner):
+            self.faq = self.factory.makeFAQ(target=target)
+
+    def addAnswerContact(self, answer_contact):
+        """Add the test person to the faq target's answer contacts."""
+        language_set = getUtility(ILanguageSet)
+        answer_contact.addLanguage(language_set['en'])
+        self.faq.target.addAnswerContact(answer_contact)
+
+    def assertCanEdit(self, user, faq):
+        """Assert that the user can edit an FAQ."""
+        can_edit = check_permission('launchpad.Edit', faq)
+        self.assertTrue(can_edit, 'User cannot edit %s' % faq)
+
+    def assertCannotEdit(self, user, faq):
+        """Assert that the user cannot edit an FAQ."""
+        can_edit = check_permission('launchpad.Edit', faq)
+        self.assertFalse(can_edit, 'User can edit edit %s' % faq)
+
+    def test_owner_can_edit(self):
+        # The owner of an FAQ target can edit its FAQs.
+        login_person(self.owner)
+        self.assertCanEdit(self.owner, self.faq)
+
+    def test_direct_answer_contact_can_edit(self):
+        # A direct answer contact for an FAQ target can edit its FAQs.
+        direct_answer_contact = self.factory.makePerson()
+        login_person(direct_answer_contact)
+        self.addAnswerContact(direct_answer_contact)
+        self.assertCanEdit(direct_answer_contact, self.faq)
+
+    def test_indirect_answer_contact_can_edit(self):
+        # A indirect answer contact (a member of a team that is an answer
+        # contact) for an FAQ target can edit its FAQs.
+        indirect_answer_contact = self.factory.makePerson()
+        direct_answer_contact = self.factory.makeTeam()
+        with person_logged_in(direct_answer_contact.teamowner):
+            direct_answer_contact.addMember(
+                indirect_answer_contact, direct_answer_contact.teamowner)
+            self.addAnswerContact(direct_answer_contact)
+        login_person(indirect_answer_contact)
+        self.assertCanEdit(indirect_answer_contact, self.faq)
+
+    def test_nonparticipating_user_cannot_edit(self):
+        # A user that is neither an owner of, or answer contact for, an
+        # FAQ target's cannot edit a its FAQs.
+        nonparticipant = self.factory.makePerson()
+        login_person(nonparticipant)
+        self.assertCannotEdit(nonparticipant, self.faq)

=== added file 'lib/lp/answers/tests/test_faqtarget.py'
--- lib/lp/answers/tests/test_faqtarget.py	1970-01-01 00:00:00 +0000
+++ lib/lp/answers/tests/test_faqtarget.py	2010-12-07 14:17:15 +0000
@@ -0,0 +1,100 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for IFAQTarget"""
+
+__metaclass__ = type
+
+from zope.component import getUtility
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.launchpad.webapp.authorization import check_permission
+from lp.answers.interfaces.faqtarget import IFAQTarget
+from lp.services.worlddata.interfaces.language import ILanguageSet
+from lp.testing import (
+    login_person,
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+
+
+class BaseIFAQTargetTests:
+    """Common tests for all IFAQTargets."""
+
+    layer = DatabaseFunctionalLayer
+
+    def addAnswerContact(self, answer_contact):
+        """Add the test person to the faq target's answer contacts."""
+        language_set = getUtility(ILanguageSet)
+        answer_contact.addLanguage(language_set['en'])
+        self.target.addAnswerContact(answer_contact)
+
+    def assertCanAppend(self, user, target):
+        """Assert that the user can add an FAQ to an FAQ target."""
+        can_append = check_permission('launchpad.Append', IFAQTarget(target))
+        self.assertTrue(can_append, 'User cannot append to %s' % target)
+
+    def assertCannotAppend(self, user, target):
+        """Assert that the user cannot add an FAQ to an FAQ target."""
+        can_append = check_permission('launchpad.Append', IFAQTarget(target))
+        self.assertFalse(can_append, 'User can append append to %s' % target)
+
+    def test_owner_can_append(self):
+        # An owner of an FAQ target can add an FAQ.
+        login_person(self.owner)
+        self.assertCanAppend(self.owner, self.target)
+
+    def test_direct_answer_contact_can_append(self):
+        # An direct answer contact for an FAQ target can add an FAQ.
+        direct_answer_contact = self.factory.makePerson()
+        login_person(direct_answer_contact)
+        self.addAnswerContact(direct_answer_contact)
+        self.assertCanAppend(direct_answer_contact, self.target)
+
+    def test_indirect_answer_contact_can_append(self):
+        # An indirect answer contact (a member of a team that is an answer
+        # contact) for an FAQ target can add an FAQ.
+        indirect_answer_contact = self.factory.makePerson()
+        direct_answer_contact = self.factory.makeTeam()
+        with person_logged_in(direct_answer_contact.teamowner):
+            direct_answer_contact.addMember(
+                indirect_answer_contact, direct_answer_contact.teamowner)
+            self.addAnswerContact(direct_answer_contact)
+        login_person(indirect_answer_contact)
+        self.assertCanAppend(indirect_answer_contact, self.target)
+
+    def test_nonparticipating_user_cannot_append(self):
+        # A user that is neither an owner of, or answer contact for, an
+        # FAQ target cannot add an FAQ.
+        nonparticipant = self.factory.makePerson()
+        login_person(nonparticipant)
+        self.assertCannotAppend(nonparticipant, self.target)
+
+
+class TestDistributionPermissions(BaseIFAQTargetTests, TestCaseWithFactory):
+    """Test who can add FAQs to a distribution."""
+
+    def setUp(self):
+        super(TestDistributionPermissions, self).setUp()
+        self.target = self.factory.makeDistribution()
+        self.owner = self.target.owner
+
+
+class TestProductPermissions(BaseIFAQTargetTests, TestCaseWithFactory):
+    """Test who can add FAQs to a product."""
+
+    def setUp(self):
+        super(TestProductPermissions, self).setUp()
+        self.target = self.factory.makeProduct()
+        self.owner = self.target.owner
+
+
+class TestDSPPermissions(BaseIFAQTargetTests, TestCaseWithFactory):
+    """Test who can add FAQs for a distribution source package."""
+
+    def setUp(self):
+        super(TestDSPPermissions, self).setUp()
+        distribution = self.factory.makeDistribution()
+        self.owner = distribution.owner
+        self.target = self.factory.makeDistributionSourcePackage(
+            sourcepackagename='fnord', distribution=distribution)