Opinions from plugin maintainers wanted: permissions and roles

Development
#1

Hi,
I recently started identifying problematic areas in Permissions and Roles,
especially with regard to plugins. Foreman provides 'Viewer' and 'Manager'
roles out of the box and users expect these roles to work for plugins as
well. But plugins generally do not add their permissions to core's roles,
some of them just have their own plugin-specific roles, some not. So if a
plugin is installed, user has to go and find what role/permission is
missing or ask someone who can grant permissions.

After a brief discussion in team C, it was decided that plugins should add
their permissions to 'Manager' role and their 'view_' permissions to
'Viewer' role. They should also keep their plugin-specific roles (or create
them) for higher granularity and backward compatibility.
I started initial work which should allow plugin maintainers to do this, it
can be found at [1]. I decided to create 2 methods that would be called
from Engine, but we could take a different approach and add permissions
from plugins automatically by modifying the 'permission' method that is
called from 'security_block'.

The way I see things:

  • adding permissions explicitly, using DSL
    • extra work for plugin maintainers
    • permissions can be ommited/forgotten

      can be covered with tests? something like the already existing

AccessPermissionsTest

  • extra code in Engine
  • better control, can handle special cases
  • adding permissions automatically without plugin knowing about it by
    modifying 'permission' method
    • no need for plugin maintainers to do anything
    • cannot handle special cases
    • if user removes permission from these default roles, there is no way to
      detect it and permissions get added again on server restart

      users do not modify default roles, they can clone it and modify the

cloned one
# if the default roles are not meant to be modified, they should be
'frozen' and user should not be able to modify them, implementing this
would add code and complexity

Anything that I have missed? Where do you stand as a plugin maintainers?
Can you think of other ways to go?

Ondra

[1] https://github.com/theforeman/foreman/pull/4168

#2

> Hi,
> I recently started identifying problematic areas in Permissions and Roles,
> especially with regard to plugins. Foreman provides 'Viewer' and 'Manager'
> roles out of the box and users expect these roles to work for plugins as
> well. But plugins generally do not add their permissions to core's roles,
> some of them just have their own plugin-specific roles, some not. So if a
> plugin is installed, user has to go and find what role/permission is missing
> or ask someone who can grant permissions.
>
> After a brief discussion in team C, it was decided that plugins should add
> their permissions to 'Manager' role and their 'view_' permissions to
> 'Viewer' role. They should also keep their plugin-specific roles (or create
> them) for higher granularity and backward compatibility.
> I started initial work which should allow plugin maintainers to do this, it
> can be found at [1]. I decided to create 2 methods that would be called from
> Engine, but we could take a different approach and add permissions from
> plugins automatically by modifying the 'permission' method that is called
> from 'security_block'.
>
> The way I see things:
>
> * adding permissions explicitly, using DSL
> - extra work for plugin maintainers
> - permissions can be ommited/forgotten
> # can be covered with tests? something like the already existing
> AccessPermissionsTest
> - extra code in Engine
> - better control, can handle special cases
>
> * adding permissions automatically without plugin knowing about it by
> modifying 'permission' method
> - no need for plugin maintainers to do anything
> - cannot handle special cases
> - if user removes permission from these default roles, there is no way to
> detect it and permissions get added again on server restart

I'm afraid this could be quite frustrating if you try to modify your
roles. I'm not entirely sure what part of permissions is stored in the
db and what is in the code, but would it be possible to add
permissions during seed and don't touch it on server start?

> # users do not modify default roles, they can clone it and modify the
> cloned one
> # if the default roles are not meant to be modified, they should be
> 'frozen' and user should not be able to modify them, implementing this would
> add code and complexity

Even though it's more work for plugin maintainers I prefer option A
because it gives you more control over what gets added.

BTW how would both approaches behave on migration of existing
installations? Would it add the permissions too?

··· On Mon, Jan 16, 2017 at 6:55 PM, oprazak wrote:

Anything that I have missed? Where do you stand as a plugin maintainers? Can
you think of other ways to go?

Ondra

[1] https://github.com/theforeman/foreman/pull/4168

–
You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

#3

I do not think the proposed approach is the best, here is my lengthy
explanation:

https://github.com/theforeman/foreman/pull/4168#issuecomment-273187422

··· On Mon, Jan 16, 2017 at 6:55 PM, oprazak wrote: > So if a plugin is installed, user has to go and find what role/permission is > missing or ask someone who can grant permissions.

–
Later,
Lukas @lzap Zapletal

#4

> > Hi,
> > I recently started identifying problematic areas in Permissions and
> Roles,
> > especially with regard to plugins. Foreman provides 'Viewer' and
> 'Manager'
> > roles out of the box and users expect these roles to work for plugins as
> > well. But plugins generally do not add their permissions to core's roles,
> > some of them just have their own plugin-specific roles, some not. So if a
> > plugin is installed, user has to go and find what role/permission is
> missing
> > or ask someone who can grant permissions.
> >
> > After a brief discussion in team C, it was decided that plugins should
> add
> > their permissions to 'Manager' role and their 'view_' permissions to
> > 'Viewer' role. They should also keep their plugin-specific roles (or
> create
> > them) for higher granularity and backward compatibility.
> > I started initial work which should allow plugin maintainers to do this,
> it
> > can be found at [1]. I decided to create 2 methods that would be called
> from
> > Engine, but we could take a different approach and add permissions from
> > plugins automatically by modifying the 'permission' method that is called
> > from 'security_block'.
> >
> > The way I see things:
> >
> > * adding permissions explicitly, using DSL
> > - extra work for plugin maintainers
> > - permissions can be ommited/forgotten
> > # can be covered with tests? something like the already existing
> > AccessPermissionsTest
> > - extra code in Engine
> > - better control, can handle special cases
> >
> > * adding permissions automatically without plugin knowing about it by
> > modifying 'permission' method
> > - no need for plugin maintainers to do anything
> > - cannot handle special cases
> > - if user removes permission from these default roles, there is no way
> to
> > detect it and permissions get added again on server restart
>
> I'm afraid this could be quite frustrating if you try to modify your
> roles. I'm not entirely sure what part of permissions is stored in the
> db and what is in the code, but would it be possible to add
> permissions during seed and don't touch it on server start?
>

