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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented: EZP-23343: Add support for IPv6 addresses and address ranges to DebugByIP feature in eZDebug class to enable debug output #1072

Merged
merged 1 commit into from Oct 23, 2014

Conversation

5 participants
@brookinsconsulting
Copy link
Contributor

brookinsconsulting commented Sep 14, 2014

Hello,

Recently we tried on our localhost development environment to enable the DebugByIP feature using my computers localhost IPv6 IP Address '::1'.

The DebugOutput would not display with the IPv6 IP Address '::1' entered properly into the settings configuration and all caches cleared.

We traced the problem to code in eZDebug::isAllowedByCurrentIP() where it tested only for IPv4 IP Addresses and not IPv6 IP Addresses.

We then wrote replacement code to enable debug output on a computer which uses IPv6 IP Addresses.

We also wrote replacement code to support IPv6 IP Address Ranges to display DebugOutput to an entire subnet (IP Address Range).

We write today to respectfully request your review of our code improvements which provide for DebugByIP / eZDebug / DebugOutput support of IPv6 Addresses and Address ranges.

We have created a new issue ticket about this feature request, you can find it here: https://jira.ez.no/browse/EZP-23343

Please review and let us know what you think.

Thank you for your continued support!

Cheers,
Brookins Consulting

@@ -1065,6 +1065,42 @@ static function isIPInNet( $ipaddress, $network, $mask = 24 )
/*!
\static
Determine if an ipv6 ipaddress is in a network. E.G. 2620:0:860:2:: in 2620:0:860:2::/64.
\return true or false.
*/

This comment has been minimized.

@lolautruche

lolautruche Sep 15, 2014

Contributor

Could you please use phpDoc format ?

Determine if an ipv6 ipaddress is in a network. E.G. 2620:0:860:2:: in 2620:0:860:2::/64.
\return true or false.
*/
static function isIPInNetIPv6( $ipAddressRange, $network, $mask = 24 )

This comment has been minimized.

@lolautruche

lolautruche Sep 15, 2014

Contributor

I guess this method should be marked as private.

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Sep 15, 2014

Hello,

Thank you very much for your prompt and detailed feedback.

We have made and submitted the changes requests.

Please let us know what you think!

Cheers,
Brookins Consulting

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Sep 16, 2014

$ipAddressCheck = inet_pton( $ipAddress );
$ipNetworkAddressCheck = inet_pton( $ipAddress );

This comment has been minimized.

@yannickroger

yannickroger Sep 16, 2014

Contributor

Shouldn't it be $network ?

case 1: $ipAddressMask .= '8'; break;
}
$ipAddressMask = str_pad( $ipAddressMask, $ipAddressLen >> 2, '0' );

This comment has been minimized.

@yannickroger

yannickroger Sep 16, 2014

Contributor

I would add more comment in this method, to make it easier to debug later. Explaining what all these operation are meant to do.

