Skip to content
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

[Security] Expose the required roles in AccessDeniedException #18661

Closed
wants to merge 1,836 commits into from

Conversation

Nicofuma
Copy link
Contributor

@Nicofuma Nicofuma commented Apr 28, 2016

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());
    }

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

Choose a reason for hiding this comment

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

This should be array(), not short syntax.

@linaori
Copy link
Contributor

linaori commented Apr 28, 2016

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
Copy link
Member

stof commented Apr 28, 2016

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

@stof
Copy link
Member

stof commented Apr 28, 2016

And tests are broken

@stof
Copy link
Member

stof commented Apr 28, 2016

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

@Nicofuma
Copy link
Contributor Author

@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
Copy link
Member

fabpot commented Jun 14, 2016

@Nicofuma What's the status of this PR?

fabpot and others added 17 commits June 16, 2016 16:59
… 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 ``isValid()`` 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
fabpot and others added 23 commits July 19, 2016 12:47
* 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.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
…-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
…olas-grekas)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Cache] Use AdapterTestCase for new adapters

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

Commits
-------

cbc1e54 [Cache] Use AdapterTestCase for new adapters
@fabpot
Copy link
Member

fabpot commented Jul 28, 2016

@Nicofuma Still here?

@Nicofuma
Copy link
Contributor Author

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

@Nicofuma Nicofuma closed this Jul 29, 2016
fabpot added a commit that referenced this pull request Aug 9, 2016
…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
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.

None yet