All permissions are in db and plugins get loaded before migration runs,
so it should be possible I think. Seems much better (and less frustrating
for users) than resetting to initial state on each restart,
the downside is you will need a migration each time you introduce new
permissions.

>
> > # users do not modify default roles, they can clone it and modify the
> > cloned one
> > # if the default roles are not meant to be modified, they should be
> > 'frozen' and user should not be able to modify them, implementing this
> would
> > add code and complexity
>
> Even though it's more work for plugin maintainers I prefer option A
> because it gives you more control over what gets added.
>
> BTW how would both approaches behave on migration of existing
> installations? Would it add the permissions too?
>

Yes, both would add the permissions, forgot to mention that they are
added on each restart for the case A as well :frowning:
Maybe we could add some sort of 'dirty' flag on the role, as in: when
modified by user, do not touch this role. But then it would not add
permissions when plugins were installed after role was modified. Tricky.

··· On Tue, Jan 17, 2017 at 12:08 PM, Tomas Strachota wrote: > On Mon, Jan 16, 2017 at 6:55 PM, oprazak wrote:

Anything that I have missed? Where do you stand as a plugin maintainers?
Can
you think of other ways to go?

Ondra

[1] https://github.com/theforeman/foreman/pull/4168

–
You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

–
You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

#5

Let's continue the discussion here since it might read more people. I think
that as a user I don't care that my installation consists of core and several
plugins and I want to have Viewer role that gathers all view permission for
the whole app.

This does not in conflict with also providing "$plugin Viewer" and "$plugin
Manager" roles so if user wants to create a user group from subset of
permission he can still do it.

If you want to keep current Viewer and Manager roles to contain only core
permissions then I'd suggest renaming them to Core Viewer and Core Manager

··· On úterý 17. ledna 2017 15:46:56 CET Lukas Zapletal wrote: > On Mon, Jan 16, 2017 at 6:55 PM, oprazak wrote: > > So if a plugin is installed, user has to go and find what role/permission > > is missing or ask someone who can grant permissions. > > I do not think the proposed approach is the best, here is my lengthy > explanation: > > https://github.com/theforeman/foreman/pull/4168#issuecomment-273187422

–
Marek

#6

> Let's continue the discussion here since it might read more people. I think
> that as a user I don't care that my installation consists of core and several
> plugins and I want to have Viewer role that gathers all view permission for
> the whole app.
>
> This does not in conflict with also providing "$plugin Viewer" and "$plugin
> Manager" roles so if user wants to create a user group from subset of
> permission he can still do it.
>
> If you want to keep current Viewer and Manager roles to contain only core
> permissions then I'd suggest renaming them to Core Viewer and Core Manager

The price for that is too high in my eyes. I think these roles and
permissions should be strictly separated for now and forever and we
need to come up with different approach of handling that. What I like
the best is a help text, better documentation and renaming the core
roles to something that is more obvious.

Allowing plugins to modify core roles will end up with a mess that is
very difficult to clean! Both adding and deleting permissions for
existing roles during upgrades is very challenging, we usually want to
tell administrators "hey, danger ahead, during upgrade all these users
will get/lost some permissions" so it's a dilemma to do this via
migration/seed or explicitly ask the user to do the change in upgrade
notes. When reviewing these changes, we need to be careful and we are
doing great job in core, but I wonder what happens if we open the
doors to any plugin to basically play around with the two most
important roles in the application.

··· -- Later, Lukas @lzap Zapletal
#7

> > > Hi,
> > > I recently started identifying problematic areas in Permissions and
> >
> > Roles,
> >
> > > especially with regard to plugins. Foreman provides 'Viewer' and
> >
> > 'Manager'
> >
> > > roles out of the box and users expect these roles to work for plugins as
> > > well. But plugins generally do not add their permissions to core's
> > > roles,
> > > some of them just have their own plugin-specific roles, some not. So if
> > > a
> > > plugin is installed, user has to go and find what role/permission is
> >
> > missing
> >
> > > or ask someone who can grant permissions.
> > >
> > > After a brief discussion in team C, it was decided that plugins should
> >
> > add
> >
> > > their permissions to 'Manager' role and their 'view_' permissions to
> > > 'Viewer' role. They should also keep their plugin-specific roles (or
> >
> > create
> >
> > > them) for higher granularity and backward compatibility.
> > > I started initial work which should allow plugin maintainers to do this,
> >
> > it
> >
> > > can be found at [1]. I decided to create 2 methods that would be called
> >
> > from
> >
> > > Engine, but we could take a different approach and add permissions from
> > > plugins automatically by modifying the 'permission' method that is
> > > called
> > > from 'security_block'.
> > >
> > > The way I see things:
> > >
> > > * adding permissions explicitly, using DSL
> > >
> > > - extra work for plugin maintainers
> > > - permissions can be ommited/forgotten
> > >
> > > # can be covered with tests? something like the already existing
> > >
> > > AccessPermissionsTest
> > >
> > > - extra code in Engine
> > > - better control, can handle special cases
> > >
> > > * adding permissions automatically without plugin knowing about it by
> > > modifying 'permission' method
> > >
> > > - no need for plugin maintainers to do anything
> > > - cannot handle special cases
> > > - if user removes permission from these default roles, there is no way
> >
> > to
> >
> > > detect it and permissions get added again on server restart
> >
> > I'm afraid this could be quite frustrating if you try to modify your
> > roles. I'm not entirely sure what part of permissions is stored in the
> > db and what is in the code, but would it be possible to add
> > permissions during seed and don't touch it on server start?
>
> All permissions are in db and plugins get loaded before migration runs,
> so it should be possible I think. Seems much better (and less frustrating
> for users) than resetting to initial state on each restart,
> the downside is you will need a migration each time you introduce new
> permissions.

Or use the flag the same way as you describe below so it would happen only
once, basically when you install the plugin?

