[Security] add port in access_control #28061

roukmoute · 2018-07-25T15:22:33Z

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#10359

Add port in access_control

Please Squash this P.R.

lyrixx · 2018-07-25T15:41:58Z

src/Symfony/Component/HttpFoundation/RequestMatcher.php

     * @param string|string[]|null $methods
     * @param string|string[]|null $ips
     * @param array                $attributes
     * @param string|string[]|null $schemes
     */
-    public function __construct(string $path = null, string $host = null, $methods = null, $ips = null, array $attributes = array(), $schemes = null)
+    public function __construct(string $path = null, string $host = null, $port = null, $methods = null, $ips = null, array $attributes = array(), $schemes = null)


This is a BC break, you should add the new arguments at the end of the list

ronfroy · 2018-08-13T14:18:49Z

src/Symfony/Component/HttpFoundation/RequestMatcher.php

+     */
+    public function matchPort($port)
+    {
+        $this->port = (int) $port;


(int) null => 0 (in this case the value is not clear)

We can also use a type hint here.

xabbuh · 2018-08-14T15:21:35Z

src/Symfony/Component/HttpFoundation/RequestMatcher.php

@@ -167,6 +184,10 @@ public function matches(Request $request)
            return false;
        }

+        if (0 < $this->port && $request->getPort() !== $this->port) {


we need to ignore null here

javiereguiluz · 2018-09-18T08:41:49Z

@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!

chalasr · 2018-09-18T10:03:04Z

src/Symfony/Component/HttpFoundation/RequestMatcher.php

     * @param string|string[]|null $methods
     * @param string|string[]|null $ips
     * @param array                $attributes
     * @param string|string[]|null $schemes
     */
-    public function __construct(string $path = null, string $host = null, $methods = null, $ips = null, array $attributes = array(), $schemes = null)
+    public function __construct(string $path = null, string $host = null, $methods = null, $ips = null, array $attributes = array(), $schemes = null, $port = null)


should be int $port = null

Oh yeah, now we can use typehint in this version 👍

chalasr · 2018-09-18T10:04:28Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php

@@ -140,6 +140,7 @@ private function addAccessControlSection(ArrayNodeDefinition $rootNode)
                                ->example('^/path to resource/')
                            ->end()
                            ->scalarNode('host')->defaultNull()->end()
+                            ->scalarNode('port')->defaultNull()->end()


we probably don't want to deal with strings here, should be an integerNode() to me

chalasr · 2018-09-18T10:31:14Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

            $methods = isset($firewall['methods']) ? $firewall['methods'] : array();
-            $matcher = $this->createRequestMatcher($container, $pattern, $host, $methods);
+            $matcher = $this->createRequestMatcher($container, $pattern, $host, $port, $methods);


$firewall['port'] ?? null, removing the assignment above

Can I do the same things for the rest (host, …) or is it must be in another P.R.?

I would keep them unchanged to avoid conflicts when merging lower branches up to master

chalasr · 2018-09-18T10:31:39Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

@@ -668,20 +670,20 @@ private function createExpression($container, $expression)
        return $this->expressions[$id] = new Reference($id);
    }

-    private function createRequestMatcher($container, $path = null, $host = null, $methods = array(), $ip = null, array $attributes = array())
+    private function createRequestMatcher($container, $path = null, $host = null, $port = null, $methods = array(), $ip = null, array $attributes = array())


int $port = null

chalasr · 2018-09-18T10:37:05Z

src/Symfony/Component/HttpFoundation/RequestMatcher.php

+     *
+     * @param int|null $port The port number to connect to
+     */
+    public function matchPort($port)


this one should use int $port = null, the existing public methods of this class have been added before php scalar typehints and cannot be changed for BC

chalasr · 2018-09-18T10:39:37Z

src/Symfony/Component/HttpFoundation/RequestMatcher.php

+    {
+        $portInt = (int) $port;
+
+        $this->port = is_numeric($port) || $port == $portInt ? $portInt : null;


should just assign $this->port to $port (see my comment above)

chalasr · 2018-09-18T14:01:22Z

src/Symfony/Component/HttpFoundation/RequestMatcher.php

+     *
+     * @param int|null $port The port number to connect to
+     */
+    public function matchPort(int $port): void


missing = null :) (see tests)

please remove the return type also, we don't use void generally

chalasr · 2018-09-18T14:22:14Z

src/Symfony/Component/HttpFoundation/CHANGELOG.md

@@ -5,6 +5,7 @@ CHANGELOG
 -----

 * added `getAcceptableFormats()` for reading acceptable formats based on Accept header
+ * added `port` property and `matchPort()` in RequestMatcher


only the method is relevant here, the property is private

chalasr · 2018-09-18T14:24:14Z

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php

@@ -245,7 +245,7 @@ public function testAccess()

        $rules = array();
        foreach ($container->getDefinition('security.access_map')->getMethodCalls() as $call) {
-            if ('add' == $call[0]) {
+            if ('add' === $call[0]) {


this one should be reverted

chalasr · 2018-09-18T14:33:36Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

@@ -247,7 +248,7 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
            $pattern = isset($firewall['pattern']) ? $firewall['pattern'] : null;
            $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);


this looks for a port option on a firewall but it does not exist yet

symfony/src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php

Line 183 in e923f80

->scalarNode('host')->end()

If we add the option to firewalls (I think we should but it could be done in another PR, in this case null should be passed here) then it should be covered by tests and mentioned in the CHANGELOG as for access_controls

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?

the check can't be true, let's remove it (pass null) 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 :)

You should pass null here, the option does not exist.

chalasr · 2018-09-18T19:43:25Z

src/Symfony/Component/HttpFoundation/Tests/RequestMatcherTest.php

-        $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'));


please revert this :)

chalasr · 2018-09-18T19:43:33Z

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.php

@@ -442,7 +442,7 @@ public function testCustomAccessDecisionManagerService()
     */
    public function testAccessDecisionManagerServiceAndStrategyCannotBeUsedAtTheSameTime()
    {
-        $container = $this->getContainer('access_decision_manager_service_and_strategy');
+        $this->getContainer('access_decision_manager_service_and_strategy');


chalasr · 2018-09-19T11:24:01Z

src/Symfony/Component/HttpFoundation/CHANGELOG.md

@@ -5,6 +5,7 @@ CHANGELOG
 -----

 * added `getAcceptableFormats()` for reading acceptable formats based on Accept header
+ * added `port` property


that's the inverse actually :) we need to mention the addition of RequestMatcher::matchPort(), not the private port property as it's not exposed

Oups sorry…

chalasr · 2018-09-30T15:34:20Z

@roukmoute What's the state of this PR?
I see that you finally added the port option to firewalls as well, no objection from me but it should be covered by tests as for access controls.
Please tell us when it's ready to review or if you need some help :)

chalasr · 2018-10-02T15:18:56Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php

@@ -180,6 +181,7 @@ private function addFirewallsSection(ArrayNodeDefinition $rootNode, array $facto
        $firewallNodeBuilder
            ->scalarNode('pattern')->end()
            ->scalarNode('host')->end()
+            ->integerNode('port')->defaultNull()->end()


You should remove this option, we want it only for the access control

xabbuh · 2018-10-03T17:54:52Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

@@ -275,7 +276,7 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a
            $pattern = isset($firewall['pattern']) ? $firewall['pattern'] : null;
            $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);


Why is the port always null here? Shouldn't we use the configured value?

Here it is :)
#28061 (comment)

fabpot

I don't really get the use case, but ok.

fabpot · 2018-10-10T10:32:10Z

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#10359 Add port in access_control __Please Squash this P.R.__ Commits ------- 6413dcb [Security] add port in access_control

carsonbot added Status: Needs Review Feature labels Jul 25, 2018

chalasr added HttpFoundation SecurityBundle labels Jul 25, 2018

chalasr added this to the next milestone Jul 25, 2018

lyrixx reviewed Jul 25, 2018

View reviewed changes

ronfroy reviewed Aug 13, 2018

View reviewed changes

xabbuh reviewed Aug 14, 2018

View reviewed changes

chalasr changed the title ~~Security add port~~ [Security] add port in access_control Sep 18, 2018

carsonbot removed the Status: Needs Review label Sep 18, 2018

chalasr requested changes Sep 18, 2018

View reviewed changes

carsonbot added the Status: Needs Work label Sep 18, 2018

chalasr reviewed Sep 18, 2018

View reviewed changes

chalasr reviewed Sep 19, 2018

View reviewed changes

chalasr reviewed Oct 2, 2018

View reviewed changes

chalasr approved these changes Oct 3, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 3, 2018

xabbuh requested changes Oct 3, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 3, 2018

xabbuh approved these changes Oct 3, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Oct 3, 2018

fabpot approved these changes Oct 10, 2018

View reviewed changes

[Security] add port in access_control

6413dcb

fabpot merged commit 6413dcb into symfony:master Oct 10, 2018

roukmoute deleted the security_add_port branch October 10, 2018 12:14

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

[Security] add port in access_control #28061

[Security] add port in access_control #28061

Conversation

roukmoute commented Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javiereguiluz commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roukmoute Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot left a comment

Choose a reason for hiding this comment

fabpot commented Oct 10, 2018

roukmoute commented Jul 25, 2018 •

edited

Loading

roukmoute Sep 18, 2018 •

edited

Loading

chalasr Sep 19, 2018 •

edited

Loading