You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2018/05/15 17:46:44 UTC
[GitHub] guacamole-client pull request #287: GUACAMOLE-540: Make remote address proce...
GitHub user necouchman opened a pull request:
https://github.com/apache/guacamole-client/pull/287
GUACAMOLE-540: Make remote address processing consistent
This PR moves the setting of the remote address and remote hostname processing into the `Credentials` class and removes it from the client classes. This should resolve the issue in GUACAMOLE-540, making determination of remote address more consistent across connection and user objects.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/guacamole-client jira/540
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-client/pull/287.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #287
----
commit 396b5f62947f8eaae828af63649647127eafdab8
Author: Nick Couchman <vn...@...>
Date: 2018-05-15T17:43:39Z
GUACAMOLE-540: Move remote address processing to Credentials class for consistency.
----
---
[GitHub] guacamole-client pull request #287: GUACAMOLE-540: Make remote address proce...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/287#discussion_r199348882
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUserContext.java ---
@@ -137,7 +137,7 @@ public void init(ModeledAuthenticatedUser currentUser) {
userRecord = new ActivityRecordModel();
userRecord.setUsername(currentUser.getIdentifier());
userRecord.setStartDate(new Date());
- userRecord.setRemoteHost(currentUser.getCredentials().getRemoteHostname());
+ userRecord.setRemoteHost(currentUser.getCredentials().getRemoteAddress());
--- End diff --
The main driver behind this JIRA issue was to gain consistency behind which host information was used in user login history, connection history, active connections, and log messages. The `getRemoteHost()` method of `RemoteAuthenticatedUser`, used for outputting to logs and for active connections (and thus connection history records) would first look at `X-Forwarded-For` and, if that wasn't present, revert to the `getRemoteAddr()` method on the `HttpServletRequest` class. As you note above, the user login record was tracked by `getRemoteHostname()` from the `Credentials` object.
In trying to figure out the most reliable way to make this consistent I opted for IP address - it's used already by everything except user login tracking, and, in my anecdotal experience, getting the IP address from the request seems to be more reliable than retrieving the hostname - seems like you often end up with the IP address from the `getRemoteHost()` call in the `HttpServletRequest`, anyway, or something like "localhost."
All that said, I'm not particularly attached to one method or the other - I'd just like to make it consistent. So, I'm happy to switch all of it over to `getRemoteHostname()` instead of `getRemoteAddress()` if you feel like that's more useful/desired behavior, or we can leave it like this.
---
[GitHub] guacamole-client pull request #287: GUACAMOLE-540: Make remote address proce...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/287#discussion_r199340632
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledUserContext.java ---
@@ -137,7 +137,7 @@ public void init(ModeledAuthenticatedUser currentUser) {
userRecord = new ActivityRecordModel();
userRecord.setUsername(currentUser.getIdentifier());
userRecord.setStartDate(new Date());
- userRecord.setRemoteHost(currentUser.getCredentials().getRemoteHostname());
+ userRecord.setRemoteHost(currentUser.getCredentials().getRemoteAddress());
--- End diff --
Why is this changing from `getRemoteHostname()` to `getRemoteAddress()`?
---
[GitHub] guacamole-client pull request #287: GUACAMOLE-540: Make remote address proce...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-client/pull/287
---