Adding a shortcuts for the main security functionality #24337
Conversation
| @@ -4,6 +4,8 @@ CHANGELOG | |||
| 3.4.0 | |||
| ----- | |||
|
|
|||
| * Added new `security.helper` service that is an instance of `Symfony\Component\Security\Core` | |||
There was a problem hiding this comment.
Symfony\Component\Security\Core\Security
| /** | ||
| * @return UserInterface|null | ||
| */ | ||
| public function getUser() |
There was a problem hiding this comment.
Return type instead of phpdoc?
There was a problem hiding this comment.
No, this targets 3.4 which supports PHP 5.6.
|
I think I good name for this would be $access->getUser();
$access->getToken(); |
|
@weaverryan small conflict to resolve |
| <argument type="service"> | ||
| <service class="Symfony\Component\DependencyInjection\ServiceLocator"> | ||
| <tag name="container.service_locator" /> | ||
| <argument type="collection"> |
There was a problem hiding this comment.
You should make the Security class implement ServiceSubscriberInterface instead. But maybe you prefer no dep on the DI component?
There was a problem hiding this comment.
Yea... that was the basic idea: it felt better to keep the Security class from the component coupled only to psr/container. And actually, that wasn't listed as a dep in composer.json. I'm going to add that now... tests must've passed because another dep of symfo
8000
ny/security already required it?
There was a problem hiding this comment.
Haha, or because I don't have a test for that class... fixing that now :)
|
|
||
| $user = $token->getUser(); | ||
| if (!is_object($user)) { | ||
| return; |
There was a problem hiding this comment.
as of PHP 7.1 we have the void return type (reference) so I suggest making the null explicit:
return null;|
I'm still 👎 on this for a few important reasons:
I feel that making shortcuts like this improves the short-term problem solving of applications, but also makes it easier to entangle your http layer with your domain layer. 3+ years later after making such decisions, I still have to suffer through this Technical Debt. |
|
This proposal looks like a small change ... but it's a "game changer". See the before/after comparison: Beforepublic function indexAction(TokenStorageInterface $tokenStorage)
{
$user = $tokenStorage->getToken()->getUser();
}Afterpublic function indexAction(Security $security)
{
$user = $security->getUser();
}Beforepublic function indexAction(AuthorizationCheckerInterface $authorizationChecker)
{
if ($authorizationChecker->isGranted('EDIT', $item)) {
// ...
}
}Afterpublic function indexAction(Security $security)
{
if ($security->isGranted('EDIT', $item)) {
// ...
}
}The Security part is the biggest problem for Symfony users. We need to make big and immediate changes to improve the situation. This proposal goes in that direction hiding internal details that users shouldn't care about. |
|
Truly, the security component is one of the biggest Symfony problems. In the current implementation, I would keep only |
|
@javiereguiluz but what about this? /** @Security("is_granted('EDIT', item)") */
public function indexAction(UserInterface $user, Item $item)
{
// ...
}Granted, this requires the SensioFrameworkExtraBundle, but I rather see a generic solution for this (security annotation) as action requirement being added. If the length of the AuthorizationCheckerInterface is the problem, maybe this can be solved by making an alias? Edit: don't get me wrong, |
|
@iltar that's fine ... but as an alternative. Symfony is pushing hard towards autowiring + service injection. That's why we must introduce this public function indexAction(AuthorizationCheckerInterface $authorizationChecker) |
|
Just brainfarting here, but what about: public function indexAction(IsGranted $isGranted, Item $item)
{
if ($isGranted('EDIT', $item)) {
// ...
}
}Which would feel similar to: /**
* @IsGranted("EDIT", subject="item")
*/ |
|
@iltar I don't like the
A single and centralized |
|
The downside is that it promotes another solution to fetch the User object, execute authorization checks and getting the token. I know why you want to add it, I just don't think it's a good idea because it often implies you might be on the wrong track of doing things. If it's about long names, you could also simply rename the
Side note, my signatures often are |
|
@iltar we'll never agree on this ... but that's fine. I still think we need this to solve the most common security needs in a simple way. |
|
The idea is to make things simpler, but this change will give them more options to do the same, which seems counter productive to me, that's what's bothering me at the moment. If that can be fixed, I'll be all 👍 |
|
Im not sure this helps in terms of teaching people they should decouple their security user from it's source of truth (an entity or so). Which IMHO is the real thing to solve, by teaching better practices. Today i do it like this Thanks to this awesome article; https://stovepipe.systems/post/decoupling-your-security-user Works perfect today and doesnt require any additional wrapper/god objects etc. |
9a7f4f5 to
04c774d
Compare
|
Hey guys! About decoupling the For 3.4, we need this shortcut. And we can always deprecate and remove this if we move forward with a different philosophy in 4.1. In fact, I would love that - the Security component is benefiting from fresh thinking :). This PR is ready for review again: unit & functional tests added, and |
|
I believe the failure is false: it's failing (https://travis-ci.org/symfony/symfony/jobs/280454407#L2945) because the new method on |
| @@ -17,6 +17,7 @@ | |||
| ], | |||
| "require": { | |||
| "php": "^5.5.9|>=7.0.8", | |||
| "psr/container": "^1.0", | |||
There was a problem hiding this comment.
Shouldn't this be in require-dev? The dep is required only when using new Security() (same below).
| /** | ||
| * This class holds security information. | ||
| * Helper class for commonly-needed security tasks. | ||
| * | ||
| * @author Johannes M. Schmitt <schmittjoh@gmail.com> |
|
Thank you Ryan. |
… (weaverryan, javiereguiluz) This PR was squashed before being merged into the 3.4 branch (closes #24337). Discussion ---------- Adding a shortcuts for the main security functionality | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | Big ol' TODO I'd like one class that I can inject (especially with autowiring) to get access to the User and `isGranted()` methods. This is *really* important... because to get the User currently, you need to type-hint `TokenStorageInterface`... and there are *two*! That's really bad DX! Questions: A) I hi-jacked the existing `Security` class... I wanted a simple class called Security B) I called the service `security.helper`... for lack of a better id. C) I did not make `Security` implement the 2 other interfaces (`TokenStorageInterface`, `AuthorizationCheckerInterface`... but I suppose we could?) Cheers! Commits ------- 0851189 Adding a shortcuts for the main security functionality
|
wow, why was it rushed out like this? I'm more than skeptical about these changes. |
|
@fabpot There is one specific problem that we must solve (and this PR is one solution for it). To get the User object in auto-wired way, you need to type-hint Secondarily, this gives an easier way to access the user, versus Wdyt? Would you like to accomplish that in a different way? Or were your concerns more minor? |
| } | ||
|
|
||
| $user = $token->getUser(); | ||
| if (!is_object($user)) { |
There was a problem hiding this comment.
Can't the user be a simple string in some cases?
I mean, the TokenInterface says this:
/**
* Returns a user representation.
*
* @return mixed Can be a UserInterface instance, an object implementing a __toString method,
* or the username as a regular string
*
* @see AbstractToken::setUser()
*/
public function getUser();So instead, this should return mixed instead of UserInterface...
There was a problem hiding this comment.
Yep, but I'm purposely not allowing for people who use a string User to make use of this feature. We do that in other places. Allowing it to be a string makes everyone's user experience worse. So, you can do this, but you need to use the lower-level tools :)
|
There is a UserInterface argument resolver. Why would you want the token storage in your controllers? Imo, this is moving in completely the opposite direction: The concept of user had to be removed, not emphasized... See also #21068 (which was closed because "a clearer Security roadmap had to be created before changing anything") |
Not in the controllers, it's for when you want the User in a service. In a controller, I would just say I think longer-term, we need to look into bigger changes. But right now, this We need this as a short-term solution to that one problem. Longer term? Yea, let's investigate some other stuff. But we can't do that other stuff for 3.4 :). |
|
And we can't simply rename TokenStorageInterface to something else as a short term solution? The problem here imho is that you're not only solving this autowire problem, you're also reintroducing SecurityContext which was removed with very good arguments. |
|
@wouterj I disagree. To me, removing the SecurityContext was a mistake. As an end-user I don't care about the internals: I just need to get the user and protect resources. If Symfony internally separates that and implements classes and interfaces ... that's an internal detail which I don't care about (unless I care about and investigate the internals of Symfony ... but let's not make that mandatory for all users). |
As an end-user, I just need to secure stuff. That's all the security component should do. The fact that authentication results in a unique identifier, giving you the ability to provide details for that specific client, is just a bonus. And this bonus is used in such a domain specific way, that a framework shouldn't imply anything about it. This is clearly shown, as even the domains used in all the Symfony Documentation examples don't fit the imposed methods of Security's UserInterface. I know that's hijacking this PR a bit. But my point is: We should remove the I need to get the user idea sooner than later from the Security component. In #21068 , if anything, I proofed that doing this is a real hard game. Adding fancy classes like this make this game even more complex. Until we get at Symfony 4.3, have still no Security roadmap and it's impossible to move away from users as the core team kept working using the I need to get the user-concept for 3 releases. Now, I understand the problem Ryan is trying to solve (I've faced it as well). However, it can be solved in a much smaller way by just renaming |
|
Just a quick note: SecurityContext was split in large part because it caused lazy loading problems between the service for authorization and the token storage. That’s not a problem anymore due to the service locator used here. But I’m fine if we’d rather rename the interface. But I’d still prefer this (until we have time to dive further into bigger changes) as it also gives you the getUser() shortcut, which just makes sense given how people currently configure their security. Also, nothing in this PR prevents broader changes in the future. I vote to keep it as is. |
|
Sorry to comment on such old PR, but I'd like to ask a question, maybe I'm missing something. This class is PITA in tests, it has This is the code I came up with: class SecurityMockFactory
{
public static function create(User $user = null)
{
$prophet = new Prophet();
$token = $prophet->prophesize(TokenInterface::class);
$token->getUser()->willReturn($user);
$tokenStorage = $prophet->prophesize(TokenStorageInterface::class);
$tokenStorage->getToken()->willReturn($token);
$container = $prophet->prophesize(ContainerInterface::class);
$container->get('security.token_storage')->willReturn($tokenStorage);
$authorizationChecker = $prophet->prophesize(AuthorizationCheckerInterface::class);
$authorizationChecker->isGranted(AuthenticatedVoter::IS_AUTHENTICATED_REMEMBERED, null)->willReturn(null !== $user);
$container->get('security.authorization_checker')->willReturn($authorizationChecker);
return new Security($container->reveal());
}
}Do you see any easier way to mock this class? |
…ss (xabbuh) This PR was merged into the 3.4 branch. Discussion ---------- [Security] use final annotation to allow mocking the class | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29946 | License | MIT | Doc PR | When the class was initially marked as `final`, it did only contain constants. Since #24337 the `Security` class also contains useful shortcut methods so allowing developers to mock the class in tests looks reasonable to me. Commits ------- 1da00db use final annotation to allow mocking the class
Description ----------- The Security Helper has been added in Symfony 3.4: symfony/symfony#24337 This PR does two things: 1. It uses `isGranted` instead of checking the Contao user object, which is totally incorrect in a Symfony context 2. It simplifies a lot of code because the helper class already abstracts a `null` token when fetching the user. I consider this a bug fix because we **must** always use `isGranted` and not check the Contao user. ~~There are usages of the user in the pickers as well, but I'm not (yet) sure how to migrate that.~~ fixed in f0981e7 Commits ------- 7d20c9b Use the security helper f0981e7 Rewrite the picker providers to use security voters 919f974 Use default value for the access validation 7235615 Catch exception if firewall is not active (e.g. 404 pages) 4d4c603 Fix the coding style
Description ----------- The Security Helper has been added in Symfony 3.4: symfony/symfony#24337 This PR does two things: 1. It uses `isGranted` instead of checking the Contao user object, which is totally incorrect in a Symfony context 2. It simplifies a lot of code because the helper class already abstracts a `null` token when fetching the user. I consider this a bug fix because we **must** always use `isGranted` and not check the Contao user. ~~There are usages of the user in the pickers as well, but I'm not (yet) sure how to migrate that.~~ fixed in f0981e7cf6546bac6a776a64826ab7b96fe956d4 Commits ------- 7d20c9b3 Use the security helper f0981e7c Rewrite the picker providers to use security voters 919f9744 Use default value for the access validation 7235615d Catch exception if firewall is not active (e.g. 404 pages) 4d4c6032 Fix the coding style
Description ----------- The Security Helper has been added in Symfony 3.4: symfony/symfony#24337 This PR does two things: 1. It uses `isGranted` instead of checking the Contao user object, which is totally incorrect in a Symfony context 2. It simplifies a lot of code because the helper class already abstracts a `null` token when fetching the user. I consider this a bug fix because we **must** always use `isGranted` and not check the Contao user. ~~There are usages of the user in the pickers as well, but I'm not (yet) sure how to migrate that.~~ fixed in f0981e7cf6546bac6a776a64826ab7b96fe956d4 Commits ------- 7d20c9b3 Use the security helper f0981e7c Rewrite the picker providers to use security voters 919f9744 Use default value for the access validation 7235615d Catch exception if firewall is not active (e.g. 404 pages) 4d4c6032 Fix the coding style
Description ----------- The Security Helper has been added in Symfony 3.4: symfony/symfony#24337 This PR does two things: 1. It uses `isGranted` instead of checking the Contao user object, which is totally incorrect in a Symfony context 2. It simplifies a lot of code because the helper class already abstracts a `null` token when fetching the user. I consider this a bug fix because we **must** always use `isGranted` and not check the Contao user. ~~There are usages of the user in the pickers as well, but I'm not (yet) sure how to migrate that.~~ fixed in f0981e7cf6546bac6a776a64826ab7b96fe956d4 Commits ------- 7d20c9b3 Use the security helper f0981e7c Rewrite the picker providers to use security voters 919f9744 Use default value for the access validation 7235615d Catch exception if firewall is not active (e.g. 404 pages) 4d4c6032 Fix the coding style
Description ----------- The Security Helper has been added in Symfony 3.4: symfony/symfony#24337 This PR does two things: 1. It uses `isGranted` instead of checking the Contao user object, which is totally incorrect in a Symfony context 2. It simplifies a lot of code because the helper class already abstracts a `null` token when fetching the user. I consider this a bug fix because we **must** always use `isGranted` and not check the Contao user. ~~There are usages of the user in the pickers as well, but I'm not (yet) sure how to migrate that.~~ fixed in f0981e7cf6546bac6a776a64826ab7b96fe956d4 Commits ------- 7d20c9b3 Use the security helper f0981e7c Rewrite the picker providers to use security voters 919f9744 Use default value for the access validation 7235615d Catch exception if firewall is not active (e.g. 404 pages) 4d4c6032 Fix the coding style
Description ----------- The Security Helper has been added in Symfony 3.4: symfony/symfony#24337 This PR does two things: 1. It uses `isGranted` instead of checking the Contao user object, which is totally incorrect in a Symfony context 2. It simplifies a lot of code because the helper class already abstracts a `null` token when fetching the user. I consider this a bug fix because we **must** always use `isGranted` and not check the Contao user. ~~There are usages of the user in the pickers as well, but I'm not (yet) sure how to migrate that.~~ fixed in f0981e7cf6546bac6a776a64826ab7b96fe956d4 Commits ------- 7d20c9b3 Use the security helper f0981e7c Rewrite the picker providers to use security voters 919f9744 Use default value for the access validation 7235615d Catch exception if firewall is not active (e.g. 404 pages) 4d4c6032 Fix the coding style
I'd like one class that I can inject (especially with autowiring) to get access to the User and
isGranted()methods. This is really important... because to get the User currently, you need to type-hintTokenStorageInterface... and there are two! That's really bad DX!Questions:
A) I hi-jacked the existing
Securityclass... I wanted a simple class called SecurityB) I called the service
security.helper... for lack of a better id.C) I did not make
Securityimplement the 2 other interfaces (TokenStorageInterface,AuthorizationCheckerInterface... but I suppose we could?)Cheers!