Fixes #15490 - adding view_host filter and improving error messages

[lzap]

Fixing both missing permissions and incorrect error message.

[ares]

avoid double map, TemplateKind.all.map { |kind| kind.name.to_sym }

[ares]

also I'd prefer using some hard list (maybe just put this into constant https://github.com/theforeman/foreman/blob/develop/db/seeds.d/07-config_templates.rb#L9),

• one reason is that if plugin introduces new kind it should also map it's own permission, this would allow to review any custom template kind with view_host permission
• second is that this is required in initializer https://github.com/theforeman/foreman/blob/develop/config/initializers/foreman.rb#L1 and it's good to avoid using AR there

[ares]

In redmine issue you've mentioned review_templates permissions, did you change your mind?

[lzap]

Yeah, changed my mind because it looks like unattended with provision already works okay. Unattended does not have permission checks enabled, it's only reviews what need it. I don't see any security risk there. Let me change the AR to static list. AFAIK the only plugin that adds a template kind is bootdisk currently, but there is no way of previewing bootdisk templates therefore we don't need to add the permission symbol there?

[lzap]

Fixed, what you think?

[lzap]

Fixed failing tests. FYI these were expecting incorrect behavior. If you test this, the controller errors out with "Undefined method: layout" error because actually layout is not usable with this particular controller.

[ares]

could you deduplicate this list? https://github.com/theforeman/foreman/blob/develop/db/seeds.d/07-config_templates.rb#L9

just put it into a constant somewhere in TemplateKind and use it in both places?

[ares]

Other than the duplication issue seems all right, I'll continue with testing when it's resolved.

[ohadlevy]

@lzap bump?

[lzap]

Done!

diff --git a/app/models/template_kind.rb b/app/models/template_kind.rb
index 7c1e5a9..d647c1c 100644
--- a/app/models/template_kind.rb
+++ b/app/models/template_kind.rb
@@ -6,4 +6,8 @@ class TemplateKind < ActiveRecord::Base
   has_many :os_default_templates
   validates :name, :presence => true, :uniqueness => true
   scoped_search :on => :name
+
+  def self.predefined
+    [:PXELinux, :PXEGrub, :iPXE, :provision, :finish, :script, :user_data, :ZTP]
+  end
 end
diff --git a/app/services/foreman/access_permissions.rb b/app/services/foreman/access_permissions.rb
index a732de6..9b4488a 100644
--- a/app/services/foreman/access_permissions.rb
+++ b/app/services/foreman/access_permissions.rb
@@ -318,13 +318,12 @@ Foreman::AccessControl.map do |permission_set|
     pc_ajax_actions = [:parameters]
     subnets_ajax_actions = [:freeip]
     tasks_ajax_actions = [:show]
-    template_kinds =  [:PXELinux, :PXEGrub, :iPXE, :provision, :finish, :script, :user_data, :ZTP]

     map.permission :view_hosts,    {:hosts => [:index, :show, :errors, :active, :out_of_sync, :disabled, :pending, :vm,
                                       :externalNodes, :pxe_config, :storeconfig_klasses, :auto_complete_search, :bmc,
                                       :runtime, :resources, :templates, :overview, :nics],
                                     :dashboard => [:OutOfSync, :errors, :active],
-                                    :unattended => [:template, :provision] + template_kinds,
+                                    :unattended => [:template, :provision] + TemplateKind.predefined,
                                      :"api/v1/hosts" => [:index, :show, :status],
                                      :"api/v2/hosts" => [:index, :show, :status, :vm_compute_attributes],
                                      :"api/v2/interfaces" => [:index, :show],

[ares]

Could you please also use TemplateKind.predefined in https://github.com/theforeman/foreman/blob/develop/db/seeds.d/07-provisioning_templates.rb (that's why I wanted you to extract it)

[ares]

Why did you choose to use class method instead of constant? This will be harder to extend if we need it (only with alias_method_chain), if it was an array in constant, one could push to that array.

[lzap]

Right, added and rebased on top of develop.

[domcleal]

On re-reviewing this, it seems the issue has already been fixed in b1997f5 when I refactored the controller in the same way that was suggested in the #10689 comments. Previewing templates as a non-admin user works fine.

It does though mean we're vulnerable in develop to the security issue I mentioned at #2428 (comment), so this needs addressing. I've filed this as http://projects.theforeman.org/issues/15490, and updated foreman-security + requested a CVE.

What I'd like to do is:

1. Close #10689 as a resolved or a dupe of #13039, which fixes the issue.
2. Update this PR to reference #15490, as it contains the authorisation fixes and fixes the error handling on 403s.
3. Update this PR to fix some review comments which I'll add shortly.

Sorry for the ticket change, but I'd like to be absolutely clear about what this is fixing.

[domcleal]

This shouldn't be necessary now, since the action is always going to be either :host_template or :hostgroup_template - the routing changed in b1997f5 to route the template kinds to a single action.

[domcleal]

I'd like to add, that given the security issue, please let me know if you don't plan to update this PR to fix it and instead, I'll patch it myself - rather than leave it months again.

[domcleal]

@lzap if you could please update this soon to fix the security issue, or let me know if you don't plan to.

[lzap]

@domcleal had a huge github backlog and noticed just now. I can start tomorrow with this.

[theforeman-bot]

There were the following issues with the commit message:

• commit message for cd5ab4d is not wrapped at 72nd column
• commit message for cd5ab4d is not wrapped at 72nd column
• commit message for cd5ab4d is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[lzap]

Amended all your last comments, rebased and pushed.

[domcleal]

Merged as c3c186d, thanks @lzap!