launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02114
[Merge] lp:~sinzui/launchpad/faq-mailing-list-permissions-0 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/faq-mailing-list-permissions-0 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#682135 Answer contacts for packages cannot create/edit FAQs
https://bugs.launchpad.net/bugs/682135
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
--
https://code.launchpad.net/~sinzui/launchpad/faq-mailing-list-permissions-0/+merge/42888
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/faq-mailing-list-permissions-0 into lp:launchpad.
=== 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-06 23:38:12 +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,
@@ -1677,8 +1678,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 no 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-06 23:38:12 +0000
@@ -192,13 +192,7 @@
>>> check_permission('launchpad.Edit', firefox_faq)
True
-But No Privileges Person doesn't:
-
- >>> login('no-priv@xxxxxxxxxxxxx')
- >>> 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-06 23:38:12 +0000
@@ -0,0 +1,67 @@
+# 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):
+
+ 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):
+ language_set = getUtility(ILanguageSet)
+ answer_contact.addLanguage(language_set['en'])
+ self.faq.target.addAnswerContact(answer_contact)
+
+ def assertCanEdit(self, user, faq):
+ can_edit = check_permission('launchpad.Edit', faq)
+ self.assertTrue(can_edit, 'User cannot edit %s' % faq)
+
+ def assertCannotEdit(self, user, faq):
+ can_edit = check_permission('launchpad.Edit', faq)
+ self.assertFalse(can_edit, 'User can edit edit %s' % faq)
+
+ def test_owner_can_edit(self):
+ login_person(self.owner)
+ self.assertCanEdit(self.owner, self.faq)
+
+ def test_direct_answer_contact_can_edit(self):
+ 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):
+ 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):
+ 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-06 23:38:12 +0000
@@ -0,0 +1,88 @@
+# 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):
+ language_set = getUtility(ILanguageSet)
+ answer_contact.addLanguage(language_set['en'])
+ self.target.addAnswerContact(answer_contact)
+
+ def assertCanAppend(self, user, target):
+ can_append = check_permission('launchpad.Append', IFAQTarget(target))
+ self.assertTrue(can_append, 'User cannot append to %s' % target)
+
+ def assertCannotAppend(self, user, 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):
+ login_person(self.owner)
+ self.assertCanAppend(self.owner, self.target)
+
+ def test_direct_answer_contact_can_append(self):
+ 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):
+ 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):
+ nonparticipant = self.factory.makePerson()
+ login_person(nonparticipant)
+ self.assertCannotAppend(nonparticipant, self.target)
+
+
+class TestDistributionPermissions(BaseIFAQTargetTests, TestCaseWithFactory):
+
+ def setUp(self):
+ super(TestDistributionPermissions, self).setUp()
+ self.target = self.factory.makeDistribution()
+ self.owner = self.target.owner
+
+
+class TestProductPermissions(BaseIFAQTargetTests, TestCaseWithFactory):
+
+ def setUp(self):
+ super(TestProductPermissions, self).setUp()
+ self.target = self.factory.makeProduct()
+ self.owner = self.target.owner
+
+
+class TestDSPPermissions(BaseIFAQTargetTests, TestCaseWithFactory):
+
+ def setUp(self):
+ super(TestDSPPermissions, self).setUp()
+ distribution = self.factory.makeDistribution()
+ self.owner = distribution.owner
+ self.target = self.factory.makeDistributionSourcePackage(
+ sourcepackagename='fnord', distribution=distribution)
