Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Credentials] credentials and groups #154

Closed
pjam opened this issue May 26, 2015 · 13 comments
Closed

[Credentials] credentials and groups #154

pjam opened this issue May 26, 2015 · 13 comments

Comments

@pjam
Copy link
Contributor

pjam commented May 26, 2015

Hi.

Could you give some pointers on how to use credentials in generator.yml?

I want a list field visible only with some credentials and tried

fields:
    fieldname:
        credentials: [ROLE_SUPER_ADMIN]

without luck, error thrown "Could not set option "credentials" on "fieldname" column in "Entity" list builder.". Is it possible to use credentials for fields? How?

Regards

@tobias-93
Copy link
Member

@pjam you need to utilize the groups functionality.
First you need to define the right getGroups(Object $object) function in the EditController and NewController in which you assign groups with a certain name to the people with the right credentials like:

 /**
   * Get groups.
   *
   * @param Object $Object
   * @return array
   */
  protected function getGroups(Object $Object)
  {
    return $this->get('security.authorization_checker')->isGranted("ROLE_SUPER_ADMIN") ? array(0 -> 'SUPER_ADMIN') : array();   
  }

Then you need to attach the credentials to the field in the generator.yml like:

fields:
  fieldname:
    groups: [SUPER_ADMIN]

@pjam
Copy link
Contributor Author

pjam commented May 28, 2015

Thanks for the help @tobias-93 it is working now. 👍

But this is some kind of workaround right? It's supposed to work the way I tried, it is like that in the current docs. Or will this really be the way to go?

@mikeyudin
Copy link

I'm curious to the reasoning behind this? In 1.x, the authorization of a field was very straightforward with the 'credentials' option. Why would we force the usage of groups? In a normal security model, actions would be permission/specific-credential based, not group-based?

This seems like a time-consuming process (not to mention unnecessary code) for permissions. I need to write a getGroups method for every single CRUD controller in my system? Additionally, what if one field in a form requires one permission, but another field requires a different permission?

@sescandell
Copy link
Member

Hi,

This is somethig we planned to improve using a centralized service to manage groups (take a look to #82 ).

I understand your point "it was easier before", but it was also less flexible (once refactoring will be done). Any idea is welcome: how could we let the functionnality of groups available but also making it easier to use with simple ROLEs constraints?

Maybe the group functionnality is not the right way to proceed?

In my personnal projects I'm using JMSSecurityExtraBundle and custom security function for this. And I dont really need groups. But groups might be more flexible...

Maybe we should let @loostro give us its feedbacks.

@mikeyudin yours are of course welcome too... Without groups, how would we manage credentials based on something not limited to "ROLES"? (actually... Voters might make the trick?)

@mikeyudin
Copy link

If anything, I think the previous method was more flexible. I could specify different credentials for different fields. Now I am forced to say "All fields on this page are allowed if user has X group." Permissions are often more complex than that. If something more complex than hasRole was needed, JMSSecurityExtra allows us to write custom security logic.

I think advanced security is out of the scope of this bundle. We should only have to worry about passing a value to isGranted() and accept the returned boolean value. In what cases would hasRole & JMSSecurityExtra not be enough for this? My applications have some very complex credentials, and I am yet to run into any issues with these methods.

How does groups solve something like this: "User can see field if they have A and B, but not C?". I can think of several ways of doing it with the credentials method.

Let's not also forget the major BC break that this causes. I understand this is a major release, but even still, reducing the amount of BC breaks should always be at the forefront of everyone's mind.

@tobias-93
Copy link
Member

@mikeyudin keep in mind that the example I have was very basic and simple. You can define many groups in the getGroups method and use these for certain fields (you can still keep specifying credentials for different fields). All you say is already possible.

@mikeyudin
Copy link

I guess I just fail to see the point of this. The entire principle behind the yaml-based admin generator is to reduce code and keep as much as possible in a single configuration file. Now we are pulling a major piece of that (security) out of the config and into code, increasing the complexity ten-fold on large applications.

A precedent is being set for this to happen in other areas as well. If I wanted to code out the specs of all of my admin pages, I would've gone with SonataAdmin.

The answer still has not been stated: What does this new method offer that the previous method did not? Perhaps this is mentioned in another issue/PR? It should be a good reason to cause such a major BC break. I am curious to know what @loostro has to say about this.

@pjam
Copy link
Contributor Author

pjam commented May 29, 2015

@mikeyudin from what I understand from the example given by @tobias-93 you can achieve those complex permissions the same way. Just define the returned array as you want. I guess the size of that array doesn't need to be equal to the number of different roles you have in your app.
But I agree with you, I think this contradicts the purpose of a generator.yml. It should straightforward and simple like in symfony1. I fail to see the use of this approach.

Either way, using the groups way or the symfony1 way, I think we can take advantage of the Role Hierarchy system to build complex permissions and also to "simulate" groups, or not?

And would it be hard to implement the "credentials" option in the generator like the old way? I would try but I just recently started to use sf2 instead of sf1 and I still lose myself in the code. lol

@ksn135
Copy link
Contributor

ksn135 commented May 29, 2015

Just add my 2¢...
I think current groups implementation is very powerful and useful. Especially when it used in conjunctions with JMSSecurityExtraBundle and custom security function. _BUT_ in some cases we need to do simple things as we do it before in old sf1.4 way, and then this proposal comes to my mind. Personally, I vote 👍 for this issue. I think we have to add this additional behaviour as a nice feature to our plans.
I will try to make an PR for this issue later this month. @loostro tell me your opinion?

@sescandell
Copy link
Member

@mikeyudin I see your point... let's find a way to make all of this the most easy to use.

As I said, I'm using a lot JMSecurityExtraBundle for these crendentials. And it do the trick... I didn't use yet the new version on a real project to let you know what are benefits from the group concept.

Let's try to find use case where JMS is not suffisant enough.
If we don't, I think we should revert to "simple" approch like before.

@bobvandevijver
Copy link
Member

I guess it would also be acceptable to use both systems: I suggest that the old credential method is reinstated, but that the group configuration can also stay. The credentials/group check would then be depending on your own preference: The more flexible one, or the easier one.

@ioleo
Copy link
Member

ioleo commented May 30, 2015

I guess the easiest implementation would be to force either groups or credentials to be empty (throw an exception if both are defined). The credentials system would simply call isGranted, which would be enough for simple cases. Combined with JMSSecurityExtraBundle it gives even more flexibility.

However I think that groups functionality is more readable and flexible and it should stay.

@sescandell sescandell changed the title Credentials [Credentials] credentials and groups Jun 20, 2015
@sescandell
Copy link
Member

See #221

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants