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

2018-02-04 Thread necouchman
Github user necouchman commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/242#discussion_r165853678
  
--- Diff: 
extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java
 ---
@@ -155,9 +158,21 @@ public AuthenticatedUser authenticateUser(Credentials 
credentials)
 // This is a response to a previous challenge, authenticate with 
that.
 else {
 try {
+String stateString = 
request.getParameter(RadiusStateField.PARAMETER_NAME);
+if (stateString == null) {
+logger.warn("Expected state parameter was not present 
in challenge/response.");
+throw new 
GuacamoleInvalidCredentialsException("Authentication error.", 
CredentialsInfo.USERNAME_PASSWORD);
+}
+
+byte[] stateBytes = 
DatatypeConverter.parseHexBinary(stateString);
 radPack = 
radiusService.sendChallengeResponse(credentials.getUsername(),
- challengeResponse,
- 
request.getParameter(RadiusStateField.PARAMETER_NAME));
+  
challengeResponse,
+  stateBytes);
+}
+catch (IllegalArgumentException e) {
+logger.warn("Illegal hexadecimal value while parsing 
RADIUS state string.", e.getMessage());
+logger.debug("Encountered exception while attepmting to 
perse the hexidecimanl state value.", e);
--- End diff --

Whoa, not my best job of speling.


---


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

2018-02-04 Thread mike-jumper
Github user mike-jumper commented on a diff in the pull request:

https://github.com/apache/guacamole-client/pull/242#discussion_r165853668
  
--- Diff: 
extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java
 ---
@@ -155,9 +158,21 @@ public AuthenticatedUser authenticateUser(Credentials 
credentials)
 // This is a response to a previous challenge, authenticate with 
that.
 else {
 try {
+String stateString = 
request.getParameter(RadiusStateField.PARAMETER_NAME);
+if (stateString == null) {
+logger.warn("Expected state parameter was not present 
in challenge/response.");
+throw new 
GuacamoleInvalidCredentialsException("Authentication error.", 
CredentialsInfo.USERNAME_PASSWORD);
+}
+
+byte[] stateBytes = 
DatatypeConverter.parseHexBinary(stateString);
 radPack = 
radiusService.sendChallengeResponse(credentials.getUsername(),
- challengeResponse,
- 
request.getParameter(RadiusStateField.PARAMETER_NAME));
+  
challengeResponse,
+  stateBytes);
+}
+catch (IllegalArgumentException e) {
+logger.warn("Illegal hexadecimal value while parsing 
RADIUS state string.", e.getMessage());
--- End diff --

If you are going to include `e.getMessage()` within the log message, you 
will need to use the `{}` placeholder (analogous to `%s` of `printf`). See:


https://github.com/apache/guacamole-client/blob/9b5483edc24d88a05075343c76377057de09f9e8/extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java#L127


---


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

2018-02-03 Thread 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...

2018-02-03 Thread 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...

2018-02-03 Thread 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...

2018-02-03 Thread 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...

2018-02-03 Thread 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 #242: GUACAMOLE-197: Convert state to Hex stri...

2018-02-03 Thread 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...

2018-02-03 Thread 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...

2018-02-03 Thread 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.


---