launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/buildmaster/browser/builder.py b/lib/lp/buildmaster/browser/builder.py
> index 1c67f11..3db020e 100644
> --- a/lib/lp/buildmaster/browser/builder.py
> +++ b/lib/lp/buildmaster/browser/builder.py
> @@ -412,16 +414,36 @@ class BuilderSetAddView(LaunchpadFormView):
>          return canonical_url(self.context)
>  
>  
> -class BuilderEditView(LaunchpadEditFormView):
> +class BuilderEditViewAdmins(LaunchpadEditFormView):

BuilderEditView was a better name.  We use FooAdminView for admin-specific views, but I don't think that applies here.

>      """View class for changing builder details."""
>  
> -    schema = IBuilder
> +    field_names = None
> +
> +    @cachedproperty
> +    def schema(self):
> +        class BuilderEditSchema(Interface):
> +            """Defines the fields for the edit form.
> +
> +            This is necessary to make various fields editable that are not
> +            normally editable through the interface.
> +            """
> +
> +            name = copy_field(IBuilder["name"], readonly=False)
> +            title = copy_field(IBuilder["title"], readonly=False)
> +            processors = copy_field(IBuilder["processors"], readonly=False)
> +            url = copy_field(IBuilder["url"], readonly=False)
> +            owner = copy_field(IBuilder["owner"], readonly=False)
> +            virtualized = copy_field(IBuilder["virtualized"], readonly=False)
> +            vm_host = copy_field(IBuilder["vm_host"], readonly=False)
> +            vm_reset_protocol = copy_field(IBuilder["vm_reset_protocol"], readonly=False)
> +            active = copy_field(IBuilder["active"], readonly=False)
> +
> +            use_template(IBuilder, include=["builderok"])
> +            use_template(IBuilder, include=["manual"])
> +            use_template(IBuilder, include=["failnotes"])
> +
> +        return BuilderEditSchema
>  
> -    field_names = [
> -        'name', 'title', 'processors', 'url', 'manual', 'owner',
> -        'virtualized', 'builderok', 'failnotes', 'vm_host',
> -        'vm_reset_protocol', 'active',
> -        ]

If you apply the configure.zcml fix I suggest below (with set_schema="lp.buildmaster.interfaces.builder.IBuilderModerateAttributes"), then you shouldn't need to manually work around permissions like this; I'd expect it to be possible to revert all the changes to this file.

(Note that ~launchpad-buildd-admins don't need to be able to do this sort of thing in the web interface; it's sufficient to be able to poke at things using the API via the `manage-builders` script.  So I wouldn't bother spending effort to make the web interface usable if you have launchpad.Moderate but not launchpad.Edit on a builder.)

>      custom_widget_processors = LabeledMultiCheckBoxWidget
>  
>      @action(_('Change'), name='update')
> diff --git a/lib/lp/buildmaster/browser/configure.zcml b/lib/lp/buildmaster/browser/configure.zcml
> index c1a40b4..18a8899 100644
> --- a/lib/lp/buildmaster/browser/configure.zcml
> +++ b/lib/lp/buildmaster/browser/configure.zcml
> @@ -90,9 +90,9 @@
>      </browser:pages>
>      <browser:page
>          name="+edit"
> -        for="lp.buildmaster.interfaces.builder.IBuilder"
> -        class="lp.buildmaster.browser.builder.BuilderEditView"
> -        permission="launchpad.Edit"
> +        for="lp.buildmaster.interfaces.builder.IBuilderModerateAttributes"
> +        class="lp.buildmaster.browser.builder.BuilderEditViewAdmins"
> +        permission="launchpad.Moderate"

As above, I'd prefer to revert this and only concern ourselves with allowing launchpad-buildd-admins to set these attributes using the API.  As a buildd admin you don't normally want to bother editing builders using the web UI anyway, since more often than not you're working on many builders at once and having to click through dozens of edit forms is no fun.

>          template="../templates/builder-edit.pt"/>
>      <browser:menus
>          classes="BuilderSetOverviewMenu"
> diff --git a/lib/lp/buildmaster/configure.zcml b/lib/lp/buildmaster/configure.zcml
> index 3d34cb1..35ccf9d 100644
> --- a/lib/lp/buildmaster/configure.zcml
> +++ b/lib/lp/buildmaster/configure.zcml
> @@ -32,14 +32,17 @@
>      </securedutility>
>  
>      <!-- Builder -->
> -    <class
> -        class="lp.buildmaster.model.builder.Builder">
> -        <allow
> -            interface="lp.buildmaster.interfaces.builder.IBuilderView"/>
> +    <class class="lp.buildmaster.model.builder.Builder">
> +        <require
> +            permission="zope.Public"
> +            interface="lp.app.interfaces.launchpad.IPrivacy
> +                       lp.buildmaster.interfaces.builder.IBuilderView" />
> +        <require
> +            permission="launchpad.Moderate"
> +            interface="lp.buildmaster.interfaces.builder.IBuilderModerateAttributes"/>
>          <require
>              permission="launchpad.Edit"
> -            interface="lp.buildmaster.interfaces.builder.IBuilderEdit"
> -            set_schema="lp.buildmaster.interfaces.builder.IBuilderView"/>
> +            interface="lp.buildmaster.interfaces.builder.IBuilderEdit"/>

The 'interface' attribute declares getattr() permissions for names in the given interface(s), while the 'set_schema' attribute declares setattr() permissions for names in the given interface(s).  This should probably be more like this (and drop the zope.Public bit):

    <allow
        interface="lp.buildmaster.interfaces.builder.IBuilderView
                   lp.buildmaster.interfaces.builder.IBuilderModerateAttributes"/>
    <require
        permission="launchpad.Edit"
        interface="lp.buildmaster.interfaces.builder.IBuilderEdit"/>
    <require
        permission="launchpad.Moderate"
        set_schema="lp.buildmaster.interfaces.builder.IBuilderModerateAttributes"/>

>      </class>
>  
>  
> diff --git a/lib/lp/security.py b/lib/lp/security.py
> index 243c1a8..af7ecbc 100644
> --- a/lib/lp/security.py
> +++ b/lib/lp/security.py
> @@ -1970,10 +1971,24 @@ class AdminBuilder(AdminByBuilddAdmin):
>      usedfor = IBuilder
>  
>  
> -class EditBuilder(AdminByBuilddAdmin):
> +class EditBuilder(AuthorizationBase):
>      permission = 'launchpad.Edit'
>      usedfor = IBuilder
>  
> +    def checkAuthenticated(self, user):
> +        return (user.in_admin
> +                or user.in_buildd_admin)
> +
> +
> +class ModerateEditBuilder(EditBuilder):

This should be spelled ModerateBuilder.

> +    permission = 'launchpad.Moderate'
> +    usedfor = IBuilderModerateAttributes
> +
> +    def checkAuthenticated(self, user):
> +        return (user.in_admin
> +                or user.in_registry_experts
> +                or user.in_buildd_admin)

I'd suggest leaving EditBuilder as AdminByBuilddAdmin without defining its own checkAuthenticated method, and then define ModerateBuilder.checkAuthenticated as `return user.in_registry_experts or super(ModerateBuilder, self).checkAuthenticated(user)`, so that it's clearer which bits are being customised.

> +
>  
>  class AdminBuildRecord(AdminByBuilddAdmin):
>      usedfor = IBuildFarmJob


-- 
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/393716
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:change-permissions-builder-reset into launchpad:master.