>
> > > # users do not modify default roles, they can clone it and modify
> > > the
> > >
> > > cloned one
> > >
> > > # if the default roles are not meant to be modified, they should be
> > >
> > > 'frozen' and user should not be able to modify them, implementing this
> >
> > would
> >
> > > add code and complexity
> >
> > Even though it's more work for plugin maintainers I prefer option A
> > because it gives you more control over what gets added.
> >
> > BTW how would both approaches behave on migration of existing
> > installations? Would it add the permissions too?
>
> Yes, both would add the permissions, forgot to mention that they are
> added on each restart for the case A as well :frowning:
> Maybe we could add some sort of 'dirty' flag on the role, as in: when
> modified by user, do not touch this role. But then it would not add
> permissions when plugins were installed after role was modified. Tricky.

The flag would have to be on the permission - role association I guess and it
would have to persist even after it's removal. For other resources in seeds we
abuse audits for this, if audit exists, we don't touch the resource.

··· On úterý 17. ledna 2017 14:45:53 CET Ondrej Prazak wrote: > On Tue, Jan 17, 2017 at 12:08 PM, Tomas Strachota > > wrote: > > On Mon, Jan 16, 2017 at 6:55 PM, oprazak wrote:

Anything that I have missed? Where do you stand as a plugin maintainers?

Can

you think of other ways to go?

Ondra

[1] https://github.com/theforeman/foreman/pull/4168

–
You received this message because you are subscribed to the Google
Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send
an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

–
You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

#8

Sorry I don't get it, especially the price. What's the difference between core
and plugins in terms of permissions upgrades? If you rename plugin permission
you need to provide the same migration as if you renamed it in core.

Currently we don't support plugin uninstallation, when we do we'll likely have
a tool to say which permission should be removed.

The only other option I see to make this usable is to precreate user group
that plugins would assign their roles to. But I'd say that's the same thing
just in different model. The user reports (see the mirroring BZ [1]) and
especially the way they were reported convinces me we need to change the
current status. If users open issues because they can't see a button even when
they have role "viewers" then I think it's our fault.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1304608

··· On čtvrtek 19. ledna 2017 8:44:59 CET Lukas Zapletal wrote: > > Let's continue the discussion here since it might read more people. I > > think > > that as a user I don't care that my installation consists of core and > > several plugins and I want to have Viewer role that gathers all view > > permission for the whole app. > > > > This does not in conflict with also providing "$plugin Viewer" and > > "$plugin > > Manager" roles so if user wants to create a user group from subset of > > permission he can still do it. > > > > If you want to keep current Viewer and Manager roles to contain only core > > permissions then I'd suggest renaming them to Core Viewer and Core Manager > > The price for that is too high in my eyes. I think these roles and > permissions should be strictly separated for now and forever and we > need to come up with different approach of handling that. What I like > the best is a help text, better documentation and renaming the core > roles to something that is more obvious. > > Allowing plugins to modify core roles will end up with a mess that is > very difficult to clean! Both adding and deleting permissions for > existing roles during upgrades is very challenging, we usually want to > tell administrators "hey, danger ahead, during upgrade all these users > will get/lost some permissions" so it's a dilemma to do this via > migration/seed or explicitly ask the user to do the change in upgrade > notes. When reviewing these changes, we need to be careful and we are > doing great job in core, but I wonder what happens if we open the > doors to any plugin to basically play around with the two most > important roles in the application.

–
Marek

#9

> Sorry I don't get it, especially the price. What's the difference between core
> and plugins in terms of permissions upgrades? If you rename plugin permission
> you need to provide the same migration as if you renamed it in core.

Once we merge everything into core roles, there is no easy way back. I
described above why I think a pull-mechanism (with separated roles) is
better that the proposal.

> Currently we don't support plugin uninstallation, when we do we'll likely have
> a tool to say which permission should be removed.

We had a need of removing a permission for existing plugin, this had
nothing to do with uninstallation. We added it by mistake basically.
But what happens if we decide to implement plugin uninstallation? The
proposal only makes it more tough.

> The only other option I see to make this usable is to precreate user group
> that plugins would assign their roles to. But I'd say that's the same thing
> just in different model. The user reports (see the mirroring BZ [1]) and
> especially the way they were reported convinces me we need to change the
> current status. If users open issues because they can't see a button even when
> they have role "viewers" then I think it's our fault.

I think we need to improve how we report missing permissions overall,
that it will not be such a pain in finding particular role or
permissions and we can keep it separate.

> [1] https://bugzilla.redhat.com/show_bug.cgi?id=1304608

I really think we need to say "this is by design, add required roles
which are provided by plugins" here. Not all RFEs are valid.

Anyway, summary of my opinion:

  • I am not against adding an API call to add permissions to roles in general.
  • I'd prefer pull mechanism rather than push.
  • I do not like particularly modifications of core roles, I think we
    should keep it separate and more focus on improving usability,
    reporting and documentation around.
  • I described my counter-proposal in the PR comment from this morning.

There is one alternative proposal that comes to my mind - what if we
start tracking what created permissions. It could be core permission
installed during seed, plugin permission created by plugin API or
user-defined permission. This could help with permission deletion (if
it is not user-defined it will be less likely intrusive change to
delete it), it can possibly help with plugin uninstallation in the
future, it can improve debugging (ability to compare current state
with expected permission list). But I still think a pull mechanism API
would be better (that separates core and plugin permissions clearly
from user-defined).

Please do not understand this as hard-blocking your PR, I am trying to
write constructive notes on how to take a different approach. If you
think what is proposed in the PR is better, then just go ahead and I
will simply put my hands off from the review. I would love to hear
other opinions on that topic, I am surprised it's just us three/four
discussing this as everyone is using permissions and roles as
developers or as users.

··· -- Later, Lukas @lzap Zapletal
#10

Thanks for update, sending few more comments below in text

> > Sorry I don't get it, especially the price. What's the difference between
> > core and plugins in terms of permissions upgrades? If you rename plugin
> > permission you need to provide the same migration as if you renamed it in
> > core.
> Once we merge everything into core roles, there is no easy way back. I
> described above why I think a pull-mechanism (with separated roles) is
> better that the proposal.

If you're concerned about the permissions table, this does not help.
Permission are created there by plugin. If the permission is removed later, it
should be removed from all roles anyway, user could already assign it to both
core and plugin role.

> > Currently we don't support plugin uninstallation, when we do we'll likely
> > have a tool to say which permission should be removed.
>
> We had a need of removing a permission for existing plugin, this had
> nothing to do with uninstallation. We added it by mistake basically.
> But what happens if we decide to implement plugin uninstallation? The
> proposal only makes it more tough.

I suppose you wrote a migration that unassigned this permission from all
filters and removed the permission from permissions table. Why does this not
work for core?

> > The only other option I see to make this usable is to precreate user group
> > that plugins would assign their roles to. But I'd say that's the same
> > thing
> > just in different model. The user reports (see the mirroring BZ [1]) and
> > especially the way they were reported convinces me we need to change the
> > current status. If users open issues because they can't see a button even
> > when they have role "viewers" then I think it's our fault.
>
> I think we need to improve how we report missing permissions overall,
> that it will not be such a pain in finding particular role or
> permissions and we can keep it separate.
>
> > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1304608
>
> I really think we need to say "this is by design, add required roles
> which are provided by plugins" here. Not all RFEs are valid.

IMHO this is usability issue. And therefore valid RFE.

> Anyway, summary of my opinion:
>
> * I am not against adding an API call to add permissions to roles in
> general.
> * I'd prefer pull mechanism rather than push.
> * I do not like particularly modifications of core roles, I think we
> should keep it separate and more focus on improving usability,
> reporting and documentation around.
> * I described my counter-proposal in the PR comment from this morning.

I read your counter-proposal (I don't think it's "counter" :slight_smile: If I understand
it correctly, we could break i down into following steps.

  1. add DSL to extend core roles
  2. make core roles locked for editing
  3. add DSL to remove permissions from roles

This thread started by discussion on automatically adding plugin permissions
to core roles. So I guess that could only happen if we had 1-3. I'm fine with
skipping automatically extending core roles until we're ready.

> > There is one alternative proposal that comes to my mind - what if we
> start tracking what created permissions. It could be core permission
> installed during seed, plugin permission created by plugin API or
> user-defined permission. This could help with permission deletion (if
> it is not user-defined it will be less likely intrusive change to
> delete it), it can possibly help with plugin uninstallation in the
> future, it can improve debugging (ability to compare current state
> with expected permission list). But I still think a pull mechanism API
> would be better (that separates core and plugin permissions clearly
> from user-defined).

I find the locking better since it's similar concept to templates where we
have the same problem. But I like the proposal in general for future plugin
uninstallation. If audit can't give us the answer we should start tracking it,
ideally the same way for all resources created by plugin.

> Please do not understand this as hard-blocking your PR, I am trying to
> write constructive notes on how to take a different approach. If you
> think what is proposed in the PR is better, then just go ahead and I
> will simply put my hands off from the review. I would love to hear
> other opinions on that topic, I am surprised it's just us three/four
> discussing this as everyone is using permissions and roles as
> developers or as users.

+1

··· On pondělí 23. ledna 2017 12:48:46 CET Lukas Zapletal wrote:

–
Marek

#11

> If you're concerned about the permissions table, this does not help.
> Permission are created there by plugin. If the permission is removed later, it
> should be removed from all roles anyway, user could already assign it to both
> core and plugin role.

Down below.

> I suppose you wrote a migration that unassigned this permission from all
> filters and removed the permission from permissions table. Why does this not
> work for core?

I agree that technically no difference, but ability to have a list of
default roles and permissions (what I call "pull mechanism" but
basically its a list or hash of these) allows us to audit this. It's
something we already have today (hashes for core is in seeds, hashes
for plugins can be added via the existing API hash mentioned). I
simply think that we lost track of this when we will be creating new
permissions for existing roles via this call (usually in engine.rb).

>> I really think we need to say "this is by design, add required roles
>> which are provided by plugins" here. Not all RFEs are valid.
>
> IMHO this is usability issue. And therefore valid RFE.

Sure, I made it to extreme, but I am trying to put the requirement
from the RFE on weights together with above.

> I read your counter-proposal (I don't think it's "counter" :slight_smile: If I understand
> it correctly, we could break i down into following steps.
> 1) add DSL to extend core roles
> 2) make core roles locked for editing

Locking is indeed nice, but it does not solve my concerns above - lost
track. But I like it, let me build on top of that: what if the API was
adding the permission twice - once into the core (locked) role and
once into "PluginName Manager/Reader" (locked) role as a copy. So
users could decide what to include for their Users or UserGroups. This
could keep some flexibility and also enable better audit. The API
would be responsible for copying them so plugin authors would not
care.

> 3) add DSL to remove permissions from roles

I don't insist on this one, removing permissions can be done via
migrations as you noted. This only makes sense if it's actually better
to have such an API method(s). It might end up in situation when
migration would have been much more easier and cleaner - then this is
a no go of course.

> I find the locking better since it's similar concept to templates where we
> have the same problem. But I like the proposal in general for future plugin
> uninstallation. If audit can't give us the answer we should start tracking it,
> ideally the same way for all resources created by plugin.

Sure, locking is a good start, my biggest concern was to have a role
(e.g. Manager) modified by core, plugins, users. Three individual
subjects, locking should help here.

If locking is the thing we agreed here, I vote for implementing it
with the new API so we are clean from the day one. Technically we can
make the upgrade easier if we decide to create new locked roles with
names like "Default Manager" and "Default Reader" (or Viewer not sure)
keeping the original "Manager" and "Reader" unchanged. If we also
implement a comparison tool, that is huge improvement in user
experience IMHO.

··· -- Later, Lukas @lzap Zapletal
#12

Lukas Zapletal <lzap@redhat.com> writes:

