add subnet mask IP support for maintenance mode #10570

AntonEvers · 2017-08-17T13:04:22Z

Description

Maintenance mode should accept IP ranges instead of separate IP's so that for instance you can add the whole range of your payment service provider or CDN etc...

Fixed Issues (if relevant)

No issues so far

Manual testing scenarios

run bin/magento maintenance:enable
run bin/magento maintenance:allow-ips 192.168.33.0/16 (for instance)
get access to the site

Mostly I am curious what you think about this kind of functionality and the use of darsyn/ip for this. I did take IPv6 into account.

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

ishakhsuvarov · 2017-08-17T15:49:37Z

@ajpevers I like the idea, however I'm not really a fan of adding additional dependency to solve a single problem, since it requires an adapter on magento side, to make sure it can be replaceable and testable.
Did you consider implementing cidr check with plain php and maybe existing dependencies?
Also please check the merge conflict in composer.lock.

AntonEvers · 2017-08-18T17:46:04Z

Sure, it's a proof of concept. I'll see if I can get it to work with existing dependencies. Doesn't look like very exotic functionality so I expect it would be somewhere in ZF or Symfony or something. As long as it works with IPv6 as well. I'll come back on this.

AntonEvers · 2017-08-21T05:27:47Z

Hi @ishakhsuvarov I took a good hard look but there is no existing code that does the cidr check for IPv6. So I included symfony/http-foundation to take care of this in favor of darsyn/ip. It's more elegant an fits in with the already existing Symfony components dependencies.

The IpValidator could be built using the existing Zend_Validate_Ip.

What do you think, should we just copy the one file (https://github.com/symfony/http-foundation/blob/master/IpUtils.php) and risk missing bug fixes, or should we include the package as part of the dependencies?

ishakhsuvarov · 2017-08-23T09:03:31Z

@ajpevers I don't think just copying a single file is a good idea, however adding a dependency requires a wrapper on the Magento side, which would allow to use any different dependency in place without breaking existing API.
Also I would suggest to avoid using outdated Zend 1 components, since it is end-of-life. Actually there are some PR's around which replace those usages to a newer versions.

AntonEvers · 2017-08-23T09:17:19Z

Okay, I'll work it out. Can you point out an existing wrapper so that I can follow the example?

ishakhsuvarov · 2017-08-23T09:35:11Z

@ajpevers I guess more common name would be Adapter, you may refer to the wiki for the basic description.
Some of the existing which first came to mind – would be \Magento\Framework\Serialize\SerializerInterface which is simply a wrapper for php's built-in json_encode, serialize calls, or some other implementations provided by third-parties. Or \Magento\Framework\Code\Minifier\AdapterInterface which is an adapter for different implementations of minifiers.

AntonEvers · 2017-08-23T09:45:32Z

Great, that is clear enough

orlangur · 2017-08-23T10:34:54Z

I don't think adapter is suitable here, looks more like a https://en.wikipedia.org/wiki/Facade_pattern to me.

So, the idea is that Symfony's IpUtils must be encapsulated in singe Magento Framework component and not referred anywhere outside of this component.

For example, it can be \Magento\Framework\HTTP\IpChecker class with whatever methods are best suitable for Magento (I don't think we need separate method for IP v4/v6 exposed, just a single method IpChecker->check(...) seems enough).

ishakhsuvarov · 2017-09-14T17:35:03Z

Hi @ajpevers
Would you like to proceed with this PR? Any help required from our side?

Thanks

AntonEvers · 2017-09-22T05:31:42Z

Hi @ishakhsuvarov,

The thing I need most is spare time :) can you help me with that?
I'm going to finish this today.

Cheers,

Anton

vrann · 2017-10-02T18:46:27Z

@ajpevers can you please merge it with the latest mainline?

AntonEvers · 2017-10-03T05:39:18Z

@vrann done

…aintenance should be on, not off

orlangur · 2017-10-25T13:12:49Z

lib/internal/Magento/Framework/App/Test/Unit/MaintenanceModeTest.php

@@ -58,7 +58,7 @@ public function testisOnWithIP()
        ];
        $this->flagDir->expects($this->exactly(2))->method('isExist')
            ->will(($this->returnValueMap($mapisExist)));
-        $this->assertFalse($this->model->isOn());
+        $this->assertTrue($this->model->isOn());


Why need this change?

This test was wrong, it basically stated:
If there is a maintenance flag file AND if there is an empty maintenance op file maintenance mode should be off.

This is not true. If the ip file is present but empty maintenance mode should be on, not off.

Oh, nice catch 👍 Just still don't see corresponding changes in implementation which could invert this assertion.

Wait... this cannot be right... Ha! https://github.com/magento/magento2/pull/10570/files#diff-4b45df6c223e8d98dcc949e45bfc4f1bR73 This is the wrong way around! Sorry.

