Skip to content

[DI] Randomize private services #19683

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ public function __construct()
)),
new CheckExceptionOnInvalidReferenceBehaviorPass(),
));

$this->afterRemovingPasses = array(array(
new RandomizePrivateServiceIdentifiers(),
));
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Alias;
use Symfony\Component\DependencyInjection\Reference;

/**
* Randomizes private service identifiers, effectively making them unaccessable from outside.
*
* @author Roland Franssen <franssen.roland@gmail.com>
*/
class RandomizePrivateServiceIdentifiers implements CompilerPassInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, randomization shouldn't be handled by a compiler pass, but by the PhpDumper directly. Otherwise it won't be applied without the compiler, and many apps won't have it enabled when migrating to 4.0

Copy link
Contributor Author

@ro0NL ro0NL Aug 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this. But eventually chose to separate it into a compiler pass (yes; making it an optional feature). Without compiling you have a normal container.

Thing is i dont really like the concept of coupliing privates with a container. The container shouldnt care about privates (it's just a normal service from that perspective). Hence $this->privates could be totally unneeded...

No need to bypass get or checking private definitions in PhpDumper. We work with the container :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you call a normal container doesn't have private services. The container shouldn't care about private services, right, but the PhpDumper should (and already does). So I still think that the PhpDumper should do this.

Copy link
Contributor Author

@ro0NL ro0NL Aug 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We disagree :) i think we have compiler passes for that (optional) or otherwise Container::compile (by design).

A dumper (eg. PhpDumper) dumps the given container as is. Like others do, YAML, XML etc. I dont know why PhpDumer would randomize your services, whereas XmlDumper doesnt.

Currently it only deals with privates to hack the dumped container (ie. output), saying; here's a list of privates for you to validate business logic (which it shouldnt care about :)).

Copy link
Member

@nicolas-grekas nicolas-grekas Aug 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it makes no sense to randomize with XmlDumper and in fact would be wrong: it would rename services for no reason and make dumps harder and not accurate.
What matters is having dumpers deal with private services, which XmlDumper does by adding the public="false" attribute, and PhpDumper doing some randomization. That! would make sense.

Copy link
Contributor Author

@ro0NL ro0NL Aug 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reconsidered :)

Because it makes no sense to randomize with XmlDumper and in fact would be wrong: it would rename services for no reason and make dumps harder and not accurate.

My first thought was, yeah this totally counts for a PhpDumper as well. Which in some ways is still true, but i guess you're right. Randomizing could be the way a php dumper deals with privates by design (whereas xml adds public="false" 👍 ).

Do we need to consider DX here?

  • Make it depend on some debug flag?
  • Have 2 implementations (think DebugPhpDumper)
  • Forget about randomizing, and compile different method names (think getPrivate(.+)Service)
    • Nah.. this makes the Container work with "privates" again 😕

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how DX is involved here. So maybe but I don't what it'd mean :)

Forget about randomizing, and compile different method names

maybe :) the goal is to have private services be hidden from the outside, no interference with the public interface.
How it's done is an implementation detail We should chose the simplest one... :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should chose the simplest one

True.

DX, in the sense that you may not want to have random id's in the output, or at least customize/mock it in some way (think setRandomizer).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe the simplest and most DX friendly is to make private services leak a bit by reserving their id, throwing an exception whenever one uses or fetches an id that collides with a private service.

