Skip to content
This repository has been archived by the owner on Feb 24, 2023. It is now read-only.

Fix #574, split IsGrantedListener event #602

Closed
wants to merge 3 commits into from
Closed

Fix #574, split IsGrantedListener event #602

wants to merge 3 commits into from

Conversation

TomaszGasior
Copy link

@TomaszGasior TomaszGasior commented Jan 15, 2019

Split IsGrantedListener::onKernelControllerArguments() event into
two separate events.

  • New IsGrantedListener::onKernelController() is attached to
    KernelEvents::CONTROLLER and runs only @IsGranted annotations
    WITHOUT subject.
  • IsGrantedListener::onKernelControllerArguments() is attached to
    KernelEvents::CONTROLLER_ARGUMENTS (no change here) but only
    runs @IsGranted annotations WITH subject.

This way simple @IsGranted annotations will be interpreted before
preparing arguments for controller action, which fixes #574.
As example — now, there is a way to type-hint UserInterface (from
Security bundle) in controller action without need to set nullable
type-hint.

Split IsGrantedListener::onKernelControllerArguments() event into
two separate events.

* New IsGrantedListener::onKernelController() is attached to
  KernelEvents::CONTROLLER and runs only @IsGranted annotations
  WITHOUT subject.
* IsGrantedListener::onKernelControllerArguments() is attached to
  KernelEvents::CONTROLLER_ARGUMENTS (no change here) but only
  runs @IsGranted annotations WITH subject.

This way simple @IsGranted annotations will be interpreted *before*
preparing arguments for controller action, which fixes #574.
As example — now, there is a way to type-hint UserInterface (from
Security bundle) in controller action without need to set nullable
type-hint.
TomaszGasior added a commit to TomaszGasior/RadioLista-v3 that referenced this pull request Jan 15, 2019
@TomaszGasior
Copy link
Author

Unfortunately, my pull request fixes the problem only for one @IsGanted use case — when it's used WITHOUT subject. With subject specified, problem still exists and it's not possible to completely fix it with current Symfony architecture. But, for better developer experience, I added IsGrantedListener::onKernelException which warns Symfony user about unsupported usage of @IsGranted (with subject and UserInterface type hint).

https://i.imgur.com/jGmwfz9.png

It's a little bit hackish but in my opinion Symfony users are worth it. ;)

@Koc
Copy link
Contributor

Koc commented May 15, 2019

I have the same problem, thanx for opening PR. Would be nice to merge it.
Only one thing that I don't understand - why are you subscribe to kernel.exception event?

// cc @fabpot

@TomaszGasior
Copy link
Author

I have the same problem, thanx for opening PR. Would be nice to merge it.
Only one thing that I don't understand - why are you subscribe to kernel.exception event?

My intention is to show for the user clear error message and instruct him how to fix the problem. Please see my comment: #602 (comment)

@mpoiriert
Copy link

When this pull request will be merge ?

Also a work around for using IsGranted with a subject is to have two is IsGranted annotation, one that validate a role (@IsGranted("ROLE_AUTHENTICATED")) and the other one with the subject. That way the first one will fail if the user is not authenticated and will stop everything else. Afterwards injection or annotation will be executed properly. This would be cleaner than have optional argument, will work with all the case. Still need this PR to be merge for it to work...

@azjezz
Copy link

azjezz commented Jul 18, 2019

@TomaszGasior needs a rebase :)

@nicolas-grekas
Copy link
Collaborator

@azjezz would you like to take over this PR? I'd then invite you to squash all commits from @TomaszGasior in one, then add yours + send a new PR, when you have some time :)

@azjezz
Copy link

azjezz commented Jul 23, 2019

@nicolas-grekas sure 👍 :)

}
}

public function onKernelControllerArguments(FilterControllerArgumentsEvent $event)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this type hints a deprecated class, isn't it?
better remove the type hint and replace it with an annotation with the non-deprecated type
that'll preserve compat with 3.4 while preparing for 5.0
please check other event too

@TomaszGasior
Copy link
Author

TomaszGasior commented Jul 24, 2019

Since I am busy on other work I hope @azjezz will do good job. Sorry for late response. Closing this PR since @azjezz may want create new created new. Thx!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants