Allow to specify list of IPs in a body on maintenance.flag which will be granted access even if the flag is on

[tim-bezhashvyly]

This is a new functionality allowing to specify list of IPs which will be granted access to the website even if maintenance flag is on.

The IPs just has to be specified in the app/etc/allowed_ips.xml file. The format of the file has to be the following:

<?xml version="1.0"?>
<ips>
    <ip>33.33.33.1</ip>
    <ip>178.178.189.9</ip>
</ips>

The names of nodes doesn't matter. The just has to be one root node and then each IP has to be stored in a child node with the same tag.

This functionality can be useful for release processes when the developer needs to make sure that the website is running as it should while general public is not able to access the website. Another use case can be the occasion when the problem critical is found on production website and seems to be a server specific. In this case developer will be able to put the maintenance flag but still access the website by himself in order to trace the problem.

The implemented feature is not covered with tests as "maintenance.flag" related functionality is not covered in stock.

There is no XML validation implemented. The allowed_ips.xml is supposed to be a valid XML file. If file doesn't exits the script will just move on considering there are no allowed IPs.

[benmarks]

Great idea for a PR, Tim! Assuming that this is useful to others (I'm certain it is), I'd like to see the concept abstracted. Ideally we have a mechanism of whitelisting which is a bit more open to implementation, so that the provider of the whitelist is not just raw text via core PHP methods.

Further, it's an unnecessary risk to store these "important" IPs in a plain-text format which will be served by the web server (not sure if this applies to pub-compiled instances or not ATM). Perhaps this PR should be modified to include an "allowed IPs" PHP file, and the maintenance.flag logic should use this list as a minimal way to get the functionality in the M2 core.

[tim-bezhashvyly]

What would you say about storing IPs in environment variable (SetEnv/fastcgi_param)? Should be the safest.

[benmarks]

It's certainly an approach, and one which is in keeping with the usage of environment variables in the bootstrap/initialization phase of the app. The application of this is clunky as hell though:

# Apache
SetEnv foo[1] bar
SetEnv foo[2] baz

Also, I've run into some "fun" and not-unknown issues regarding environment variables set dynamically in Apache, so I'm a bit wary of this approach.

[ihor-sviziev]

I think we shouldn't have relation with IPs. I think we should make ability set in .htaccess (or virtual host section) secret key, and when user go to our website with get param ?secret={my_secret_from_htaccess} we should create special cookie for them and grant access for them.

[tim-bezhashvyly]

@igor-svizev what is the benefit over IPs approach?

[ihor-sviziev]

@tim-bezhashvyly we can have load balancer, and in some cases we may have load balancer ip. Also admin user may be under NAT, so we may grand access not only from that user.

[tim-bezhashvyly]

@igor-svizev can you illuminate a little bit regarding load balancer? How the IP based approach can harm it?

[ihor-sviziev]

@tim-bezhashvyly when we using load balancer, all requests are going through load balancer, and in some cases(if configuration is wrong) client ip may be detected as load balancer ip, so all users may have access to website.
PS: we may create ability to use allowed IPs only or allowed IPs + secret code or secret code only

[tim-bezhashvyly]

@igor-svizev but in this case of such wrong configuration of balancing server all functionality base on \Mage_Core_Helper_Http::getRemoteAddr will fail.

[tim-bezhashvyly]

@benmarks updated. Moved IPs declaration to app/etc/allowed_ips.xml. Details in pull request description. Please let me know what do you think.

[Flyingmana]

I think this is something nice, but nothing which belongs into magento itself.

The index.php is an entry point, and should be as simple as possible and should only contain really needed configuration.

maintenance is a very company specific process and strongly depends on eventual loadbalancers and server configurations.

My personal point where Iam afraid: you have to fight the opcache for switches between On and Off and for changes on the allowed IPs

[tim-bezhashvyly]

@Flyingmana with such an approach you can remove 50% of Magento functions. If due to some configuration IP restriction will not work for you just don't use it.

I completely agree that front end controller has to be as clean as possible but there is no other way to plug this functionality in. And it is just couple of lines.

[elenleonova]

Guys, I've read your discussions here. While I'm totally agree that "allowed IPs in the maintenance mode" is a great idea. I don't see that we have come to the agreement how to add this to Magento 2 core. is everybody fine with the latest approach that @tim-bezhashvyly suggested?

[airbone42]

I like the approach of @tim-bezhashvyly so far if the decision is that Magento itself will handle maintenance.

There's also a point at @Flyingmana that maintenance should not be handled by Magento as it's probably really something company/instance specific, but then the whole maintenance.flag implementation can be removed. So there's no point in rejecting this pull request for that.

And if someone doesn't know his ips, cannot configure a loadbalancer and adds the loadbalancers ip to the allowed-ips list, then he anyway has some bigger problems. ;)

So, be happy that someone is pushing a good extension here. <3

[ScreamingDev]

sorry Tim. this is a great idea but reaolving an IP is never reliable and having maintenance disabled for accessing remote magento by some ip is more a missing development instance than needed functionality.
so to speak: which developer wants the live webserver when it's broken?
owner and developer will change their ips once a day.

[SchumacherFM]

@igor-svizev is right. We also have troubles with the IP addresses within our AWS network. IPs are too inconsistent.

I would like to propose a HTTP Header Flag like the guys from theguardian.com did it. AFAIK it should be much easier to forward a HTTP Header.

http://www.theguardian.com/info/developer-blog/2014/feb/18/how-the-guardian-successfully-moved-domain

The solution was simple, we would release internally. Access to www.theguardian.com would be denied unless you had a special HTTP header set. This allowed anyone inside or outside the Guardian to immediately start testing the new domain by setting the special header using a browser extension. Our CDN provider Fastly made this HTTP header logic simple to implement because we could quickly write and deploy our own Varnish configuration to their servers.

Sure it is more config work on client side to gain access via a HTTP Header ...

[mzeis]

I agree that blocking everybody isn't useful. If you put a store in maintenance mode the change is high that the developers want to access the store (backend or frontend).

We don't use the maintenance flag but use htaccess to show maintenance pages to customers.

For us the IP-based access restriction is good enough but I'd be OK with alternatives (HTTP Header Flags, special cookies, ...).

[verklov]

@tim-bezhashvyly, we have processed your pull request. The code has been released in version dev68. We will also add information on using maintenance.flag and the list of IP addresses to the documentation. So far, please add IP addresses separated with comma directly to the maintenance.flag file.