[Security] Expose the required roles in AccessDeniedException #18661

Nicofuma · 2016-04-28T09:38:18Z

Q	A
Branch?	master (or earlier, it is rather small)
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
License	MIT

Nowadays it is more and more common to protect some sensitive actions and part of a website using 2FA or some re-authentication mechanism (per example, on Github you have to enter your password again when you add an ssh key). But currently, in Symfony, it is really hard to implement without having to duplicate the logic, provide an explicit list of URLs to protect or hack into the security component.

A good way to achieve that would be to add a special role (like IS_AUTHENTICATED_FULLY) and use it in the access map. But it requires us to be able to have a custom logic in an ExceptionListener depending on the roles behind an AccessDeniedException.

With this patch we could write an ExceptionListener of this kind (a similar logic could also be used in an AccessDeniedHandler):

    public function onKernelException(GetResponseForExceptionEvent $event)
    {
        $exception = $event->getException();
        do {
            if ($exception instanceof AccessDeniedException) {
                foreach ($exception->getAttributes() as $role) {
                    if ($role === 'IS_AUTHENTICATED_2FA' && !$this->accessDecisionManager->decide($this->tokenStorage->getToken(), $role, $exception->getObject())) {
                        // Start 2FA
                    }
                }
            }
        } while (null !== $exception = $exception->getPrevious());
    }

stloyd · 2016-04-28T09:58:44Z

src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php

