Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[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.

Sign up for GitHub

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
ro0NL wants to merge 5 commits into from
Closed

[DI] Randomize private services #19683

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
286536f
poc randomizing private services
ro0NL Aug 20, 2016
bc3d636
fixed tests
ro0NL Aug 20, 2016
1313bc1
refactored
ro0NL Aug 21, 2016
2cc36db
try public randomized definitions
ro0NL Aug 21, 2016
e62470e
guarantee BC
ro0NL Aug 21, 2016
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
Next Next commit
poc randomizing private services
  • Loading branch information
@ro0NL
ro0NL committed Aug 20, 2016
commit 286536feb57c0887452b2db6556b4bf5bc7310be
4 changes: 4 additions & 0 deletions 4 src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php
View file
Open in desktop
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
90 changes: 90 additions & 0 deletions 90 src/Symfony/Component/DependencyInjection/Compiler/RandomizePrivateServiceIdentifiers.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
<?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\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Alias;
use Symfony\Component\DependencyInjection\Definition;
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

@nicolas-grekas nicolas-grekas Aug 23, 2016

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 •
edited
Loading

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

@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.

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

@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.

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

@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 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

@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.

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;

public function process(ContainerBuilder $container)
{
// build id map
$this->idMap = array();
foreach ($container->getDefinitions() as $id => $definition) {
if (!$definition->isPublic()) {
$this->idMap[$id] = md5(uniqid($id));
}
}

// update private definitions + alias map
$aliases = $container->getAliases();
foreach ($this->idMap as $id => $randId) {
$definition = $container->getDefinition($id);
Definition::markAsPrivateOrigin($id, $definition);
$container->setDefinition($randId, $definition);
$container->removeDefinition($id);
foreach ($aliases as $aliasId => $aliasedId) {
if ((string) $aliasedId === $id) {
$aliases[$aliasId] = new Alias($randId, $aliasedId->isPublic());
}
}
}
$container->setAliases($aliases);

// 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]);
}
}
}

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;
}
}
23 changes: 16 additions & 7 deletions 23 src/Symfony/Component/DependencyInjection/Container.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ class Container implements ResettableContainerInterface

protected $services = array();
protected $methodMap = array();
protected $privates = array();
protected $aliases = array();
protected $loading = array();
protected $privateOriginIds = array();

private $underscoreMap = array('_' => '', '.' => '_', '\\' => '_');

Expand Down Expand Up @@ -178,7 +178,7 @@ public function set($id, $service)
unset($this->services[$id]);
}

