launchpad-reviewers team mailing list archive

[Ines Almeida <mp+493451@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/bugs/configure.zcml b/lib/lp/bugs/configure.zcml
> index 0c6c864..beb005e 100644
> --- a/lib/lp/bugs/configure.zcml
> +++ b/lib/lp/bugs/configure.zcml
> @@ -594,6 +594,19 @@
>              <require
>                  permission="launchpad.AnyPerson"
>                  set_schema="lp.bugs.interfaces.cve.ICve"/>
> +            <require
> +                permission="launchpad.InternalScriptsOnly"
> +                set_attributes="
> +                    sequence
> +                    status
> +                    description
> +                    datecreated
> +                    datemodified
> +                    references
> +                    date_made_public
> +                    discovered_by
> +                    _cvss

Should this actually be `cvss` instead?
There is a `cvss` setter and property decorators, so I think we should expose that instead of _cvss

> +                    metadata"/>
>  
>              <!-- IBugLinkTarget -->
>  
> diff --git a/lib/lp/bugs/security.py b/lib/lp/bugs/security.py
> index 4d961ac..418d279 100644
> --- a/lib/lp/bugs/security.py
> +++ b/lib/lp/bugs/security.py
> @@ -453,3 +454,30 @@ class BugTargetOwnerOrBugSupervisorOrAdmins(AuthorizationBase):
>              or user.inTeam(self.obj.owner)
>              or user.in_admin
>          )
> +
> +
> +class EditCve(DelegatedAuthorization):

Defining who can actually edit this is a great idea

But afaics these permissions aren't being used - they would probably need something in the configure.zcml file that sets what the `launchpad.Edit` and `launchpad.Delete` mean for the Cve model (similar to what the `launchpad.InternalScriptsOnly` is doing

Right now, an Admin has `launchpad.Edit` permission to a CVE, but the `launchpad.Edit` permissions doesn't grant permission to edit

Either we don't define tat permission here (if we think admins don't need it, and only scripts do), or we add it to the configure.zcml file

> +    """Only Admin users can edit Cves. InternalScripts will be also able to
> +    edit as we are also using InternalScriptsOnly permission."""
> +
> +    permission = "launchpad.Edit"
> +    usedfor = ICve
> +
> +    def checkUnauthenticated(self):
> +        return False
> +
> +    def checkAuthenticated(self, user):
> +        return user.in_admin
> +
> +
> +class DeleteCve(DelegatedAuthorization):
> +    """Only Admin users can delete Cves."""
> +
> +    permission = "launchpad.Delete"
> +    usedfor = ICve
> +
> +    def checkUnauthenticated(self):
> +        return False
> +
> +    def checkAuthenticated(self, user):
> +        return user.in_admin
> diff --git a/lib/lp/bugs/tests/test_cve.py b/lib/lp/bugs/tests/test_cve.py
> index 23fd423..3f3107d 100644
> --- a/lib/lp/bugs/tests/test_cve.py
> +++ b/lib/lp/bugs/tests/test_cve.py
> @@ -383,3 +260,51 @@ class TestCve(TestCaseWithFactory):
>          # Admin can see the PROPRIETARY vulnerability
>          with admin_logged_in():
>              self.assertEqual(vulnerability, cve.vulnerabilities[0])
> +
> +    def test_cve_permissions_anonymous(self):
> +        """Test that anonymous user cannot view, edit or delete."""
> +        self.assertFalse(checkPermission("launchpad.View", self.cve))
> +        self.assertFalse(checkPermission("launchpad.Edit", self.cve))
> +        self.assertFalse(checkPermission("launchpad.Delete", self.cve))
> +
> +    def test_cve_permissions_authenticated(self):
> +        """Test that logged in user can view but not edit or delete."""
> +        person = self.factory.makePerson()
> +
> +        with person_logged_in(person):
> +            self.assertTrue(checkPermission("launchpad.View", self.cve))
> +            self.assertFalse(checkPermission("launchpad.Edit", self.cve))
> +            self.assertFalse(checkPermission("launchpad.Delete", self.cve))
> +
> +    def test_cve_permissions_admin(self):
> +        """Test that admin can view, edit and delete."""
> +        with admin_logged_in():
> +            self.assertTrue(checkPermission("launchpad.View", self.cve))
> +            self.assertTrue(checkPermission("launchpad.Edit", self.cve))
> +            self.assertTrue(checkPermission("launchpad.Delete", self.cve))
> +
> +    def test_cve_readonly(self):

I think it's useful, it makes it clear what is editable and what is not (which in this case is none are editable...). We do add the `launchpad.Edit` permission to be able to edit these, I'd go as far as add a test showing one can edit (perhaps not necessary if we keep to the `InternalScripts` and that is already testing that in test_cveimport.py)

> +        """Test that app code cannot update Cve attributes, but
> +        InternalScripts can."""
> +        failure_regex = ".*InternalScriptsOnly"
> +
> +        with ExpectedException(Unauthorized, failure_regex):
> +            self.cve.sequence = "2099-9876"
> +        with ExpectedException(Unauthorized, failure_regex):
> +            self.cve.status = CveStatus.DEPRECATED
> +        with ExpectedException(Unauthorized, failure_regex):
> +            self.cve.description = "example"
> +        with ExpectedException(Unauthorized, failure_regex):
> +            self.cve.datecreated = datetime.utcnow()
> +        with ExpectedException(Unauthorized, failure_regex):
> +            self.cve.datemodified = datetime.utcnow()
> +        with ExpectedException(Unauthorized, failure_regex):
> +            self.cve.references = []
> +        with ExpectedException(Unauthorized, failure_regex):
> +            self.cve.date_made_public = datetime.utcnow()
> +        with ExpectedException(Unauthorized, failure_regex):
> +            self.cve.discovered_by = "example person"
> +        with ExpectedException(Unauthorized, failure_regex):
> +            self.cve._cvss = {"example authority": ["example score"]}
> +        with ExpectedException(Unauthorized, failure_regex):
> +            self.cve.metadata = {"meta": "data"}


-- 
https://code.launchpad.net/~enriqueesanchz/launchpad/+git/launchpad/+merge/493451
Your team Launchpad code reviewers is requested to review the proposed merge of ~enriqueesanchz/launchpad:add-metadata-cve-model into launchpad:master.