launchpad-reviewers team mailing list archive

[Enrique Sánchez <mp+493451@xxxxxxxxxxxxxxxxxx>]

Replied to the comments and addressed them in separate commits. I will rebase before merging.

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

Addressed. Tbh, I did it as it is a quick change but I'm wondering if we should have that getter and setter (it is from 3 years ago). I see it is only used in tests, we can easily refactor it.

> +                    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):

Thank you! Looking at this again with a fresh mind, I think it is better to remove this and let only internal scripts permissions. 
I don't think we need this extra and the less code the better :)

> +    """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


-- 
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.