[SecurityBundle] added acl:set command #9990

dunglas · 2014-01-10T01:20:02Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	no
License	MIT
Doc PR	n/a

This new command allows to set ACL directly from the command line. This useful to quickly set up an environment and for debugging / maintenance purpose.

This PR also includes a functional test system for the ACL component. As an example, it is used to test the acl:set command.
The provided entity class is not mandatory (tests will still be green without it) but can be useful to test other ACL related things. I can remove it if necessary.

The instantiation of the MaskBuilder object is done in a separate method to be easily overridable to use a custom one (e.g. the SonataAdmin one).

lyrixx · 2014-01-12T14:49:27Z

composer.json

@@ -70,6 +70,7 @@
        "doctrine/data-fixtures": "1.0.*",
        "doctrine/dbal": "~2.2",
        "doctrine/orm": "~2.2,>=2.2.3",
+        "doctrine/doctrine-bundle": "~1.2",


A soft dependency could be better.

You can play with class_exist and Command::IsEnable method

@lyrixx doctrine-bundle is only needed to run the tests. It's a require-dev dependency.
Making it a soft dependency is awkward because all functional tests on the ACL system will be skipped by default.
If someone clone the repository, start hacking, run phpunit and make a PR, he can silently broke the ACL system.

It will also require tweaking .travis.yml (to make Travis install doctrine-bundle before running the tests) and update the doc to explain that tests should be run with doctrine-bundle installed.

If it's really annoying to have doctrine-bundle as a development dependency, I prefer registering the DBAL connection directly in the test app's kernel.

Let me know which solution is better.

Sorry. I did not see it was a dev deps ;)

You're welcome.

ErichHartmann · 2014-01-16T19:17:05Z

If any, could you please describe risks or security implications that might result from the ability to set the ACL directly from the command line?

dunglas · 2014-01-16T19:55:27Z

@ErichHartmann I think there is no specific security risk or implication for this command. If an attacker get a shell on your server, it's over. Even without this command.

He can access to database's credentials through app/config/parameters.yml, run any SQL query (including an ACL related one) with doctrine:query:sql, run arbitrary PHP code with php -a and do even worse with the shell itself (rm -Rf /).

dunglas · 2014-02-11T09:49:59Z

Is this PR ready to be merged? Can I start working on the PR in the doc?

fabpot · 2014-03-03T17:35:09Z

src/Symfony/Bundle/SecurityBundle/Command/SetAclCommand.php

+<info>php %command.full_name%</info>
+
+The ACL system must have been initialized with the <info>init:acl</info> command.
+EOF


The Help should be expanded to explain the main use cases (probably using the defined options).

fabpot · 2014-03-03T17:38:15Z

@dunglas There is no need to document the command in the docs, but the help message should be expanded instead.

stof · 2014-03-04T21:08:52Z

the isEnabled method should be overwritten so that the command is available only when the ACL system is enabled. See the router:debug command for an example of usage

stof · 2014-03-04T21:10:18Z

src/Symfony/Bundle/SecurityBundle/Command/SetAclCommand.php

+            ->addOption('security-class', 'sc', InputOption::VALUE_OPTIONAL, 'The user security class')
+            ->addOption('security-username', 'su', InputOption::VALUE_OPTIONAL | InputOption::VALUE_IS_ARRAY, 'The user security username')
+            ->addOption('role', 'r', InputOption::VALUE_OPTIONAL | InputOption::VALUE_IS_ARRAY, 'A list of roles')
+            ->addOption('class-scope', 'cs', InputOption::VALUE_OPTIONAL, 'Use class-scope entries', false)


This should be VALUE_NONE IMO

dunglas · 2014-03-04T22:35:18Z

@fabpot @stof thanks for the review. I'll be very busy next week but I'll make needed changes when I'll be back.

dunglas · 2014-03-19T20:40:11Z

@fabpot @stof Fixed reported issues and added the ability to use / as a namespace delimiter. Added a more precise (but still rude) help message.

@inheritdoc

