NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Create impersonation_exit_path() and *_url() functions#32841

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromdFayet:twig_bridge_impersonation

Sep 6, 2020

Merged

Create impersonation_exit_path() and *_url() functions#32841

fabpot merged 1 commit intosymfony:masterfromdFayet:twig_bridge_impersonation

Sep 6, 2020

Conversation

Copy link

dFayet commentedJul 31, 2019•
edited by javiereguiluz
Loading

Q	A
Branch?	4.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes (not added atm)
Fixed tickets	#24676
License	MIT
Doc PR	To come later

This is a relaunch of the PR#24737

It adds more flexibility to the previous try.
You have two twig functionsimpersonation_exit_url() andimpersonation_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

<p>{{ impersonation_exit_url() }}</p><p>{{ impersonation_exit_path() }}</p><p>&nbsp;</p><p>{{ impersonation_exit_url(path('app_default_other')) }}</p><p>{{ impersonation_exit_path(path('app_default_other')) }}</p><p>&nbsp;</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 theSwitchUserToken it might be possible to display it in the profiler:

WDYT?

carsonbot added Status: Needs Review Feature labels

Jul 31, 2019

dFayet force-pushed thetwig_bridge_impersonation branch fromf898bf1 toef15c60Compare

July 31, 2019 19:11

dFayet commented

Jul 31, 2019

View reviewed changes

src/Symfony/Component/Security/Http/Impersonate/ImpersonateUrlGenerator.php Outdated

		$this->firewallMap =$firewallMap;
		}

		publicfunctiongenerateImpersonateExitUrl(string$exitTo =null,$referenceType = UrlGeneratorInterface::ABSOLUTE_URL):string

Copy link

Author

dFayetJul 31, 2019

Choose a reason for hiding this comment

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

I definitely don't like the nameexitTo, any suggestion?

Copy link

Contributor

ro0NLAug 10, 2019•
edited
Loading

Choose a reason for hiding this comment

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

i suggestgenerateExitUrl/Path(string $targetUri = null): string

dFayet force-pushed thetwig_bridge_impersonation branch fromef15c60 to16a96f1Compare

July 31, 2019 19:17

nicolas-grekas added this to thenext milestone

Aug 2, 2019

Copy link

Author

dFayet commentedAug 8, 2019

ping@Taluu

fabpot reviewed

Aug 9, 2019

View reviewed changes

src/Symfony/Bridge/Twig/CHANGELOG.md OutdatedShow resolvedHide resolved

Taluu approved these changes

Aug 9, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Aug 9, 2019

dFayet force-pushed thetwig_bridge_impersonation branch 2 times, most recently fromb13daf3 toe0c8ebbCompare

August 10, 2019 11:12

ro0NL reviewed

Aug 10, 2019

View reviewed changes

