Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[TwigBridge] Create impersonation_exit_path() and *_url() functions#24737
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
[TwigBridge] Create impersonation_exit_path() and *_url() functions#24737
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Simperfit commentedOct 29, 2017
Status: Needs work |
dc9057a tob84b82fComparejaviereguiluz commentedOct 29, 2017
I haven't looked at the code yet ... but couldn't we move these functions to |
Simperfit commentedOct 29, 2017
I guess we could move these ans Logout's one to the SecurityExtension, if it's ok, Ill do it |
Simperfit commentedNov 1, 2017
@javiereguiluz Do we merge all in securityExtension ? |
yceruto left a comment
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.
Thank you 👍
| * | ||
| * @return string The Impersonate exit URL | ||
| */ | ||
| privatefunctiongenerateImpersonateExitUrl($referenceType) |
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.
we're in master so I guess we can use typehint everywhere?
| if ($this->getImpersonatedUser() &&null !==$switchUserConfig =$firewallConfig->getSwitchUser()) { | ||
| $exitPath =$request->getRequestUri(); | ||
| $exitPath .=null ===$request->getQueryString() ?'?' :'&'; | ||
| $exitPath .=sprintf( |
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.
one line?
| /** | ||
| * Generates the absolute logout path for the firewall. | ||
| **. |
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.
broken line.
| return$url; | ||
| } | ||
| privatefunctiongetImpersonatedUser() |
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.
I think this function is useful only to know if the current user is being impersonated, thus it should return a boolean value always and should be named something likeisImpersonatedUser()?
| private$tokenStorage; | ||
| private$firewallMap; | ||
| publicfunction__construct(RequestStack$requestStack =null,UrlGeneratorInterface$router =null,TokenStorageInterface$tokenStorage =null,FirewallMapInterface$firewallMap) |
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.
I'm not sure if making these arguments nullable is meaningful, because this service depends exclusively on them. Instead the service definition could be removed if some of them don't exists?
javiereguiluz commentedMar 19, 2018
@Simperfit I know this feature is not a game-changer for Symfony ... but it improves DX a lot for this use case. Although it's almost finished, we'd need a final push before the "feature freeze" starts in 12 days. Thanks a lot 🙏 |
Simperfit commentedMar 21, 2018
I understand@javiereguiluz I'm going to finish this feature before the feature freeze. (This week-end I think). |
c8186e6 to0296974CompareSimperfit commentedMar 26, 2018
I'm adding tests today. |
0296974 todbe432eComparedbe432e to9e5ba7fCompare
ostrolucky left a comment
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.
I don't see why does this feature differentiate if current request is/ or not.
Also big flaw is assumption user wants to exit at current URL. I don't know how you guys do this, but I always exit impersonation at URL where impersonation was started (e.g. user edit page in admin section)
| private$tokenStorage; | ||
| private$firewallMap; | ||
| publicfunction__construct(RequestStack$requestStack,UrlGeneratorInterface$router,TokenStorageInterface$tokenStorage =null,FirewallMapInterface$firewallMap) |
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 class does not work with other implementation than FirewallMap, so you can as well just require it, instead of interface and get rid of an instanceof check. Also not sure why is non-optional argument last.
| $assignedRoles =$token->getRoles(); | ||
| foreach ($assignedRolesas$role) { | ||
| if ($roleinstanceof SwitchUserRole) { |
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.
Rather than this mechanism to check if user is currently impersonating, shouldn't it be done the way docs recommend? That means checking if user has ROLE_PREVIOUS_ADMIN via AuthorizationChecker.
https://symfony.com/doc/current/security/impersonating_user.html
| return$this->generateImpersonateExitUrl(UrlGeneratorInterface::ABSOLUTE_URL); | ||
| } | ||
| privatefunctiongenerateImpersonateExitUrl($referenceType):string |
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.
It would be more flexible if this was public and twig functions would call this with correct parameters. We then wouldn't need two public methods. Also, missing typehint.
| if ($this->isImpersonatedUser() &&null !==$switchUserConfig =$firewallConfig->getSwitchUser()) { | ||
| $exitPath =$request->getRequestUri(); | ||
| $exitPath .=null ===$request->getQueryString() ?'?' :'&'; | ||
| $exitPath .=sprintf('%s=%s',urlencode($switchUserConfig['parameter']), SwitchUserListener::EXIT_VALUE); |
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.
using http_build_query would be better
| } | ||
| if ('/' ===$exitPath[0]) { | ||
| if (!$this->requestStack) { |
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 is a dead code. RequestStack is required so will never be null
| $url .='?'.http_build_query($parameters); | ||
| } | ||
| }else { | ||
| if (!$this->router) { |
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.
another dead path
| thrownew \LogicException('Unable to generate the impersonate exit URL without a Router.'); | ||
| } | ||
| $url =$this->router->generate($exitPath,array(),$referenceType); |
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 is wrong, first parameter of route path is name of the route, query string is given instead
| $container->removeDefinition('security.access.expression_voter'); | ||
| } | ||
| if (!class_exists('Symfony\Component\HttpFoundation\RequestStack') || !class_exists('Symfony\Component\Routing\Generator\UrlGeneratorInterface')) { |
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.
Why would RequestStack not exist? It's required by dependencies
| private$impersonateUrlGenerator; | ||
| publicfunction__construct(AuthorizationCheckerInterface$securityChecker =null) | ||
| publicfunction__construct(AuthorizationCheckerInterface$securityChecker =null,ImpersonateUrlGenerator$impersonateUrlGenerator) |
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.
I think PHP strongly discourages non-optional arguments after optional
http://php.net/manual/en/functions.arguments.php
Note that when using default arguments, any defaults should be on the right side of any non-default arguments; otherwise, things will not work as expected.
javiereguiluz commentedSep 12, 2018
Could we please make an effort to finish this PR before this month's end so it can make it for Symfony 4.2? Thanks! |
fabpot commentedOct 10, 2018
Let's close as there is not more activity here and it looks like it is far from being finished. |
… (dFayet)This PR was merged into the 5.2-dev branch.Discussion----------Create impersonation_exit_path() and *_url() functions| Q | A| ------------- | ---| Branch? | 4.4| Bug fix? | no| New feature? | yes <!-- please update src/**/CHANGELOG.md files -->| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass? | yes (not added atm) <!-- please add some, will be required by reviewers -->| Fixed tickets |#24676 <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | To come later <!-- symfony/symfony-docs#... --><!-- required for new features -->This is a relaunch of the PR#24737It adds more flexibility to the previous try.You have two twig functions `impersonation_exit_url()` and `impersonation_exit_path()`You can either leave on the same path, redirect to the path where was the user at the user switch, or to the path you want.Example:The following code```twig<p>{{ impersonation_exit_url() }}</p><p>{{ impersonation_exit_path() }}</p><p> </p><p>{{ impersonation_exit_url(path('app_default_other')) }}</p><p>{{ impersonation_exit_path(path('app_default_other')) }}</p><p> </p><p>{{ impersonation_exit_url('_use_impersonated_from_url') }}</p><p>{{ impersonation_exit_path('_use_impersonated_from_url') }}</p>```will output**Note:** If this proposal appears to be better, I'll add tests, update changelog, and prepare the doc.**Bonus:**As the "impersonated from url" is stored in the `SwitchUserToken` it might be possible to display it in the profiler:WDYT?<!--Replace this notice by a short README for your feature/bugfix. This will help peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://symfony.com/roadmap): - Bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too). - Features and deprecations must be submitted against branch 4.4. - Legacy code removals go to the master branch.-->Commits-------c1e3703 Create impersonation_exit_path() and *_url() functions
I opened the PR in order to see if the implementation is right