Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Expose the required roles in AccessDeniedException#18661
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Closed
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| */ | ||
| class AccessDeniedExceptionextends \RuntimeException | ||
| { | ||
| private$attributes = []; |
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This should bearray(), not short syntax.
Contributor
linaori commentedApr 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 publicfunction handleException(\Throwable$exception):Response; This interface could be added to your voter. The class YourVoterextends Voterimplements VoterExceptionHandlerInterface |
Member
stof commentedApr 28, 2016
@Nicofuma it would make sense to call these setters in the FrameworkBundle Controller shortcut too (when they are available) |
Member
stof commentedApr 28, 2016
And tests are broken |
Member
stof commentedApr 28, 2016
btw, this is a new feature, so it cannot go into 2.8 |
ContributorAuthor
Nicofuma commentedApr 28, 2016
@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. |
Member
fabpot commentedJun 14, 2016
@Nicofuma What's the status of this PR? |
… 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#6623After@javiereguiluz pointed it insymfony#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 onsymfony#18999Commits-------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 fragmentadded 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 | noneCommits-------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 (closessymfony#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 382PHP 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 string4f13a76 [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 fromsymfony#12979 led to a conflict withsymfony#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 %}{% 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 insymfony#7737Commits-------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 mergeConflicts:src/Symfony/Component/DependencyInjection/composer.json
* 3.0: Fix merge
* 3.1: Fix merge Fix mergeConflicts: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 ETagConflicts: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 ETagConflicts: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 404Commits-------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 (closessymfony#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. Onlyhttp://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@ecanovasCommits-------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: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 filesConflicts: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
Member
fabpot commentedJul 28, 2016
@Nicofuma Still here? |
ContributorAuthor
Nicofuma commentedJul 29, 2016
Sorry, I was a bit busy and I completely forgot about it. Replaced by#19473 (against maser) |
fabpot added a commit that referenced this pull requestAug 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 | MITNowadays 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#18661Commits-------6618c18 [Security] Expose the required roles in AccessDeniedExceptionSign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
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):