$ipAddress = eZSys::clientIP();
if ( $ipAddress )
{
foreach( $allowedIpList as $itemToMatch )
{
if ( preg_match("/^(([0-9]+)\.([0-9]+)\.([0-9]+)\.([0-9]+))(\/([0-9]+)$|$)/", $itemToMatch, $matches ) )
if ( preg_match( "/:/", $ipAddress ) )

This comment has been minimized.

@yannickroger

yannickroger Sep 16, 2014

Contributor

I would also add a bit of comment in this method (same reason as above).

@yannickroger

This comment has been minimized.

Copy link
Contributor

yannickroger commented Sep 16, 2014

These 2 methods could be unit tested to make sure they behave the way they should.

@yannickroger

This comment has been minimized.

Copy link
Contributor

yannickroger commented Sep 16, 2014

+1 with more comments and unit test.

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Sep 18, 2014

Hello,

Thank you again for your detailed feedback.

We have reimplemented the internals of the solution to provide a more reliable and dependable solution (AKA our last version was critically flawed and not properly testable).

We may not be able to provide the unit tests on this one. Can you help us understand how we can provide what you require in greater detail. Please let us know what you think.

Thank you for your continued support!

Cheers,
Brookins Consulting

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Sep 24, 2014

Review Request (Round 2) Ping @lolautruche @yannickroger

@brookinsconsulting brookinsconsulting force-pushed the brookinsconsulting:debug-by-ipv6-ip-and-ipv6-network-range branch from 3fbd1d8 to e2b9c4a Oct 16, 2014

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Oct 16, 2014

Hello,

In the interest of keeping this pull request clean we have flattened our commits into one.

We would still like to get just a little bit of help on what needs to be unit tested and how to create those tests (this is our first time). Like what functions of the eZDebug class need to tested, how should they be tested, where would we store our test, etc.

Please let us know what you think.

Thank you again for your continued support!

Cheers,
Brookins Consulting

@yannickroger

This comment has been minimized.

Copy link
Contributor

yannickroger commented Oct 17, 2014

Functions needed to be tested would be those you added and the one you modified. The goal is to make sure they behave the way they are supposed to.

Unit test are using phpunit (you can start by looking at tutorials online). You can find documentation on how to run legacy unit test here: http://www.ezpedia.org/ez/testing_ez_publish_test_system

You can add new tests in: https://github.com/ezsystems/ezpublish-legacy/blob/master/tests/tests/lib/ezutils/ezdebug_regression.php

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Oct 20, 2014

Hello @yannickroger

Thank you very much for your replies they really helped!

We have added unit test code per your request. Please review these tests for your acceptance. The tests all pass and work properly.

We did run into one problem that we need further guidance and direction.

The Problem with eZDebug::isAllowedByCurrentIP

The problems with unit testing the existing version of eZDebug::isAllowedByCurrentIP class method ....

It seems that the eZDebug::isAllowedByCurrentIP method (before our changes were even made) can not currently be tested with unit testing properly due to the design of the existing code. Rather it can but the bulk of the code is skipped due to the design of the class and method.

It seems that the unit tests invoke a sort of command line mode and the method internals are skipped entirely. This seems to first be caused by the fact that the eZSys::clientIP() used in eZDebug::isAllowedByCurrentIP does not return an IP Address when the code is run from a unit test perspective. We think this is because the $_SERVER array does not contain a $_SERVER['REMOTE_ADDR'] variable to return when the code is run from a unit test perspective.

We wrote a stub unit test for this method in the interim which matches the current code design expected behavior but it does not test the IP Address matching code before or after our additions because of the above reasons.

Directions given asking for help over the weekend

We tried to get support this weekend on #ezpublish freenode irc and we were guided by Bratt who said basically the following:

[11:36] hello
[11:36] I've been requested to write unit tests for a pull request for ezpublish
[11:37] but I'm running into problems and wondered if there was anyone here who could take a moment to talk with me about the problems
[11:39] This is the pull request where you can see the ezcrew has asked me to write unit tests: #1072

[11:40] the first real problem seems to be unit tests are command line based and $_SERVER does not contain the variables that a web based request would have ...

[11:42] so i don't know how to -inject- a fake ip address into the $_SERVER ['REMOTE_ADDR'] variable which the default ezpublish code requires to even be able to run the default method (forget my PR changes, i can't even test the isAllowedByCurrentIP method (properly) )

[11:57] <Bratt_> bc: Make your code independent of eZSys::clientIP(). For instance by setting the IP users IP as a class variabel in this class' constructor?
[11:57] <Bratt_> -IP
[11:57] the code was already (this way) before I touched it, depended on that method
[11:58] <Bratt_> Yes, I see that. Refactor it
[11:58] you are kidding me

[11:58] <Bratt_> Nope. No kidding you :)
[11:58] they expect me to do all of that?

[11:59] <Bratt_> bc: Do you propose any other way of making unit tests?
[11:59] ok, i'm trying to calm down
[12:00] how do i make the code independent of ezsys:clientIP

