launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28496
[Merge] ~lgp171188/launchpad:new-security-role-permission into launchpad:master
Guruprasad has proposed merging ~lgp171188/launchpad:new-security-role-permission into launchpad:master.
Commit message:
Add a new security admin role and restrict vulnerability permissions to it
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/423363
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:new-security-role-permission into launchpad:master.
diff --git a/lib/lp/bugs/configure.zcml b/lib/lp/bugs/configure.zcml
index 895b8e9..4960677 100644
--- a/lib/lp/bugs/configure.zcml
+++ b/lib/lp/bugs/configure.zcml
@@ -604,7 +604,7 @@
interface="lp.bugs.interfaces.vulnerability.IVulnerabilityView
lp.bugs.interfaces.vulnerability.IVulnerabilityEditableAttributes" />
<require
- permission="launchpad.Edit"
+ permission="launchpad.SecurityAdmin"
interface="lp.bugs.interfaces.vulnerability.IVulnerabilityEdit"
set_schema="lp.bugs.interfaces.vulnerability.IVulnerabilityEditableAttributes" />
diff --git a/lib/lp/bugs/model/tests/test_vulnerability.py b/lib/lp/bugs/model/tests/test_vulnerability.py
index f2cac72..ea0c036 100644
--- a/lib/lp/bugs/model/tests/test_vulnerability.py
+++ b/lib/lp/bugs/model/tests/test_vulnerability.py
@@ -2,8 +2,10 @@
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for the vulnerability and related models."""
+from testtools.matchers import MatchesStructure
from zope.component import getUtility
+from lp.bugs.enums import VulnerabilityStatus
from lp.bugs.interfaces.buglink import IBugLinkTarget
from lp.bugs.interfaces.vulnerability import (
IVulnerability,
@@ -39,33 +41,87 @@ class TestVulnerability(TestCaseWithFactory):
vulnerability = self.factory.makeVulnerability()
self.assertTrue(verifyObject(IBugLinkTarget, vulnerability))
- def test_random_user(self):
+ def test_random_user_permissions(self):
with person_logged_in(self.factory.makePerson()):
self.assertTrue(
check_permission("launchpad.View", self.vulnerability))
self.assertFalse(
check_permission("launchpad.Edit", self.vulnerability))
+ self.assertFalse(
+ check_permission(
+ "launchpad.SecurityAdmin", self.vulnerability
+ )
+ )
- def test_admin(self):
+ def test_admin_permissions(self):
with admin_logged_in():
self.assertTrue(
check_permission("launchpad.View", self.vulnerability))
self.assertTrue(
check_permission("launchpad.Edit", self.vulnerability))
+ self.assertTrue(
+ check_permission(
+ "launchpad.SecurityAdmin", self.vulnerability
+ )
+ )
- def test_non_admin(self):
+ def test_distribution_owner_permissions(self):
with person_logged_in(self.distribution.owner):
self.assertTrue(
check_permission("launchpad.View", self.vulnerability))
self.assertTrue(
check_permission("launchpad.Edit", self.vulnerability))
+ self.assertTrue(
+ check_permission(
+ "launchpad.SecurityAdmin", self.vulnerability
+ )
+ )
+
+ def test_security_admin_permissions(self):
+ person = self.factory.makePerson()
+ security_team = self.factory.makeTeam(members=[person])
+ with person_logged_in(self.distribution.owner):
+ self.distribution.security_admin = security_team
- def test_anonymous(self):
+ with person_logged_in(person):
+ self.assertTrue(
+ check_permission(
+ "launchpad.SecurityAdmin", self.vulnerability
+ )
+ )
+
+ def test_anonymous_permissions(self):
with anonymous_logged_in():
self.assertFalse(
check_permission("launchpad.View", self.vulnerability))
self.assertFalse(
check_permission("launchpad.Edit", self.vulnerability))
+ self.assertFalse(
+ check_permission(
+ "launchpad.SecurityAdmin", self.vulnerability
+ )
+ )
+
+ def test_edit_vulnerability_security_admin(self):
+ person = self.factory.makePerson()
+ security_team = self.factory.makeTeam(members=[person])
+ vulnerability = self.factory.makeVulnerability(
+ distribution=self.distribution
+ )
+ cve = self.factory.makeCVE('2022-1234')
+ with person_logged_in(self.distribution.owner):
+ self.distribution.security_admin = security_team
+
+ with person_logged_in(security_team):
+ vulnerability.cve = cve
+ vulnerability.status = VulnerabilityStatus.ACTIVE
+ self.assertThat(
+ vulnerability,
+ MatchesStructure.byEquality(
+ cve=cve,
+ status=VulnerabilityStatus.ACTIVE
+ )
+ )
class TestVulnerabilityActivity(TestCaseWithFactory):
diff --git a/lib/lp/bugs/security.py b/lib/lp/bugs/security.py
index 5e40c90..8ea8185 100644
--- a/lib/lp/bugs/security.py
+++ b/lib/lp/bugs/security.py
@@ -23,10 +23,12 @@ from lp.bugs.interfaces.bugtracker import IBugTracker
from lp.bugs.interfaces.bugwatch import IBugWatch
from lp.bugs.interfaces.hasbug import IHasBug
from lp.bugs.interfaces.structuralsubscription import IStructuralSubscription
+from lp.bugs.interfaces.vulnerability import IVulnerability
from lp.registry.interfaces.role import (
IHasAppointedDriver,
IHasOwner,
)
+from lp.security import is_commercial_case
from lp.services.messages.interfaces.message import IMessage
@@ -384,3 +386,18 @@ class EditBugSubscriptionFilter(AuthorizationBase):
def checkAuthenticated(self, user):
return user.inTeam(self.obj.structural_subscription.subscriber)
+
+
+class EditVulnerabilities(AuthorizationBase):
+ """The security admins of a distribution should be able to edit
+ vulnerabilities in that distribution."""
+ permission = 'launchpad.SecurityAdmin'
+ usedfor = IVulnerability
+
+ def checkAuthenticated(self, user):
+ return (
+ user.isOwner(self.obj.distribution)
+ or is_commercial_case(self.obj.distribution, user)
+ or user.in_admin
+ or user.inTeam(self.obj.distribution.security_admin)
+ )
diff --git a/lib/lp/bugs/tests/test_vulnerability.py b/lib/lp/bugs/tests/test_vulnerability.py
index be3457a..add4320 100644
--- a/lib/lp/bugs/tests/test_vulnerability.py
+++ b/lib/lp/bugs/tests/test_vulnerability.py
@@ -16,6 +16,7 @@ from lp.bugs.interfaces.bugtask import BugTaskImportance
from lp.services.webapp.interfaces import OAuthPermission
from lp.testing import (
api_url,
+ person_logged_in,
TestCaseWithFactory,
)
from lp.testing.layers import DatabaseFunctionalLayer
@@ -195,6 +196,68 @@ class TestVulnerabilityWebService(TestCaseWithFactory):
self.assertEqual(209, response.status)
self.assertEqual(vulnerability.cve, another_cve)
+ def test_security_admin_can_edit_vulnerability(self):
+ vulnerability = removeSecurityProxy(
+ self.factory.makeVulnerability()
+ )
+ owner = vulnerability.distribution.owner
+ person = self.factory.makePerson()
+ security_team = self.factory.makeTeam(members=[person])
+
+ with person_logged_in(owner):
+ vulnerability.distribution.security_admin = security_team
+
+ self.assertThat(
+ vulnerability,
+ MatchesStructure.byEquality(
+ status=VulnerabilityStatus.NEEDS_TRIAGE,
+ importance=BugTaskImportance.UNDECIDED,
+ information_type=InformationType.PUBLIC,
+ cve=None,
+ description=None,
+ notes=None,
+ mitigation=None,
+ date_made_public=None,
+ )
+ )
+ vulnerability_url = api_url(vulnerability)
+ webservice = webservice_for_person(
+ person,
+ permission=OAuthPermission.WRITE_PRIVATE,
+ default_api_version="devel"
+ )
+ now = datetime.datetime.now(pytz.UTC)
+
+ response = webservice.patch(
+ vulnerability_url,
+ "application/json",
+ json.dumps(
+ {
+ "status": "Active",
+ "information_type": "Private",
+ "importance": "Critical",
+ "description": "Foo bar",
+ "notes": "lgp171188> Foo bar",
+ "mitigation": "Foo bar baz",
+ "importance_explanation": "Foo bar bazz",
+ "date_made_public": now.isoformat(),
+ }
+ )
+ )
+ self.assertEqual(209, response.status)
+ self.assertThat(
+ vulnerability,
+ MatchesStructure.byEquality(
+ status=VulnerabilityStatus.ACTIVE,
+ importance=BugTaskImportance.CRITICAL,
+ description="Foo bar",
+ notes="lgp171188> Foo bar",
+ mitigation="Foo bar baz",
+ importance_explanation="Foo bar bazz",
+ date_made_public=now,
+ )
+ )
+
def test_user_without_edit_permissions_cannot_edit_vulnerability(self):
vulnerability = removeSecurityProxy(
self.factory.makeVulnerability()
@@ -226,4 +289,4 @@ class TestVulnerabilityWebService(TestCaseWithFactory):
)
)
self.assertEqual(401, response.status)
- self.assertIn('launchpad.Edit', response.body.decode())
+ self.assertIn('launchpad.SecurityAdmin', response.body.decode())
diff --git a/lib/lp/permissions.zcml b/lib/lp/permissions.zcml
index 646d9ca..dd5a177 100644
--- a/lib/lp/permissions.zcml
+++ b/lib/lp/permissions.zcml
@@ -70,6 +70,10 @@
id="launchpad.LanguagePacksAdmin" title="Administer Language Packs."
access_level="write" />
+ <permission
+ id="launchpad.SecurityAdmin" title="Administer vulnerabilities."
+ access_level="write" />
+
<!-- XXX: GuilhermeSalgado 2005-08-25
To be removed soon, this is only needed by the page to upload SSH
keys, cause we decided to not allow admins to upload keys in behalf of other
diff --git a/lib/lp/registry/configure.zcml b/lib/lp/registry/configure.zcml
index 10d677f..eefe667 100644
--- a/lib/lp/registry/configure.zcml
+++ b/lib/lp/registry/configure.zcml
@@ -1834,6 +1834,9 @@
interface="lp.registry.interfaces.distribution.IDistributionEditRestricted"
set_schema="lp.app.interfaces.informationtype.IInformationType"/>
<require
+ permission="launchpad.SecurityAdmin"
+ interface="lp.registry.interfaces.distribution.IDistributionSecurityAdminRestricted" />
+ <require
permission="launchpad.Edit"
set_attributes="answers_usage blueprints_usage codehosting_usage
bug_tracking_usage uses_launchpad"/>
@@ -1866,6 +1869,7 @@
package_derivatives_email
redirect_default_traversal
redirect_release_uploads
+ security_admin
summary
vcs
"/>
diff --git a/lib/lp/registry/interfaces/distribution.py b/lib/lp/registry/interfaces/distribution.py
index 58e10fc..727d204 100644
--- a/lib/lp/registry/interfaces/distribution.py
+++ b/lib/lp/registry/interfaces/distribution.py
@@ -480,6 +480,11 @@ class IDistributionView(
has_current_commercial_subscription = Attribute(
"Whether the distribution has a current commercial subscription.")
+ security_admin = Choice(
+ title=_("Security Administrator"),
+ description=_("The distribution security administrator."),
+ required=False, vocabulary='ValidPersonOrTeam')
+
vulnerabilities = exported(
doNotSnapshot(CollectionField(
description=_("Vulnerabilities in this distribution."),
@@ -889,6 +894,10 @@ class IDistributionEditRestricted(IOfficialBugTagTargetRestricted):
def deleteOCICredentials():
"""Delete any existing OCI credentials for the distribution."""
+
+class IDistributionSecurityAdminRestricted(Interface):
+ """IDistribution properties requiring launchpad.SecurityAdmin permission"""
+
@call_with(creator=REQUEST_USER)
@operation_parameters(
status=Choice(
@@ -954,10 +963,10 @@ class IDistributionEditRestricted(IOfficialBugTagTargetRestricted):
@exported_as_webservice_entry(as_of="beta")
class IDistribution(
- IDistributionEditRestricted, IDistributionPublic,
- IDistributionLimitedView, IDistributionView, IHasBugSupervisor,
- IFAQTarget, IQuestionTarget, IStructuralSubscriptionTarget,
- IInformationType):
+ IDistributionEditRestricted, IDistributionSecurityAdminRestricted,
+ IDistributionPublic,IDistributionLimitedView, IDistributionView,
+ IHasBugSupervisor, IFAQTarget, IQuestionTarget,
+ IStructuralSubscriptionTarget, IInformationType):
"""An operating system distribution.
Launchpadlib example: retrieving the current version of a package in a
diff --git a/lib/lp/registry/model/distribution.py b/lib/lp/registry/model/distribution.py
index 3abe8fb..4606d82 100644
--- a/lib/lp/registry/model/distribution.py
+++ b/lib/lp/registry/model/distribution.py
@@ -685,6 +685,9 @@ class Distribution(SQLBase, BugTargetBase, MakesAnnouncements,
language_pack_admin = ForeignKey(
dbName='language_pack_admin', foreignKey='Person',
storm_validator=validate_public_person, notNull=False, default=None)
+ security_admin = ForeignKey(
+ dbName='security_admin', foreignKey='Person',
+ storm_validator=validate_public_person, notNull=False, default=None)
@cachedproperty
def main_archive(self):
diff --git a/lib/lp/registry/tests/test_distribution.py b/lib/lp/registry/tests/test_distribution.py
index b6d6ad6..8c80631 100644
--- a/lib/lp/registry/tests/test_distribution.py
+++ b/lib/lp/registry/tests/test_distribution.py
@@ -87,6 +87,7 @@ from lp.soyuz.interfaces.distributionsourcepackagerelease import (
)
from lp.testing import (
admin_logged_in,
+ anonymous_logged_in,
api_url,
celebrity_logged_in,
login,
@@ -1826,6 +1827,33 @@ class TestDistributionVulnerabilities(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
+ def assert_newVulnerability_only_the_required_params(
+ self,
+ distribution,
+ creator
+ ):
+ vulnerability = distribution.newVulnerability(
+ status=VulnerabilityStatus.NEEDS_TRIAGE,
+ information_type=InformationType.PUBLIC,
+ creator=creator,
+ )
+
+ self.assertThat(
+ vulnerability,
+ MatchesStructure.byEquality(
+ status=VulnerabilityStatus.NEEDS_TRIAGE,
+ creator=creator,
+ importance=BugTaskImportance.UNDECIDED,
+ information_type=InformationType.PUBLIC,
+ cve=None,
+ description=None,
+ notes=None,
+ mitigation=None,
+ importance_explanation=None,
+ date_made_public=None
+ )
+ )
+
def test_vulnerabilities_no_vulnerability_present(self):
distribution = self.factory.makeDistribution()
self.assertEqual(0, distribution.vulnerabilities.count())
@@ -1843,33 +1871,34 @@ class TestDistributionVulnerabilities(TestCaseWithFactory):
set(distribution.vulnerabilities),
)
+ def test_set_security_admin_permissions(self):
+ distribution = self.factory.makeDistribution()
+ person = self.factory.makePerson()
+ person2 = self.factory.makePerson()
+ security_team = self.factory.makeTeam(members=[person])
+
+ with person_logged_in(distribution.owner):
+ distribution.security_admin = security_team
+ self.assertEqual(security_team, distribution.security_admin)
+
+ with admin_logged_in():
+ distribution.security_admin = None
+ self.assertIsNone(distribution.security_admin)
+
+ with person_logged_in(person2), ExpectedException(Unauthorized):
+ distribution.security_admin = security_team
+
+ with anonymous_logged_in(), ExpectedException(Unauthorized):
+ distribution.security_admin = security_team
+
def test_newVulnerability_default_arguments(self):
distribution = self.factory.makeDistribution()
owner = distribution.owner
with person_logged_in(owner):
- # The distribution owner can create a new vulnerability in
- # the distribution.
- vulnerability = distribution.newVulnerability(
- status=VulnerabilityStatus.NEEDS_TRIAGE,
- information_type=InformationType.PUBLIC,
- creator=owner,
- )
-
- self.assertThat(
- vulnerability,
- MatchesStructure.byEquality(
- status=VulnerabilityStatus.NEEDS_TRIAGE,
- creator=owner,
- importance=BugTaskImportance.UNDECIDED,
- information_type=InformationType.PUBLIC,
- cve=None,
- description=None,
- notes=None,
- mitigation=None,
- importance_explanation=None,
- date_made_public=None
- )
+ self.assert_newVulnerability_only_the_required_params(
+ distribution,
+ creator=owner
)
def test_newVulnerability_all_parameters(self):
@@ -1909,6 +1938,54 @@ class TestDistributionVulnerabilities(TestCaseWithFactory):
)
)
+ def test_newVulnerability_permissions(self):
+ person = self.factory.makePerson()
+ distribution = self.factory.makeDistribution()
+ security_team = self.factory.makeTeam(members=[person])
+
+ # The distribution owner, admin, can create a new vulnerability
+ # in the distribution.
+ with person_logged_in(distribution.owner):
+ self.assert_newVulnerability_only_the_required_params(
+ distribution,
+ creator=distribution.owner
+ )
+
+ with admin_logged_in():
+ self.assert_newVulnerability_only_the_required_params(
+ distribution,
+ creator=person
+ )
+ self.factory.makeCommercialSubscription(pillar=distribution)
+
+ with celebrity_logged_in('commercial_admin'):
+ self.assert_newVulnerability_only_the_required_params(
+ distribution,
+ creator=person
+ )
+
+ with person_logged_in(distribution.owner):
+ distribution.security_admin = security_team
+
+ # When the security admin is set for the distribution,
+ # users in that team can create a new vulnerability in the
+ # distribution.
+ with person_logged_in(person):
+ self.assert_newVulnerability_only_the_required_params(
+ distribution,
+ creator=person
+ )
+
+ def test_newVulnerability_cannot_be_called_by_unprivileged_users(self):
+ person = self.factory.makePerson()
+ distribution = self.factory.makeDistribution()
+ with person_logged_in(person), ExpectedException(Unauthorized):
+ distribution.newVulnerability(
+ status=VulnerabilityStatus.NEEDS_TRIAGE,
+ information_type=InformationType.PUBLIC,
+ creator=person,
+ )
+
def test_getVulnerability_non_existent_id(self):
distribution = self.factory.makeDistribution()
vulnerability = distribution.getVulnerability(9999999)
@@ -2159,6 +2236,66 @@ class TestDistributionVulnerabilitiesWebService(TestCaseWithFactory):
"date_made_public": Is(None),
}))
+ def test_newVulnerability_security_admin(self):
+ distribution = self.factory.makeDistribution()
+ person = self.factory.makePerson()
+ security_team = self.factory.makeTeam(members=[person])
+ api_base = "http://api.launchpad.test/devel";
+ distribution_url = api_base + api_url(distribution)
+ owner_url = api_base + api_url(person)
+
+ with person_logged_in(distribution.owner):
+ distribution.security_admin = security_team
+
+ webservice = webservice_for_person(
+ person,
+ permission=OAuthPermission.WRITE_PRIVATE,
+ default_api_version="devel"
+ )
+ response = webservice.named_post(
+ distribution_url,
+ 'newVulnerability',
+ status='Needs triage',
+ information_type='Public',
+ creator=owner_url,
+ )
+ self.assertEqual(200, response.status)
+ self.assertThat(response.jsonBody(), ContainsDict({
+ "distribution_link": Equals(distribution_url),
+ "id": IsInstance(int),
+ "cve_link": Is(None),
+ "creator_link": Equals(owner_url),
+ "status": Equals("Needs triage"),
+ "description": Is(None),
+ "notes": Is(None),
+ "mitigation": Is(None),
+ "importance": Equals("Undecided"),
+ "importance_explanation": Is(None),
+ "information_type": Equals("Public"),
+ "date_made_public": Is(None),
+ }))
+
+ def test_newVulnerability_unauthorized_users(self):
+ distribution = self.factory.makeDistribution()
+ person = self.factory.makePerson()
+ api_base = "http://api.launchpad.test/devel";
+ distribution_url = api_base + api_url(distribution)
+ owner_url = api_base + api_url(person)
+
+ webservice = webservice_for_person(
+ person,
+ permission=OAuthPermission.WRITE_PRIVATE,
+ default_api_version="devel"
+ )
+ response = webservice.named_post(
+ distribution_url,
+ 'newVulnerability',
+ status='Needs triage',
+ information_type='Public',
+ creator=owner_url,
+ )
+ self.assertEqual(401, response.status)
+
def test_newVulnerability_all_parameters(self):
distribution = self.factory.makeDistribution()
owner = distribution.owner
diff --git a/lib/lp/security.py b/lib/lp/security.py
index c8f518f..2076988 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -5,6 +5,7 @@
__all__ = [
'BugTargetOwnerOrBugSupervisorOrAdmins',
+ 'is_commercial_case',
'ModerateByRegistryExpertsOrAdmins',
]
@@ -1290,6 +1291,21 @@ class EditDistributionByDistroOwnersOrAdmins(AuthorizationBase):
or user.in_admin)
+class EditDistributionVulnerabilities(AuthorizationBase):
+ """The security admins of a distribution should be able to create
+ and edit vulnerabilities in the distribution."""
+ permission = 'launchpad.SecurityAdmin'
+ usedfor = IDistribution
+
+ def checkAuthenticated(self, user):
+ return (
+ user.isOwner(self.obj)
+ or is_commercial_case(self.obj, user)
+ or user.in_admin
+ or user.inTeam(self.obj.security_admin)
+ )
+
+
class ModerateDistributionByDriversOrOwnersOrAdmins(AuthorizationBase):
"""Distribution drivers, owners, and admins may plan releases.
Follow ups
