Added ViewHandler to AccessDeniedListener to allow correct serialization #309

Merged
merged 1 commit into from Nov 6, 2012

Conversation

Projects
None yet
3 participants
Contributor

borisguery commented Oct 4, 2012

See my comment on PR #308.

@stof stof commented on an outdated diff Oct 4, 2012

EventListener/AccessDeniedListener.php
* Constructor.
*
- * @param ContainerInterface $container container
- * @param boolean $setParamsAsAttributes params as attributes
+ * @param ViewHandlerInterface $viewHandler
+ * @param boolean $setParamsAsAttributes params as attributes
@stof

stof Oct 4, 2012

Owner

Please fix the phpdoc. There is no boolean here but an array

@stof stof commented on an outdated diff Oct 4, 2012

EventListener/AccessDeniedListener.php
*/
- public function __construct($formats = array())
+ public function __construct(ViewHandlerInterface $viewHandler = null, $formats = array())
@stof

stof Oct 4, 2012

Owner

could you add the typehint for the array too ?

@stof stof and 1 other commented on an outdated diff Oct 4, 2012

EventListener/AccessDeniedListener.php
@@ -48,7 +60,14 @@ public function onKernelException(GetResponseForExceptionEvent $event)
$request = $event->getRequest();
if (!empty($this->formats[$request->getRequestFormat()])) {
- $response = new Response('You dont have the necessary permissions', 403);
+ $message = "You don't have the necessary permissions";
+ if ($this->viewHandler) {
@stof

stof Oct 4, 2012

Owner

does it really make sense to make the view handler optional, requiring to add this check ?

@borisguery

borisguery Oct 4, 2012

Contributor

I was not sure, I thought that it may be needed if the view_response_listener was disabled. Not sure if it is needed though.

@stof

stof Oct 4, 2012

Owner

You are not relying on the view response listener here as you are using the ViewHandler explicitly (you cannot rely on the ViewResponseListener btw)

@borisguery

borisguery Oct 4, 2012

Contributor

Indeed, what I meant is that if you disable the view response listener, the view handler would never be called, except manually, so to make things clearer I thought we may enable it in the same way in the listener.

On 4 oct. 2012, at 20:31, Christophe Coevoet notifications@github.com wrote:

In EventListener/AccessDeniedListener.php:

@@ -48,7 +60,14 @@ public function onKernelException(GetResponseForExceptionEvent $event)

     $request = $event->getRequest();
     if (!empty($this->formats[$request->getRequestFormat()])) {
  •        $response = new Response('You dont have the necessary permissions', 403);
    
  •        $message = "You don't have the necessary permissions";
    
  •        if ($this->viewHandler) {
    
    You are not relying on the view response listener here as you are using the ViewHandler explicitly (you cannot rely on the ViewResponseListener btw)


Reply to this email directly or view it on GitHub.

@stof

stof Oct 4, 2012

Owner

As you always inject the handler in this listener in the container, this if will always return true.
The only reason for this if is that you added = null in the constructor to allow someone to use the class without an handler.

@borisguery

borisguery Oct 5, 2012

Contributor

I know, that was just unfinished work here ;)

Owner

lsmith77 commented Oct 4, 2012

Not sure I agree with this approach. Like I said I would prefer to reuse the ExceptionController. F.e. this code here would break if I would enable the 403 response for the html format since I would then have to set a template for the ViewHandler.

Contributor

borisguery commented Oct 5, 2012

Yup I agree with both of you, the idea was just a draft and something out of my head I saved in a commit.

Will take a look on that today.

On 5 oct. 2012, at 00:27, Lukas Kahwe Smith notifications@github.com wrote:

Not sure I agree with this approach. Like I said I would prefer to reuse the ExceptionController. F.e. this code here would break if I would enable the 403 response for the html format since I would then have to set a template for the ViewHandler.


Reply to this email directly or view it on GitHub.

Owner

lsmith77 commented Oct 13, 2012

are you still working on this?

Contributor

borisguery commented Oct 13, 2012

Yes, I got really busy this week, I'll get in touch on Monday for this.

Sent from my iPhone

On 13 oct. 2012, at 12:44, Lukas Kahwe Smith notifications@github.com wrote:

are you still working on this?


Reply to this email directly or view it on GitHub.

Contributor

borisguery commented Oct 16, 2012

(I squashed your previous commit with mine to keep it clear)

@lsmith77 lsmith77 and 1 other commented on an outdated diff Oct 16, 2012

EventListener/AccessDeniedListener.php
$exception = $event->getException();
if (!$exception instanceof AccessDeniedException) {
return;
}
- $request = $event->getRequest();
- if (!empty($this->formats[$request->getRequestFormat()])) {
@lsmith77

lsmith77 Oct 16, 2012

Owner

i intentionally made it possible to configure the formats, because in many cases at least for html people will most likely prefer the current behavior.

@borisguery

borisguery Oct 16, 2012

Contributor

Ah ok, I wasn't sure, because of this borisguery/FOSRestBundle@34f44a7#L0L44.

I'll fix it tomorrow.

@lsmith77

lsmith77 Oct 16, 2012

Owner

we could also turn it into a blacklist of for what formats it should be disabled and default it to html .. not sure ..

@borisguery

borisguery Oct 17, 2012

Contributor

Dunno either, I've no such use cases. Being pragmatic I'm tempted to say that we should blacklist because I can't see many cases when it should be disabled compared to those when it shouldn't. But we may try to gather some feedbacks for this.

Contributor

borisguery commented Oct 22, 2012

Last week I had a talk about another (but similar) problem and we raised a question. Shouldn't we use the AccessDeniedHandler? Afaik, it is a perfect candidate to handle our AccessDeniedException in the ExceptionListener.

I also remember that @schmittjoh said something about the Listener, I may be wrong but IIRC we should not use the Listener in such case. (https://speakerdeck.com/schmittjoh/security-in-real-life?slide=28).

Any thoughts on this?

Owner

lsmith77 commented Oct 25, 2012

the issue with a listener is that you need one for every entry point. this is cleaner but not really feasible.

Owner

lsmith77 commented Nov 6, 2012

see #319 .. could you rebase this PR to just contain the tests?

@lsmith77 lsmith77 added a commit that referenced this pull request Nov 6, 2012

@lsmith77 lsmith77 Merge pull request #319 from FriendsOfSymfony/access_denied_tweaks
reuse the ExceptionController in the AccessDeniedListener, ref #308, #309
b145d92
Contributor

borisguery commented Nov 6, 2012

@lsmith77, do you want me to rebase against master and adapt the test? The Listener signature is different from my implementation and it is likely to fail.

Owner

lsmith77 commented Nov 6, 2012

yes .. that would be great!

@lsmith77 lsmith77 added a commit that referenced this pull request Nov 6, 2012

@lsmith77 lsmith77 Merge pull request #309 from borisguery/access-listener-view-handler
Added ViewHandler to AccessDeniedListener to allow correct serialization
f361ac1

@lsmith77 lsmith77 merged commit f361ac1 into FriendsOfSymfony:master Nov 6, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment