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

[SecurityBundle] Move cache of the firewall context into the request parameters#22943

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

Closed

GromNaN wants to merge2 commits intosymfony:3.2fromGromNaN:firewall-context

Closed

[SecurityBundle] Move cache of the firewall context into the request parameters#22943

GromNaN wants to merge2 commits intosymfony:3.2fromGromNaN:firewall-context

Conversation

Copy link

Member

GromNaN commentedMay 29, 2017•
edited
Loading

Followingthis proposal. Since the matching context relates to the request, this information should have been cached inside the request parameters.

Q	A
Branch?	3.2
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#22605
License	MIT
Doc PR	n/a

Avoid memory leak when handling multiple requests
Adding the new request parameter_firewall_context might be considered as a breaking change. That adds a new "public" property that could be used by end developers.

carsonbot added Status: Needs Review SecurityBundle labels

May 29, 2017

stof requested changes

May 29, 2017

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php

		{
		if ($this->contexts->contains($request)) {
		return$this->contexts[$request];
		if ($request->attributes->has('_firewall_context')) {

Copy link

Member

stofMay 29, 2017

Choose a reason for hiding this comment

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

you should validate that$request->attributes->get('_firewall_context') is a valid key of$this->map

Copy link

MemberAuthor

GromNaNMay 29, 2017

Choose a reason for hiding this comment

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

Done. And added test assertions on the value of this attribute.

Copy link

Member

stof commentedMay 29, 2017

why is it opened against the master branch if you want it for 3.4 ? master is 4.0

GromNaN changed the base branch frommaster to3.4

May 29, 2017 17:25

GromNaN changed the base branch from3.4 tomaster

May 29, 2017 17:26

GromNaN force-pushed thefirewall-context branch from9aa00ce to664b413Compare

May 29, 2017 17:26

GromNaN changed the base branch frommaster to3.4

May 29, 2017 17:26

GromNaN force-pushed thefirewall-context branch fromaef5787 to1622703Compare

May 29, 2017 17:37

ogizanagi approved these changes

May 29, 2017

View reviewed changes

chalasr requested changes

May 30, 2017

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php Outdated

		return$this->contexts[$request];
		if ($request->attributes->has('_firewall_context')) {
		$contextId =$request->attributes->get('_firewall_context');
		if (array_key_exists($contextId,$this->map)) {

Copy link

Member

chalasrMay 30, 2017

Choose a reason for hiding this comment

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

$this->map can beTraversable (andit is)

nicolas-grekas added this to the3.4 milestone

May 30, 2017

chalasr approved these changes

May 31, 2017

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

Build failures unrelated

GromNaN force-pushed thefirewall-context branch fromd9625f4 toe4e7454Compare

June 5, 2017 20:27

Copy link

MemberAuthor

GromNaN commentedJun 5, 2017

Is it possible that a sub-request get created with all the attributes of its master request? In that case, using the request attributes as a cache would be wrong.

fabpot requested changes

Jun 9, 2017

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.

Why does it target 3.4. Looks like a bug fix to me, so it should be done in earliest branches as well.

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php Outdated

		}

		/**
		* @param Request $request

Copy link

Member

fabpotJun 9, 2017

Choose a reason for hiding this comment

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

should be removed

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php Outdated

		/**
		* @param Request $request
		*
		* @return \Symfony\Bundle\SecurityBundle\Security\FirewallContext

Copy link

Member

fabpotJun 9, 2017

Choose a reason for hiding this comment

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

You should add ause statement and useFirewallContext here instead.

Copy link

MemberAuthor

GromNaNJun 10, 2017

Choose a reason for hiding this comment

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

After this change, fabbot reports is as an unused import and removes theuse statement.

fabpot mentioned this pull request

Jun 9, 2017

[SecurityBundle] Fixed a memory leak in SecurityBundle\Security\FirewallMap#22605

Closed

GromNaN force-pushed thefirewall-context branch 2 times, most recently fromf335537 toe306ec0Compare

June 9, 2017 12:19

GromNaN changed the base branch from3.4 to3.2

June 9, 2017 12:19

GromNaN force-pushed thefirewall-context branch frome306ec0 toaa9745dCompare

June 9, 2017 12:23

Copy link

MemberAuthor

GromNaN commentedJun 9, 2017

@fabpot The cache was introduced in 3.2. I've updated the PR.

fabpot requested changes

Jun 10, 2017

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php Outdated

		namespaceSymfony\Bundle\SecurityBundle\Security;

		usePsr\Container\ContainerInterface;
		useSymfony\Bundle\SecurityBundle\Security\FirewallContext

Copy link

Member

fabpotJun 10, 2017

Choose a reason for hiding this comment

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

missing; at then end

Copy link

MemberAuthor

GromNaNJun 10, 2017

Choose a reason for hiding this comment

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

Sorry, fixed.

GromNaN force-pushed thefirewall-context branch fromaa9745d to3108023Compare

June 10, 2017 16:32

Move cache of the firewall context into the request parameters

ffaceb1

Validate that _firewall_context attribute is in the map keys

GromNaN force-pushed thefirewall-context branch from3108023 toffaceb1Compare

June 10, 2017 16:41

Copy link

Member

fabpot commentedJun 10, 2017

Tests are broken.

Fix test for Symfony 3.2, code was written for Symfony 3.4

86bae5a

GromNaN commented

Jun 10, 2017

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Tests/Security/FirewallMapTest.php


		$firewallContext =$this->getMockBuilder(FirewallContext::class)->disableOriginalConstructor()->getMock();
		$firewallContext->expects($this->once())->method('getConfig')->willReturn('CONFIG');
		$firewallContext->expects($this->once())->method('getContext')->willReturn(array('LISTENERS','EXCEPTION LISTENER'));

Copy link

MemberAuthor

GromNaNJun 10, 2017•
edited
Loading

Choose a reason for hiding this comment

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

After merging into 3.3,getContext is replaced bygetListeners andgetExceptionListener, update this line to:

$firewallContext->expects($this->once())->method('getListeners')->willReturn('LISTENERS');$firewallContext->expects($this->once())->method('getExceptionListener')->willReturn('EXCEPTION LISTENER');

Copy link

Member

fabpotJun 14, 2017

Choose a reason for hiding this comment

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

Done, thanks

fabpot approved these changes

Jun 14, 2017

View reviewed changes

Copy link

Member

fabpot commentedJun 14, 2017

Thank you@GromNaN.

fabpot added a commit that referenced this pull request

Jun 14, 2017

bug#22943[SecurityBundle] Move cache of the firewall context into t…

dddc5bd

…he request parameters (GromNaN)This PR was squashed before being merged into the 3.2 branch (closes#22943).Discussion----------[SecurityBundle] Move cache of the firewall context into the request parametersFollowing [this proposal](#22605 (comment)). Since the matching context relates to the request, this information should have been cached inside the request parameters.| Q             | A| ------------- | ---| Branch?       | 3.2| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#22605| License       | MIT| Doc PR        | n/a* Avoid memory leak when handling multiple requests* Adding the new request parameter `_firewall_context` might be considered as a breaking change. That adds a new "public" property that could be used by end developers.Commits-------b3203cb [SecurityBundle] Move cache of the firewall context into the request parameters

Copy link

Member

chalasr commentedJun 14, 2017

should be closed

fabpot closed this

Jun 14, 2017

This was referencedJul 4, 2017

Release v3.2.10#23368

Merged

Release v3.3.3#23369

Merged

KateSyrotchuk mentioned this pull request

Jul 26, 2017

[Security Bundle][Route] Hash _fragment is lost during generation of login_path#23675

Closed

fancyweb mentioned this pull request

Dec 13, 2019

[SecurityBundle][FirewallMap] Remove unused property#34973

Merged

nicolas-grekas added a commit that referenced this pull request

Dec 13, 2019

minor#34973[SecurityBundle][FirewallMap] Remove unused property (fa…

30e6ddf

…ncyweb)This PR was merged into the 3.4 branch.Discussion----------[SecurityBundle][FirewallMap] Remove unused property| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | no| License       | MIT| Doc PR        | -This property is unused. It was removed in#22943. It was likely reintroduced in a merge commit. It is still on master.Commits-------0904e57 [SecurityBundle][FirewallMap] Remove unused property