[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/242#discussion_r165833343
  
--- Diff: 
extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java
 ---
@@ -282,15 +282,15 @@ public RadiusPacket authenticate(String username, 
String secret, String state)
  * @throws GuacamoleException
  * If an error is encountered trying to talk to the RADIUS server.
  */
-public RadiusPacket sendChallengeResponse(String username, String 
response, String state)
+public RadiusPacket sendChallengeResponse(String username, String 
response, byte[] state)
 throws GuacamoleException {
 
 if (username == null || username.isEmpty()) {
 logger.error("Challenge/response to RADIUS requires a 
username.");
 return null;
 }
 
-if (state == null || state.isEmpty()) {
+if (state == null || state.length < 1) {
--- End diff --

Okay, should be done in a slightly more acceptable fashion, now.


---

[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/242#discussion_r16589
  
--- Diff: 
extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java
 ---
@@ -155,9 +157,10 @@ public AuthenticatedUser authenticateUser(Credentials 
credentials)
 // This is a response to a previous challenge, authenticate with 
that.
 else {
 try {
+byte[] stateBytes = 
javax.xml.bind.DatatypeConverter.parseHexBinary(request.getParameter(RadiusStateField.PARAMETER_NAME));
--- End diff --

Okay, removed the extra verbosity, handled the case where state field is 
null, and the `IllegalArgumentException`.


---

[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/242#discussion_r165833178
  
--- Diff: 
extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java
 ---
@@ -155,9 +157,10 @@ public AuthenticatedUser authenticateUser(Credentials 
credentials)
 // This is a response to a previous challenge, authenticate with 
that.
 else {
 try {
+byte[] stateBytes = 
javax.xml.bind.DatatypeConverter.parseHexBinary(request.getParameter(RadiusStateField.PARAMETER_NAME));
--- End diff --

Very true.


---

[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/242#discussion_r165833167
  
--- Diff: 
extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java
 ---
@@ -155,9 +157,10 @@ public AuthenticatedUser authenticateUser(Credentials 
credentials)
 // This is a response to a previous challenge, authenticate with 
that.
 else {
 try {
+byte[] stateBytes = 
javax.xml.bind.DatatypeConverter.parseHexBinary(request.getParameter(RadiusStateField.PARAMETER_NAME));
--- End diff --

At least I was consistent.


---

[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...

[necouchman]

Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/242#discussion_r165833158
  
--- Diff: 
extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java
 ---
@@ -97,7 +99,7 @@ private CredentialsInfo getRadiusChallenge(RadiusPacket 
challengePacket) {
 
 // We have the required attributes - convert to strings and then 
generate the additional login box/field
 String replyMsg = replyAttr.toString();
-String radiusState = new String(stateAttr.getValue().getBytes());
+String radiusState = 
javax.xml.bind.DatatypeConverter.printHexBinary(stateAttr.getValue().getBytes());
--- End diff --

Haha.  Well, that was silly.  Removed.


---

[GitHub] guacamole-client pull request #243: GUACAMOLE-316: Tweak to handling IE Comp...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/243#discussion_r165833126
  
--- Diff: guacamole/src/main/webapp/index.html ---
@@ -19,7 +19,8 @@
 -->
 
 
-
+
+
--- End diff --

Is it "x-ua-compatible" or "X-UA-Compatible"?


---

[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/242#discussion_r165832815
  
--- Diff: 
extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java
 ---
@@ -97,7 +99,7 @@ private CredentialsInfo getRadiusChallenge(RadiusPacket 
challengePacket) {
 
 // We have the required attributes - convert to strings and then 
generate the additional login box/field
 String replyMsg = replyAttr.toString();
-String radiusState = new String(stateAttr.getValue().getBytes());
+String radiusState = 
javax.xml.bind.DatatypeConverter.printHexBinary(stateAttr.getValue().getBytes());
--- End diff --

Yowza. My knee-jerk reaction was to suggest that you import 
`javax.xml.bind.DatatypeConverter`, but I see you've actually already done 
that. ;)

This should be switched over to just 
`DatatypeConverter.printHexBinary(...)`.


---

[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/242#discussion_r165832875
  
--- Diff: 
extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java
 ---
@@ -155,9 +157,10 @@ public AuthenticatedUser authenticateUser(Credentials 
credentials)
 // This is a response to a previous challenge, authenticate with 
that.
 else {
 try {
+byte[] stateBytes = 
javax.xml.bind.DatatypeConverter.parseHexBinary(request.getParameter(RadiusStateField.PARAMETER_NAME));
--- End diff --

I'd like to also mention that this will likely throw a 
`NullPointerException` if the state field is not present in the request. At 
best, the behavior of 
[`parseHexBinary()`](https://docs.oracle.com/javase/7/docs/api/javax/xml/bind/DatatypeConverter.html#parseHexBinary(java.lang.String))
 is not defined for the case where its sole parameter is `null`. The potential 
lack of the parameter in the request should probably be dealt with.


---

[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...

[mike-jumper]

Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/242#discussion_r165833012
  
--- Diff: 
extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java
 ---
@@ -282,15 +282,15 @@ public RadiusPacket authenticate(String username, 
String secret, String state)
  * @throws GuacamoleException
  * If an error is encountered trying to talk to the RADIUS server.
  */
-public RadiusPacket sendChallengeResponse(String username, String 
response, String state)
+public RadiusPacket sendChallengeResponse(String username, String 
response, byte[] state)
 throws GuacamoleException {
 
 if (username == null || username.isEmpty()) {
 logger.error("Challenge/response to RADIUS requires a 
username.");
 return null;
 }
 
-if (state == null || state.isEmpty()) {
+if (state == null || state.length < 1) {
--- End diff --

Testing whether the array length is less than one is an odd way to check 
whether the array is empty. There is exactly one length value which represents 
an empty array, and while that value is indeed less than one, it'd make more 
sense to test against that value directly.


---

[GitHub] guacamole-client pull request #243: GUACAMOLE-316: Tweak to handling IE Comp...

[necouchman]

GitHub user necouchman opened a pull request:

https://github.com/apache/guacamole-client/pull/243

GUACAMOLE-316: Tweak to handling IE Compatibility Mode

This is a quick tweak suggested by the person who opened -316 in JIRA to 
correctly preventing Compatibility Mode from kicking in for IE when it is 
enabled for subnets or certain networks.

Tested with Chrome on Linux and Windows to make sure there were no adverse 
impacts, there, and then tested on IE11 and Edge in Windows 10.  I don't have 
compatibility mode enabled across my network, so I don't know that it actually 
resolves the issue, but it doesn't make things worse.  Will ping back on the 
JIRA issue and see if I can get the owner to test it for me - unless someone 
else can give it a try??

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/necouchman/guacamole-client GUACAMOLE-316

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/guacamole-client/pull/243.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 #243


commit 8747acfc7f3650750bd89c8259d556675f48dc01
Author: Nick Couchman 
Date:   2018-02-03T22:39:48Z

GUACAMOLE-316: Make sure IE compatibility mode does not impact Guacamole.




---

[GitHub] guacamole-client pull request #241: GUACAMOLE-197: Specify Charset when crea...

[necouchman]

Github user necouchman closed the pull request at:

https://github.com/apache/guacamole-client/pull/241


---