src/Symfony/Component/Security/Http/Impersonate/ImpersonateUrlGenerator.php Outdated


		privatefunctionisImpersonatedUser():bool
		{
		if (null ===$token =$this->tokenStorage->getToken()) {

Copy link

Contributor

ro0NLAug 10, 2019

Choose a reason for hiding this comment

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

return $this->tokenStorage->getToken() instanceof SwitchUserToken?

src/Symfony/Bridge/Twig/Extension/SecurityExtension.phpShow resolvedHide resolved

src/Symfony/Component/Security/Core/Authentication/Token/SwitchUserToken.php Outdated

		class SwitchUserTokenextends UsernamePasswordToken
		{
		private$originalToken;

Copy link

Contributor

ro0NLAug 10, 2019

Choose a reason for hiding this comment

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

newline not needed

src/Symfony/Component/Security/Core/Authentication/Token/SwitchUserToken.php Outdated

		{
		private$originalToken;

		private$impersonatedFromUri;

Copy link

Contributor

ro0NLAug 10, 2019

Choose a reason for hiding this comment

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

what about$originatedFromUri

src/Symfony/Component/Security/Core/Authentication/Token/SwitchUserToken.php Outdated

		* @param mixed $credentials This usually is the password of the user
		* @param string $providerKey The provider key
		* @param string[] $roles An array of roles
		* @param string\|null $impersonatedFromUri The Url where was the user at the switch

Copy link

Contributor

ro0NLAug 10, 2019

Choose a reason for hiding this comment

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

The URI ...

src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php Outdated

		$roles[] =newSwitchUserRole('ROLE_PREVIOUS_ADMIN',$this->tokenStorage->getToken(),false);

		$token =newSwitchUserToken($user,$user->getPassword(),$this->providerKey,$roles,$token);
		$token =newSwitchUserToken(

Copy link

Contributor

ro0NLAug 10, 2019

Choose a reason for hiding this comment

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

i still think we prefer one line :)

Copy link

Member

fabpotAug 11, 2020

Choose a reason for hiding this comment

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

me too :)

src/Symfony/Component/Security/Http/Impersonate/ImpersonateUrlGenerator.php Outdated

		class ImpersonateUrlGenerator
		{
		private$requestStack;

Copy link

Contributor

ro0NLAug 10, 2019

Choose a reason for hiding this comment

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

newlines not needed

src/Symfony/Component/Security/Http/Impersonate/ImpersonateUrlGenerator.php Outdated


		publicfunctiongenerateImpersonateExitUrl(string$exitTo =null,$referenceType = UrlGeneratorInterface::ABSOLUTE_URL):string
		{
		$request =$this->requestStack->getCurrentRequest();

Copy link

Contributor

ro0NLAug 10, 2019

Choose a reason for hiding this comment

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

can be moved down the if the below

src/Symfony/Component/Security/Http/Impersonate/ImpersonateUrlGenerator.php Outdated


		useSymfony\Bundle\SecurityBundle\Security\FirewallMap;
		useSymfony\Component\HttpFoundation\RequestStack;
		useSymfony\Component\Routing\Generator\UrlGeneratorInterface;

Copy link

Contributor

ro0NLAug 10, 2019

Choose a reason for hiding this comment

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

this is only a dev-dependency, not sure we should rely on it here (we dont have to)

perhaps create 2 separate methods as done inSecurityExtension, or inline the consts

ro0NL reviewed

Aug 10, 2019

View reviewed changes

src/Symfony/Component/Security/Http/Impersonate/ImpersonateUrlGenerator.php Outdated

		thrownew \LogicException('Unable to generate the impersonate exit URL without a firewall configured for the user switch.');
		}

		if ('_use_impersonated_from_url' ===$exitTo) {

Copy link

Contributor

ro0NLAug 10, 2019

Choose a reason for hiding this comment

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

perhaps we should decouple this into its own (twig) method so we can do

{{ impersonation_exit_url(impersonation_start_url()) }}

meaning we can implementimpersonation_start_url as a hybrid;
if impersonated: points to "originated from uri"
else: points to "switch user uri"

Copy link

Contributor

ro0NLAug 10, 2019

Choose a reason for hiding this comment

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

and perhaps for completenessis_impersonated()

Copy link

Author

dFayetAug 11, 2019

Choose a reason for hiding this comment

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

and perhaps for completenessis_impersonated()

I don't think it's worth adding this function, as this information can be simply retrieved by doing{% if is_granted('ROLE_PREVIOUS_ADMIN') %}, your thoughts?

Copy link

Author

dFayetAug 11, 2019•
edited
Loading

Choose a reason for hiding this comment

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

@ro0NL About your idea ofimpersonation_start_url should we also consider to create function to get url/path to start impersonation?

I thing the nameimpersonation_start_url might be misleading as it looks like the*_exit* ones, but does not really generate urls to "start" the impersonation.

I would be more confident with names likeimpersonation_get_start_url orimpersonation_started_url.

Copy link

Member

fabpotAug 11, 2020

Choose a reason for hiding this comment

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

I'm not sure I like this_use_impersonated_from_url feature. I would drop it completely (at least at first).

fabpot requested changes

Aug 11, 2020

View reviewed changes

Copy link

Member

fabpot left a comment

Choose a reason for hiding this comment

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

@dFayet I've just made some comments (mainly because core code has changed), can you take the comments into account and rebase on current master?

src/Symfony/Bridge/Twig/CHANGELOG.mdShow resolvedHide resolved

src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml Outdated

		<argumenttype="service"id="request_stack" />
		<argumenttype="service"id="security.firewall.map" />
		<argumenttype="service"id="security.token_storage" />
		</service>

Copy link

Member

fabpotAug 11, 2020

Choose a reason for hiding this comment

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

Should be moved to the new PHP configuration file

src/Symfony/Bundle/SecurityBundle/Resources/config/templating_twig.xml Outdated

		<serviceid="twig.extension.security"class="Symfony\Bridge\Twig\Extension\SecurityExtension">
		<tagname="twig.extension" />
		<argumenttype="service"id="security.authorization_checker"on-invalid="ignore" />
		<argumenttype="service"id="security.impersonate_url_generator"on-invalid="ignore" />

Copy link

Member

fabpotAug 11, 2020

Choose a reason for hiding this comment

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

Should be moved to the new PHP configuration file

src/Symfony/Bundle/SecurityBundle/composer.json Outdated

		"symfony/security-csrf":"^4.2\|^5.0",
		"symfony/security-guard":"^4.2\|^5.0",
		"symfony/security-http":"^4.3"
		"symfony/security-http":"^4.4"

Copy link

Member

fabpotAug 11, 2020

Choose a reason for hiding this comment

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

5.2

src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php Outdated

		$roles[] =newSwitchUserRole('ROLE_PREVIOUS_ADMIN',$this->tokenStorage->getToken(),false);

		$token =newSwitchUserToken($user,$user->getPassword(),$this->providerKey,$roles,$token);
		$token =newSwitchUserToken(

Copy link

Member

fabpotAug 11, 2020

Choose a reason for hiding this comment

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

me too :)

src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php Outdated

		$this->providerKey,
		$roles,
		$token,
		str_replace('/&','/?',preg_replace('#[&?]'.$this->usernameParameter.'=[^&]*#','',$request->getRequestUri()))

Copy link

Member

fabpotAug 11, 2020

Choose a reason for hiding this comment

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

you might want to create a temp var instead

src/Symfony/Component/Security/Http/Impersonate/ImpersonateUrlGenerator.php Outdated

		thrownew \LogicException('Unable to generate the impersonate exit URL without a firewall configured for the user switch.');
		}

		if ('_use_impersonated_from_url' ===$exitTo) {

Copy link

Member

fabpotAug 11, 2020

Choose a reason for hiding this comment

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

I'm not sure I like this_use_impersonated_from_url feature. I would drop it completely (at least at first).

carsonbot added Status: Needs Work and removed Status: Reviewed labels

Aug 11, 2020

Copy link

Member

fabpot commentedSep 6, 2020

@dFayet Do you have time to finish this PR?

Copy link

Author

dFayet commentedSep 6, 2020

@fabpot I will try to finish it today, otherwise, at worst, this week it should be ready

dFayet force-pushed thetwig_bridge_impersonation branch frome0c8ebb to0ac0dc9Compare

September 6, 2020 07:56

dFayet requested review fromdunglas,jderusse,lyrixx,sroze andxabbuh ascode owners

September 6, 2020 07:56

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Sep 6, 2020

dFayet changed the base branch from4.4 tomaster

September 6, 2020 07:57

dFayet force-pushed thetwig_bridge_impersonation branch from0ac0dc9 to3894373Compare

September 6, 2020 07:59

dFayet force-pushed thetwig_bridge_impersonation branch 3 times, most recently from945d41f to18604f6Compare

September 6, 2020 08:40

Create impersonation_exit_path() and *_url() functions

c1e3703

dFayet force-pushed thetwig_bridge_impersonation branch from18604f6 toc1e3703Compare

September 6, 2020 08:52

Copy link

Author

dFayet commentedSep 6, 2020•
edited
Loading

@fabpot This should be ready now.

dFayet requested a review fromfabpot

September 6, 2020 09:05

fabpot approved these changes

Sep 6, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Sep 6, 2020

Copy link

Member

fabpot commentedSep 6, 2020

Thank you@dFayet.

fabpot mentioned this pull request

Sep 6, 2020

Create impersonation_exit_path() and *_url() functionssymfony/symfony-docs#14183

Closed

fabpot merged commit9d15211 intosymfony:master

Sep 6, 2020

dFayet mentioned this pull request

Sep 6, 2020

Leave impersonation functionssymfony/symfony-docs#14185

Merged

xabbuh reviewed

Sep 7, 2020

View reviewed changes

src/Symfony/Component/Security/Core/Authentication/Token/SwitchUserToken.php

		publicfunction__serialize():array
		{
		return [$this->originalToken,parent::__serialize()];
		return [$this->originalToken,$this->originatedFromUri,parent::__serialize()];

Copy link

Member

xabbuhSep 7, 2020

Choose a reason for hiding this comment

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

Don't we need to add at the end to avoid issues with existing data when applications are updated?

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request

Sep 7, 2020

minor#14185Leave impersonation functions (dFayet)

093401e

This PR was merged into the master branch.Discussion----------Leave impersonation functionsWill solve#14183Documents new Twig functions `impersonation_exit_path` and `impersonation_exit_url` added insymfony/symfony#32841Commits-------b7bd572 Leave impersonation functions

nicolas-grekas modified the milestones:next,5.2

Oct 5, 2020

fabpot mentioned this pull request

Oct 5, 2020

Release v5.2.0-BETA1#38417

Merged