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

Use credentials back instead of groups #221

Merged
merged 47 commits into from
Jan 4, 2016
Merged

Use credentials back instead of groups #221

merged 47 commits into from
Jan 4, 2016

Conversation

sescandell
Copy link
Member

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:

@bobvandevijver
Copy link
Member

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!

@tobias-93
Copy link
Member

👍 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))) {
Copy link
Member

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 }})
Copy link
Member

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.

@ioleo
Copy link
Member

ioleo commented Dec 28, 2015

I like this change, as it simplifies implementation, but keeps all the functionality. Great job @sescandell 👍

@sescandell
Copy link
Member Author

Ready to merge.

Tested on the demo repo.
Once merged, I'll push my modifications on that repo too.

Waiting for your feedbacks.

I'll merge tomorrow depending on these

@ioleo
Copy link
Member

ioleo commented Jan 4, 2016

👍 for merge

sescandell added a commit that referenced this pull request Jan 4, 2016
@sescandell sescandell merged commit f29b802 into master Jan 4, 2016
@sescandell sescandell deleted the feat-credentials branch January 4, 2016 21:08
@bobvandevijver bobvandevijver mentioned this pull request Jan 4, 2016
20 tasks
@bobvandevijver
Copy link
Member

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?

@ioleo
Copy link
Member

ioleo commented Jan 4, 2016

@bobvandevijver Lets move this discussion to #223 as this issue is closed now and #223 deals with documentation.

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

Successfully merging this pull request may close these issues.

4 participants