Skip to content

UserValueResolver and SecurityUserValueResolver improvement #26971

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

UserValueResolver and SecurityUserValueResolver improvement #26971

wants to merge 3 commits into from

Conversation

davidkmenta
Copy link

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

UserValueResolver and SecurityUserValueResolver should also resolve class implementing UserInterface.

David Kmenta added 2 commits April 18, 2018 11:51
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

this was on my list :)

@@ -40,7 +40,7 @@ public function __construct(TokenStorageInterface $tokenStorage)
public function supports(Request $request, ArgumentMetadata $argument)
{
// only security user implementations are supported
if (UserInterface::class !== $argument->getType()) {
if (!$argument->getType() || !$this->implementsCorrectInterface($argument->getType())) {
Copy link
Member

Choose a reason for hiding this comment

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

Should be in Symfony\Component\Security\Http\Controller\UserValueResolver instead (see deprecation notice above)

Copy link
Author

Choose a reason for hiding this comment

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

It's in both classes :)

Copy link
Member

Choose a reason for hiding this comment

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

Deprecated classes are frozen until they are suppressed, it should not get new features.

*/
private function implementsCorrectInterface($type)
{
return UserInterface::class === $type || array_key_exists(UserInterface::class, class_implements($type));
Copy link
Member

Choose a reason for hiding this comment

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

Let's inline this logic in the calling condition

Copy link
Member

Choose a reason for hiding this comment

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

isn't this what is_a() or is_subclass_of() does?

Copy link
Author

Choose a reason for hiding this comment

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

Didn't know 😳thanks!

@linaori
Copy link
Contributor

linaori commented Apr 18, 2018

Discussion why this feature wasn't introduced when initially adding the resolver: #18510 (comment)

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Apr 18, 2018
@davidkmenta
Copy link
Author

@iltar thanks for the link. I agree that it may collide with the ParamConverter. On the other hand I still think the UserValueResolver should support classes implementing the UserInterface.

Few quick ideas:

  • What about making it configurable or togglable?
  • Is a ParamConverter called before or after a ValueResolver? Or not at all?

@jvasseur
Copy link
Contributor

jvasseur commented Apr 19, 2018

I agree with @davidkmenta here, the param converters are executed before the value resolver so the concern highlighted in #18510 (comment) shouldn't be a problem.

Forcing users to type they controller arguments with UserInterface gives bad DX for IDE auto-completion/static analysis.

@linaori
Copy link
Contributor

linaori commented Apr 19, 2018

@davidkmenta @jvasseur If you type-hint an entity, it will actually try to inject that entity based on the request, which will fail, because there's no way of fetching it. This is because the parameter converters are triggered first.

Forcing users to type they controller arguments with UserInterface gives bad DX for IDE auto-completion/static analysis.

If people use it as quick way to get their current user object, why not provide a (by the SecurityBundle) configurable AVR that can return a given object based on the current UserInterface? That would be a generic solution to a common problem.

@jvasseur
Copy link
Contributor

@iltar didn't think of that since I tend do disable the automatic conversion.

We need to think of the future of the future of param converters (sensiolabs/SensioFrameworkExtraBundle#436 ?) maybe it's time to deprecate them and introduce configurable argument resolvers, this would allow us to have a configurable security user resolver.

@linaori
Copy link
Contributor

linaori commented Apr 19, 2018

Yes, something has to be done about the parameter converters, but I'm not sure what and I don't have much time myself 😢

Regarding the current AVR, what if we add an additional interface to the UserProviderInterface?

interface DomainUserProviderInterface
{
    // would return your entity for example, names to be discussed
    public function convertToDomainUser(UserInterface $user);
}
# follows the basic configuration
security
    providers:
        hostnet:
            id: App\Security\Authentication\CustomerPortal\UserProvider

This would make it possible for the AVR to resolve the security user to a domain user. If you implement the interface on your user provider, it will simply be "enabled" for whatever has that user. We have the firewall information available, so perhaps it's not even hard to find the user provider.

@davidkmenta
Copy link
Author

So I guess this PR can be closed for now :)

@gmponos
Copy link
Contributor

gmponos commented Jul 6, 2019

TBH I do not follow why this was closed because of bundle that it's not inside symfony.. Personally I do not use Sensio bundles... :(

I believe that this is a problem of sensio bundle that should either decorate the user AVR or overwrite it.

@linaori
Copy link
Contributor

linaori commented Jul 6, 2019

Symfony needs a generic controller configuration system that works similar to the idea behind the annotations from that bundle (but configurable without annotations as well). When this is done, AVR can fully replace the parameter converters and it won't be an issue anymore.

The issue arises because a lot of developers do use the SensioFrameworkExtraBundle, because it also adds features such as @Security. There's also effort being made to deprecate the user object from the Token, so the idea is to be able to inject your Domain user into an action based on this token, which means this AVR wouldn't even be necessary anymore. This won't solve the aforementioned issue though.

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.

7 participants