Fixes #17962 - extend core roles #6703

ares · 2017-03-24T15:59:16Z

Most of this PR is moving permissions definition to plugin so it's executed in right order. Then I added add_all_permissions_to_default_roles at the end of plugin registration block that extends core Manager and Viewer role. This bumps Foreman dependency to 1.15.

mention-bot · 2017-03-24T15:59:17Z

@ares, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ehelms, @waldenraines and @jlsherrill to be potential reviewers.

ares · 2017-03-27T08:50:42Z

I highly recommend getting this in before next release. The test failures seems to be related to theforeman/foreman-packaging#1573

EDIT: nope, just the error is similar

ares · 2017-03-28T07:22:04Z

Should be fixed in core now [test]

ehelms · 2017-03-28T13:06:41Z

@ares can you explain a bit more why we have to pile all the permissions into a single file? You mentioned to ensure correct order, but it's not obvious to me why ordering is required to load in permissions.

jlsherrill · 2017-03-28T13:08:06Z

Being in a single file by itself would be better than putting it in plugin.rb IMHO. plugin.rb is getting quite crowded with lots of little things, sticking all the perms in it makes it a bit unwieldy

ares · 2017-03-28T14:59:41Z

I'm not sure how to do it right. If I extract it to a file and I use require, I need to hook into plugin context somehow. The plugin is not registered yet at that point. I could use eval but... Or I can create a class that receives plugin context and call permissions on it (see second commit), probably most clean. But IMHO all these solutions are wrong.

Honestly I'd recommend trying to avoid diverging from how other plugins do permissions definition. That's the way Katello can avoid breakage by Foreman changes in future. The way Katello defined permissions so far is a good example.

I understand the list is huge. I'd encourage to raising an issue in Foreman or send a patch to core that would address Katello needs, instead of workarounding it. Maybe by introducing PermissionCreator that would only receive data required to define permissions. Core would make sure it would be defined at the right time.

Anyway, let me know if you find this better and want me to squash.

houndci-bot · 2017-03-28T14:59:46Z

lib/katello/permission_creator.rb

+                       :resource_type => 'Katello::KTEnvironment'
+  end
+
+  def product_permissions


Method has too many lines. [76/30]

ares · 2017-03-28T15:25:36Z

@ares can you explain a bit more why we have to pile all the permissions into a single file? You mentioned to ensure correct order, but it's not obvious to me why ordering is required to load in permissions.

Katello used to find the registered plugin and run security_block on it. We need to call add_all_permissions_to_default_roles within register block. If we simply required permission definitions in register block, it would not find the plugin since it's registration hasn't finished yet.

Note that plugin register also defines some order, e.g. after_intialize block run just after the register block is evaluated, therefore if some after_initialize code relied on permission to exist, it would fail.

ehelms · 2017-03-29T13:42:05Z

@ares Could the require be at the bottom of the plugin definition? I feel like being able to organize your code the way ruby is designed to allow you shouldn't be classified as diverging from other plugins :) Code should just work. If it doesn't, then we will just accept this as is and let's file a bug.

ares · 2017-03-29T14:47:04Z

@ehelms I don't think that require helps. Let's assume this is how other plugin defines it.

Plugin.register do
  define_permission
  extend_roles_with_defined_permissions
end

Not perfect since extend_roles_with_defined_permissions needs to be run after define_permission and both needs to be run withing plugin registration block. But that's the way core currently allows to add permissions.

What Katello did before the patch was something like

Plugin.register do
  ...
end

# in after_initialize that gets run after the plugin is registered
require 'define_permissions_in_external_file'

and an external file was doing the definition like this

Foreman::Plugin.find(:katello).security_block :capsule_content do
 define_permission
end

The main difference is that it defined permissions outside of register block. Note that the register method ensures other stuff is run before/after the block. So far it worked for Katello but it there was another after initialize code in core that would rely on all permissions being defined, it would not find Katello permissions.

Now the change I'm proposing needs to run add_all_permissions_to_default_roles in register block. Therefore I moved the permission registration before it. I can't use require there, since the required file relies on the fact that the plugin is already registered but we'd require it from the middle of registration. As I said, it's not ideal to force plugin to define everything in the register block but that's how other plugins do it. So as a compromise I added a new class that contains the permission registration logic so it is in separate file. The diverging I was referring to is how Katello defined permissions before and how it does it in this PR. It's not a big deal but if there's a bug in the PermissionCreator class, it would fall on Katello devs to fix it. Therefore I'd recommend keep it as close as possible to other plugins.

houndci-bot · 2017-03-29T14:52:33Z

lib/katello/permission_creator.rb

+                       :resource_type => "SmartProxy"
+  end
+
+  def content_view_permissions


Method has too many lines. [78/30]

ares · 2017-04-06T07:21:28Z

Is there something I should explain better or I could do to get this moving?

ehelms · 2017-04-07T13:22:13Z

lib/katello/permission_creator.rb

@@ -0,0 +1,359 @@
+class PermissionCreator


Can we wrap this in module Katello for safety?

good idea 👍

ehelms · 2017-04-07T13:22:21Z

lib/katello/engine.rb

@@ -90,6 +90,7 @@ def find_assets(args = {})
    # hook so that the resumed Dynflow tasks can rely on everything ready.
    initializer 'katello.register_plugin', :before => :finisher_hook do
      require 'katello/plugin'
+      # extend buildin permissions from core with new actions


Minor s/buildin/builtin

ehelms · 2017-04-07T13:22:51Z

I think the compromise is a good one and appreciated @ares

Two minor comments

houndci-bot · 2017-04-07T13:41:01Z

lib/katello/permission_creator.rb

+                         },
+                         :resource_type => 'Katello::SyncPlan'
+    end
+


Trailing whitespace detected.

houndci-bot · 2017-04-07T13:41:01Z

lib/katello/permission_creator.rb

+                         },
+                         :resource_type => 'Katello::Subscription'
+    end
+


Trailing whitespace detected.

houndci-bot · 2017-04-07T13:41:01Z

lib/katello/permission_creator.rb

+                         },
+                         :resource_type => 'Katello::Product'
+    end
+


Trailing whitespace detected.

houndci-bot · 2017-04-07T13:41:01Z

lib/katello/permission_creator.rb

+                         {},
+                         :resource_type => 'Katello::KTEnvironment'
+    end
+


Trailing whitespace detected.

houndci-bot · 2017-04-07T13:41:01Z

lib/katello/permission_creator.rb

+                           'katello/api/v2/environments' => [:destroy]
+                         },
+                         :resource_type => 'Katello::KTEnvironment'
+


Trailing whitespace detected.

houndci-bot · 2017-04-07T13:41:02Z

lib/katello/permission_creator.rb

+                         },
+                         :resource_type => "SmartProxy"
+    end
+


Trailing whitespace detected.

houndci-bot · 2017-04-07T13:41:02Z

lib/katello/permission_creator.rb

+                           'katello/api/v2/capsules' => [:index, :show]
+                         },
+                         :resource_type => 'SmartProxy'
+


Trailing whitespace detected.

houndci-bot · 2017-04-07T13:41:02Z

lib/katello/permission_creator.rb

+                         },
+                         :resource_type => 'Katello::ActivationKey'
+    end
+


Trailing whitespace detected.

houndci-bot · 2017-04-07T13:41:02Z

lib/katello/permission_creator.rb

+      sync_plan_permissions
+      user_permissions
+    end
+


Trailing whitespace detected.

houndci-bot · 2017-04-07T13:41:02Z

lib/katello/permission_creator.rb

+    def initialize(plugin)
+      @plugin = plugin
+    end
+


Trailing whitespace detected.

ares · 2017-04-07T13:53:38Z

I updated the PR and squashed commits. Hopefully it will remain green :-)

ehelms

APJ

ares · 2017-04-07T16:03:58Z

🍏 :-)

jlsherrill · 2017-04-07T16:14:06Z

Thanks @ares !!!

theforeman-bot added Needs testing Not yet reviewed labels Mar 24, 2017

ares force-pushed the fix/17962 branch from 8b32c27 to 0809e8e Compare March 28, 2017 14:59

houndci-bot reviewed Mar 28, 2017

View reviewed changes

ares force-pushed the fix/17962 branch from 0809e8e to 1a8932d Compare March 28, 2017 15:05

ares force-pushed the fix/17962 branch from 1a8932d to 57c7c35 Compare March 29, 2017 14:52

houndci-bot reviewed Mar 29, 2017

View reviewed changes

ares force-pushed the fix/17962 branch 2 times, most recently from fe000ec to 614e891 Compare March 30, 2017 07:07

ehelms reviewed Apr 7, 2017

View reviewed changes

ares force-pushed the fix/17962 branch from 614e891 to c916f61 Compare April 7, 2017 13:40

houndci-bot reviewed Apr 7, 2017

View reviewed changes

ares force-pushed the fix/17962 branch from c916f61 to 2f63d3c Compare April 7, 2017 13:42

Fixes #17962 - extend core roles

2b2dc09

ares force-pushed the fix/17962 branch from 2f63d3c to 2b2dc09 Compare April 7, 2017 13:45

ehelms approved these changes Apr 7, 2017

View reviewed changes

jlsherrill merged commit ecebc22 into Katello:master Apr 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #17962 - extend core roles #6703

Fixes #17962 - extend core roles #6703

ares commented Mar 24, 2017

mention-bot commented Mar 24, 2017

ares commented Mar 27, 2017 •

edited

Loading

ares commented Mar 28, 2017

ehelms commented Mar 28, 2017

jlsherrill commented Mar 28, 2017

ares commented Mar 28, 2017

houndci-bot Mar 28, 2017

ares commented Mar 28, 2017

ehelms commented Mar 29, 2017

ares commented Mar 29, 2017

houndci-bot Mar 29, 2017

ares commented Apr 6, 2017

ehelms Apr 7, 2017

ares Apr 7, 2017

ehelms Apr 7, 2017

ehelms commented Apr 7, 2017

houndci-bot Apr 7, 2017

houndci-bot Apr 7, 2017

houndci-bot Apr 7, 2017

houndci-bot Apr 7, 2017

houndci-bot Apr 7, 2017

houndci-bot Apr 7, 2017

houndci-bot Apr 7, 2017

houndci-bot Apr 7, 2017

houndci-bot Apr 7, 2017

houndci-bot Apr 7, 2017

ares commented Apr 7, 2017

ehelms left a comment

ares commented Apr 7, 2017

jlsherrill commented Apr 7, 2017

Fixes #17962 - extend core roles #6703

Fixes #17962 - extend core roles #6703

Conversation

ares commented Mar 24, 2017

mention-bot commented Mar 24, 2017

ares commented Mar 27, 2017 • edited Loading

ares commented Mar 28, 2017

ehelms commented Mar 28, 2017

jlsherrill commented Mar 28, 2017

ares commented Mar 28, 2017

Choose a reason for hiding this comment

ares commented Mar 28, 2017

ehelms commented Mar 29, 2017

ares commented Mar 29, 2017

Choose a reason for hiding this comment

ares commented Apr 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares commented Apr 7, 2017

ehelms left a comment

Choose a reason for hiding this comment

ares commented Apr 7, 2017

jlsherrill commented Apr 7, 2017

ares commented Mar 27, 2017 •

edited

Loading