Skip to content

Navigation Menu

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

[Bridge][Twig] Decouple the SecurityExtension from its runtime #24692

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 2 commits into from
Closed

[Bridge][Twig] Decouple the SecurityExtension from its runtime #24692

wants to merge 2 commits into from

Conversation

CarsonF
Copy link
Contributor

@CarsonF CarsonF commented Oct 25, 2017

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

This indirectly fixes two bugs in Silex regarding Security and Twig.

The first is if twig is loaded before boot() some services/parameters are not defined.

  • twig asks for security.authorization_checker
  • security.authorization_checker asks for security.authentication_manager
  • security.authentication_manager asks for security.authentication_providers
  • security.authentication_providers are not completely defined until security.firewall_map is loaded (here).

I fixed this locally by wrapping the security.authorization_checker service and loading the security.firewall_map before hand:

// Load firewall map before this, because it defines a dependency needed.
$factory = $app->raw('security.authorization_checker');
$app['security.authorization_checker'] = function ($app) use ($factory) {
    $app['security.firewall_map'];

    return $factory($app);
};

This technically fixed the problem, but it wouldn't be needed if the service wasn't loaded until boot() where the firewall is loaded by default.


The second problem is a circular dependency. I was implementing an AccessDeniedHandler that rendered a template.

  • security.firewall_map asks for AccessDeniedHandler service
  • AccessDeniedHandler service asks for twig
  • twig asks for security.firewall_map ....:boom:

And again this wouldn't be a problem if twig didn't need the SecurityRuntime until it's is_granted is called.


I don't see this as a feature since it only modifies private services. And I do see this as a bugfix even though it's for Silex and not Symfony. But I know it's a gray area. Twig Runtimes seem to be the way we are heading so I don't think this is a step in the wrong direction either.

I have a PR ready for Silex depending on the direction of this issue. Here's the diff.

$this->securityChecker = $securityChecker;
}

public function isGranted($role, $object = null, $field = null)
Copy link
Member

Choose a reason for hiding this comment

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

This is a BC break, it has to be deprecated first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even though it's never called directly only through the twig function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fabpot did the same thing without a BC break to HttpKernelExtension
812fbb4

Copy link
Member

@chalasr chalasr Oct 25, 2017

Choose a reason for hiding this comment

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

Anyone extending this class (e.g. to replace the security extension by a custom one relying on these methods) would be broken. IMO it's not acceptable.

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 can fix BC break if it really is considered a BC break. I don't want that to be a blocker.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this is acceptable, for the reason stated: i.e. 812fbb4 … namely, this is moving what always should have been to where it, um, should have been … calling it a BC break versus history is just bikeshedding

Copy link
Member

@chalasr chalasr Oct 26, 2017

Choose a reason for hiding this comment

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

Well, it was a BC break, we just accepted it (and merged as a feature, not a bugfix).
There is a lot of classes in the codebase that should not be used as extension points, and we don't change their public API from minor to minor, per semver and our BC promise.
I would flag it as @final (same for other extensions) and stop injecting the authorization checker in the extension service for 4.1, then remove its methods in 5.0. At least the class would remain as is for 4.x.
Let's see what others think

@chalasr chalasr added this to the 4.1 milestone Oct 25, 2017
if (null !== $securityChecker) {
@trigger_error(sprintf('The $securityChecker parameter is deprecated since 3.4 and will be removed in 4.0. Use %s instead.', SecurityRuntime::class), E_USER_DEPRECATED);
}

$this->securityChecker = $securityChecker;
}

public function isGranted($role, $object = null, $field = null)
Copy link
Member

Choose a reason for hiding this comment

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

needs a docblock with @deprecated

@CarsonF
Copy link
Contributor Author

CarsonF commented Jul 10, 2018

Due to Silex being EoL I'm closing this.

@CarsonF CarsonF closed this Jul 10, 2018
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
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.

5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.