if (isset($this->privates[$id])) {
if (false !== $randomizedId = array_search($id, $this->privateOriginIds, true)) {
Copy link
Contributor

@sstok sstok Aug 21, 2016

Choose a reason for hiding this comment

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

It's more performand to store a flipped array so you can still use isset() which is much faster then a function.
Use usage is to resolve an original id to a private id, so you should target for that 👍

The Container is called many times during a request, so performance is key (no pun) here.

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);
} else {
Expand Down Expand Up @@ -207,8 +207,9 @@ public function has($id)
if (--$i && $id !== $lcId = strtolower($id)) {
$id = $lcId;
} else {
if (isset($this->privates[$id])) {
if (false !== $randomizedId = array_search($id, $this->privateOriginIds, true)) {
@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 = $randomizedId;
}

return method_exists($this, 'get'.strtr($id, $this->underscoreMap).'Service');
Expand Down Expand Up @@ -263,6 +264,16 @@ 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 (false !== $randomizedId = array_search($id, $this->privateOriginIds, true)) {
@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);

if ($randomizedId === $id) {
throw new \LogicException(sprintf('Cannot reference a private origin service "%s" with the same id.', $id));
}

return $this->get($randomizedId);
}

if (self::EXCEPTION_ON_INVALID_REFERENCE === $invalidBehavior) {
if (!$id) {
throw new ServiceNotFoundException($id);
Expand All @@ -281,9 +292,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 @@ -341,7 +349,8 @@ public function getServiceIds()
$ids = array();
foreach (get_class_methods($this) as $method) {
if (preg_match('/^get(.+)Service$/', $method, $match)) {
$ids[] = self::underscore($match[1]);
$id = self::underscore($match[1]);
$ids[] = isset($this->privateOriginIds[$id]) ? $this->privateOriginIds[$id] : $id;
}
}
$ids[] = 'service_container';
Expand Down
21 changes: 17 additions & 4 deletions 21 src/Symfony/Component/DependencyInjection/ContainerBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ public function set($id, $service)
throw new BadMethodCallException(sprintf('Setting service "%s" for an unknown or non-synthetic service definition on a frozen container is not allowed.', $id));
}

unset($this->definitions[$id], $this->aliasDefinitions[$id]);
unset($this->definitions[$id], $this->aliasDefinitions[$id], $this->privateOriginIds[$id]);

parent::set($id, $service);
}
Expand All @@ -367,7 +367,9 @@ public function set($id, $service)
*/
public function removeDefinition($id)
{
unset($this->definitions[strtolower($id)]);
$id = strtolower($id);

unset($this->definitions[$id], $this->privateOriginIds[$id]);
}

/**
Expand Down Expand Up @@ -541,14 +543,25 @@ public function compile()
$compiler->compile($this);
$this->compiled = true;

$renameDefinitions = array();
foreach ($this->definitions as $id => $definition) {
if (!$definition->isPublic()) {
$this->privates[$id] = true;
if (null === $privateOriginId = $definition->getPrivateOriginId()) {
Copy link
Contributor

@sstok sstok Aug 21, 2016

Choose a reason for hiding this comment

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

This should be kept within a compiler pass, as depends on the existence of another Compiler pass (which could be disabled).

Copy link
Contributor Author

@ro0NL ro0NL Aug 21, 2016

Choose a reason for hiding this comment

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

Yeah about this.. i intended this as a fallback scenario when there's no randomizing compiler pass.

So if randomizing is bypassed, how do we deal with privates? Track them as before?

Definition::markAsPrivateOrigin($id, $definition);
$privateOriginId = $id;
$id = md5(uniqid($id));
$renameDefinitions[$privateOriginId] = $id;
}
$this->privateOriginIds[$id] = $privateOriginId;
}
if ($this->trackResources && $definition->isLazy() && ($class = $definition->getClass()) && class_exists($class)) {
$this->addClassResource(new \ReflectionClass($class));
}
}
foreach ($renameDefinitions as $oldId => $newId) {
$this->definitions[$newId] = $this->definitions[$oldId];
unset($this->definitions[$oldId]);
}

$this->extensionConfigs = array();

Expand Down Expand Up @@ -735,7 +748,7 @@ public function setDefinition($id, Definition $definition)

$id = strtolower($id);

unset($this->aliasDefinitions[$id]);
unset($this->aliasDefinitions[$id], $this->privateOriginIds[$id]);

return $this->definitions[$id] = $definition;
}
Expand Down
12 changes: 12 additions & 0 deletions 12 src/Symfony/Component/DependencyInjection/Definition.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class Definition
private $decoratedService;
private $autowired = false;
private $autowiringTypes = array();
private $privateOriginId;

protected $arguments;

Expand All @@ -51,6 +52,17 @@ public function __construct($class = null, array $arguments = array())
$this->arguments = $arguments;
}

public static function markAsPrivateOrigin($id, Definition $origin)
{
$origin->public = false;
$origin->privateOriginId = $id;
}

final public function getPrivateOriginId()
{
return !$this->isPublic() ? $this->privateOriginId : null;
}

/**
* Sets a factory.
*
Expand Down
8 changes: 5 additions & 3 deletions 8 src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
use Symfony\Component\DependencyInjection\LazyProxy\PhpDumper\DumperInterface as ProxyDumper;
use Symfony\Component\DependencyInjection\LazyProxy\PhpDumper\NullDumper;
use Symfony\Component\DependencyInjection\ExpressionLanguage;
use Symfony\Component\DependencyInjection\ExpressionLanguageProvider;
use Symfony\Component\ExpressionLanguage\Expression;
use Symfony\Component\HttpKernel\Kernel;

Expand Down Expand Up @@ -839,6 +840,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 •
edited
Loading

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 @@ -903,16 +905,16 @@ private function addPrivateServices()
$code = '';
ksort($definitions);
foreach ($definitions as $id => $definition) {
if (!$definition->isPublic()) {
$code .= ' '.var_export($id, true)." => true,\n";
if (null !== $privateOriginId = $definition->getPrivateOriginId()) {
$code .= ' '.var_export($id, true).' => '.var_export($privateOriginId, true).",\n";
}
}

if (empty($code)) {
return '';
}

$out = " \$this->privates = array(\n";
$out = " \$this->privateOriginIds = array(\n";
$out .= $code;
$out .= " );\n";

Expand Down
29 changes: 24 additions & 5 deletions 29 src/Symfony/Component/DependencyInjection/Tests/ContainerTest.php
View file
Open in desktop
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 @@ -387,14 +397,23 @@ public function __construct()
$this->__bar = new \stdClass();
$this->__foo_bar = new \stdClass();
$this->__foo_baz = new \stdClass();
$this->__internal = new \stdClass();
$this->privates = array('internal' => true);
$this->__rand1_internal = new \stdClass();
$this->__sharing_internal = new \stdClass();
$this->privateOriginIds = array('rand1_internal' => 'internal');
$this->aliases = array('alias' => 'bar');
}

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

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

return $instance;
}

protected function getBarService()
Expand Down
9 changes: 9 additions & 0 deletions 9 src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container9.php
View file
Open in desktop
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;
5 changes: 5 additions & 0 deletions 5 src/Symfony/Component/DependencyInjection/Tests/Fixtures/graphviz/services9.dot
View file
Open in desktop
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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.