launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/question-content-lockdown into lp:launchpad.

Commit message:
Lock down Question.title/description edits from random users.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #568276 in Launchpad itself: "Questions can be modified by anyone, but it is not obvious that it has been modified, or who modified it."
  https://bugs.launchpad.net/launchpad/+bug/568276

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/question-content-lockdown/+merge/279069

Lock down Question.title/description edits from random users.

Question content doesn't usually evolve through the life cycle as bug content does, and there is no audit trail, so letting any user edit the title and description is mostly useless and very abusable. Those fields are now behind launchpad.Edit, which is a union of launchpad.Owner (creator) and launchpad.Append (target owner, ~admins, and now ~registry).

-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/question-content-lockdown into lp:launchpad.

=== modified file 'lib/lp/answers/browser/tests/views.txt'
--- lib/lp/answers/browser/tests/views.txt	2015-09-28 11:13:42 +0000
+++ lib/lp/answers/browser/tests/views.txt	2015-12-01 06:43:00 +0000
@@ -447,12 +447,6 @@
 
     >>> view = getMultiAdapter((question_three, request), name='+edit')
     >>> view.initialize()
-    >>> question_three.title
-    u'Better Title'
-
-    >>> question_three.description
-    u'A better description.'
-
     >>> print question_three.distribution.name
     ubuntu
 
@@ -462,9 +456,16 @@
     >>> print question_three.product
     None
 
-Since a user must have launchpad.Moderator privilege to change the
-assignee and launchpad.Admin privilege to change status whiteboard, the
-values are unchanged.
+Since a user must have launchpad.Edit (question creator or target answer
+contact) to change the title or description, launchpad.Append (target
+answer contact) to change the assignee and launchpad.Admin (target
+owner) to change status whiteboard, the values are unchanged.
+
+    >>> question_three.title
+    u'Firefox is slow and consumes too much RAM'
+
+    >>> question_three.description
+    u"I'm running on a 486 with 32 MB ram. And Firefox is slow! What should I do?"
 
     >>> question_three.assignee is None
     True
@@ -489,6 +490,12 @@
     >>> request.method = 'POST'
     >>> view = getMultiAdapter((question_three, request), name='+edit')
     >>> view.initialize()
+    >>> question_three.title
+    u'Better Title'
+
+    >>> question_three.description
+    u'A better description.'
+
     >>> print question_three.assignee.displayname
     Foo Bar
 

=== modified file 'lib/lp/answers/configure.zcml'
--- lib/lp/answers/configure.zcml	2015-09-25 02:33:15 +0000
+++ lib/lp/answers/configure.zcml	2015-12-01 06:43:00 +0000
@@ -116,7 +116,7 @@
       attributes="requestInfo giveAnswer expireQuestion addComment
                   addCommentWithoutNotify linkFAQ subscribe unsubscribe
                   setCommentVisibility"
-      set_attributes="title description datedue target language"
+      set_attributes="datedue target language"
       />
     <require
       permission="launchpad.Append"
@@ -124,6 +124,10 @@
       set_attributes="assignee"
       />
     <require
+      permission="launchpad.Edit"
+      set_attributes="title description"
+      />
+    <require
       permission="launchpad.Admin"
       attributes="setStatus"
       set_attributes="priority whiteboard"

=== modified file 'lib/lp/answers/doc/notifications.txt'
--- lib/lp/answers/doc/notifications.txt	2015-09-25 03:02:28 +0000
+++ lib/lp/answers/doc/notifications.txt	2015-12-01 06:43:00 +0000
@@ -88,8 +88,7 @@
     >>> from lazr.lifecycle.event import ObjectModifiedEvent
     >>> from lazr.lifecycle.snapshot import Snapshot
 
-    >>> login('no-priv@xxxxxxxxxxxxx')
-    >>> no_priv = getUtility(ILaunchBag).user
+    >>> login('test@xxxxxxxxxxxxx')
     >>> unmodified_question = Snapshot(
     ...     ubuntu_question, providing=providedBy(ubuntu_question))
     >>> ubuntu_question.title = "Installer doesn't work on a Mac"
@@ -147,6 +146,7 @@
 Changing the assignee will trigger a notification.
 
     >>> login('foo.bar@xxxxxxxxxxxxx')
+    >>> no_priv = getUtility(IPersonSet).getByName('no-priv')
     >>> unmodified_question = Snapshot(
     ...     ubuntu_question, providing=providedBy(ubuntu_question))
     >>> ubuntu_question.assignee = no_priv

=== modified file 'lib/lp/answers/stories/question-edit.txt'
--- lib/lp/answers/stories/question-edit.txt	2012-12-11 02:19:13 +0000
+++ lib/lp/answers/stories/question-edit.txt	2015-12-01 06:43:00 +0000
@@ -1,7 +1,9 @@
 = Editing Questions =
 
-To edit the title and description of question, one uses the 'Edit Question'
-menu item. You need to be logged in to perform that action:
+To edit the title and description of question, one uses the 'Edit
+Question' menu item. You need to be logged in to see the edit form, and
+only the question creator or an answer contact can change the title and
+description.
 
     >>> anon_browser.open('http://launchpad.dev/firefox/+question/2')
     >>> anon_browser.getLink('Edit question').click()
@@ -9,14 +11,15 @@
     ...
     Unauthorized...
 
-    >>> user_browser.open('http://launchpad.dev/firefox/+question/2')
-    >>> user_browser.getLink('Edit question').click()
-    >>> print user_browser.url
+    >>> test_browser = setupBrowser(auth='Basic test@xxxxxxxxxxxxx:test')
+    >>> test_browser.open('http://launchpad.dev/firefox/+question/2')
+    >>> test_browser.getLink('Edit question').click()
+    >>> print test_browser.url
     http://answers.launchpad.dev/firefox/+question/2/+edit
 
 There is a cancel link should the user decide otherwise:
 
-    >>> print user_browser.getLink('Cancel').url
+    >>> print test_browser.getLink('Cancel').url
     http://answers.launchpad.dev/firefox/+question/2
 
 When we post the form, we should be redirected back to the question page.
@@ -24,17 +27,17 @@
     >>> description = (
     ...   "Hi! I'm trying to learn about SVG but I can't get it to work at "
     ...   "all in firefox. Maybe there is a plugin? Help! Thanks. Mark")
-    >>> user_browser.getControl('Description').value = description
+    >>> test_browser.getControl('Description').value = description
     >>> summary = "Problem showing the SVG demo on W3C web site"
-    >>> user_browser.getControl('Summary').value = summary
-    >>> user_browser.getControl('Save Changes').click()
+    >>> test_browser.getControl('Summary').value = summary
+    >>> test_browser.getControl('Save Changes').click()
 
-    >>> print user_browser.url
+    >>> print test_browser.url
     http://answers.launchpad.dev/firefox/+question/2
 
 And viewing that page should show the updated information.
 
-    >>> soup = find_main_content(user_browser.contents)
+    >>> soup = find_main_content(test_browser.contents)
     >>> print soup.first('div', 'report').renderContents().strip()
     <p>Hi! I&#x27;m trying to learn about SVG but I can&#x27;t get it to
     work at all in firefox. Maybe there is a plugin? Help! Thanks.
@@ -49,11 +52,11 @@
     ...     print extract_text(
     ...         find_tag_by_id(browser.contents, 'question-status'))
 
-    >>> user_browser.open('http://launchpad.dev/ubuntu/+question/3')
-    >>> print_question_status(user_browser)
+    >>> test_browser.open('http://launchpad.dev/ubuntu/+question/3')
+    >>> print_question_status(test_browser)
     Status: Invalid
 
-    >>> user_browser.getLink('Edit question')
+    >>> test_browser.getLink('Edit question')
     <Link...>
 
 

=== 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):
+        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))
 
 
 class AppendQuestion(AdminQuestion):
@@ -2012,6 +2012,16 @@
         return user.inTeam(self.obj.owner)
 
 
+class EditQuestion(AuthorizationBase):
+    permission = 'launchpad.Edit'
+    usedfor = IQuestion
+
+    def checkAuthenticated(self, user):
+        return (
+            AppendQuestion(self.obj).checkAuthenticated(user)
+            or QuestionOwner(self.obj).checkAuthenticated(user))
+
+
 class ViewQuestion(AnonymousAuthorization):
     usedfor = IQuestion