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 2017/01/25 15:41:34 UTC
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
GitHub user necouchman opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/109
GUACAMOLE-47: Get client hostname for use in guac RDP session
These changes allow for the client IP (and/or hostname) to be passed through as parameters to connections, similar to username and password.
I haven't updated any documentation, but it may be necessary given that some configuration changes might be necessary in Tomcat and any proxy servers in front of Tomcat in order to make this work properly.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/incubator-guacamole-client GUACAMOLE-47
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/109.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 #109
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97835616
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,27 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Try a few methods to get client info.
+ if(request.getHeader("X-Guacamole-Client-Hostname") != null && request.getHeader("X-Guacamole-Client-Hostname") != "") {
+ this.remoteHost = request.getHeader("X-Guacamole-Client-Hostname");
+ } else if(request.getHeader("X-Forwarded-For") != null && request.getHeader("X-Forwarded-For") != "") {
+ this.remoteHost = request.getHeader("X-Forwarded-For");
+ } else if(request.getRemoteHost() != null && request.getRemoteHost() != "") {
+ this.remoteHost = request.getRemoteHost();
+ } else {
+ this.remoteHost = "";
+ }
+
+ if(request.getHeader("X-Guacamole-Client-IP") != null && request.getHeader("X-Guacamole-Client-IP") != "") {
--- End diff --
Please follow the same style as the other code in guacamole-client for the sake of readability. There should be a space between the `if` and the `(`, as `if` is not a function, and we try to avoid:
* Using braces when unnecessary (unless it hurts readability)
* Cuddling the brace with the `else` or `else if`
If unclear, we also have [general style guidelines](http://guacamole.incubator.apache.org/guac-style/), but the best way is usually to simply take a quick look around and make sure your code doesn't stand out.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r98336197
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,18 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Grab the remote host info.
+ if (request.getRemoteHost() != null && !request.getRemoteHost().isEmpty())
--- End diff --
Removed the unnecessary checks and just assigned both remote host and remote addr.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97833220
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,27 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Try a few methods to get client info.
+ if(request.getHeader("X-Guacamole-Client-Hostname") != null && request.getHeader("X-Guacamole-Client-Hostname") != "") {
--- End diff --
Why is a custom header being checked?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97837365
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -42,6 +43,16 @@
public static final String PASSWORD_TOKEN = "GUAC_PASSWORD";
/**
+ * The name of the client token added via addStandardTokens().
+ */
+ public static final String REMHOST_TOKEN = "GUAC_REMHOST";
--- End diff --
I'm not the biggest fan of "GUAC_REMHOST" and "GUAC_REMIP", given the verbosity of other tokens ("GUAC_USERNAME" and "GUAC_PASSWORD"), the ambiguity of "remote" in a multi-layered stack, and the specificity of "IP" compared to the "address" property it's pulled from.
What would you think of "GUAC_CLIENT_HOSTNAME" and "GUAC_CLIENT_ADDRESS"?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97836874
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,27 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Try a few methods to get client info.
+ if(request.getHeader("X-Guacamole-Client-Hostname") != null && request.getHeader("X-Guacamole-Client-Hostname") != "") {
+ this.remoteHost = request.getHeader("X-Guacamole-Client-Hostname");
+ } else if(request.getHeader("X-Forwarded-For") != null && request.getHeader("X-Forwarded-For") != "") {
+ this.remoteHost = request.getHeader("X-Forwarded-For");
+ } else if(request.getRemoteHost() != null && request.getRemoteHost() != "") {
+ this.remoteHost = request.getRemoteHost();
+ } else {
+ this.remoteHost = "";
+ }
+
+ if(request.getHeader("X-Guacamole-Client-IP") != null && request.getHeader("X-Guacamole-Client-IP") != "") {
--- End diff --
Removed the unnecessary braces and fixed if ( spacing in APIRequest.java.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97834919
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,27 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Try a few methods to get client info.
+ if(request.getHeader("X-Guacamole-Client-Hostname") != null && request.getHeader("X-Guacamole-Client-Hostname") != "") {
+ this.remoteHost = request.getHeader("X-Guacamole-Client-Hostname");
+ } else if(request.getHeader("X-Forwarded-For") != null && request.getHeader("X-Forwarded-For") != "") {
+ this.remoteHost = request.getHeader("X-Forwarded-For");
+ } else if(request.getRemoteHost() != null && request.getRemoteHost() != "") {
+ this.remoteHost = request.getRemoteHost();
+ } else {
+ this.remoteHost = "";
--- End diff --
Yeah, that occurred to me as I was reviewing the other changes...will remove that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97840482
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -42,6 +43,16 @@
public static final String PASSWORD_TOKEN = "GUAC_PASSWORD";
/**
+ * The name of the client token added via addStandardTokens().
+ */
+ public static final String REMHOST_TOKEN = "GUAC_REMHOST";
--- End diff --
Yeah, had no problem with it this time around, so probably is was another issue that I confused for the length of the token. Changed to your suggested values.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r98336710
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,12 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Grab the remote host info.
+ this.remoteHost = request.getRemoteHost();
+
+ // Grab the remote ip info.
--- End diff --
This poor comment is misaligned.
Also, to match the style of the rest of the guac codebase, one-line non-block comments should not end with a period (see the style of nearby lines). Same goes for the comment just above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97836251
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,27 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Try a few methods to get client info.
+ if(request.getHeader("X-Guacamole-Client-Hostname") != null && request.getHeader("X-Guacamole-Client-Hostname") != "") {
+ this.remoteHost = request.getHeader("X-Guacamole-Client-Hostname");
+ } else if(request.getHeader("X-Forwarded-For") != null && request.getHeader("X-Forwarded-For") != "") {
+ this.remoteHost = request.getHeader("X-Forwarded-For");
+ } else if(request.getRemoteHost() != null && request.getRemoteHost() != "") {
+ this.remoteHost = request.getRemoteHost();
+ } else {
+ this.remoteHost = "";
+ }
+
+ if(request.getHeader("X-Guacamole-Client-IP") != null && request.getHeader("X-Guacamole-Client-IP") != "") {
+ this.remoteAddr = request.getHeader("X-Guacamole-Client-IP");
+ } else if(request.getHeader("X-Forwarded-For") != null && request.getHeader("X-Forwarded-For") != "") {
+ this.remoteAddr = request.getHeader("X-Forwarded-For");
--- End diff --
Yes, that's correct...I was going under the assumption that if you're relying on X-Forwarded-For (which is usually an IP address) you'd want that in both tokens. I can remove the check from the hostname one so that the X-Forwarded-For only applies to the IP, or I could, instead, check to see if X-Forwarded-For is an IP-type value and assign it based on whether or not an IP is present??
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97834257
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,27 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Try a few methods to get client info.
+ if(request.getHeader("X-Guacamole-Client-Hostname") != null && request.getHeader("X-Guacamole-Client-Hostname") != "") {
+ this.remoteHost = request.getHeader("X-Guacamole-Client-Hostname");
+ } else if(request.getHeader("X-Forwarded-For") != null && request.getHeader("X-Forwarded-For") != "") {
--- End diff --
Got it, will fix this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97834210
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,27 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Try a few methods to get client info.
+ if(request.getHeader("X-Guacamole-Client-Hostname") != null && request.getHeader("X-Guacamole-Client-Hostname") != "") {
--- End diff --
Depending on how Guacamole + Tomcat is configured, namely behind a proxy, it can be difficult to get the original client information through. By default, the remote host/IP info when Tomcat is behind an Apache httpd or nginx server is whatever the hostname and IP of the proxy is, rather than the client. This is not the intended behavior of the module, since you want the hostname and/or IP of the actual client, not the proxy server. I implemented it this way in order to give some options - you can set up custom headers that pass through, you can do some Apache+Tomcat configuration, or you can rely on the standard remote host/addr fields. See comments on the JIRA issue: https://issues.apache.org/jira/browse/GUACAMOLE-47
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97834747
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,27 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Try a few methods to get client info.
+ if(request.getHeader("X-Guacamole-Client-Hostname") != null && request.getHeader("X-Guacamole-Client-Hostname") != "") {
+ this.remoteHost = request.getHeader("X-Guacamole-Client-Hostname");
+ } else if(request.getHeader("X-Forwarded-For") != null && request.getHeader("X-Forwarded-For") != "") {
+ this.remoteHost = request.getHeader("X-Forwarded-For");
+ } else if(request.getRemoteHost() != null && request.getRemoteHost() != "") {
+ this.remoteHost = request.getRemoteHost();
+ } else {
+ this.remoteHost = "";
--- End diff --
Why initialize to blank? Is there a reason you don't want to use `null` to represent the lack of a value?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97837582
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -42,6 +43,16 @@
public static final String PASSWORD_TOKEN = "GUAC_PASSWORD";
/**
+ * The name of the client token added via addStandardTokens().
+ */
+ public static final String REMHOST_TOKEN = "GUAC_REMHOST";
--- End diff --
I'm fine with that...I think the reason I settled on the shorter ones is I actually ran into an issue with a limit on the length of the tokens?? Can't remember exactly off the top of my head, but I think even GUAC_HOSTNAME and GUAC_ADDRESS were too long, but never fully tracked it down. I'll try, again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97838500
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -42,6 +43,16 @@
public static final String PASSWORD_TOKEN = "GUAC_PASSWORD";
/**
+ * The name of the client token added via addStandardTokens().
+ */
+ public static final String REMHOST_TOKEN = "GUAC_REMHOST";
--- End diff --
Hm ... There should definitely be no limit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r98336836
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,12 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Grab the remote host info.
+ this.remoteHost = request.getRemoteHost();
+
+ // Grab the remote ip info.
--- End diff --
Fixed, I think - I had used a tab instead of spaces :-/.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/109
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97833880
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,27 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Try a few methods to get client info.
+ if(request.getHeader("X-Guacamole-Client-Hostname") != null && request.getHeader("X-Guacamole-Client-Hostname") != "") {
+ this.remoteHost = request.getHeader("X-Guacamole-Client-Hostname");
+ } else if(request.getHeader("X-Forwarded-For") != null && request.getHeader("X-Forwarded-For") != "") {
--- End diff --
You can't safely do something like `someString != ""` - this is a comparison which checks the value of the reference, not the value of the string itself. What you're looking for here is `!someString.isEmpty()`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r97834447
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,27 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Try a few methods to get client info.
+ if(request.getHeader("X-Guacamole-Client-Hostname") != null && request.getHeader("X-Guacamole-Client-Hostname") != "") {
+ this.remoteHost = request.getHeader("X-Guacamole-Client-Hostname");
+ } else if(request.getHeader("X-Forwarded-For") != null && request.getHeader("X-Forwarded-For") != "") {
+ this.remoteHost = request.getHeader("X-Forwarded-For");
+ } else if(request.getRemoteHost() != null && request.getRemoteHost() != "") {
+ this.remoteHost = request.getRemoteHost();
+ } else {
+ this.remoteHost = "";
+ }
+
+ if(request.getHeader("X-Guacamole-Client-IP") != null && request.getHeader("X-Guacamole-Client-IP") != "") {
+ this.remoteAddr = request.getHeader("X-Guacamole-Client-IP");
+ } else if(request.getHeader("X-Forwarded-For") != null && request.getHeader("X-Forwarded-For") != "") {
+ this.remoteAddr = request.getHeader("X-Forwarded-For");
--- End diff --
There is no guarantee that `X-Forwarded-For` is an IP or a hostname. As written, the remote host and remote address tokens would both be populated with the same value, regardless of whether that value is actually what the token represents.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r98336845
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -42,6 +42,16 @@
private final Map<String, String[]> parameters;
/**
+ * The remote hostname that initiated the request.
+ */
+ private final String remoteHost;
+
+ /**
+ * The remote ip address that initiated the request.
--- End diff --
Tweaked the language.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r98336745
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -42,6 +42,16 @@
private final Map<String, String[]> parameters;
/**
+ * The remote hostname that initiated the request.
+ */
+ private final String remoteHost;
+
+ /**
+ * The remote ip address that initiated the request.
--- End diff --
Perhaps a bit of a semantic nitpick, but this is not the IP address that initiated the request, because IP addresses can't do things. It's the IP address of the client that initiated the request. Same thing for the hostname above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r98336899
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,12 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Grab the remote host info.
+ this.remoteHost = request.getRemoteHost();
+
+ // Grab the remote ip info.
--- End diff --
Ugh ... you monster.
I'll double-check that no further tabs exist in these changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #109: GUACAMOLE-47: Get client hostn...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/109#discussion_r98336104
--- Diff: guacamole/src/main/java/org/apache/guacamole/rest/APIRequest.java ---
@@ -58,6 +68,18 @@ public APIRequest(HttpServletRequest request,
super(request);
+ // Grab the remote host info.
+ if (request.getRemoteHost() != null && !request.getRemoteHost().isEmpty())
--- End diff --
Is it necessary to sanity check `getRemoteHost()` and `getRemoteAddr()` against `isEmpty()`, given that their return values are guaranteed by the standard? Assuming that the empty string, if it ever does occur, is a legitimate value, doesn't this boil down to:
if (request.getRemoteHost() != null)
this.remoteHost = request.getRemoteHost();
else
this.remoteHost = null;
and thus could simply be:
this.remoteAddr = request.getRemoteAddr();
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---