-
Notifications
You must be signed in to change notification settings - Fork 29
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
Use credentials back instead of groups #221
Conversation
Remove JMS dependency for security (temporary, make it optionnal): use Voters if possible
Add credentials security checks to columns
I can only say 👍 for this! I did not test the PR, but I find current the group functionality unclear, while indeed JMS provides enough functionality for this. Good work! |
👍 from my side, but this should be incorporated very well in the documentation including descriptions how to work with voters. |
|
||
$configurations = array(); | ||
foreach ($builder[$param] as $name => $configuration) { | ||
if (!(is_array($configuration) || is_null($configuration))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if (!is_array($configuration) && !is_null($configuration)) {
would be more clear whats going on here
Rewrite if condition
@@ -28,8 +27,7 @@ class EditController extends BaseController | |||
protected function getEditForm(\{{ model }} ${{ builder.ModelClass }}) | |||
{ | |||
return $this->createForm($this->getEditType(), ${{ builder.ModelClass }}, array_merge( | |||
$this->getFormOptions(${{ builder.ModelClass }}), | |||
array('groups' => $this->getGroups(${{ builder.ModelClass }})) | |||
$this->getFormOptions(${{ builder.ModelClass }}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should drop array_merge
here.
I like this change, as it simplifies implementation, but keeps all the functionality. Great job @sescandell 👍 |
…or not on new / edit / filters forms based on the same credential. Sample available on the demo repository
Ready to merge. Tested on the demo repo. Waiting for your feedbacks. I'll merge tomorrow depending on these |
👍 for merge |
Use credentials back instead of groups
I'm missing some upgrade notes/documentation. I'm working on new docs right now (#223), so maybe it is handy to add those to the PR? |
@bobvandevijver Lets move this discussion to #223 as this issue is closed now and #223 deals with documentation. |
After thinking about credentials, and groups functionnality (and in particulary issues related to groups), I think we should better back to a simple solution but as powerfull as groups.
The main idea is to remove "groups" terms and come back to credentials and use Symfony native capacities to handle security. With credentials (based on security authorization checker service), we can manage security in a "natural" way in a Symfony project using simple ROLE_XXX checks. For improved tests, users might still implement custom voters (which is re-usable).
JMS Extra Security Bundle has been moved as a suggested package. To enable it, use the new config parameter
use_jms_security
and and the bundle to your composer.json file.This PR is a starting point for discussion as already started in issue #154
What do you think about reverting back to simple credentials ?
I really think, we can do everything with credentials keeping things simple for simple needs, but still powerfull and flexible for advanced use cases thanks to Voters or JMSSecurityBundle.
Tasks:
use_jms_security