>> If you're concerned about the permissions table, this does not help.
>> Permission are created there by plugin. If the permission is removed later, it
>> should be removed from all roles anyway, user could already assign it to both
>> core and plugin role.
>
> Down below.
>
>> I suppose you wrote a migration that unassigned this permission from all
>> filters and removed the permission from permissions table. Why does this not
>> work for core?
>
> I agree that technically no difference, but ability to have a list of
> default roles and permissions (what I call "pull mechanism" but
> basically its a list or hash of these) allows us to audit this. It's
> something we already have today (hashes for core is in seeds, hashes
> for plugins can be added via the existing API hash mentioned). I
> simply think that we lost track of this when we will be creating new
> permissions for existing roles via this call (usually in engine.rb).
>
>>> I really think we need to say "this is by design, add required roles
>>> which are provided by plugins" here. Not all RFEs are valid.
>>
>> IMHO this is usability issue. And therefore valid RFE.
>
> Sure, I made it to extreme, but I am trying to put the requirement
> from the RFE on weights together with above.
>
>> I read your counter-proposal (I don't think it's "counter" :slight_smile: If I understand
>> it correctly, we could break i down into following steps.
>> 1) add DSL to extend core roles
>> 2) make core roles locked for editing
>
> Locking is indeed nice, but it does not solve my concerns above - lost
> track. But I like it, let me build on top of that: what if the API was
> adding the permission twice - once into the core (locked) role and
> once into "PluginName Manager/Reader" (locked) role as a copy. So
> users could decide what to include for their Users or UserGroups. This
> could keep some flexibility and also enable better audit. The API
> would be responsible for copying them so plugin authors would not
> care.
>
>> 3) add DSL to remove permissions from roles
>
> I don't insist on this one, removing permissions can be done via
> migrations as you noted. This only makes sense if it's actually better
> to have such an API method(s). It might end up in situation when
> migration would have been much more easier and cleaner - then this is
> a no go of course.
>
>> I find the locking better since it's similar concept to templates where we
>> have the same problem. But I like the proposal in general for future plugin
>> uninstallation. If audit can't give us the answer we should start tracking it,
>> ideally the same way for all resources created by plugin.
>
> Sure, locking is a good start, my biggest concern was to have a role
> (e.g. Manager) modified by core, plugins, users. Three individual
> subjects, locking should help here.
>
> If locking is the thing we agreed here, I vote for implementing it
> with the new API so we are clean from the day one. Technically we can
> make the upgrade easier if we decide to create new locked roles with
> names like "Default Manager" and "Default Reader" (or Viewer not sure)
> keeping the original "Manager" and "Reader" unchanged. If we also
> implement a comparison tool, that is huge improvement in user
> experience IMHO.

Let me offer a bit different view on this. When looking at the
permissions from user perspective, they are very granular. This is great
when you need to achieve something really specific, but most of the
time, viewer (read only access), user (use the feature) and manager
(configure the feature) could work for 80% of users that need something
more than just admin.

Therefore, I wonder if we should not start thinking about grouping
permissions based on features they are related to and scope of power,
and then let the roles to be build using this metadata.

Example: provisioning permission group would contain Host, Domain, Compute
Resource, Image… permissions. Orthogonal to the feature, the permissions
would also have a flag, if they are meant for viewer, user or manager
(the same decision you make if you put the permission in core/plugin
viewer/manager role)

In a role, instead of putting the individual permissions there directly,
I would say, using this role, I want to be able to use provisioning, so
including the viewer and user permission from provision group. The
permission groups would be controlled by the code (not users). What
users could do, when including a specific permissions group, would be to
say if they want opt-in/opt-out specific permission. I expect, most of
the time people would trust the developers to put the right permissions
into the features and scopes.

The similar way, I could add OpenSCAP permission, rex permissions,
Puppet permissions… And the default manager/viewer roles would have by
default corresponding groups from all features.

I know it's a bit more work, but it would help not only the people that
are happy with defaults, but it would help the whole permissions model.

– Ivan

··· > > -- > Later, > Lukas @lzap Zapletal > > -- > You received this message because you are subscribed to the Google Groups "foreman-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
#13

> > If you're concerned about the permissions table, this does not help.
> > Permission are created there by plugin. If the permission is removed
> > later, it should be removed from all roles anyway, user could already
> > assign it to both core and plugin role.
>
> Down below.
>
> > I suppose you wrote a migration that unassigned this permission from all
> > filters and removed the permission from permissions table. Why does this
> > not work for core?
>
> I agree that technically no difference, but ability to have a list of
> default roles and permissions (what I call "pull mechanism" but
> basically its a list or hash of these) allows us to audit this. It's
> something we already have today (hashes for core is in seeds, hashes
> for plugins can be added via the existing API hash mentioned). I
> simply think that we lost track of this when we will be creating new
> permissions for existing roles via this call (usually in engine.rb).
>
> >> I really think we need to say "this is by design, add required roles
> >> which are provided by plugins" here. Not all RFEs are valid.
> >
> > IMHO this is usability issue. And therefore valid RFE.
>
> Sure, I made it to extreme, but I am trying to put the requirement
> from the RFE on weights together with above.
>
> > I read your counter-proposal (I don't think it's "counter" :slight_smile: If I
> > understand it correctly, we could break i down into following steps.
> > 1) add DSL to extend core roles
> > 2) make core roles locked for editing
>
> Locking is indeed nice, but it does not solve my concerns above - lost
> track. But I like it, let me build on top of that: what if the API was
> adding the permission twice - once into the core (locked) role and
> once into "PluginName Manager/Reader" (locked) role as a copy. So
> users could decide what to include for their Users or UserGroups. This
> could keep some flexibility and also enable better audit. The API
> would be responsible for copying them so plugin authors would not
> care.

Yup, that was the plan from the very beginning, just without locking. Locking
can be added and everyone is happy.

>
> > 3) add DSL to remove permissions from roles
>
> I don't insist on this one, removing permissions can be done via
> migrations as you noted. This only makes sense if it's actually better
> to have such an API method(s). It might end up in situation when
> migration would have been much more easier and cleaner - then this is
> a no go of course.
>
> > I find the locking better since it's similar concept to templates where we
> > have the same problem. But I like the proposal in general for future
> > plugin
> > uninstallation. If audit can't give us the answer we should start tracking
> > it, ideally the same way for all resources created by plugin.
>
> Sure, locking is a good start, my biggest concern was to have a role
> (e.g. Manager) modified by core, plugins, users. Three individual
> subjects, locking should help here.
>
> If locking is the thing we agreed here, I vote for implementing it
> with the new API so we are clean from the day one. Technically we can
> make the upgrade easier if we decide to create new locked roles with
> names like "Default Manager" and "Default Reader" (or Viewer not sure)
> keeping the original "Manager" and "Reader" unchanged. If we also
> implement a comparison tool, that is huge improvement in user
> experience IMHO.

Ok, we discussed the migration path. We should be (with small changes*) able
to detect whether users modified existing Manager role. In case they did, we
would rename them to "Customized Manager" and create new Manager role with
default permissions but locked. Locked means no changes to filters and
attributes can be made by user, plugin can extend via plugin DSL. If existing
Manager role was not changed we just lock it. The same for the Viewer. No
locking/unlocking actions as we have for templates. If this is acceptable
migration path, then I think we can start working on it.

Ivan had also a good point with task oriented roles. Once we have locking we
can add more locked roles like "Provisioning capabilities". And potentially
revisit automatic adding permissions to locked Manager and Viewer roles so
they won't get out of sync.

Sounds as a plan?

*small changes means we extract hashes of permissions in seeds to some
constant so it's available without running seeds and extending existing plugin
DSL method "role" so we know whether plugin extends existing Manager/Viewer
role or user did it manually.

··· On pondělí 23. ledna 2017 17:07:30 CET Lukas Zapletal wrote:

–
Marek

#14

Thanks for another idea. I wonder whether it functionality wise differs from
locked roles and using user groups to group such roles. Honestly I don't think
introducing one more entity to the whole stack would help. Also while we talk
only about roles - permissions relation it's not always that simple. Note that
each permission is always assigned through a filter which can define condition
and taxonomy restriction. So when permission group would be assigned to role,
we'd need to clone all permissions and let user to configure filters? And in
such case how would we keep it in sync with later permission group changes?

··· On úterý 24. ledna 2017 0:02:15 CET Ivan Necas wrote: > Lukas Zapletal writes: > >> If you're concerned about the permissions table, this does not help. > >> Permission are created there by plugin. If the permission is removed > >> later, it should be removed from all roles anyway, user could already > >> assign it to both core and plugin role. > > > > Down below. > > > >> I suppose you wrote a migration that unassigned this permission from all > >> filters and removed the permission from permissions table. Why does this > >> not work for core? > > > > I agree that technically no difference, but ability to have a list of > > default roles and permissions (what I call "pull mechanism" but > > basically its a list or hash of these) allows us to audit this. It's > > something we already have today (hashes for core is in seeds, hashes > > for plugins can be added via the existing API hash mentioned). I > > simply think that we lost track of this when we will be creating new > > permissions for existing roles via this call (usually in engine.rb). > > > >>> I really think we need to say "this is by design, add required roles > >>> which are provided by plugins" here. Not all RFEs are valid. > >> > >> IMHO this is usability issue. And therefore valid RFE. > > > > Sure, I made it to extreme, but I am trying to put the requirement > > from the RFE on weights together with above. > > > >> I read your counter-proposal (I don't think it's "counter" :-) If I > >> understand it correctly, we could break i down into following steps. > >> 1) add DSL to extend core roles > >> 2) make core roles locked for editing > > > > Locking is indeed nice, but it does not solve my concerns above - lost > > track. But I like it, let me build on top of that: what if the API was > > adding the permission twice - once into the core (locked) role and > > once into "PluginName Manager/Reader" (locked) role as a copy. So > > users could decide what to include for their Users or UserGroups. This > > could keep some flexibility and also enable better audit. The API > > would be responsible for copying them so plugin authors would not > > care. > > > >> 3) add DSL to remove permissions from roles > > > > I don't insist on this one, removing permissions can be done via > > migrations as you noted. This only makes sense if it's actually better > > to have such an API method(s). It might end up in situation when > > migration would have been much more easier and cleaner - then this is > > a no go of course. > > > >> I find the locking better since it's similar concept to templates where > >> we > >> have the same problem. But I like the proposal in general for future > >> plugin > >> uninstallation. If audit can't give us the answer we should start > >> tracking it, ideally the same way for all resources created by plugin. > > > > Sure, locking is a good start, my biggest concern was to have a role > > (e.g. Manager) modified by core, plugins, users. Three individual > > subjects, locking should help here. > > > > If locking is the thing we agreed here, I vote for implementing it > > with the new API so we are clean from the day one. Technically we can > > make the upgrade easier if we decide to create new locked roles with > > names like "Default Manager" and "Default Reader" (or Viewer not sure) > > keeping the original "Manager" and "Reader" unchanged. If we also > > implement a comparison tool, that is huge improvement in user > > experience IMHO. > > Let me offer a bit different view on this. When looking at the > permissions from user perspective, they are very granular. This is great > when you need to achieve something really specific, but most of the > time, viewer (read only access), user (use the feature) and manager > (configure the feature) could work for 80% of users that need something > more than just admin. > > Therefore, I wonder if we should not start thinking about grouping > permissions based on features they are related to and scope of power, > and then let the roles to be build using this metadata. > > Example: provisioning permission group would contain Host, Domain, Compute > Resource, Image… permissions. Orthogonal to the feature, the permissions > would also have a flag, if they are meant for viewer, user or manager > (the same decision you make if you put the permission in core/plugin > viewer/manager role) > > In a role, instead of putting the individual permissions there directly, > I would say, using this role, I want to be able to use provisioning, so > including the viewer and user permission from provision group. The > permission groups would be controlled by the code (not users). What > users could do, when including a specific permissions group, would be to > say if they want opt-in/opt-out specific permission. I expect, most of > the time people would trust the developers to put the right permissions > into the features and scopes. > > The similar way, I could add OpenSCAP permission, rex permissions, > Puppet permissions… And the default manager/viewer roles would have by > default corresponding groups from all features. > > I know it's a bit more work, but it would help not only the people that > are happy with defaults, but it would help the whole permissions model.

–
Marek

– Ivan

#15

> Ok, we discussed the migration path. We should be (with small changes*) able
> to detect whether users modified existing Manager role. In case they did, we
> would rename them to "Customized Manager" and create new Manager role with
> default permissions but locked. Locked means no changes to filters and
> attributes can be made by user, plugin can extend via plugin DSL. If existing
> Manager role was not changed we just lock it. The same for the Viewer. No
> locking/unlocking actions as we have for templates. If this is acceptable
> migration path, then I think we can start working on it.

