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

Conversation

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Jul 11, 2019

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

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Jul 12, 2019

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

@richard67

This comment has been minimized.

Copy link
Contributor

richard67 commented Jul 16, 2019

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

This comment has been minimized.

Copy link

twrhills commented Jul 16, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

richard67 commented Jul 16, 2019

@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

This comment has been minimized.

Copy link

twrhills commented Jul 16, 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.

@alikon

This comment has been minimized.

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.

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

franz-wohlkoenig commented Aug 5, 2019

Status "Ready To Commit".

@joomla-cms-bot joomla-cms-bot added the RTC label Aug 5, 2019
@HLeithner

This comment has been minimized.

Copy link
Member

HLeithner commented Aug 8, 2019

@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 label Aug 8, 2019
@SharkyKZ

This comment has been minimized.

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 label Aug 8, 2019
@HLeithner HLeithner removed the RTC label Aug 8, 2019
@HLeithner

This comment has been minimized.

Copy link
Member

HLeithner commented Aug 8, 2019

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

@joomla-cms-bot joomla-cms-bot added the RTC label Aug 8, 2019
@HLeithner

This comment has been minimized.

Copy link
Member

HLeithner commented Aug 23, 2019

I created pr #26000 please test.

@richard67

This comment has been minimized.

Copy link
Contributor

richard67 commented Aug 23, 2019

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

@SharkyKZ

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Aug 23, 2019

@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

This comment has been minimized.

Copy link
Member

HLeithner commented Oct 12, 2019

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

@SharkyKZ

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Oct 13, 2019

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

@mbabker

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Oct 13, 2019

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

@HLeithner

This comment has been minimized.

Copy link
Member

HLeithner commented Oct 13, 2019

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

This comment has been minimized.

Copy link
Contributor Author

SharkyKZ commented Oct 13, 2019

Added empty string check in the logger.

HLeithner added 2 commits Oct 18, 2019
@HLeithner HLeithner merged commit 946aa61 into joomla:staging Oct 29, 2019
4 checks passed
4 checks passed
Hound No violations found. Woof!
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@HLeithner

This comment has been minimized.

Copy link
Member

HLeithner commented Oct 29, 2019

Thanks

@joomla-cms-bot joomla-cms-bot removed the RTC label Oct 29, 2019
@SharkyKZ SharkyKZ deleted the SharkyKZ:j3/loggerIP branch Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.