Skip to content
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

Use IpHelper to get IP in Logger #25520

Merged
merged 10 commits into from Oct 29, 2019
Merged

Use IpHelper to get IP in Logger #25520

merged 10 commits into from Oct 29, 2019

Conversation

SharkyKZ
Copy link
Contributor

Pull Request for Issue #25518 .

Summary of Changes

Use IpHelper to get IP in Joomla\CMS\Log\Logger\FormattedtextLogger.

Testing Instructions

Install Joomla behind a reverse proxy which provides both remote_addr and http_x_forwarded_for headers.
Perform a login failure on /administrator
Check the /logs/error.php log

Expected result

To see the ip address from http_x_forwarded_for reported as the offending IP address.

Actual result

To see the ip address from remote_addr reported as the offending IP address.

Documentation Changes Required

No.

@SharkyKZ
Copy link
Contributor Author

Failing tests related to framework. Submitted PR there joomla-framework/utilities#23.

@richard67
Copy link
Member

I can't test because I don't have a reverse proxy.

@twrhills As you had the issue handled by this PR, could you test?

@twrhills
Copy link

I can't test because I don't have a reverse proxy.

@twrhills As you had the issue handled by this PR, could you test?

@richard67 I have tested this and it is the correct solution for my case. I am unfamiliar with github so if you need me to confirm in another way I would be grateful if you can link documentation / instructions.

@Quy
Copy link
Contributor

Quy commented Jul 16, 2019

Thank you for testing!

Please mark your test here: https://issues.joomla.org/tracker/joomla-cms/25520

@richard67
Copy link
Member

@twrhills Follow the link posted in comment before by Quy, then click the "Test this" button at the top left corner above the description, then in the "Submit test result" select your test result with the check box, then click "Submit test result" button below the text field for the (otional) comment.

@twrhills
Copy link

I have tested this item ✅ successfully on 6ed695e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25520.

@alikon
Copy link
Contributor

alikon commented Aug 5, 2019

I have tested this item ✅ successfully on 6ed695e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25520.

@ghost
Copy link

ghost commented Aug 5, 2019

Status "Ready To Commit".

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 5, 2019
@HLeithner
Copy link
Member

@SharkyKZ can you please fix the error messages found by travis?

  1. JLogLoggerFormattedTextTest::testAddEntry
    Undefined index: REMOTE_ADDR
    /home/travis/build/joomla/joomla-cms/tests/unit/core/helper.php:55
    /home/travis/build/joomla/joomla-cms/libraries/vendor/joomla/utilities/src/IpHelper.php:488
    /home/travis/build/joomla/joomla-cms/libraries/vendor/joomla/utilities/src/IpHelper.php:440
    /home/travis/build/joomla/joomla-cms/libraries/vendor/joomla/utilities/src/IpHelper.php:57
    /home/travis/build/joomla/joomla-cms/libraries/src/Log/Logger/FormattedtextLogger.php:195
    /home/travis/build/joomla/joomla-cms/libraries/src/Log/Logger/FormattedtextLogger.php:171
    /home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/log/loggers/JLogLoggerFormattedTextTest.php:174
  2. JLogLoggerFormattedTextTest::testDeferWritingEntry
    Undefined index: REMOTE_ADDR
    /home/travis/build/joomla/joomla-cms/tests/unit/core/helper.php:55
    /home/travis/build/joomla/joomla-cms/libraries/vendor/joomla/utilities/src/IpHelper.php:488
    /home/travis/build/joomla/joomla-cms/libraries/vendor/joomla/utilities/src/IpHelper.php:440
    /home/travis/build/joomla/joomla-cms/libraries/vendor/joomla/utilities/src/IpHelper.php:57
    /home/travis/build/joomla/joomla-cms/libraries/src/Log/Logger/FormattedtextLogger.php:195
    /home/travis/build/joomla/joomla-cms/libraries/src/Log/Logger/FormattedtextLogger.php:139
    /home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/log/loggers/JLogLoggerFormattedTextTest.php:237
  3. JLogLoggerW3CTest::testAddEntry
    Undefined index: REMOTE_ADDR
    /home/travis/build/joomla/joomla-cms/tests/unit/core/helper.php:55
    /home/travis/build/joomla/joomla-cms/libraries/vendor/joomla/utilities/src/IpHelper.php:488
    /home/travis/build/joomla/joomla-cms/libraries/vendor/joomla/utilities/src/IpHelper.php:440
    /home/travis/build/joomla/joomla-cms/libraries/vendor/joomla/utilities/src/IpHelper.php:57
    /home/travis/build/joomla/joomla-cms/libraries/src/Log/Logger/FormattedtextLogger.php:195
    /home/travis/build/joomla/joomla-cms/libraries/src/Log/Logger/FormattedtextLogger.php:171
    /home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/log/loggers/JLogLoggerW3CTest.php:37

@HLeithner HLeithner removed the RTC This Pull Request is Ready To Commit label Aug 8, 2019
@SharkyKZ
Copy link
Contributor Author

SharkyKZ commented Aug 8, 2019

@HLeithner need to update Framework's Utilities package to fix the notice. Submitted a separate PR #25601.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 8, 2019
@HLeithner HLeithner removed the RTC This Pull Request is Ready To Commit label Aug 8, 2019
@HLeithner
Copy link
Member

@SharkyKZ thx for the info so this is RTC when the composer update is merged.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 8, 2019
@HLeithner
Copy link
Member

I created pr #26000 please test.

@richard67
Copy link
Member

@HLeithner Ist this a duplicate then to @SharkyKZ 's PR #25601 ?

@SharkyKZ
Copy link
Contributor Author

@richard67 almost. But just realized my PR reverts some changes to Composer (I was using older version). So I'll close it.

@HLeithner HLeithner added this to the Joomla! 3.9.13 milestone Sep 17, 2019
@HLeithner
Copy link
Member

@SharkyKZ IpHelper::getIp(); seams to behave different then the current code in the testing. Could you please have a look?

@SharkyKZ
Copy link
Contributor Author

@HLeithner should I change the PR so it does not break tests or update tests?

@mbabker
Copy link
Contributor

mbabker commented Oct 13, 2019

The tests should be updated. I’m going to take a wild guess that superglobal values aren’t being properly set and reset.

@SharkyKZ
Copy link
Contributor Author

There's also discrepancy between empty IP values. Before it was null, now it's an empty string 😕

@HLeithner
Copy link
Member

The w3c logger returned - with the old code now it shows an empty stringand I think thats wrong. So the null seams to be right.

@SharkyKZ
Copy link
Contributor Author

Added empty string check in the logger.

@HLeithner HLeithner merged commit 946aa61 into joomla:staging Oct 29, 2019
@HLeithner
Copy link
Member

Thanks

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 29, 2019
@SharkyKZ SharkyKZ deleted the j3/loggerIP branch October 29, 2019 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants