You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/04/23 14:55:37 UTC
[GitHub] [guacamole-client] holograph opened a new pull request #500: GUACAMOLE-1048: Support server control commands during handshake
holograph opened a new pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500
There aren't many tests in `guacamole-common`; I have a separate utility class that can help test this, but it's way outside the scope and will be a separate PR once I've cleaned it up a bit.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] holograph commented on issue #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
holograph commented on issue #500:
URL: https://github.com/apache/guacamole-client/pull/500#issuecomment-618444609
This can be fairly easily tested with the before/after versions of apache/guacamole-server#271.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500#discussion_r637511477
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleServerErrorCommandException.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole;
+
+import org.apache.guacamole.protocol.GuacamoleStatus;
+
+/**
+ * The exception thrown when the Guacamole server explicitly sends an error
+ * command to the client. The error message and status code reflect the arguments
+ * of the error command as determined by the server.
+ */
+public class GuacamoleServerErrorCommandException extends GuacamoleServerException {
Review comment:
What about adding a static utility method like `fromStatus()` to `GuacamoleServerException`? For example:
```java
public static GuacamoleServerException fromStatus(GuacamoleStatus status, String message) {
...
}
```
As we already provide an exception for each status code, code attempting to connect using `ConfiguredGuacamoleSocket` would be able to handle specific error conditions by catching the appropriate exceptions, rather than catching a new type of exception and inspecting the status.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] holograph commented on pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
holograph commented on pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500#issuecomment-778782947
@mike-jumper Adjusted the code per your comments, have another go?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] holograph commented on a change in pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
holograph commented on a change in pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500#discussion_r575809104
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleServerErrorCommandException.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole;
+
+import org.apache.guacamole.protocol.GuacamoleStatus;
+
+/**
+ * The exception thrown when the Guacamole server explicitly sends an error
+ * command to the client. The error message and status code reflect the arguments
+ * of the error command as determined by the server.
+ */
+public class GuacamoleServerErrorCommandException extends GuacamoleServerException {
Review comment:
You're right about the command/instruction thing, I missed it. The problem with `GuacamoleHandshakeException` is that it ties the error to the specific connection phase; an `error` instruction can occur at any time in any phase. I do believe it deserves special semantics to distinguish it from any other error condition, because that allows for very explicit and clear `catch` blocks - this was the original impetus for this PR.
I honestly couldn't find a more descriptive, and semantically appropriate, name than `GuacamoleServerErrorInstructionException` (adjusting for your comment, of course). Considering any part of it:
* Guacamole can be omitted since it's contextual and present in the package name, but then it won't follow the already-established convention;
* Server can't really be omitted, as it's not denoted by the package and it's semantically important;
* ErrorInstruction is, well, what it is;
* and Exception can be omitted, but that would be against the preexisting convention in this codebase (and the Java ecosystem as a whole).
I know at this point most people reading this think this is splitting hairs, sorry ladies and gentlemen :-) Serious though @mike-jumper, if you have any better names I'd be happy to take them. This one is a mouthful.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] holograph commented on a change in pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
holograph commented on a change in pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500#discussion_r575814178
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/ConfiguredGuacamoleSocket.java
##########
@@ -83,14 +111,24 @@ private GuacamoleInstruction expect(GuacamoleReader reader, String opcode)
if (instruction == null)
throw new GuacamoleServerException("End of stream while waiting for \"" + opcode + "\".");
+ // Handle server control commands
+ if ("disconnect".equals(instruction.getOpcode()))
+ throw new GuacamoleServerException("Server disconnected while waiting for \"" + opcode + "\".");
+ if ("error".equals(instruction.getOpcode())) {
+ GuacamoleServerErrorCommandException e = parseServerErrorCommandArgs(instruction.getArgs());
+ if (e == null)
+ throw new GuacamoleServerException("Invalid command received from server: " + instruction);
Review comment:
Fair enough, incorporated the feedback. After a couple of iterations it does look better, have a look.
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/ConfiguredGuacamoleSocket.java
##########
@@ -63,12 +64,39 @@
*/
private GuacamoleProtocolVersion protocolVersion =
GuacamoleProtocolVersion.VERSION_1_0_0;
-
+
+ /**
+ * Parses the arguments for the Guacamole "error" server command and returns
+ * the corresponding exception.
+ * @param args The arguments as provided by the server command.
+ * @return An instance of {@link GuacamoleServerErrorCommandException} configured
+ * with the server-provided arguments, or {@literal null} if the specified
+ * arguments are invalid.
+ */
+ private static GuacamoleServerErrorCommandException parseServerErrorCommandArgs(List<String> args) {
+ if (args == null || args.size() != 2)
+ return null;
+
+ int code;
+ try {
+ code = Integer.parseInt(args.get(1));
+ } catch (NumberFormatException e) {
+ return null;
+ }
+ GuacamoleStatus status = GuacamoleStatus.fromGuacamoleStatusCode(code);
+ return (status == null)
+ ? null
+ : new GuacamoleServerErrorCommandException(args.get(0), status);
+ }
+
/**
* Waits for the instruction having the given opcode, returning that
* instruction once it has been read. If the instruction is never read,
* an exception is thrown.
- *
+ *
+ * Respects server control commands that are allowed during the handshake
Review comment:
Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] holograph commented on a change in pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
holograph commented on a change in pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500#discussion_r637563123
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleServerErrorCommandException.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole;
+
+import org.apache.guacamole.protocol.GuacamoleStatus;
+
+/**
+ * The exception thrown when the Guacamole server explicitly sends an error
+ * command to the client. The error message and status code reflect the arguments
+ * of the error command as determined by the server.
+ */
+public class GuacamoleServerErrorCommandException extends GuacamoleServerException {
Review comment:
Considering I no longer work for Strigo I don't really have a strong opinion or even a test harness. Feel free to do with this pull request as you will...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper closed pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
mike-jumper closed pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500#issuecomment-840853151
@mike-jumper This one is waiting on your review...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] holograph commented on a change in pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
holograph commented on a change in pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500#discussion_r575814130
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/ConfiguredGuacamoleSocket.java
##########
@@ -63,12 +64,39 @@
*/
private GuacamoleProtocolVersion protocolVersion =
GuacamoleProtocolVersion.VERSION_1_0_0;
-
+
+ /**
+ * Parses the arguments for the Guacamole "error" server command and returns
+ * the corresponding exception.
+ * @param args The arguments as provided by the server command.
+ * @return An instance of {@link GuacamoleServerErrorCommandException} configured
+ * with the server-provided arguments, or {@literal null} if the specified
+ * arguments are invalid.
+ */
+ private static GuacamoleServerErrorCommandException parseServerErrorCommandArgs(List<String> args) {
+ if (args == null || args.size() != 2)
+ return null;
Review comment:
1. Fixed
2. None, apparently. I've changed the signature per your other comments, so this is no longer an issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500#discussion_r575728206
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleServerErrorCommandException.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole;
+
+import org.apache.guacamole.protocol.GuacamoleStatus;
+
+/**
+ * The exception thrown when the Guacamole server explicitly sends an error
+ * command to the client. The error message and status code reflect the arguments
+ * of the error command as determined by the server.
+ */
+public class GuacamoleServerErrorCommandException extends GuacamoleServerException {
Review comment:
Regarding `GuacamoleServerErrorCommandException` and its description, "error command" isn't terminology used within the Guacamole protocol. In this case, the correct term would be "error instruction", but I don't think simply renaming things to `GuacamoleServerErrorInstructionException` would be a good choice, as that too closely ties the error to the vehicle carrying it. The naming of the exception and the documentation describing the exception should capture the context, rather than the means.
I suggest something like `GuacamoleHandshakeException`, for an error that prevents the Guacamole protocol handshake from continuing. I'm sure there are other possibilities.
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/ConfiguredGuacamoleSocket.java
##########
@@ -83,14 +111,24 @@ private GuacamoleInstruction expect(GuacamoleReader reader, String opcode)
if (instruction == null)
throw new GuacamoleServerException("End of stream while waiting for \"" + opcode + "\".");
+ // Handle server control commands
Review comment:
instructions*
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/ConfiguredGuacamoleSocket.java
##########
@@ -63,12 +64,39 @@
*/
private GuacamoleProtocolVersion protocolVersion =
GuacamoleProtocolVersion.VERSION_1_0_0;
-
+
+ /**
+ * Parses the arguments for the Guacamole "error" server command and returns
+ * the corresponding exception.
+ * @param args The arguments as provided by the server command.
+ * @return An instance of {@link GuacamoleServerErrorCommandException} configured
+ * with the server-provided arguments, or {@literal null} if the specified
+ * arguments are invalid.
+ */
+ private static GuacamoleServerErrorCommandException parseServerErrorCommandArgs(List<String> args) {
+ if (args == null || args.size() != 2)
+ return null;
Review comment:
1. It's pretty common for instructions to be extended down the line by adding new arguments, relying on older implementations to ignore any additional arguments. I think it would be better here to test `args.size() < 2`.
2. Under what circumstance would `args` be `null`?
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/ConfiguredGuacamoleSocket.java
##########
@@ -83,14 +111,24 @@ private GuacamoleInstruction expect(GuacamoleReader reader, String opcode)
if (instruction == null)
throw new GuacamoleServerException("End of stream while waiting for \"" + opcode + "\".");
+ // Handle server control commands
+ if ("disconnect".equals(instruction.getOpcode()))
+ throw new GuacamoleServerException("Server disconnected while waiting for \"" + opcode + "\".");
+ if ("error".equals(instruction.getOpcode())) {
+ GuacamoleServerErrorCommandException e = parseServerErrorCommandArgs(instruction.getArgs());
+ if (e == null)
+ throw new GuacamoleServerException("Invalid command received from server: " + instruction);
Review comment:
These are referred to as instructions, not commands.
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleServerErrorCommandException.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole;
+
+import org.apache.guacamole.protocol.GuacamoleStatus;
+
+/**
+ * The exception thrown when the Guacamole server explicitly sends an error
+ * command to the client. The error message and status code reflect the arguments
+ * of the error command as determined by the server.
+ */
+public class GuacamoleServerErrorCommandException extends GuacamoleServerException {
+ /**
+ * The Guacamole protocol status code, as determined by the server;
+ */
+ private final GuacamoleStatus status;
+
+ /**
+ * Creates a new GuacamoleServerException with the given message and status.
+ *
+ * @param message The error message, as determined by the server where the error
+ * originated.
+ * @param status The status code, as determined by the server where the error originated.
Review comment:
Please follow the style used for documentation elsewhere in the code. For example:
https://github.com/apache/guacamole-client/blob/0091bb1aea14c567c8166f0ed8eadf7c31b6bd6e/guacamole-common/src/main/java/org/apache/guacamole/GuacamoleUpstreamUnavailableException.java#L30-L42
The style you've used here is similar to what we used to do, but we migrated away from that since it can be difficult to read, particularly for long variable names. New code should use the new style.
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleServerErrorCommandException.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole;
+
+import org.apache.guacamole.protocol.GuacamoleStatus;
+
+/**
+ * The exception thrown when the Guacamole server explicitly sends an error
+ * command to the client. The error message and status code reflect the arguments
+ * of the error command as determined by the server.
+ */
+public class GuacamoleServerErrorCommandException extends GuacamoleServerException {
+ /**
+ * The Guacamole protocol status code, as determined by the server;
+ */
+ private final GuacamoleStatus status;
+
+ /**
+ * Creates a new GuacamoleServerException with the given message and status.
Review comment:
This should note the name of the exception in question, not `GuacamoleServerException`.
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/ConfiguredGuacamoleSocket.java
##########
@@ -63,12 +64,39 @@
*/
private GuacamoleProtocolVersion protocolVersion =
GuacamoleProtocolVersion.VERSION_1_0_0;
-
+
+ /**
+ * Parses the arguments for the Guacamole "error" server command and returns
+ * the corresponding exception.
+ * @param args The arguments as provided by the server command.
+ * @return An instance of {@link GuacamoleServerErrorCommandException} configured
+ * with the server-provided arguments, or {@literal null} if the specified
+ * arguments are invalid.
+ */
+ private static GuacamoleServerErrorCommandException parseServerErrorCommandArgs(List<String> args) {
+ if (args == null || args.size() != 2)
+ return null;
+
+ int code;
+ try {
+ code = Integer.parseInt(args.get(1));
+ } catch (NumberFormatException e) {
+ return null;
+ }
+ GuacamoleStatus status = GuacamoleStatus.fromGuacamoleStatusCode(code);
+ return (status == null)
+ ? null
+ : new GuacamoleServerErrorCommandException(args.get(0), status);
+ }
+
/**
* Waits for the instruction having the given opcode, returning that
* instruction once it has been read. If the instruction is never read,
* an exception is thrown.
- *
+ *
+ * Respects server control commands that are allowed during the handshake
Review comment:
instructions*
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/ConfiguredGuacamoleSocket.java
##########
@@ -83,14 +111,24 @@ private GuacamoleInstruction expect(GuacamoleReader reader, String opcode)
if (instruction == null)
throw new GuacamoleServerException("End of stream while waiting for \"" + opcode + "\".");
+ // Handle server control commands
+ if ("disconnect".equals(instruction.getOpcode()))
+ throw new GuacamoleServerException("Server disconnected while waiting for \"" + opcode + "\".");
+ if ("error".equals(instruction.getOpcode())) {
+ GuacamoleServerErrorCommandException e = parseServerErrorCommandArgs(instruction.getArgs());
+ if (e == null)
+ throw new GuacamoleServerException("Invalid command received from server: " + instruction);
Review comment:
Might be better to let it be the responsibility of the function parsing the `error` instruction to decide whether the instruction is valid and throw an appropriate exception if it isn't. This would simplify the logic here and more cleanly separate concerns.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500#issuecomment-846912912
> Considering I no longer work for Strigo I don't really have a strong opinion or even a test harness. Feel free to do with this pull request as you will...
Sorry to hear that, but thanks for letting us know. I've leveraged these changes as a basis and opened #611.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] holograph commented on a change in pull request #500: GUACAMOLE-1048: Support server control commands during handshake
Posted by GitBox <gi...@apache.org>.
holograph commented on a change in pull request #500:
URL: https://github.com/apache/guacamole-client/pull/500#discussion_r575814039
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleServerErrorCommandException.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole;
+
+import org.apache.guacamole.protocol.GuacamoleStatus;
+
+/**
+ * The exception thrown when the Guacamole server explicitly sends an error
+ * command to the client. The error message and status code reflect the arguments
+ * of the error command as determined by the server.
+ */
+public class GuacamoleServerErrorCommandException extends GuacamoleServerException {
+ /**
+ * The Guacamole protocol status code, as determined by the server;
+ */
+ private final GuacamoleStatus status;
+
+ /**
+ * Creates a new GuacamoleServerException with the given message and status.
+ *
+ * @param message The error message, as determined by the server where the error
+ * originated.
+ * @param status The status code, as determined by the server where the error originated.
Review comment:
Fixed
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/GuacamoleServerErrorCommandException.java
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole;
+
+import org.apache.guacamole.protocol.GuacamoleStatus;
+
+/**
+ * The exception thrown when the Guacamole server explicitly sends an error
+ * command to the client. The error message and status code reflect the arguments
+ * of the error command as determined by the server.
+ */
+public class GuacamoleServerErrorCommandException extends GuacamoleServerException {
+ /**
+ * The Guacamole protocol status code, as determined by the server;
+ */
+ private final GuacamoleStatus status;
+
+ /**
+ * Creates a new GuacamoleServerException with the given message and status.
Review comment:
Fixed
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/ConfiguredGuacamoleSocket.java
##########
@@ -83,14 +111,24 @@ private GuacamoleInstruction expect(GuacamoleReader reader, String opcode)
if (instruction == null)
throw new GuacamoleServerException("End of stream while waiting for \"" + opcode + "\".");
+ // Handle server control commands
+ if ("disconnect".equals(instruction.getOpcode()))
+ throw new GuacamoleServerException("Server disconnected while waiting for \"" + opcode + "\".");
+ if ("error".equals(instruction.getOpcode())) {
+ GuacamoleServerErrorCommandException e = parseServerErrorCommandArgs(instruction.getArgs());
+ if (e == null)
+ throw new GuacamoleServerException("Invalid command received from server: " + instruction);
Review comment:
Fixed
##########
File path: guacamole-common/src/main/java/org/apache/guacamole/protocol/ConfiguredGuacamoleSocket.java
##########
@@ -83,14 +111,24 @@ private GuacamoleInstruction expect(GuacamoleReader reader, String opcode)
if (instruction == null)
throw new GuacamoleServerException("End of stream while waiting for \"" + opcode + "\".");
+ // Handle server control commands
Review comment:
Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org