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.

Sign up for GitHub

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

Jump to bottom

Use getRemoteAddress which supports reverse proxies #10735

Merged
merged 1 commit into from
Feb 25, 2015
Merged

Use getRemoteAddress which supports reverse proxies #10735

merged 1 commit into from
Feb 25, 2015

Conversation

LukasReschke
Copy link
Member

Breaking change for 8.1 wiki (Security > Administrators):

The log format for failed logins has changed and uses now the remote address and is considering reverse proxies for such scenarios when configured correctly.

@PVince81
Copy link
Contributor

PVince81 commented Sep 3, 2014

Try increasing your grepping power. I found 4 instances of REMOTE_ADDR instead of 3
Fourth one is in lib/private/user/manager.php

@LukasReschke
Copy link
Member Author

Yes. But there we log the X-Forwarded-For header instead. I'm not sure if we want to break this again. (some use this together with Fail2Ban)

But why not - as long as it is only for master. Objections?

@bantu
Copy link

bantu commented Sep 15, 2014

👍

@LukasReschke LukasReschke force-pushed the use_remote_addr branch 2 times, most recently from 7006ce1 to b66017e Compare September 16, 2014 15:01
@PVince81
Copy link
Contributor

👍

@PVince81
Copy link
Contributor

It did not break the preview, I was still able to login and OC seemed to work fine.

@LukasReschke
Copy link
Member Author

'll take care of the failing unit tests.

@LukasReschke
Copy link
Member Author

Failing tests has been fixed with #11019 - please review this as well, afterwards we can rebase and run the tests again.

@PVince81
Copy link
Contributor

Needs rebase

@MorrisJobke
Copy link
Contributor

@LukasReschke Revive this for 8.0 or for 8.1?

@LukasReschke
Copy link
Member Author

8.1 as well - changes logging format with possible annoyed users otherwise.

@MorrisJobke MorrisJobke added this to the 8.1-next milestone Jan 9, 2015
@LukasReschke
Use getRemoteAddress which supports reverse proxies
165afb0 
Breaking change for 8.1 wiki (Security > Administrators):

The log format for failed logins has changed and uses now the remote address and is considering reverse proxies for such scenarios when configured correctly.
@LukasReschke
Copy link
Member Author

Rebased - ready to review.

@LukasReschke
Copy link
Member Author

@nickvergessen @th3fallen Please review.

@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@ghost
Copy link

ghost commented Feb 24, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9878/
Test PASSed.

@nickvergessen
Copy link
Contributor

👍

@RobinMcCorkell
Copy link
Member

👍

RobinMcCorkell pushed a commit that referenced this pull request Feb 25, 2015
Merge pull request #10735 from owncloud/use_remote_addr
695f43a 
Use getRemoteAddress which supports reverse proxies
@RobinMcCorkell RobinMcCorkell merged commit 695f43a into master Feb 25, 2015
@RobinMcCorkell RobinMcCorkell deleted the use_remote_addr branch February 25, 2015 13:24
@LukasReschke
Copy link
Member Author

Thanks guys! 😄

@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
3 - To Review
Projects
None yet
Milestone
8.1
Development

Successfully merging this pull request may close these issues.
8 participants
@LukasReschke @PVince81 @bantu @MorrisJobke @scrutinizer-notifier @nickvergessen @RobinMcCorkell @DeepDiver1975