[12:02] <Bratt_> I think I would move it to the constructor, set a class variable there, then make a simple set function to modify it from the unit test code

[12:02] <Bratt_> Shouldn't be much work. But you can always ask in the pull request if they have other suggestions
[12:03] <Bratt_> Sorry, I need to run...
[12:03] thanks for the guidance

Request for unit test review

At this point I need to know for certain what you want me to do. I've written the unit tests for the methods added which work and can be extended. I've identified a unit test specific problem with the class in question but I am more than hesitant to refactor the class for proper unit testing per Bratt's direction.

Please advise.

In short what do you want me to do? Can we ignore the problem with unit testing the class method and test it per the existing method design or ... does the class need to be further refactored to support more accurate unit testing? If the class does need to be refactored further can you please provide more detailed and exacting instructions on what you want changed specifically.

Thank you again for your continued support!

Cheers,
Brookins Consulting

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Oct 20, 2014

This is ok, we don't really want to refactor to much in legacy, it is supposed to stay stable.

As for the test it seem to fail on PHP 5.5, I have added PHP 5.6 to master now so if you rebase again we'll get testing of that also. The failure on 5.5 is due to changes in pack/unpack: http://php.net/manual/en/migration55.incompatible.php

/**
* Test for EZP-23343
*
* Make sure debug output is displayed for an IPv6 address

This comment has been minimized.

@andrerom

andrerom Oct 20, 2014

Member

IPv4 here right?

*/
function testDebugOutputIPv4LocalHost()
{
self::markTestSkipped( "Incomplete test" );

This comment has been minimized.

@andrerom

andrerom Oct 20, 2014

Member

why is this skipped?
Ideally (no worries, we seldom remember to do this either) when approaching legacy code this test should have been created first, making sure the old behavior of the code works as expected both before and after the inclusion of additional behavior.

@brookinsconsulting brookinsconsulting force-pushed the brookinsconsulting:debug-by-ipv6-ip-and-ipv6-network-range branch from 5950663 to 9fe418b Oct 21, 2014

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Oct 21, 2014

Hello @andrerom

Thank you very much for your prompt and detailed replies!

Per your direction we have made the following changes for your review.

  • Fixed testDebugOutputIPv4LocalHost test comment header to say IPv4 instead of IPv6
  • Fixed testDebugOutputIPv4LocalHost test to not be skipped
  • Added testDebugOutputIPv4PublicHost test
  • Added testDebugOutputIPv6PublicHost test
  • Added eZDebug::packedToBin function code block using version_compare to add version specific support for php 5.5.0 >=
  • Extended eZDebug::packedToBin function comment documentation
  • Tested unit tests and runtime locally with php5.4, php5.5 and php5.6
  • Rebased to keep this pull request commit history clean

Please let us know what you think is needed next.

Thank you for your continued support!

Cheers,
Brookins Consulting

* @param string $ipAddress IP Address
* @return bool Returns true if address is in the network and false if not in the network
*/
private static function isIPInNetIPv6( $ipAddressRange, $ipAddress )

This comment has been minimized.

@bdunogier

bdunogier Oct 21, 2014

Member

With the method named "is IP in NetIPV6" (wouldn't it make more sense with "IPV6Net"), I'd pass the IP as the first element, and the nw as the second.

This comment has been minimized.

@andrerom

andrerom Oct 21, 2014

Member

@bdunogier did you mean something like isIPv6InIPRange( $ipAddress, $ipAddressRange )?

Better to be explicit here to avoid more review rounds then needed, it has already gone in more loops then a roller coaster ;)

This comment has been minimized.

@bdunogier

bdunogier Oct 21, 2014

Member

I was just highlighting the fact that the current method's name would indicate that the IP would be first, and the NW second. But of course we could also change the method name :-)

This comment has been minimized.

@andrerom

andrerom Oct 21, 2014

Member

So your talking about argument then in your parenthesis?

This comment has been minimized.

@bdunogier

bdunogier Oct 21, 2014

Member

I was talking about two things.

The parenthesis was a real parenthesis, questioning the ordering of words in the method's name. What I'm suggesting in terms of prototype is indeed isIPInNetIPv6( $ipAddress, $ipAddressRange ).

// ip address into their binary representation
// Truncate each string to the length given by the netmask
// If they match at this point, they are part of the range
return ( substr( self::packedToBin( inet_pton( $ipAddresRangeNetwork ) ), 0, $networkMaskBits ) === substr( self::packedToBin( inet_pton( $ipAddress ) ), 0, $networkMaskBits ) );

This comment has been minimized.

@bdunogier

bdunogier Oct 21, 2014

Member

This line is too long imho.

Instead of writing 4 lines of comments, I'd use meaningful variables for the compared elements. Maybe:

$maskedBinaryNetwork = substr( self::packedToBin( inet_pton( $ipAddresRangeNetwork ) ), 0, $networkMaskBits );
$maskedBinaryIpAddress = substr( self::packedToBin( inet_pton( $ipAddress ) ), 0, $networkMaskBits ) );
return $maskedBinaryNetwork === $maskedBinaryIpAddress;
@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Oct 21, 2014