{
private $idMap;
private $randomizer;

public function process(ContainerBuilder $container)
{
// update private definitions + build id map
$this->idMap = array();
foreach ($container->getDefinitions() as $id => $definition) {
if (!$definition->isPublic()) {
$this->idMap[$id] = $this->randomizer ? (string) call_user_func($this->randomizer, $id) : hash('sha256', mt_rand().$id);
$definition->setPublic(true);
}
}

// rename definitions
$aliases = $container->getAliases();
foreach ($this->idMap as $oldId => $newId) {
$container->setDefinition($newId, $container->getDefinition($oldId));
$container->removeDefinition($oldId);
}

// update referencing definitions
foreach ($container->getDefinitions() as $id => $definition) {
$definition->setArguments($this->processArguments($definition->getArguments()));
$definition->setMethodCalls($this->processArguments($definition->getMethodCalls()));
$definition->setProperties($this->processArguments($definition->getProperties()));
$definition->setFactory($this->processFactory($definition->getFactory()));
if (null !== ($decorated = $definition->getDecoratedService()) && isset($this->idMap[$decorated[0]])) {
$definition->setDecoratedService($this->idMap[$decorated[0]], $decorated[1], $decorated[2]);
}
}

// update alias map
$aliases = $container->getAliases();
foreach ($container->getAliases() as $oldId => $alias) {
if(isset($this->idMap[$oldId]) && $oldId === (string) $alias) {
$container->setAlias(new Alias($this->idMap[$oldId], $alias->isPublic()), $this->idMap[$oldId]);
}
}

// for BC
$reflectionProperty = new \ReflectionProperty(ContainerBuilder::class, 'privates');
$reflectionProperty->setAccessible(true);
$reflectionProperty->setValue($container, $this->idMap);
}

public function setRandomizer(callable $randomizer = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it to allow mocking in tests..(see diff), but many tests are affected so maybe it needs a different approach. But before continuing i want to know if this PR is viable.

{
$this->randomizer = $randomizer;
}

private function processArguments(array $arguments)
{
foreach ($arguments as $k => $argument) {
if (is_array($argument)) {
$arguments[$k] = $this->processArguments($argument);
} elseif ($argument instanceof Reference && isset($this->idMap[$id = (string) $argument])) {
$arguments[$k] = new Reference($this->idMap[$id], $argument->getInvalidBehavior());
}
}

return $arguments;
}

private function processFactory($factory)
{
if (null === $factory || !is_array($factory) || !$factory[0] instanceof Reference) {
return $factory;
}
if (isset($this->idMap[$id = (string) $factory[0]])) {
$factory[0] = new Reference($this->idMap[$id], $factory[0]->getInvalidBehavior());
}

return $factory;
}
}
33 changes: 22 additions & 11 deletions src/Symfony/Component/DependencyInjection/Container.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class Container implements ResettableContainerInterface

protected $services = array();
protected $methodMap = array();
protected $privates = array();
protected $privates = array(); // for BC
protected $aliases = array();
protected $loading = array();

Expand Down Expand Up @@ -172,19 +172,21 @@ public function set($id, $service)
unset($this->aliases[$id]);
}

$this->services[$id] = $service;

if (null === $service) {
unset($this->services[$id]);
}