@@ -18,8 +18,43 @@
 */
 class AccessDeniedException extends \RuntimeException
 {
+    private $attributes = [];


This should be array(), not short syntax.

linaori · 2016-04-28T10:07:26Z

Just a wild brainfart, wouldn't it be easier to register on specific attributes to abstract away the kernel.exception part?

interface VoterExceptionHandlerInterface
    public function handleException(\Throwable $exception): Response;

This interface could be added to your voter. The kernel.exception handler can iterate over the known voters, call "supports" if the attribute/data match and then call handleException on that specific voter.

class YourVoter extends Voter implements VoterExceptionHandlerInterface

stof · 2016-04-28T10:33:03Z

@Nicofuma it would make sense to call these setters in the FrameworkBundle Controller shortcut too (when they are available)

stof · 2016-04-28T10:33:25Z

And tests are broken

stof · 2016-04-28T10:33:40Z

btw, this is a new feature, so it cannot go into 2.8

Nicofuma · 2016-04-28T12:15:05Z

@iltar you could do that, but I don't think it is the role of the voter. The authenticated voter knows how to tell if the user is authenticated or not but it shouldn't know what to do.

@stof indeed it is a new feature... but it would be sad to have that in 3.2 only and small features (in terms of code and implications) have already been introduced in minor versions in the past.

About the tests (php7, high), I guess the error is related to the security-core dependency in security-http but I don't really know how it is tested and what should be done here.

fabpot · 2016-06-14T06:14:12Z

@Nicofuma What's the status of this PR?

@javiereguiluz

… CommandTester (chalasr) This PR was merged into the 3.2-dev branch. Discussion ---------- [Console] Simplify simulation of user inputs in CommandTester | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | symfony/symfony-docs#6623 After @javiereguiluz pointed it in symfony#17470, I open this PR to simplify the simulation of user inputs for testing a Command. It would be done by calling `CommandTester::setUserInputs()` with an array of inputs as argument, and so make the CommandTester creating an input stream from the inputs set by the developer, then call `QuestionHelper::setInputStream` and assign it to the helperSet of the command, sort as all is done automatically in one call. Depends on symfony#18999 Commits ------- c7ba38a [Console] Set user inputs from CommandTester

…nt fragment (rodnaph) This PR was merged into the 3.2-dev branch. Discussion ---------- [Router] added appending of new optional document fragment added a new optional parameter to the generate method for the document fragment. when specified this is appended to generated urls. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | none Commits ------- 6d79a56 [Routing] adds _fragment special option to url generation for document fragment

…ists (jkphl) This PR was squashed before being merged into the 3.2-dev branch (closes symfony#19029). Discussion ---------- [YAML] Fixed parsing problem with nested DateTime lists | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | ___ Without this fix, DateTime-aware parsing of a YAML source containing nested lists of dates result in an error. Consider this: ```php $data = ['date' => ['annivesary' => new \DateTime('now')]]; $yaml = Yaml::dump($data); var_dump($yaml); $parsed = Yaml::parse($yaml, Yaml::PARSE_DATETIME); print_r($parsed); ``` Everything is fine, result is: ``` string(48) "date: annivesary: 2016-06-11T11:26:30+02:00 " Array ( [date] => Array ( [annivesary] => DateTime Object ( [date] => 2016-06-11 11:26:30.000000 [timezone_type] => 1 [timezone] => +02:00 ) ) ) ``` But making the `anniversary` a list of dates ```php $data = ['date' => ['annivesary' => [new \DateTime('now')]]]; $yaml = Yaml::dump($data); var_dump($yaml); $parsed = Yaml::parse($yaml, Yaml::PARSE_DATETIME); print_r($parsed); ``` will result in: ``` string(50) "date: annivesary: [2016-06-11T12:00:05+02:00] " PHP Warning: strpos() expects parameter 1 to be string, object given in [...]\vendor\symfony\yaml\Inline.php on line 382 PHP Catchable fatal error: Object of class DateTime could not be converted to string in [...]\vendor\symfony\yaml\Inline.php on line 386 ``` (I didn't capture the error messages with the most recent master branch, so line numbers differ somewhat) Commits ------- 52384cf [YAML] Fixed parsing problem with nested DateTime lists

…ists (jkphl, xabbuh) This PR was merged into the 3.1 branch. Discussion ---------- [YAML] Fixed parsing problem with nested DateTime lists | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#19029 | License | MIT | Doc PR | The new handling for `DateTimeInterface` instances was introduced in Symfony 3.1. Commits ------- 0f47712 parse embedded mappings only if value is a string 4f13a76 [YAML] Fixed parsing problem with nested DateTime lists

…ng params (xabbuh) This PR was merged into the 3.2-dev branch. Discussion ---------- [Routing] treat fragment after resolving query string params | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | The implementation from symfony#12979 led to a conflict with symfony#18280 which was merged in the meantime. Commits ------- 7475aa8 treat fragment after resolving query string params

…tted form (Ener-Getick) This PR was merged into the 3.2-dev branch. Discussion ---------- Deprecate using Form::isValid() with an unsubmitted form | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | symfony#7737 | License | MIT | Doc PR | not yet > I'm calling out for you to decide how to resolve the inconsistency between the Form::isValid() method and the ``valid`` variable available in the template: > > - ``isValid()`` returns ``false`` if a form was not submitted. This way it is possible to write concise controller code: > > ```php > $form = $this->createForm(...); > $form->handleRequest($request); > if ($form->isValid()) { > // only executed if the form is submitted AND valid > } > ``` > > - ``valid`` contains ``true`` if a form was not submitted. This way it is possible to rely on this variable for error styling of a form. > > ```twig > <div{% if not form.vars.valid %} class="error"{% endif %}> > ``` > > We have two alternatives for resolving this problem: > > 1. Leave the inconsistency as is. > 2. Make ``isVal 8000 id()`` return ``true`` if a form was not submitted (consistent with ``valid``) > 3. Revert to the 2.2 behavior of throwing an exception if ``isValid()`` is called on a non-submitted form (not consistent with ``valid``). > > Both 2. and 3. will require additional code in the controller: > > ```php > $form = $this->createForm(...); > $form->handleRequest($request); > if ($form->isSubmitted() && $form->isValid()) { > // only executed if the form is submitted AND valid > } > ``` > > What do you think? This PR implements the option 3 as it was the most chosen in symfony#7737 Commits ------- 2c3a7cc Deprecate using Form::isValid() with an unsubmitted form

* 3.1: fixed bad merge Fix PHP 7.1 related failures [VarDumper] Fix for 7.1 fixed CS Added class existence check if is_subclass_of() fails in compiler passes Fix the DBAL session handler version check for Postgresql

* 2.8: Fix merge Conflicts: src/Symfony/Component/DependencyInjection/composer.json

* 3.0: Fix merge

* 3.1: Fix merge Fix merge Conflicts: src/Symfony/Component/DependencyInjection/composer.json

* 2.8: [Console] Application update PHPDoc of add and register methods [Config] Extra tests for Config component Fixed bugs in names of classes and methods. [DoctrineBridge] Fixed php doc [FrameworkBundle] Fixed parameters number mismatch declaration [BrowserKit] Added test for followRedirect method (POST method) Fix the money form type render with Bootstrap3 [BrowserKit] Uppercase the "GET" method in redirects [DomCrawler] Inherit the namespace cache in subcrawlers [WebProfilerBundle] Fixed JSDoc parameter definition [HttpFoundation] HttpCache refresh stale responses containing an ETag Conflicts: src/Symfony/Component/Finder/Tests/Shell/CommandTest.php

* 3.0: [Console] Application update PHPDoc of add and register methods [Config] Extra tests for Config component Fixed bugs in names of classes and methods. [DoctrineBridge] Fixed php doc [FrameworkBundle] Fixed parameters number mismatch declaration [BrowserKit] Added test for followRedirect method (POST method) Fix the money form type render with Bootstrap3 [BrowserKit] Uppercase the "GET" method in redirects [DomCrawler] Inherit the namespace cache in subcrawlers [WebProfilerBundle] Fixed JSDoc parameter definition [HttpFoundation] HttpCache refresh stale responses containing an ETag

* 3.1: [Console] Application update PHPDoc of add and register methods [Config] Extra tests for Config component Fixed bugs in names of classes and methods. [DoctrineBridge] Fixed php doc [FrameworkBundle] Fixed parameters number mismatch declaration [BrowserKit] Added test for followRedirect method (POST method) Fix the money form type render with Bootstrap3 [BrowserKit] Uppercase the "GET" method in redirects [DomCrawler] Inherit the namespace cache in subcrawlers [WebProfilerBundle] Fixed JSDoc parameter definition [HttpFoundation] HttpCache refresh stale responses containing an ETag Conflicts: src/Symfony/Component/Console/Application.php

…n (greg0ire) This PR was merged into the 3.1 branch. Discussion ---------- Reference the actual location of the documentation | Q | A | ------------- | --- | Branch? | 3.1 because bug is not present before | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | The old link was giving a 404 Commits ------- 98dc69e Reference the actual location of the documentation

…le (David Badura) This PR was submitted for the master branch but it was merged into the 3.1 branch instead (closes symfony#19434). Discussion ---------- [Serializer] enable property info in framework bundle | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - I've been wondering why the datetime normalizer won't work in combination with object normalizer in symfony 3.1 by default. I also found nothing in the documentation. Only http://symfony.com/blog/new-in-symfony-3-1-datetime-normalizer but here is not described how to use it with the object normalizer and how to configure the config.yml. After debugging i found out that the object normalizer don't use the new property info component. With my change, you can enabled it with following config: ```yml serializer: enabled: true property_info: ~ ``` Should i add analog to `enable_annotation` an `enable_property_info` option? ```yml serializer: enabled: true enable_property_info: true property_info: ~ ``` Commits ------- c02933d enable property info

…ystemAdapter (nicolas-grekas) This PR was merged into the 3.1 branch. Discussion ---------- [Cache] Fix incorrect timestamps generated by FilesystemAdapter | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#19431 | License | MIT | Doc PR | - Commits ------- bd2e795 [Cache] Fix incorrect timestamps generated by FilesystemAdapter

@ecanovas

…-grekas) This PR was merged into the 3.1 branch. Discussion ---------- [Cache] Fix default lifetime being ignored | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/cache#1 | License | MIT | Doc PR | - Courtesy of @ecanovas Commits ------- 35ba478 [Cache] Fix default lifetime being ignored

…act (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [VarDumper] Dumping exceptions is now more compact | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | - | License | MIT | Doc PR | - Before: ![before](https://cloud.githubusercontent.com/assets/243674/16578686/2c7285b0-429c-11e6-9139-293e0c899d43.png) Commits ------- 19e9cbe [VarDumper] Dumping exceptions is now more compact

* 2.8: [TwigBundle] Removed redundant return statement. [DependencyInjection] Fixed deprecated default message template with XML [TwigBridge] Removed extra arguments in 2 places. [Process] Fix write access check for pipes on Windows [HttpKernel] Use flock() for HttpCache's lock files

* 3.0: [TwigBundle] Removed redundant return statement. [DependencyInjection] Fixed deprecated default message template with XML [TwigBridge] Removed extra arguments in 2 places. [Process] Fix write access check for pipes on Windows [HttpKernel] Use flock() for HttpCache's lock files

* 3.1: [TwigBundle] Removed redundant return statement. enable property info [Cache] Fix default lifetime being ignored [DependencyInjection] Fixed deprecated default message template with XML Reference the actual location of the documentation [TwigBridge] Removed extra arguments in 2 places. [Cache] Fix incorrect timestamps generated by FilesystemAdapter [Process] Fix write access check for pipes on Windows [HttpKernel] Use flock() for HttpCache's lock files Conflicts: src/Symfony/Component/Cache/Adapter/FilesystemAdapter.php

fabpot · 2016-07-28T20:32:48Z

@Nicofuma Still here?

Nicofuma · 2016-07-29T07:11:31Z

Sorry, I was a bit busy and I completely forgot about it. Replaced by #19473 (against maser)

…ception (Nicofuma) This PR was merged into the 3.2-dev branch. Discussion ---------- [Security] Expose the required roles in AccessDeniedException | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Nowadays it is more and more common to protect some sensitive actions and part of a website using 2FA or some re-authentication mechanism (per example, on Github you have to enter your password again when you add an ssh key). But currently, in Symfony, it is really hard to implement without having to duplicate the logic, provide an explicit list of URLs to protect or hack into the security component. A good way to achieve that would be to add a special role (like IS_AUTHENTICATED_FULLY) and use it in the access map. But it requires us to be able to have a custom logic in an ExceptionListener depending on the roles behind an AccessDeniedException. With this patch we could write an ExceptionListener of this kind (a similar logic could also be used in an AccessDeniedHandler): ```php public function onKernelException(GetResponseForExceptionEvent $event) { $exception = $event->getException(); do { if ($exception instanceof AccessDeniedException) { foreach ($exception->getAttributes() as $role) { if ($role === 'IS_AUTHENTICATED_2FA' && !$this->accessDecisionManager->decide($this->tokenStorage->getToken(), $role, $exception->getObject())) { // Start 2FA } } } } while (null !== $exception = $exception->getPrevious()); } ``` Replaces #18661 Commits ------- 6618c18 [Security] Expose the required roles in AccessDeniedException

carsonbot added the Status: Needs Review label Apr 28, 2016

stloyd reviewed Apr 28, 2016
View reviewed changes

javiereguiluz added Security Feature labels Apr 28, 2016

Nicofuma force-pushed the exp_role_ad branch from 9001f9e to ddcfc34 Compare April 28, 2016 12:15

Nicofuma force-pushed the exp_role_ad branch from ddcfc34 to 29cbf65 Compare April 28, 2016 12:27

fabpot and others added 17 commits June 16, 2016 16:59

[YAML] Fixed parsing problem with nested DateTime lists

52384cf

[YAML] Fixed parsing problem with nested DateTime lists

4f13a76

treat fragment after resolving query string params

7475aa8

parse embedded mappings only if value is a string

0f47712

[Cache] Add tags based invalidation + TagAwareRedisAdapter

19764af

Deprecate using Form::isValid() with an unsubmitted form

2c3a7cc

[Console] Add ConsoleLogger::hasErrored()

cfc578a

[Console] ProgressBar - adjust to the window width (static)

b030c24

fixed CS, simplified code

8f206c8

deprecated some Console Application methods

a589635

fixed logic

bf7a5c5

fabpot and others added 23 commits July 19, 2016 12:47

Merge branch '3.1'

a5a03aa

* 3.1: fixed bad merge Fix PHP 7.1 related failures [VarDumper] Fix for 7.1 fixed CS Added class existence check if is_subclass_of() fails in compiler passes Fix the DBAL session handler version check for Postgresql

Merge branch '2.8' into 3.0

a84ea7a

* 2.8: Fix merge Conflicts: src/Symfony/Component/DependencyInjection/composer.json

Merge branch '3.0' into 3.1

6d24c55

* 3.0: Fix merge

Fix merge

eccbe67

Merge branch '3.1'

695549f

* 3.1: Fix merge Fix merge Conflicts: src/Symfony/Component/DependencyInjection/composer.json

[Cache] Fix incorrect timestamps generated by FilesystemAdapter

bd2e795

Reference the actual location of the documentation

98dc69e

[Cache] Fix default lifetime being ignored

35ba478

enable property info

c02933d

[VarDumper] Dumping exceptions is now more compact

19e9cbe

[Cache] Use AdapterTestCase for new adapters

cbc1e54

[Security] Expose the required roles in AccessDeniedException

52a25bb

Nicofuma force-pushed the exp_role_ad branch from 29cbf65 to 52a25bb Compare July 29, 2016 07:05

Nicofuma mentioned this pull request Jul 29, 2016

[Security] Expose the required roles in AccessDeniedException #19473

Merged

Nicofuma closed this Jul 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Security] Expose the required roles in AccessDeniedException #18661

[Security] Expose the required roles in AccessDeniedException #18661

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Security] Expose the required roles in AccessDeniedException #18661

[Security] Expose the required roles in AccessDeniedException #18661

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!