launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #23462
[Merge] lp:~cjwatson/launchpad/stricter-bug-edit-permissions into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/stricter-bug-edit-permissions into lp:launchpad.
Commit message:
Add configurable heuristic karma and account age thresholds for users who are allowed to edit bugs.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/stricter-bug-edit-permissions/+merge/365131
I expect this to need further refinement, but it's a start.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/stricter-bug-edit-permissions into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2019-03-11 14:51:16 +0000
+++ database/schema/security.cfg 2019-03-26 20:55:23 +0000
@@ -1815,6 +1815,7 @@
public.job = SELECT, INSERT, UPDATE
public.karma = SELECT, INSERT
public.karmaaction = SELECT
+public.karmatotalcache = SELECT
public.language = SELECT
public.libraryfilealias = SELECT, INSERT, UPDATE
public.libraryfilecontent = SELECT, INSERT
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2019-01-10 11:41:30 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2019-03-26 20:55:23 +0000
@@ -126,7 +126,7 @@
0, 10, login_method=lambda: login(ADMIN_EMAIL))
# This may seem large: it is; there is easily another 25% fat in
# there.
- self.assertThat(recorder1, HasQueryCount(LessThan(84)))
+ self.assertThat(recorder1, HasQueryCount(LessThan(85)))
self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
def test_rendered_query_counts_constant_with_attachments(self):
@@ -137,7 +137,7 @@
lambda: self.getUserBrowser(url, person),
lambda: self.factory.makeBugAttachment(bug=task.bug),
1, 9, login_method=lambda: login(ADMIN_EMAIL))
- self.assertThat(recorder1, HasQueryCount(LessThan(85)))
+ self.assertThat(recorder1, HasQueryCount(LessThan(86)))
self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
def makeLinkedBranchMergeProposal(self, sourcepackage, bug, owner):
@@ -172,7 +172,7 @@
recorder1, recorder2 = record_two_runs(
lambda: self.getUserBrowser(url, owner),
make_merge_proposals, 0, 1)
- self.assertThat(recorder1, HasQueryCount(LessThan(92)))
+ self.assertThat(recorder1, HasQueryCount(LessThan(93)))
# Ideally this should be much fewer, but this tries to keep a win of
# removing more than half of these.
self.assertThat(
@@ -218,7 +218,7 @@
lambda: self.getUserBrowser(url, person),
lambda: add_activity("description", self.factory.makePerson()),
1, 20, login_method=lambda: login(ADMIN_EMAIL))
- self.assertThat(recorder1, HasQueryCount(LessThan(85)))
+ self.assertThat(recorder1, HasQueryCount(LessThan(86)))
self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
def test_rendered_query_counts_constant_with_milestones(self):
@@ -228,7 +228,7 @@
with celebrity_logged_in('admin'):
browses_under_limit = BrowsesWithQueryLimit(
- 85, self.factory.makePerson())
+ 86, self.factory.makePerson())
self.assertThat(bug, browses_under_limit)
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2016-07-29 16:03:33 +0000
+++ lib/lp/bugs/configure.zcml 2019-03-26 20:55:23 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2016 Canonical Ltd. This software is licensed under the
+<!-- Copyright 2009-2019 Canonical Ltd. This software is licensed under the
GNU Affero General Public License version 3 (see the file LICENSE).
-->
@@ -725,11 +725,13 @@
linked_merge_proposals
getVisibleLinkedMergeProposals"/>
<require
- permission="launchpad.Edit"
- interface="lp.bugs.interfaces.bug.IBugEdit"
+ permission="launchpad.Append"
+ interface="lp.bugs.interfaces.bug.IBugAppend"
attributes="
linkBranch
- unlinkBranch"
+ unlinkBranch"/>
+ <require
+ permission="launchpad.Edit"
set_attributes="
name
tags
=== modified file 'lib/lp/bugs/doc/bug.txt'
--- lib/lp/bugs/doc/bug.txt 2018-12-21 21:47:08 +0000
+++ lib/lp/bugs/doc/bug.txt 2019-03-26 20:55:23 +0000
@@ -181,7 +181,7 @@
>>> firefox_crashes.setPrivate(True, current_user())
Traceback (most recent call last):
...
- Unauthorized: (..., 'setPrivate', 'launchpad.Edit')
+ Unauthorized: (..., 'setPrivate', 'launchpad.Append')
We have to be logged in, so let's do that:
=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py 2016-11-12 18:24:37 +0000
+++ lib/lp/bugs/interfaces/bug.py 2019-03-26 20:55:23 +0000
@@ -10,9 +10,9 @@
'CreatedBugWithNoBugTasksError',
'IBug',
'IBugAddForm',
+ 'IBugAppend',
'IBugBecameQuestionEvent',
'IBugDelta',
- 'IBugEdit',
'IBugMute',
'IBugPublic',
'IBugSet',
@@ -684,8 +684,8 @@
"""
-class IBugEdit(Interface):
- """IBug attributes that require launchpad.Edit permission."""
+class IBugAppend(Interface):
+ """IBug attributes that require launchpad.Append permission."""
@call_with(owner=REQUEST_USER, from_api=True)
@operation_parameters(
@@ -1009,7 +1009,7 @@
"""
-class IBug(IBugPublic, IBugView, IBugEdit, IHasLinkedBranches):
+class IBug(IBugPublic, IBugView, IBugAppend, IHasLinkedBranches):
"""The core bug entry."""
export_as_webservice_entry()
=== modified file 'lib/lp/bugs/security.py'
--- lib/lp/bugs/security.py 2016-04-12 10:50:30 +0000
+++ lib/lp/bugs/security.py 2019-03-26 20:55:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Security adapters for the bugs module."""
@@ -23,7 +23,10 @@
from lp.bugs.interfaces.bugwatch import IBugWatch
from lp.bugs.interfaces.hasbug import IHasBug
from lp.bugs.interfaces.structuralsubscription import IStructuralSubscription
-from lp.registry.interfaces.role import IHasOwner
+from lp.registry.interfaces.role import (
+ IHasAppointedDriver,
+ IHasOwner,
+ )
from lp.services.messages.interfaces.message import IMessage
@@ -99,19 +102,82 @@
return not self.obj.bug.private
-class EditPublicByLoggedInUserAndPrivateByExplicitSubscribers(
- AuthorizationBase):
- permission = 'launchpad.Edit'
+class AppendBug(AuthorizationBase):
+ """Security adapter for appending to bugs.
+
+ This is used for operations that anyone who can see the bug can perform.
+ """
+ permission = 'launchpad.Append'
usedfor = IBug
def checkAuthenticated(self, user):
- """Allow any logged in user to edit a public bug, and only
- explicit subscribers to edit private bugs. Any bug that can be
- seen can be edited.
+ """Allow any logged in user to append to a public bug, and only
+ explicit subscribers to append to private bugs. Any bug that can be
+ seen can be appended to.
"""
return self.obj.userCanView(user)
def checkUnauthenticated(self):
+ """Never allow unauthenticated users to append to a bug."""
+ return False
+
+
+class EditBug(AuthorizationBase):
+ """Security adapter for editing bugs.
+
+ This is used for operations that are potentially destructive in some
+ way. They aren't heavily locked down, but only users who appear to be
+ legitimate can perform them.
+ """
+ permission = 'launchpad.Edit'
+ usedfor = IBug
+
+ def _hasAnyRole(self, user, targets):
+ """Return True if the user has any role on any of these bug targets."""
+ # XXX cjwatson 2019-03-26: This is inefficient for bugs with many
+ # targets. However, we only get here if we can't easily establish
+ # that the user seems legitimate, so it shouldn't be a big problem
+ # in practice. We can optimise this further if it turns out to
+ # matter.
+ for target in targets:
+ roles = []
+ if IHasOwner.providedBy(target):
+ roles.append('owner')
+ if IHasAppointedDriver.providedBy(target):
+ roles.append('driver')
+ if IHasBugSupervisor.providedBy(target):
+ roles.append('bug_supervisor')
+ if user.isOneOf(target, roles):
+ return True
+ return False
+
+ def checkAuthenticated(self, user):
+ """Allow sufficiently-trusted users to edit bugs.
+
+ Only users who can append to the bug can edit it; in addition, only
+ users who seem to be generally legitimate or who have a relevant
+ role on one of the targets of the bug can edit the bug.
+ """
+ if not self.forwardCheckAuthenticated(
+ user, permission='launchpad.Append'):
+ # The user cannot even see the bug.
+ return False
+ return (
+ # If the bug is private, then we don't need more elaborate
+ # checks as they must have been explicitly subscribed.
+ self.obj.private or
+ # If the user seems generally legitimate, let them through.
+ self.forwardCheckAuthenticated(
+ user, permission='launchpad.AnyLegitimatePerson') or
+ # The bug reporter can always edit their own bug.
+ user.inTeam(self.obj.owner) or
+ # Users with relevant roles can edit the bug.
+ user.in_admin or user.in_commercial_admin or
+ user.in_registry_experts or
+ self._hasAnyRole(
+ user, [task.target for task in self.obj.bugtasks]))
+
+ def checkUnauthenticated(self):
"""Never allow unauthenticated users to edit a bug."""
return False
=== modified file 'lib/lp/bugs/tests/test_bug.py'
--- lib/lp/bugs/tests/test_bug.py 2015-09-25 10:23:48 +0000
+++ lib/lp/bugs/tests/test_bug.py 2019-03-26 20:55:23 +0000
@@ -1,13 +1,16 @@
-# Copyright 2009-2014 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for lp.bugs.model.Bug."""
__metaclass__ = type
+from datetime import timedelta
+
from lazr.lifecycle.snapshot import Snapshot
from zope.component import getUtility
from zope.interface import providedBy
+from zope.security.management import checkPermission
from zope.security.proxy import removeSecurityProxy
from lp.bugs.enums import BugNotificationLevel
@@ -23,8 +26,10 @@
UserCannotEditBugTaskImportance,
UserCannotEditBugTaskMilestone,
)
+from lp.registry.tests.test_person import KarmaTestMixin
from lp.testing import (
admin_logged_in,
+ celebrity_logged_in,
person_logged_in,
StormStatementRecorder,
TestCaseWithFactory,
@@ -327,3 +332,83 @@
target = self.factory.makeProduct()
bug = self.createBug(owner=person, target=target)
self.assertContentEqual([person], bug.getDirectSubscribers())
+
+
+class TestBugPermissions(TestCaseWithFactory, KarmaTestMixin):
+ """Test bug permissions."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestBugPermissions, self).setUp()
+ self.pushConfig(
+ 'launchpad', min_legitimate_karma=5, min_legitimate_account_age=5)
+ self.bug = self.factory.makeBug()
+
+ def test_unauthenticated_user_cannot_edit(self):
+ self.assertFalse(checkPermission('launchpad.Edit', self.bug))
+
+ def test_new_user_cannot_edit(self):
+ with person_logged_in(self.factory.makePerson()):
+ self.assertFalse(checkPermission('launchpad.Edit', self.bug))
+
+ def test_private_bug_subscriber_can_edit(self):
+ person = self.factory.makePerson()
+ with admin_logged_in() as admin:
+ self.bug.setPrivate(True, admin)
+ self.bug.subscribe(person, admin)
+ with person_logged_in(person):
+ self.assertTrue(checkPermission('launchpad.Edit', self.bug))
+
+ def test_private_bug_non_subscriber_cannot_edit(self):
+ with admin_logged_in() as admin:
+ self.bug.setPrivate(True, admin)
+ with person_logged_in(self.factory.makePerson()):
+ self.assertFalse(checkPermission('launchpad.Edit', self.bug))
+
+ def test_user_with_karma_can_edit(self):
+ person = self.factory.makePerson()
+ self._makeKarmaTotalCache(person, 10)
+ with person_logged_in(person):
+ self.assertTrue(checkPermission('launchpad.Edit', self.bug))
+
+ def test_user_with_account_age_can_edit(self):
+ person = self.factory.makePerson()
+ naked_account = removeSecurityProxy(person.account)
+ naked_account.date_created = (
+ naked_account.date_created - timedelta(days=10))
+ with person_logged_in(person):
+ self.assertTrue(checkPermission('launchpad.Edit', self.bug))
+
+ def test_bug_reporter_can_edit(self):
+ with person_logged_in(self.bug.owner):
+ self.assertTrue(checkPermission('launchpad.Edit', self.bug))
+
+ def test_admin_can_edit(self):
+ with admin_logged_in():
+ self.assertTrue(checkPermission('launchpad.Edit', self.bug))
+
+ def test_commercial_admin_can_edit(self):
+ with celebrity_logged_in('commercial_admin'):
+ self.assertTrue(checkPermission('launchpad.Edit', self.bug))
+
+ def test_registry_expert_can_edit(self):
+ with celebrity_logged_in('registry_experts'):
+ self.assertTrue(checkPermission('launchpad.Edit', self.bug))
+
+ def test_target_owner_can_edit(self):
+ with person_logged_in(self.bug.default_bugtask.target.owner):
+ self.assertTrue(checkPermission('launchpad.Edit', self.bug))
+
+ def test_target_driver_can_edit(self):
+ person = self.factory.makePerson()
+ removeSecurityProxy(self.bug.default_bugtask.target).driver = person
+ with person_logged_in(person):
+ self.assertTrue(checkPermission('launchpad.Edit', self.bug))
+
+ def test_target_bug_supervisor_can_edit(self):
+ person = self.factory.makePerson()
+ removeSecurityProxy(self.bug.default_bugtask.target).bug_supervisor = (
+ person)
+ with person_logged_in(person):
+ self.assertTrue(checkPermission('launchpad.Edit', self.bug))
=== modified file 'lib/lp/bugs/tests/test_doc.py'
--- lib/lp/bugs/tests/test_doc.py 2018-06-30 16:09:44 +0000
+++ lib/lp/bugs/tests/test_doc.py 2019-03-26 20:55:23 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2019 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""
@@ -479,6 +479,7 @@
stdout_logging=False),
'bugs-emailinterface.txt-processmail': LayeredDocFileSuite(
'../tests/bugs-emailinterface.txt',
+ id_extensions=['bugs-emailinterface.txt-processmail'],
setUp=lambda test: setUp(test, future=True), tearDown=tearDown,
layer=ProcessMailLayer,
stdout_logging=False),
=== modified file 'lib/lp/permissions.zcml'
--- lib/lp/permissions.zcml 2013-05-09 06:04:22 +0000
+++ lib/lp/permissions.zcml 2019-03-26 20:55:23 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2009-2013 Canonical Ltd. This software is licensed under the
+<!-- Copyright 2009-2019 Canonical Ltd. This software is licensed under the
GNU Affero General Public License version 3 (see the file LICENSE).
-->
@@ -27,6 +27,11 @@
access_level="write" />
<permission
+ id="launchpad.AnyLegitimatePerson"
+ title="Any person who is Allowed and who also seems generally legitimate."
+ access_level="write" />
+
+ <permission
id="launchpad.Edit" title="Editing something" access_level="write" />
<!-- Request large downloads, or other heavyweight jobs that are not
=== modified file 'lib/lp/security.py'
--- lib/lp/security.py 2019-03-08 16:53:33 +0000
+++ lib/lp/security.py 2019-03-26 20:55:23 +0000
@@ -9,8 +9,13 @@
'ModerateByRegistryExpertsOrAdmins',
]
+from datetime import (
+ datetime,
+ timedelta,
+ )
from operator import methodcaller
+import pytz
from storm.expr import (
And,
Select,
@@ -177,6 +182,7 @@
)
from lp.registry.interfaces.wikiname import IWikiName
from lp.registry.model.person import Person
+from lp.services.config import config
from lp.services.database.interfaces import IStore
from lp.services.identity.interfaces.account import IAccount
from lp.services.identity.interfaces.emailaddress import IEmailAddress
@@ -301,6 +307,33 @@
return self.forwardCheckAuthenticated(user, self.obj, 'launchpad.View')
+class AnyLegitimatePerson(AuthorizationBase):
+ """The default ruleset for the launchpad.AnyLegitimatePerson permission.
+
+ Some operations are open to Launchpad users in general, but we still don't
+ want drive-by vandalism.
+ """
+ permission = 'launchpad.AnyLegitimatePerson'
+ usedfor = Interface
+
+ def checkUnauthenticated(self):
+ return False
+
+ def _hasEnoughKarma(self, user):
+ return user.person.karma >= config.launchpad.min_legitimate_karma
+
+ def _isOldEnough(self, user):
+ return (
+ datetime.now(pytz.UTC) - user.person.account.date_created >=
+ timedelta(days=config.launchpad.min_legitimate_account_age))
+
+ def checkAuthenticated(self, user):
+ if not self._hasEnoughKarma(user) and not self._isOldEnough(user):
+ return False
+ return self.forwardCheckAuthenticated(
+ user, self.obj, 'launchpad.AnyAllowedPerson')
+
+
class LimitedViewDeferredToView(AuthorizationBase):
"""The default ruleset for the launchpad.LimitedView permission.
=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf 2019-03-18 12:22:55 +0000
+++ lib/lp/services/config/schema-lazr.conf 2019-03-26 20:55:23 +0000
@@ -1108,6 +1108,12 @@
# specific in many contexts, but this provides a fallback.
urlfetch_timeout: 30
+# Minimum karma value that indicates a legitimate user.
+min_legitimate_karma: 0
+
+# Minimum account age in days that indicates a legitimate user.
+min_legitimate_account_age: 0
+
[launchpad_session]
# The database connection string.
# datatype: pgconnection
Follow ups