if (isset($this->privates[$id])) {
if (null === $service) {
@trigger_error(sprintf('Unsetting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
unset($this->privates[$id]);
} else {
@trigger_error(sprintf('Setting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0. A new public service will be created instead.', $id), E_USER_DEPRECATED);
$id = $this->privates[$id];
}
}

if (null === $service) {
unset($this->services[$id]);
} else {
$this->services[$id] = $service;
}
}

/**
Expand All @@ -209,6 +211,7 @@ public function has($id)
} else {
if (isset($this->privates[$id])) {
@trigger_error(sprintf('Checking for the existence of the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
$id = $this->privates[$id];
}

return method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service');
Expand Down Expand Up @@ -263,6 +266,12 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE
} elseif (method_exists($this, $method = 'get'.strtr($id, $this->underscoreMap).'Service')) {
// $method is set to the right value, proceed
} else {
if (isset($this->privates[$id])) {
@trigger_error(sprintf('Requesting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);

return $this->get($this->privates[$id]);
}

if (self::EXCEPTION_ON_INVALID_REFERENCE === $invalidBehavior) {
if (!$id) {
throw new ServiceNotFoundException($id);
Expand All @@ -281,9 +290,6 @@ public function get($id, $invalidBehavior = self::EXCEPTION_ON_INVALID_REFERENCE

return;
}
if (isset($this->privates[$id])) {
@trigger_error(sprintf('Requesting the "%s" private service is deprecated since Symfony 3.2 and won\'t be supported anymore in Symfony 4.0.', $id), E_USER_DEPRECATED);
}

$this->loading[$id] = true;

Expand Down Expand Up @@ -339,9 +345,14 @@ public function reset()
public function getServiceIds()
{
$ids = array();
$reversedPrivates = array_flip($this->privates);
foreach (get_class_methods($this) as $method) {
if (preg_match('/^get(.+)Service$/', $method, $match)) {
$ids[] = self::underscore($match[1]);
$id = self::underscore($match[1]);
if (isset($reversedPrivates[$id])) {
$id = $reversedPrivates[$id];
}
$ids[] = $id;
}
}
$ids[] = 'service_container';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -542,9 +542,6 @@ public function compile()
$this->compiled = true;

foreach ($this->definitions as $id => $definition) {
if (!$definition->isPublic()) {
$this->privates[$id] = true;
}
if ($this->trackResources && $definition->isLazy() && ($class = $definition->getClass()) && class_exists($class)) {
$this->addClassResource(new \ReflectionClass($class));
}
Expand Down
12 changes: 8 additions & 4 deletions src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,7 @@ public function __construct()

$code .= "\n \$this->services = array();\n";
$code .= $this->addMethodMap();
$code .= $this->addPrivateServices();
Copy link
Contributor Author

@ro0NL ro0NL Aug 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to trigger deprecations in get for a compiled container when dumped.

$code .= $this->addAliases();

$code .= <<<'EOF'
Expand Down Expand Up @@ -892,6 +893,8 @@ private function addMethodMap()
/**
* Adds the privates property definition.
*
* This method is intented for BC only.
*
* @return string
*/
private function addPrivateServices()
Expand All @@ -900,12 +903,13 @@ private function addPrivateServices()
return '';
}

$reflectionProperty = new \ReflectionProperty(ContainerBuilder::class, 'privates');
$reflectionProperty->setAccessible(true);

$code = '';
ksort($definitions);
foreach ($definitions as $id => $definition) {
if (!$definition->isPublic()) {
$code .= ' '.var_export($id, true)." => true,\n";
}
foreach ($reflectionProperty->getValue($this->container) as $originId => $id) {
$code .= ' '.var_export($originId, true).' => '.var_export($id, true).",\n";
}

if (empty($code)) {
Expand Down
25 changes: 22 additions & 3 deletions src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public function testGetServiceIds()

$sc = new ProjectServiceContainer();
$sc->set('foo', $obj = new \stdClass());
$this->assertEquals(array('internal', 'bar', 'foo_bar', 'foo.baz', 'circular', 'throw_exception', 'throws_exception_on_service_configuration', 'service_container', 'foo'), $sc->getServiceIds(), '->getServiceIds() returns defined service ids by getXXXService() methods, followed by service ids defined by set()');
$this->assertEquals(array('internal', 'sharing_internal', 'bar', 'foo_bar', 'foo.baz', 'circular', 'throw_exception', 'throws_exception_on_service_configuration', 'service_container', 'foo'), $sc->getServiceIds(), '->getServiceIds() returns defined service ids by getXXXService() methods, followed by service ids defined by set()');
}

public function testSet()
Expand Down Expand Up @@ -308,6 +308,15 @@ public function testThatCloningIsNotSupported()
$this->assertTrue($clone->isPrivate());
}

public function testSharedPrivateService()
{
$c = new ProjectServiceContainer();
$expected = new \stdClass();
$expected->internal = new \stdClass();
$this->assertTrue($c->has('sharing_internal'));
$this->assertEquals($expected, $c->get('sharing_internal'), '->get() returns a public service referencing a private service');
}