+1 from my side, last hurdle left is the code style comments from @bdunogier and your done in time for next release :)

@bdunogier

This comment has been minimized.

Copy link
Member

bdunogier commented Oct 21, 2014

Overall, too many comments per code line, but +1 on the approach. I'd just like to see a bit more self-explanatory code, and less explanations :-)

Implemented: EZP-23343: Add support for IPv6 addresses and address ra…
…nges to DebugByIP feature in eZDebug class to enable debug output

@brookinsconsulting brookinsconsulting force-pushed the brookinsconsulting:debug-by-ipv6-ip-and-ipv6-network-range branch from 9fe418b to 0173802 Oct 22, 2014

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Oct 22, 2014

Hello @andrerom and @bdunogier

Thank you for your detailed review and specific instructions

We believe we have made the requested modifications. We have done the following:

  • Reversed the isIPInNetIPv6 method arguments order
  • Fixed a variable name spelling error, ipAddressRangeNetworkPrefix
  • Removed long multi line comment (you pointed out) for a short simple replacement
  • Replaced the extra long return comparison line (you pointed out) with a broken up short version
  • Updated unit tests to match reversed isIPInNetIPv6 method arguments order
  • Tested unit tests on php5.4, php5.5 and php5.6 locally (all passed)
  • Rebased to keep this pull request commit history clean

Please let us know what you think of these changes and what the next steps should be.

Thank you again for your continued support and guidance!

Cheers,
Brookins Consulting

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Oct 23, 2014

+1, @yannickroger your +1 as well right?

@yannickroger

This comment has been minimized.

Copy link
Contributor

yannickroger commented Oct 23, 2014

+1 good job @brookinsconsulting

yannickroger added a commit that referenced this pull request Oct 23, 2014

Merge pull request #1072 from brookinsconsulting/debug-by-ipv6-ip-and…
…-ipv6-network-range

Implemented: EZP-23343: Add support for IPv6 addresses and address ranges to DebugByIP feature in eZDebug class to enable debug output

@yannickroger yannickroger merged commit 2fc3ad5 into ezsystems:master Oct 23, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@brookinsconsulting brookinsconsulting deleted the brookinsconsulting:debug-by-ipv6-ip-and-ipv6-network-range branch Oct 23, 2014

@brookinsconsulting

This comment has been minimized.

Copy link
Contributor Author

brookinsconsulting commented Oct 23, 2014

Hello,

A special heart felt thank you to everyone who helped out on this pull request.

@yannickroger thank you for your support and kind words we worked really hard on this one.

We are so glad this has made it into eZ Publish. This will help a lot of people today and in the future.

Well then, till the next one... Take it eZ!

Cheers,
Brookins Consulting

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Oct 23, 2014

Thanks for the contribution, and next one will be easier, it's a habit thing in the end :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment