[RFC] Added `--apply-closure` option #71

dantleech · 2013-06-15T18:08:29Z

This option gives us a very quick and flexible solution for migrations:

./bin/phpcr phpcr:nodes:update --query="SELECT * FROM [nt:unstructured] WHERE ..." --apply-closure="function ($session, $node) { if (null !== $node->getProperty('url')) { $node->setProperty('linkType', 'externalUrl'); }"

What do people think?

lsmith77 · 2013-06-16T17:26:14Z

I am not sure. seems like this opens up a lot of power to the CLI. usually anyone with CLI access can add code but still I don't feel comfortable with this.

dbu · 2013-06-16T19:29:49Z

What about making it optional? The command code would have to configure itself i guess, as commands are not Services...

----- Reply message -----
Von: "Lukas Kahwe Smith" notifications@github.com
An: "phpcr/phpcr-utils" phpcr-utils@noreply.github.com
Betreff: [phpcr-utils] [RFC] Added --apply-closure option (#71)
Datum: So., Jun. 16, 2013 19:26
I am not sure. seems like this opens up a lot of power to the CLI. usually anyone with CLI access can add code but still I don't feel comfortable with this.

—
Reply to this email directly or view it on GitHub.

lsmith77 · 2013-06-16T21:01:54Z

if it needs to be enabled by code i am not sure i see the sense in it.

dbu · 2013-06-17T06:28:10Z

what i meant: we can have it as a config option and DI can put a parameter into the container, but the command will need to do this->getContainer->hasParameter('cmf_menu.command.update_closure') or something like that. from a user point of view there is no difference, i was just thinking about the implementation already.

lsmith77 · 2013-06-17T06:44:02Z

not even sure that i like it as a config option. as soon as you allow injecting code via the CLI, there are almost no more limits to what can be done with a CLI command.

dbu · 2013-06-17T06:57:34Z

if we default the option to false, people can chose if they want this
feature or not.

the alternative would be some sort of separate git repo for this
command, then there is no way people could steal rights. makes it less
useful however. the thing is that it will allow to do any kind of crazy
migration things by copy-paste, without needing to deploy temporary
migration code.

pgodel · 2013-06-17T07:11:14Z

Although this is very practical, I agree with @lsmith77 on the concerns about this. Just look at all the apps that offer some sort of "eval" and see the consequences.

dantleech · 2013-06-17T10:17:55Z

Yeah, maybe another repo or alternatively sandbox it by embedding a scripting language, don't know how feasable that would be.

Pablo Godel notifications@github.com a écrit :

Although this is very practical, I agree with @lsmith77 on the concerns
about this. Just look at all the apps that offer some sort of "eval"
and see the consequences.

Reply to this email directly or view it on GitHub:
#71 (comment)

Envoyé de mon téléphone Android avec K-9 Mail. Excusez la brièveté.

cryptocompress · 2013-06-17T17:33:20Z

+1 ... i like this one

eval is evil and should be "create_function"
as of a hacker perspective the difference is execute- vs read- permission?

dbu · 2013-06-18T06:47:45Z

can we use the "migrator" feature for code-requiring migrations or is
there still something missing? if needed, we could refactor this command
in a way that its easily extendable and if somebody wants, he can then
do that extra parameter to inject a closure. and if he wants it safe, he
can extend the command with custom update commands that have the logic
in their code and thus can be tested.

dantleech · 2013-06-18T08:20:41Z

Well, would using create_function solve our security problems?

Alternatively we could use the V8Js extension:

http://php.net/manual/en/book.v8js.php

And pass in an array to the javascript function representing the node. Then remap the array back to the node in PHP. We could also sandbox the session object in a JS object.

We could also use this (closure) method as an additional way of filtering queries.

lsmith77 · 2013-06-18T08:32:25Z

create_function would at least mitigate the scope a bit .. i am still not convinced though. i mean you can just as well write the code to the file system and remove it again as part of your deploy script.

cryptocompress · 2013-06-19T15:20:28Z

I cannot see any security problems in here.

If someone can execute a destructive operation like bin/phpcr phpcr:nodes:update there is always a way to read/kill whole accessible DB. e.g.: read config and do it manually
End-user can't access this and there is no need to protect devs/admins from their intension.
This command may not execute arbitrary code. So create_function is the right tool for the job. Injected code still as (un-)trustworthy as current user.
This command is only accessible with/through phpcr:nodes:update.
Internal access restrictions can be implemented (but may be useless as in 1.?).

This is a very powerful and flexible tool and migrations are only one use case.

cryptocompress · 2013-06-20T13:35:10Z

err, what is the point in limiting this to phpcr:nodes:update only?
and would it make sense if closure return a bool/node/null?

this is like awk for CMF 💯

dantleech · 2013-06-26T18:04:55Z

@lsmith77 what are your concerns with create_function ?

If create_function is not good enough then what do people think about writing the closures in Javascript as I mentioned earlier?

dbu · 2013-06-27T06:12:39Z

i would prefer not to bring in js, thats too complicated. if lukas
really can't live with an apply-closure that we have a config option
that the command reads from the container to know if its even enabled,
then lets provide the command for that as a separate repo rather than
doing js.

dantleech · 2013-06-27T07:40:56Z

But if its in another repo, the use cases for this Command is to help
with migrations, so people will install this command. From that point
onwards it will be there. The same for explictly enabling it - people
will enable it and, well, its probably going to stay enabled.

But I guess at least having to manually enable it makes people more
aware.

The only security problem I can think of is that some people like to
expose their commands to the web (Sensio Desktop??)

What is a worst-case scenario?

On Wed, Jun 26, 2013 at 11:12:40PM -0700, David Buchmann wrote:

i would prefer not to bring in js, thats too complicated. if lukas
really can't live with an apply-closure that we have a config option
that the command reads from the container to know if its even enabled,
then lets provide the command for that as a separate repo rather than
doing js.

—
Reply to this email directly or [1]view it on GitHub.

References

Visible links

[RFC] Added --apply-closure option #71 (comment)

lsmith77 · 2013-06-27T17:32:53Z

create_function I guess is a compromise I can live with

- Also polished up documentation etc.

dantleech · 2013-07-08T18:14:21Z

Rebased and updated. It now uses create_function which also makes the syntax better:

     php bin/phpcr phpcr:nodes:update \
         --query="SELECT * FROM [nt:unstructured] WHERE [phpcr:class]=\"Some\Class\Here\"" \
         --apply-closure="\$session->doSomething(); \$node->setProperty('foo', 'bar');"

Are people now happy to merge this?

lsmith77 · 2013-07-08T18:59:35Z

+1

cryptocompress · 2013-07-08T20:27:01Z

👍

output should be in <comment>-tags (ob_*), if any?

dantleech · 2013-07-08T20:46:25Z

src/PHPCR/Util/Console/Helper/PhpcrCliHelper.php

+        foreach ($options['applyClosures'] as $closureString) {
+            $closure = create_function('$session, $node', $closureString);
+            $output->writeln(sprintf(
+                '<comment> > Applying closure: %s</comment>',


@cryptocompress output is already in comment tags?

[RFC] Added `--apply-closure` option

dbu · 2013-07-09T16:16:33Z

thanks a lot dan. is there a good place to document this? but then again, commands are kind of self-documenting

dbu · 2013-07-09T16:17:23Z

src/PHPCR/Util/Console/Command/BaseCommand.php

@@ -78,5 +78,9 @@ public function configureNodeManipulationInput()
            InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY,
            'Remove mixin from the nodes'
        );
+        $this->addOption('apply-closure', null,
+            InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY,


erm, why is a closure required? does that make sense?

The value is required, i.e. you can ommit the option but if you specify it you have to specify a value.

thanks. learning something new every day. today i learned (well, got reminded, rather) that i should read closely before asking dumb questions ;-)

dantleech mentioned this pull request Jun 15, 2013

[WIP] Refactored allow empty symfony-cmf/menu-bundle#68

Merged

dantleech added 2 commits July 8, 2013 17:58

Experimental node apply-closures option

c84f234

Made apply-closure use create_function instead of eval

d87a737

- Also polished up documentation etc.

dantleech reviewed Jul 8, 2013
View reviewed changes

dbu added a commit that referenced this pull request Jul 9, 2013

Merge pull request #71 from dantleech/nodes-update-closure

56af352

[RFC] Added `--apply-closure` option

dbu merged commit 56af352 into phpcr:master Jul 9, 2013

dbu reviewed Jul 9, 2013
View reviewed changes

[RFC] Added --apply-closure option #71

[RFC] Added --apply-closure option #71

Uh oh!

Conversation

dantleech commented Jun 15, 2013

Uh oh!

lsmith77 commented Jun 16, 2013

Uh oh!

dbu commented Jun 16, 2013

Uh oh!

lsmith77 commented Jun 16, 2013

Uh oh!

dbu commented Jun 17, 2013

Uh oh!

lsmith77 commented Jun 17, 2013

Uh oh!

dbu commented Jun 17, 2013

Uh oh!

pgodel commented Jun 17, 2013

Uh oh!

dantleech commented Jun 17, 2013

Uh oh!

cryptocompress commented Jun 17, 2013

Uh oh!

dbu commented Jun 18, 2013

Uh oh!

dantleech commented Jun 18, 2013

Uh oh!

lsmith77 commented Jun 18, 2013

Uh oh!

cryptocompress commented Jun 19, 2013

Uh oh!

cryptocompress commented Jun 20, 2013

Uh oh!

dantleech commented Jun 26, 2013

Uh oh!

dbu commented Jun 27, 2013

Uh oh!

dantleech commented Jun 27, 2013

Uh oh!

lsmith77 commented Jun 27, 2013

Uh oh!

dantleech commented Jul 8, 2013

Uh oh!

lsmith77 commented Jul 8, 2013

Uh oh!

cryptocompress commented Jul 8, 2013

Uh oh!

dantleech Jul 8, 2013

Choose a reason for hiding this comment

Uh oh!

dbu commented Jul 9, 2013

Uh oh!

dbu Jul 9, 2013

Choose a reason for hiding this comment

Uh oh!

dantleech Jul 9, 2013

Choose a reason for hiding this comment

Uh oh!

dbu Jul 10, 2013

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[RFC] Added `--apply-closure` option #71

[RFC] Added `--apply-closure` option #71