/**
* @group legacy
* @requires function Symfony\Bridge\PhpUnit\ErrorAssert::assertDeprecationsAreTriggered
Expand Down Expand Up @@ -379,6 +388,7 @@ class ProjectServiceContainer extends Container
public $__foo_bar;
public $__foo_baz;
public $__internal;
public $__sharing_internal;

public function __construct()
{
Expand All @@ -388,15 +398,24 @@ public function __construct()
$this->__foo_bar = new \stdClass();
$this->__foo_baz = new \stdClass();
$this->__internal = new \stdClass();
$this->privates = array('internal' => true);
$this->__sharing_internal = new \stdClass();
$this->aliases = array('alias' => 'bar');
$this->privates = array('internal' => 'semirandom_internal');
}

protected function getInternalService()
protected function getSemirandomInternalService()
{
return $this->__internal;
}

protected function getSharingInternalService()
{
$instance = $this->__sharing_internal;
$instance->internal = $this->get('semirandom_internal'); // simulates dumped behavior

return $instance;
}

protected function getBarService()
{
return $this->__bar;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\DependencyInjection\Tests\Dumper;

use Symfony\Component\DependencyInjection\Compiler\RandomizePrivateServiceIdentifiers;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Dumper\PhpDumper;
use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag;
Expand Down Expand Up @@ -122,6 +123,13 @@ public function testAddService()

// with compilation
$container = include self::$fixturesPath.'/containers/container9.php';
foreach ($container->getCompiler()->getPassConfig()->getPasses() as $pass) {
if ($pass instanceof RandomizePrivateServiceIdentifiers) {
$pass->setRandomizer(function ($id) {
return 'shared_private' === $id ? 'semirandom_'.$id : md5(uniqid($id));
});
}
}
$container->compile();
$dumper = new PhpDumper($container);
$this->assertEquals(str_replace('%path%', str_replace('\\', '\\\\', self::$fixturesPath.DIRECTORY_SEPARATOR.'includes'.DIRECTORY_SEPARATOR), file_get_contents(self::$fixturesPath.'/php/services9_compiled.php')), $dumper->dump(), '->dump() dumps services');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,5 +129,14 @@
->register('factory_service_simple', 'Bar')
->setFactory(array(new Reference('factory_simple'), 'getInstance'))
;
$container
->register('shared_private', 'stdClass')
->setPublic(false);
$container
->register('shared_private_dep1', 'stdClass')
->setProperty('dep', new Reference('shared_private'));
$container
->register('shared_private_dep2', 'stdClass')
->setProperty('dep', new Reference('shared_private'));

return $container;
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ digraph sc {
node_service_from_static_method [label="service_from_static_method\nBar\\FooClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_factory_simple [label="factory_simple\nSimpleFactoryClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_factory_service_simple [label="factory_service_simple\nBar\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_shared_private [label="shared_private\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_shared_private_dep1 [label="shared_private_dep1\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_shared_private_dep2 [label="shared_private_dep2\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_service_container [label="service_container\nSymfony\\Component\\DependencyInjection\\ContainerBuilder\n", shape=record, fillcolor="#9999ff", style="filled"];
node_foo2 [label="foo2\n\n", shape=record, fillcolor="#ff9999", style="filled"];
node_foo3 [label="foo3\n\n", shape=record, fillcolor="#ff9999", style="filled"];
Expand All @@ -43,4 +46,6 @@ digraph sc {
node_inlined -> node_baz [label="setBaz()" style="dashed"];
node_baz -> node_foo_with_inline [label="setFoo()" style="dashed"];
node_configurator_service -> node_baz [label="setFoo()" style="dashed"];
node_shared_private_dep1 -> node_shared_private [label="" style="dashed"];
node_shared_private_dep2 -> node_shared_private [label="" style="dashed"];
}
Loading