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] add port in access_control#28061
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| * @param string|string[]|null $schemes | ||
| */ | ||
| publicfunction__construct(string$path =null,string$host =null,$methods =null,$ips =null,array$attributes =array(),$schemes =null) | ||
| publicfunction__construct(string$path =null,string$host =null,$port =null,$methods =null,$ips =null,array$attributes =array(),$schemes =null) |
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 BC break, you should add the new arguments at the end of the list
| */ | ||
| publicfunctionmatchPort($port) | ||
| { | ||
| $this->port = (int)$port; |
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.
(int) null => 0 (in this case the value is not clear)
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 can also use a type hint here.
| returnfalse; | ||
| } | ||
| if (0 <$this->port &&$request->getPort() !==$this->port) { |
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 need to ignorenull here
javiereguiluz commentedSep 18, 2018
@chalasr this looks like a useful feature for some people and I can't see any security issue if we introduce this change, but could you please review it? Thanks! |
| * @param string|string[]|null $schemes | ||
| */ | ||
| publicfunction__construct(string$path =null,string$host =null,$methods =null,$ips =null,array$attributes =array(),$schemes =null) | ||
| publicfunction__construct(string$path =null,string$host =null,$methods =null,$ips =null,array$attributes =array(),$schemes =null,$port =null) |
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.
should beint $port = null
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.
Oh yeah, now we can use typehint in this version 👍
| ->example('^/path to resource/') | ||
| ->end() | ||
| ->scalarNode('host')->defaultNull()->end() | ||
| ->scalarNode('port')->defaultNull()->end() |
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 probably don't want to deal with strings here, should be anintegerNode() to me
| $port =isset($firewall['port']) ?$firewall['port'] :null; | ||
| $methods =isset($firewall['methods']) ?$firewall['methods'] :array(); | ||
| $matcher =$this->createRequestMatcher($container,$pattern,$host,$methods); | ||
| $matcher =$this->createRequestMatcher($container,$pattern,$host,$port,$methods); |
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.
$firewall['port'] ?? null, removing the assignment above
roukmouteSep 18, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Can I do the same things for the rest (host, …) or is it must be in another P.R.?
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 would keep them unchanged to avoid conflicts when merging lower branches up to master
| } | ||
| privatefunctioncreateRequestMatcher($container,$path =null,$host =null,$methods =array(),$ip =null,array$attributes =array()) | ||
| privatefunctioncreateRequestMatcher($container,$path =null,$host =null,$port =null,$methods =array(),$ip =null,array$attributes =array()) |
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.
int $port = null
| * | ||
| * @param int|null $port The port number to connect to | ||
| */ | ||
| publicfunctionmatchPort($port) |
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 one should useint $port = null, the existing public methods of this class have been added before php scalar typehints and cannot be changed for BC
| { | ||
| $portInt = (int)$port; | ||
| $this->port =is_numeric($port) ||$port ==$portInt ?$portInt :null; |
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.
should just assign$this->port to$port (see my comment above)
| * | ||
| * @param int|null $port The port number to connect to | ||
| */ | ||
| publicfunctionmatchPort(int$port):void |
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.
missing= null :) (see tests)
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.
please remove the return type also, we don't usevoid generally
| ----- | ||
| * added`getAcceptableFormats()` for reading acceptable formats based on Accept header | ||
| * added`port` property and`matchPort()` in RequestMatcher |
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.
only the method is relevant here, the property is private
| $rules =array(); | ||
| foreach ($container->getDefinition('security.access_map')->getMethodCalls()as$call) { | ||
| if ('add' ==$call[0]) { | ||
| if ('add' ===$call[0]) { |
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 one should be reverted
| $host =isset($firewall['host']) ?$firewall['host'] :null; | ||
| $methods =isset($firewall['methods']) ?$firewall['methods'] :array(); | ||
| $matcher =$this->createRequestMatcher($container,$pattern,$host,$methods); | ||
| $matcher =$this->createRequestMatcher($container,$pattern,$host,$firewall['port'] ??null,$methods); |
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 looks for aport option on a firewall but it does not exist yet
symfony/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php
Line 183 ine923f80
| ->scalarNode('host')->end() |
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.
If we add the option to firewalls (I think we should but it could be done in another PR, in this casenull should be passed here) then it should be covered by tests and mentioned in the CHANGELOG as for access_controls
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.
So I kept this and I do another P.R. for adding this options to firewall, or I remove all?
Does it finally make sense to put it on 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.
the check can't be true, let's remove it (passnull) for now and discuss it separately.All the existing access_control options are also available at the firewall level, I see no reason for not adding this one wrong,ip is not available for firewalls. let's discuss it elsewhere :)
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.
Done
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.
You should pass null here, the option does not exist.
| $httpRequest =$request =$request =Request::create(''); | ||
| $httpsRequest =$request =$request =Request::create('','get',array(),array(),array(),array('HTTPS' =>'on')); | ||
| $httpRequest = Request::create(''); | ||
| $httpsRequest = Request::create('','get',array(),array(),array(),array('HTTPS' =>'on')); |
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.
please revert this :)
| publicfunctiontestAccessDecisionManagerServiceAndStrategyCannotBeUsedAtTheSameTime() | ||
| { | ||
| $container =$this->getContainer('access_decision_manager_service_and_strategy'); | ||
| $this->getContainer('access_decision_manager_service_and_strategy'); |
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.
to revert
| ----- | ||
| * added`getAcceptableFormats()` for reading acceptable formats based on Accept header | ||
| * added`port` property |
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.
that's the inverse actually :) we need to mention the addition ofRequestMatcher::matchPort(), not the privateport property as it's not exposed
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.
Oups sorry…
chalasr commentedSep 30, 2018
@roukmoute What's the state of this PR? |
| $firewallNodeBuilder | ||
| ->scalarNode('pattern')->end() | ||
| ->scalarNode('host')->end() | ||
| ->integerNode('port')->defaultNull()->end() |
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.
You should remove this option, we want it only for the access control
| $host =isset($firewall['host']) ?$firewall['host'] :null; | ||
| $methods =isset($firewall['methods']) ?$firewall['methods'] :array(); | ||
| $matcher =$this->createRequestMatcher($container,$pattern,$host,$methods); | ||
| $matcher =$this->createRequestMatcher($container,$pattern,$host,null,$methods); |
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 is the port alwaysnull here? Shouldn't we use the configured 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.
Here it is :)
#28061 (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.
thanks :)
fabpot 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 really get the use case, but ok.
fabpot commentedOct 10, 2018
Thank you@roukmoute. |
This PR was squashed before being merged into the 4.2-dev branch (closes#28061).Discussion----------[Security] add port in access_control| Q | A| ------------- | ---| Branch? | master for features| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#26071| License | MIT| Doc PR |symfony/symfony-docs#10359Add port in access_control__Please Squash this P.R.__Commits-------6413dcb [Security] add port in access_control
Uh oh!
There was an error while loading.Please reload this page.
Add port in access_control
Please Squash this P.R.