Merge ~lgp171188/launchpad:new-security-role-permission into launchpad:master

[Guruprasad]

Add a new distribution security admin role

And restrict the permissions on creating and editing vulnerabilities
to it.

[Andrey Fedoseev (andrey-fedoseev)]

This looks good.

I have doubts about the the permission name: `SecurityAdmin`. I think there's a convention to use a verb in the permission names. `SecurityAdmin` sounds more like a role than a permission.

Could it be `ManageVulnerabilties` or `AdministerVulnerabilities` instead?

[Guruprasad (lgp171188)]

> I have doubts about the the permission name: `SecurityAdmin`. I think there's a convention to use a verb in the permission names. `SecurityAdmin` sounds more like a role than a permission.
>
> Could it be `ManageVulnerabilties` or `AdministerVulnerabilities` instead?

I thought about it when I was about to name it and found that there are some existing permissions that do not fit in this mould - `launchpad.AnyAllowedPerson`, `launchpad.AnyLegitimatePerson`, `launchpad.TranslationsAdmin`, `launchpad.LanguagePacksAdmin` etc. That is why I named it as `launchpad.SecurityAdmin`.

[Andrey Fedoseev (andrey-fedoseev)]

Approved

review: Approve

[Colin Watson (cjwatson)]

review: Needs Fixing

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/bugs/model/tests/test_vulnerability.py b/lib/lp/bugs/model/tests/test_vulnerability.py
index f2cac72..f470e40 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,34 +41,68 @@ 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))
 
-    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))
 
-    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))
 
-    def test_anonymous(self):
+    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
+
+        with person_logged_in(person):
+            self.assertTrue(
+                check_permission(
+                    "launchpad.Edit", 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))
 
+    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..ce1829a 100644
--- a/lib/lp/bugs/security.py
+++ b/lib/lp/bugs/security.py
@@ -23,6 +23,7 @@ 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,
@@ -384,3 +385,13 @@ class EditBugSubscriptionFilter(AuthorizationBase):
 
     def checkAuthenticated(self, user):
         return user.inTeam(self.obj.structural_subscription.subscriber)
+
+
+class EditVulnerability(DelegatedAuthorization):
+    """The security admins of a distribution should be able to edit
+    vulnerabilities in that distribution."""
+    permission = 'launchpad.Edit'
+    usedfor = IVulnerability
+
+    def __init__(self, obj):
+        super().__init__(obj, obj.distribution, 'launchpad.SecurityAdmin')
diff --git a/lib/lp/bugs/tests/test_vulnerability.py b/lib/lp/bugs/tests/test_vulnerability.py
index be3457a..7590be2 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()
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 97e51c0..e9903ab 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 ee458ca..c44df04 100644
--- a/lib/lp/registry/tests/test_distribution.py
+++ b/lib/lp/registry/tests/test_distribution.py
@@ -86,6 +86,7 @@ from lp.soyuz.interfaces.distributionsourcepackagerelease import (
     )
 from lp.testing import (
     admin_logged_in,
+    anonymous_logged_in,
     api_url,
     celebrity_logged_in,
     login,
@@ -1813,6 +1814,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())
@@ -1830,33 +1858,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):
@@ -1896,6 +1925,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)
@@ -2146,6 +2223,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 4eeacbc..2ab931d 100644
--- a/lib/lp/security.py
+++ b/lib/lp/security.py
@@ -55,7 +55,6 @@ from lp.blueprints.model.specificationsubscription import (
     )
 from lp.bugs.interfaces.bugtarget import IOfficialBugTagTargetRestricted
 from lp.bugs.interfaces.structuralsubscription import IStructuralSubscription
-from lp.bugs.interfaces.vulnerability import IVulnerability
 from lp.bugs.model.bugsubscription import BugSubscription
 from lp.bugs.model.bugtaskflat import BugTaskFlat
 from lp.bugs.model.bugtasksearch import get_bug_privacy_filter
@@ -1293,6 +1292,21 @@ class EditDistributionByDistroOwnersOrAdmins(AuthorizationBase):
             or user.in_admin)
 
 
+class SecurityAdminDistribution(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.
 
@@ -3780,14 +3794,3 @@ class EditCIBuild(AdminByBuilddAdmin):
         if auth_repository.checkAuthenticated(user):
             return True
         return super().checkAuthenticated(user)
-
-
-class EditVulnerability(AuthorizationBase):
-    permission = 'launchpad.Edit'
-    usedfor = IVulnerability
-
-    def checkAuthenticated(self, user):
-        return (user.in_commercial_admin or user.in_admin or
-                user.isOwner(self.obj.distribution) or
-                user.isDriver(self.obj.distribution) or
-                user.isBugSupervisor(self.obj.distribution))