Why you don't like explicit lock actions? What you described sounds
little bit complicated and confusing, we will see questions soon like
"why I can't edit my role"? I can foresee bugs when the stack locks a
role for some reason and user is stuck unable to edit a role. Explicit
locking/cloning seems more clear to me.

> Ivan had also a good point with task oriented roles. Once we have locking we
> can add more locked roles like "Provisioning capabilities". And potentially
> revisit automatic adding permissions to locked Manager and Viewer roles so
> they won't get out of sync.

Yeah, note that this is actually the exact opposite of what the patch
is trying to do (put everything into two messy roles). Why not to do
this from the day one? We could actually prevent plugins via the API
to change the Manager/Reader roles any only expose these
"fine-grained" roles, the stack would automatically add every
permission also into Manager/Reader. Let me suggest such an API:

add_permission_to_provisioning_manager (also adds to "manager" role)
add_permission_to_provisioning_reader (also adds to "reader" role)
add_permission_to_provisioning_manager (also adds to "manager" role)
add_permission_to_provisioning_reader (also adds to "reader" role)
…
This will not exist:
add_permission_to_manager
add_permission_to_reader

To put this one step furtner, plugins could register their own roles:

register_plugin_role("Discovery") (creates locked "Default Discovery
Manager/Reader" roles and also creates DSL methods
add_permission_to_discovery_manager/reader methods)

This will actually force plugin users to put permissions into these
sub-roles. We could refactor also core permissions in this way
providing similar API when seeding the core roles. Both main roles and
subroles will be explicitly locked by default and users are requested
to clone before modifying them, together with comparison tool we have
a very good user experience.

> *small changes means we extract hashes of permissions in seeds to some
> constant so it's available without running seeds and extending existing plugin
> DSL method "role" so we know whether plugin extends existing Manager/Viewer
> role or user did it manually.

Yeah, this could be even a DSL that is very same as plugins would use.

··· -- Later, Lukas @lzap Zapletal
#16

Marek Hulán <mhulan@redhat.com> writes:

>> Lukas Zapletal <lzap@redhat.com> writes:
>> >> If you're concerned about the permissions table, this does not help.
>> >> Permission are created there by plugin. If the permission is removed
>> >> later, it should be removed from all roles anyway, user could already
>> >> assign it to both core and plugin role.
>> >
>> > Down below.
>> >
>> >> I suppose you wrote a migration that unassigned this permission from all
>> >> filters and removed the permission from permissions table. Why does this
>> >> not work for core?
>> >
>> > I agree that technically no difference, but ability to have a list of
>> > default roles and permissions (what I call "pull mechanism" but
>> > basically its a list or hash of these) allows us to audit this. It's
>> > something we already have today (hashes for core is in seeds, hashes
>> > for plugins can be added via the existing API hash mentioned). I
>> > simply think that we lost track of this when we will be creating new
>> > permissions for existing roles via this call (usually in engine.rb).
>> >
>> >>> I really think we need to say "this is by design, add required roles
>> >>> which are provided by plugins" here. Not all RFEs are valid.
>> >>
>> >> IMHO this is usability issue. And therefore valid RFE.
>> >
>> > Sure, I made it to extreme, but I am trying to put the requirement
>> > from the RFE on weights together with above.
>> >
>> >> I read your counter-proposal (I don't think it's "counter" :slight_smile: If I
>> >> understand it correctly, we could break i down into following steps.
>> >> 1) add DSL to extend core roles
>> >> 2) make core roles locked for editing
>> >
>> > Locking is indeed nice, but it does not solve my concerns above - lost
>> > track. But I like it, let me build on top of that: what if the API was
>> > adding the permission twice - once into the core (locked) role and
>> > once into "PluginName Manager/Reader" (locked) role as a copy. So
>> > users could decide what to include for their Users or UserGroups. This
>> > could keep some flexibility and also enable better audit. The API
>> > would be responsible for copying them so plugin authors would not
>> > care.
>> >
>> >> 3) add DSL to remove permissions from roles
>> >
>> > I don't insist on this one, removing permissions can be done via
>> > migrations as you noted. This only makes sense if it's actually better
>> > to have such an API method(s). It might end up in situation when
>> > migration would have been much more easier and cleaner - then this is
>> > a no go of course.
>> >
>> >> I find the locking better since it's similar concept to templates where
>> >> we
>> >> have the same problem. But I like the proposal in general for future
>> >> plugin
>> >> uninstallation. If audit can't give us the answer we should start
>> >> tracking it, ideally the same way for all resources created by plugin.
>> >
>> > Sure, locking is a good start, my biggest concern was to have a role
>> > (e.g. Manager) modified by core, plugins, users. Three individual
>> > subjects, locking should help here.
>> >
>> > If locking is the thing we agreed here, I vote for implementing it
>> > with the new API so we are clean from the day one. Technically we can
>> > make the upgrade easier if we decide to create new locked roles with
>> > names like "Default Manager" and "Default Reader" (or Viewer not sure)
>> > keeping the original "Manager" and "Reader" unchanged. If we also
>> > implement a comparison tool, that is huge improvement in user
>> > experience IMHO.
>>
>> Let me offer a bit different view on this. When looking at the
>> permissions from user perspective, they are very granular. This is great
>> when you need to achieve something really specific, but most of the
>> time, viewer (read only access), user (use the feature) and manager
>> (configure the feature) could work for 80% of users that need something
>> more than just admin.
>>
>> Therefore, I wonder if we should not start thinking about grouping
>> permissions based on features they are related to and scope of power,
>> and then let the roles to be build using this metadata.
>>
>> Example: provisioning permission group would contain Host, Domain, Compute
>> Resource, Image… permissions. Orthogonal to the feature, the permissions
>> would also have a flag, if they are meant for viewer, user or manager
>> (the same decision you make if you put the permission in core/plugin
>> viewer/manager role)
>>
>> In a role, instead of putting the individual permissions there directly,
>> I would say, using this role, I want to be able to use provisioning, so
>> including the viewer and user permission from provision group. The
>> permission groups would be controlled by the code (not users). What
>> users could do, when including a specific permissions group, would be to
>> say if they want opt-in/opt-out specific permission. I expect, most of
>> the time people would trust the developers to put the right permissions
>> into the features and scopes.
>>
>> The similar way, I could add OpenSCAP permission, rex permissions,
>> Puppet permissions… And the default manager/viewer roles would have by
>> default corresponding groups from all features.
>>
>> I know it's a bit more work, but it would help not only the people that
>> are happy with defaults, but it would help the whole permissions model.
>
> Thanks for another idea. I wonder whether it functionality wise differs from
> locked roles and using user groups to group such roles. Honestly I don't think
> introducing one more entity to the whole stack would help. Also while we talk
> only about roles - permissions relation it's not always that simple. Note that
> each permission is always assigned through a filter which can define condition
> and taxonomy restriction. So when permission group would be assigned to role,
> we'd need to clone all permissions and let user to configure filters? And in
> such case how would we keep it in sync with later permission group
> changes?

I'm looking at the permissions groups more as metadata around
permissions and when using in role, it would mean defining a
recipe/rules for what permissions you want to have in the role: so
instead of being the container for permissions, it would say, what
permissions should be added to the role. Using this rules, it would be
possible to add additional permissions later from the code and things
would just work ™.

Once the permission is added to do role though the permissions group,
it would work exactly as the permissions work today. I agree that adding
the filter behavior to the permissions group would not help in making it
simpler and that was not the attempts.

– Ivan

··· > On úterý 24. ledna 2017 0:02:15 CET Ivan Necas wrote:

–
Marek

– Ivan

–
You received this message because you are subscribed to the Google Groups “foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send an email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

#17

Corrections.

> Why you don't like explicit lock actions?

I misinterpreted your statements above, looks like we both like
explicit locking.

> add_permission_to_provisioning_manager (also adds to "manager" role)
> add_permission_to_provisioning_reader (also adds to "reader" role)
> add_permission_to_configuration_manager (also adds to "manager" role)
> add_permission_to_configuration_reader (also adds to "reader" role)

This was just a copy and paste error ^ anyway.

I think I was throwing ideas too much, let me summarize my attitude on
the PR and filter out ideas that are nice to have but not part of the
PR effort. What I think the PR should do:

  • the API makes sure we can declaratively identify all permissions
    which were added by plugins (for comparison, currently "default_roles"
    hash) (*)
  • the API deprecates "default_roles" hash providing alternative list
    of default plugin roles and permissions
  • the API provides a way to work with plugin roles (so Discovery
    Manager/Reader will work with the new PAI)
  • the comparison "plugin:validate_roles" rake task is modified to work
    with the new API (it depends on "default_roles" hash)

(*) the only change in the worst case is to keep track of plugin names
which are adding permissions so we have a list of plugins and
permissions added

Nice to have:

  • the API can work fine with locking and cloning proposal
  • the API is used both for plugins and core (currently seed script)
  • the API can be easily extended to what Ivan proposed (I like the
    concept of fine-grained roles)

I still have strong opinion on modifying main Manager/Reader role, I
don't think the original assumption from the RFE that Manager must
have all permissions is not valid. IMHO if user really expect manager
of everything, then there's this Administrator flag, "a manager user"
is required to have "Manager" role plus "Plugin Manager" for all
plugins. We could provide an example (locked) user called Site Manager
as an example if the issue is understanding that. But this is
non-blocking statement. It's a different view of what is best for the
customer.

If we implement a way to automatically add all possible permissions to
Manager (and read permissions to Reader), we could automatically make
sure that Manager/Reader have all the require permissions and adding
to the main two roles is irrelevant. If we take the declaration
approach we are not closing doors to this I believe.

··· -- Later, Lukas @lzap Zapletal
#18

Thaks for summary, just one quick comment where I think it needs clarification

> Corrections.
>
> > Why you don't like explicit lock actions?
>
> I misinterpreted your statements above, looks like we both like
> explicit locking.
>
> > add_permission_to_provisioning_manager (also adds to "manager" role)
> > add_permission_to_provisioning_reader (also adds to "reader" role)
> > add_permission_to_configuration_manager (also adds to "manager" role)
> > add_permission_to_configuration_reader (also adds to "reader" role)
>
> This was just a copy and paste error ^ anyway.
>
> I think I was throwing ideas too much, let me summarize my attitude on
> the PR and filter out ideas that are nice to have but not part of the
> PR effort. What I think the PR should do:
>
> - the API makes sure we can declaratively identify all permissions
> which were added by plugins (for comparison, currently "default_roles"
> hash) ()
> - the API deprecates "default_roles" hash providing alternative list
> of default plugin roles and permissions
> - the API provides a way to work with plugin roles (so Discovery
> Manager/Reader will work with the new PAI)
> - the comparison "plugin:validate_roles" rake task is modified to work
> with the new API (it depends on "default_roles" hash)
>
> () the only change in the worst case is to keep track of plugin names
> which are adding permissions so we have a list of plugins and
> permissions added
>
> Nice to have:
>
> - the API can work fine with locking and cloning proposal
> - the API is used both for plugins and core (currently seed script)
> - the API can be easily extended to what Ivan proposed (I like the
> concept of fine-grained roles)
>
> I still have strong opinion on modifying main Manager/Reader role, I
> don't think the original assumption from the RFE that Manager must
> have all permissions is not valid. IMHO if user really expect manager
> of everything, then there's this Administrator flag, "a manager user"
> is required to have "Manager" role plus "Plugin Manager" for all
> plugins. We could provide an example (locked) user called Site Manager
> as an example if the issue is understanding that. But this is
> non-blocking statement. It's a different view of what is best for the
> customer.

Manager is not Administrator (common misunderstanding). Administrator can
modify settings which is more or less application configuration. We don't
allow any non-admin user to do this, there's no permission for it and we
explicitly check "user.admin?" in there. There are more exceptions like this,
e.g. "Any context" behavior. Also we need to address the use case where you
want to have "Manager of Organization A". We allow that by cloning Manager
role and assigning the organization to this clone.

··· On středa 25. ledna 2017 13:59:57 CET Lukas Zapletal wrote:

–
Marek

If we implement a way to automatically add all possible permissions to
Manager (and read permissions to Reader), we could automatically make
sure that Manager/Reader have all the require permissions and adding
to the main two roles is irrelevant. If we take the declaration
approach we are not closing doors to this I believe.