This PR was submitted for the 2.3-dev branch but it was merged into the 2.3 branch instead (closes #10497). Discussion ---------- [SecurityBundle] Fixed doc of InitAclCommand | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | no | License | MIT | Doc PR | n/a Use {@inheritdoc}. Consistency with #9990 (diff). Commits ------- aa49009 [SecurityBundle] Fixed doc of InitAclCommand

fabpot · 2014-03-26T09:24:30Z

src/Symfony/Bundle/SecurityBundle/Command/SetAclCommand.php

+            )
+            ->addArgument('object-class', InputArgument::REQUIRED, 'The object class name')
+            ->addOption('object-id', null, InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY, 'The object ID')
+            ->addOption('security-class', null, InputOption::VALUE_OPTIONAL, 'The user security class')


VALUE_OPTIONAL means that you don't need to pass a value when using this option, so --security-class is valid; which looks wrong to me. Again, any required value should probably be an argument and not an option.

This is not required to pass a security class. You must pass at least a security class and a security username OR a role.

This is checked lines 104 to 108.

But when using the --security-class, you must pass a class name, right? If that's the case, you must use VALUE_REQUIRED. VALUE_OPTIONAL means that the value for the option is optional, not the option itself. An option is always optional by definition.

Ok I got it. For required values, how to handle several arrays as arguments?
Object IDs and permissions are required and are arrays.

Should I use an argument for one of them (which) and leave the other as a (required) option?

dunglas · 2014-04-09T22:18:23Z

Fixed options types.

fabpot · 2014-04-15T20:42:43Z

src/Symfony/Bundle/SecurityBundle/Command/SetAclCommand.php

+    protected function execute(InputInterface $input, OutputInterface $output)
+    {
+        $objectClassName = strtr($input->getArgument('object-class'), '/', '\\');
+        $objectIds = $input->getOption('object-id');


What if the user did not provide a --object-id option? You're going to get null here. That's why I said that all required information must be managed via arguments, not options. To avoid getting too many of them, you can maybe have some conventions like class:id.

dunglas · 2014-04-21T12:33:08Z

@fabpot use arguments when required. Simplified and enhanced CLI.

fabpot · 2014-04-22T07:26:25Z

Looks good to me for 2.6. Thanks.

inso · 2014-04-22T13:20:17Z

src/Symfony/Bundle/SecurityBundle/Command/SetAclCommand.php

+            return false;
+        }
+
+        $router = $this->getContainer()->get('security.acl.provider');


weird variable name

dunglas · 2014-04-25T16:43:58Z

Renamed weird variable.

fabpot · 2014-06-03T21:19:47Z

Thank you @dunglas.

This PR was merged into the 2.6-dev branch. Discussion ---------- [SecurityBundle] added acl:set command | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | no | License | MIT | Doc PR | n/a This new command allows to set ACL directly from the command line. This useful to quickly set up an environment and for debugging / maintenance purpose. This PR also includes a functional test system for the ACL component. As an example, it is used to test the `acl:set` command. The provided entity class is not mandatory (tests will still be green without it) but can be useful to test other ACL related things. I can remove it if necessary. The instantiation of the `MaskBuilder` object is done in a separate method to be easily overridable to use a custom one (e.g. the SonataAdmin one). Commits ------- a702124 [SecurityBundle] added acl:set command

javiereguiluz · 2014-06-25T14:46:13Z

src/Symfony/Bundle/SecurityBundle/Command/SetAclCommand.php

+
+To set permissions at the class scope, use the <info>--class-scope</info> option:
+
+<info>php %command.full_name% --class-scope --user=Symfony/Component/Security/Core/User/User:anne OWNER Acme/MyClass:42</info>


Hi @dunglas, I'm redacting a "New in Symfony 2.6" blog post about the new acl:set and I wonder if this help message is correct. If you are using the --class-scope, you are granting access to all the objects of the same class, so the trailing object id (42 in this case) should be removed, right?

Original help message:

<info>php %command.full_name% --class-scope --user=Symfony/Component/Security/Core/User/User:anne OWNER Acme/MyClass:42</info>

Proposed help message:

<info>php %command.full_name% --class-scope --user=Symfony/Component/Security/Core/User/User:anne OWNER Acme/MyClass</info>

Hi @javiereguiluz. This help message is correct. It should not be modified. This how the underlying ACL system works (and I agree that this is far from perfect).

The class scope can be set only is you precise an object identity (otherwise an error will be thrown). Technically, it will not grant access to all objects of the same class but only to all objects of this class present in the ACL table.

e.g:

You have three objects: Untitled #1, Renaming "Entities" to "Entity" #2 and Untitled #3.

ACL are initialized for Untitled #1 and Untitled #3 but not for Renaming "Entities" to "Entity" #2.

You grant access to an user with the class scope.

Access will be granted for Untitled #1 and Untitled #3 but not for Renaming "Entities" to "Entity" #2.

@dunglas thanks for your reply. I now understand that everything is correct. However, I find this behavior amazingly counterintuitive. Moreover, in the scopes for access control entries section of the documentation it says:

Class-Scope: These entries apply to all objects with the same class.

This is quite different from what you have said. @wouterj don't you think that the documentation should better explain this strange behavior?

wouterj · 2014-06-25T14:51:15Z

This also should get documented in cookbook/security of the documentation, shouldn't it?

lyrixx reviewed Jan 12, 2014
View reviewed changes

jakzal added the SecurityBundle label Feb 11, 2014

fabpot reviewed Mar 3, 2014
View reviewed changes

stof reviewed Mar 4, 2014
View reviewed changes

dunglas mentioned this pull request Mar 19, 2014

[SecurityBundle] Fixed doc of InitAclCommand #10497

Closed

fabpot reviewed Mar 26, 2014
View reviewed changes

fabpot reviewed Apr 15, 2014
View reviewed changes

inso reviewed Apr 22, 2014
View reviewed changes

[SecurityBundle] added acl:set command

a702124

fabpot added Acl and removed Security labels Apr 28, 2014

fabpot merged commit a702124 into symfony:master Jun 3, 2014

javiereguiluz reviewed Jun 25, 2014
View reviewed changes

dunglas deleted the acl_commands branch December 5, 2015 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecurityBundle] added acl:set command #9990

[SecurityBundle] added acl:set command #9990

dunglas commented Jan 10, 2014

lyrixx Jan 12, 2014

dunglas Jan 12, 2014

dunglas Jan 12, 2014

lyrixx Jan 12, 2014

dunglas Jan 12, 2014

ErichHartmann commented Jan 16, 2014

dunglas commented Jan 16, 2014

dunglas commented Feb 11, 2014

fabpot Mar 3, 2014

fabpot commented Mar 3, 2014

stof commented Mar 4, 2014

stof Mar 4, 2014

dunglas commented Mar 4, 2014

dunglas commented Mar 19, 2014

fabpot Mar 26, 2014

dunglas Mar 26, 2014

fabpot Mar 26, 2014

dunglas Mar 26, 2014

dunglas commented Apr 9, 2014

fabpot Apr 15, 2014

dunglas commented Apr 21, 2014

fabpot commented Apr 22, 2014

inso Apr 22, 2014

dunglas Apr 22, 2014

dunglas commented Apr 25, 2014

fabpot commented Jun 3, 2014

javiereguiluz Jun 25, 2014

dunglas Jun 25, 2014

javiereguiluz Jun 25, 2014

wouterj commented Jun 25, 2014


		To set permissions at the class scope, use the <info>--class-scope</info> option:

		<info>php %command.full_name% --class-scope --user=Symfony/Component/Security/Core/User/User:anne OWNER Acme/MyClass:42</info>

[SecurityBundle] added acl:set command #9990

[SecurityBundle] added acl:set command #9990

Conversation

dunglas commented Jan 10, 2014

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

ErichHartmann commented Jan 16, 2014

dunglas commented Jan 16, 2014

dunglas commented Feb 11, 2014

Choose a reason for hiding this comment

fabpot commented Mar 3, 2014

stof commented Mar 4, 2014

Choose a reason for hiding this comment

dunglas commented Mar 4, 2014

dunglas commented Mar 19, 2014

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

dunglas commented Apr 9, 2014

Choose a reason for hiding this comment

dunglas commented Apr 21, 2014

fabpot commented Apr 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Apr 25, 2014

fabpot commented Jun 3, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Jun 25, 2014