added a way to enforce a 403 response on an access denied exception #308

Merged
merged 2 commits into from Oct 3, 2012

Projects

None yet

3 participants

@lsmith77
Owner
lsmith77 commented Oct 3, 2012

fixes #74

@schmittjoh can you give some feedback on this approach?

@schmittjoh schmittjoh and 1 other commented on an outdated diff Oct 3, 2012
DependencyInjection/Configuration.php
@@ -41,6 +41,11 @@ public function getConfigTreeBuilder()
$rootNode
->children()
+ ->arrayNode('access_denied_listener')
+ ->useAttributeAsKey('name')
+ ->defaultValue(array())
schmittjoh
schmittjoh Oct 3, 2012 Contributor

This is not necessary for prototypes.

@schmittjoh schmittjoh commented on the diff Oct 3, 2012
EventListener/AccessDeniedListener.php
+ *
+ * @param ContainerInterface $container container
+ * @param boolean $setParamsAsAttributes params as attributes
+ */
+ public function __construct($formats = array())
+ {
+ $this->formats = $formats;
+ }
+
+ /**
+ * @param GetResponseForExceptionEvent $event The event
+ */
+ public function onKernelException(GetResponseForExceptionEvent $event)
+ {
+ $exception = $event->getException();
+ if (!$exception instanceof AccessDeniedException) {
schmittjoh
schmittjoh Oct 3, 2012 Contributor

This would apply to all firewalls, but since it is not enabled by default, I can't think of any negative side-effects that it would have right now.

lsmith77
lsmith77 Oct 3, 2012 Owner

yeah its a bit brute force .. but easy to use .. think for most users that will be fine.

@lsmith77 lsmith77 merged commit 7dc85eb into master Oct 3, 2012

1 check passed

default The Travis build passed
Details
Contributor

Note that it correctly throws a 403 but the associated messaged is not serialized according to the Content-Type which can lead to buggy implementation on client side.

Response Headers

[...]
Content-Type:application/json
[...]

Response Body
You dont have the necessary permissions

I don't know if it is an excepted behavior, but I think IMO it should be formatted according to the Content-Type.
It should be done easily by injecting the ViewHandler to the Listener to handle() a View.

Owner
lsmith77 commented Oct 4, 2012

good point .. we should probably leverage the ExceptionController. can you look into that?

Contributor

@lsmith77 not sure what you mean, can you give me a clue?

Owner
lsmith77 commented Oct 4, 2012

i just pushed a totally untested branch where i started on my proposal. would be awesome if you can finish it .. either using my commit as a basis or just inspiration.

Contributor

Ok I'll take a look into that.

@lsmith77 lsmith77 referenced this pull request in symfony/symfony Oct 8, 2012
Closed

Returns HTTP 401 response on an XML HTTP request. #5705

@borisguery borisguery added a commit to borisguery/FOSRestBundle that referenced this pull request Oct 16, 2012
@lsmith77 @borisguery lsmith77 + borisguery Reuse the ExceptionController in the AccessDeniedListener, ref #308
(Fixed things to make them work)
* Added unit tests
* Added symfony/security to the required-dev package to
composer.json
* Removed useless constructor
* Fixed wording
a2a063d
@lsmith77 lsmith77 added a commit that referenced this pull request Nov 6, 2012
@lsmith77 lsmith77 Merge branch 'access_denied_tweaks'
* access_denied_tweaks:
  check format
  reuse the ExceptionController in the AccessDeniedListener, ref #308

Conflicts:
	EventListener/AccessDeniedListener.php
41da15b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment