-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
$this->securityChecker = $securityChecker; | ||
} | ||
|
||
public function isGranted($role, $object = null, $field = null) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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) |
There was a problem hiding this comment.
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
Due to Silex being EoL I'm closing this. |
This indirectly fixes two bugs in Silex regarding Security and Twig.
The first is if
twig
is loaded beforeboot()
some services/parameters are not defined.twig
asks forsecurity.authorization_checker
security.authorization_checker
asks forsecurity.authentication_manager
security.authentication_manager
asks forsecurity.authentication_providers
security.authentication_providers
are not completely defined untilsecurity.firewall_map
is loaded (here).I fixed this locally by wrapping the
security.authorization_checker
service and loading thesecurity.firewall_map
before hand: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 servicetwig
twig
asks forsecurity.firewall_map
....:boom:And again this wouldn't be a problem if
twig
didn't need theSecurityRuntime
until it'sis_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.