Just to clarify: this needs to be changed or currently logic is correct? How this test worked with incorrect previous logic though...

Ping @ajpevers :)

Hi @orlangur, sorry for the silence. I need to free some time to test why this worked without the PR and why it needs a change with the PR. I have it on my task list. Keep you posted.

@orlangur I revisited this PR and it appears that this test is testing !in_array("", [""]) which results in false.
So if your IP is "" (the default value of the first and only isON() parameter) and the list of whitelisted IP's is [""] then you are marked as whitelisted and maintenance is not on for you. This is wrong.
If maintenance mode is on and the ip whitelist file is present but empty, maintenance mode should be on for everyone and not off. So this test is incorrect. I will fix this by adding an array_filter around the whitelisted IP's so that you can never compare against [""], but in that case against [], which will never have a false positive on in_array comparisons.

orlangur · 2017-10-25T13:15:27Z

setup/src/Magento/Setup/Validator/IpValidator.php

+            $cidr = 32;
+            $ip = $range;
+            if (strpos($range, '/') !== false) {
+                list($ip, $cidr) = explode('/', $range);


cidr? Is there some existing logic in ZF2 or Symfony which can be reused?

Maybe there is, but I havent found it. And it seems overkill to include another class as replacement for a list = explode.

I mean this logic in general - splitting by slash etc.

Still not sure what "cidr" means, https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing#CIDR_notation looks like after splitting by slash there should be $networkMask instead.

Ah that's what you mean. Yes you are right. I'll rename it.

I'll call it $subnetMask as that is the name I heard most for this.

orlangur · 2017-10-25T13:18:30Z

setup/src/Magento/Setup/Validator/IpValidator.php

+                continue;
+            }
+
+            $ipv4Validator = new \Zend_Validate_Ip(['allowipv6' => false, 'allowipv4' => true]);


We must not add code using ZF1 classes. Looks like there is ZF2 equivalent https://framework.zend.com/manual/2.1/en/modules/zend.validator.ip.html

orlangur · 2017-10-25T13:19:12Z

setup/src/Magento/Setup/Validator/IpValidator.php

+            }
+
+            $ipv4Validator = new \Zend_Validate_Ip(['allowipv6' => false, 'allowipv4' => true]);
+            $max = $ipv4Validator->isValid($ip) ? 32 : 128;


Please choose a more meaningful name. $maxBits?

* upstream/2.3-develop: (37 commits) MAGETWO-81033: Create GraphQL single product fetch endpoint - Address static failure MAGETWO-81033: Remove unnecessary classes MAGETWO-81033: Create GraphQL endpoint for fetching single product - Adds functional tests, fixtures, and client - Adds support for simple and configurable product magento#11524: Fix Filter Customer Report Review - Integration test updated MAGETWO-82561: [TASK] Moved Customer Groups Menu Item from Other settings to Customers magento#11652 MQE-478: Deliver Pangolins Sprint 11 MQE-478: Deliver Pangolins Sprint 11 MQE-478: Deliver Pangolins Sprint 11 magento#11522: Fix Filter Customer Report Review - Integration test updated magento#11524: Fix Filter Customer Report Review - Integration test added Fix Magento/Backend/Block/Media/Uploader.php getConfigJson() method, using undefined class property _coreData MAGETWO-77673: <![CDATA[]]>in system.xml translate phrase not work, if comment starts from new line[port from 2.2-develop]. MQE-478: Deliver Pangolins Sprint 11 MQE-440: metadata changes for path parameters bug fix. MQE-237: [Generator] Add before and after logic to suites MQE-394: Pre-Install Script MQE-440: Added configurable product related metadata, data, and a sample test. MQE-429: Added a sample test to perform api PUT request. MQE-453: [DevOps] Add optional arg for consolidated test run MQE-345: [DevOps] [Customizability] Create suite schema declaration and supporting interpretation ...

okorshenko · 2017-10-31T21:39:31Z

Hi @orlangur
can we accept this PR or are you waiting for additional changes?

orlangur · 2017-11-01T06:19:02Z

@okorshenko waiting, otherwise I would resolve JIRA issue.

okorshenko · 2017-11-28T20:58:44Z

Hi @ajpevers @orlangur do we have any progress on this? Do you need some help?

orlangur · 2017-11-28T21:24:49Z

Last comment from author was 15 days ago #10570 (comment)

Basically, the changes seem pretty good to me, just that unit test needs to be investigated.

orlangur · 2017-12-04T11:01:32Z

@ajpevers let's close this for now and reopen when you or me have some new information regarding odd unit test behavior.

ishakhsuvarov self-assigned this Aug 17, 2017

ishakhsuvarov added this to the August 2017 milestone Aug 17, 2017

ishakhsuvarov added the develop label Aug 17, 2017

ishakhsuvarov removed their assignment Aug 17, 2017

ishakhsuvarov added the Progress: needs update label Aug 19, 2017

vrann added the partner contribution label Sep 14, 2017

vrann self-assigned this Sep 14, 2017

magento-engcom-team modified the milestones: August 2017, September 2017 Sep 18, 2017

okorshenko modified the milestones: September 2017, October 2017 Oct 5, 2017

magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:33

Anton Evers added 3 commits October 24, 2017 12:46

add subnet mask IP support for maintenance mode

803d0ca

Replace darsyn/ip in favour of symfony/http-foundation

e8e4a93

Create a facade for Symfony's IpUtils' checkIp method

ff37359

AntonEvers force-pushed the ip-ranges branch from 83ce0df to ff37359 Compare October 24, 2017 07:09

Anton Evers added 2 commits October 24, 2017 13:14

Remove rebase conflict code

83bc55b

If the maintenance flag is present and an empty ip file is present, m…

db6213a

…aintenance should be on, not off

Anton Evers added 4 commits October 24, 2017 14:29

Add ip range tests

c34f97a

add range tests to testOffSetMultipleAddresses

6ea05d9

Add ipv6 tests

e2bb347

Resolve codacy/pr test

d8dd590

orlangur self-assigned this Oct 25, 2017

orlangur requested changes Oct 25, 2017

View reviewed changes

Anton Evers added 4 commits October 25, 2017 20:29

resolve review comments

8120d7f

Maintenance on test was exactly the wrong way around

9f8519b

rename $cidr to $subnetMask because it is a subnet mask in cidr notation

9457972

okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017

okorshenko unassigned vrann Nov 28, 2017

okorshenko modified the milestones: November 2017, December 2017 Dec 1, 2017

orlangur closed this Dec 4, 2017

AntonEvers mentioned this pull request Jun 30, 2020

add subnet mask IP support for maintenance mode #28934

Open

5 tasks

m2-assistant bot mentioned this pull request Aug 18, 2020

[Issue] add subnet mask IP support for maintenance mode #29657

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add subnet mask IP support for maintenance mode #10570

add subnet mask IP support for maintenance mode #10570

AntonEvers commented Aug 17, 2017 •

edited

Loading

ishakhsuvarov commented Aug 17, 2017 •

edited

Loading

AntonEvers commented Aug 18, 2017

AntonEvers commented Aug 21, 2017 •

edited

Loading

ishakhsuvarov commented Aug 23, 2017

AntonEvers commented Aug 23, 2017

ishakhsuvarov commented Aug 23, 2017

AntonEvers commented Aug 23, 2017

orlangur commented Aug 23, 2017

ishakhsuvarov commented Sep 14, 2017

AntonEvers commented Sep 22, 2017

vrann commented Oct 2, 2017

AntonEvers commented Oct 3, 2017

orlangur Oct 25, 2017

AntonEvers Oct 25, 2017

orlangur Oct 25, 2017

AntonEvers Oct 25, 2017

orlangur Nov 1, 2017

orlangur Nov 10, 2017

AntonEvers Nov 14, 2017

AntonEvers Jun 30, 2020

orlangur Oct 25, 2017

AntonEvers Oct 25, 2017

orlangur Oct 25, 2017

orlangur Oct 25, 2017

AntonEvers Oct 26, 2017

AntonEvers Oct 26, 2017

orlangur Oct 25, 2017

orlangur Oct 25, 2017

okorshenko commented Oct 31, 2017

orlangur commented Nov 1, 2017

okorshenko commented Nov 28, 2017

orlangur commented Nov 28, 2017

orlangur commented Dec 4, 2017

add subnet mask IP support for maintenance mode #10570

add subnet mask IP support for maintenance mode #10570

Conversation

AntonEvers commented Aug 17, 2017 • edited Loading

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

ishakhsuvarov commented Aug 17, 2017 • edited Loading

AntonEvers commented Aug 18, 2017

AntonEvers commented Aug 21, 2017 • edited Loading

ishakhsuvarov commented Aug 23, 2017

AntonEvers commented Aug 23, 2017

ishakhsuvarov commented Aug 23, 2017

AntonEvers commented Aug 23, 2017

orlangur commented Aug 23, 2017

ishakhsuvarov commented Sep 14, 2017

AntonEvers commented Sep 22, 2017

vrann commented Oct 2, 2017

AntonEvers commented Oct 3, 2017

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

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

okorshenko commented Oct 31, 2017

orlangur commented Nov 1, 2017

okorshenko commented Nov 28, 2017

orlangur commented Nov 28, 2017

orlangur commented Dec 4, 2017

AntonEvers commented Aug 17, 2017 •

edited

Loading

ishakhsuvarov commented Aug 17, 2017 •

edited

Loading

AntonEvers commented Aug 21, 2017 •

edited

Loading