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/11/03 13:31:37 UTC
[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #308: GUACAMOLE-221: Rely on FreeRDP error code if no RDP disconnect reason is available.
mike-jumper commented on a change in pull request #308:
URL: https://github.com/apache/guacamole-server/pull/308#discussion_r516253515
##########
File path: src/protocols/rdp/error.c
##########
@@ -18,14 +18,90 @@
*/
#include "error.h"
-#include "rdp.h"
#include <freerdp/freerdp.h>
#include <guacamole/client.h>
#include <guacamole/protocol.h>
#include <guacamole/socket.h>
+#include <winpr/wtypes.h>
-void guac_rdp_client_abort(guac_client* client) {
+/**
+ * Translates the error code returned by freerdp_get_last_error() for the given
+ * RDP instance into a Guacamole status code and human-readable message. If no
+ * error was reported, a successful error code and message will be assigned.
+ *
+ * @param rdp_inst
+ * The FreeRDP client instance handling the RDP connection that failed.
+ *
+ * @param status
+ * Pointer to the variable that should receive the guac_protocol_status
+ * value equivalent to the error returned by freerdp_get_last_error().
+ *
+ * @param message
+ * Pointer to the variable that should receive a static human-readable
+ * message generally describing the error returned by
+ * freerdp_get_last_error().
+ */
+static void guac_rdp_translate_last_error(freerdp* rdp_inst,
+ guac_protocol_status* status, const char** message) {
+
+ UINT32 last_error = freerdp_get_last_error(rdp_inst->context);
+ switch (last_error) {
+
+ /* Normal disconnect */
+ case FREERDP_ERROR_NONE:
+ case FREERDP_ERROR_SUCCESS:
+ *status = GUAC_PROTOCOL_STATUS_SUCCESS;
+ *message = "Disconnected.";
+ break;
+
+ /* Authentication failure */
+ case FREERDP_ERROR_AUTHENTICATION_FAILED:
+ case FREERDP_ERROR_CONNECT_ACCESS_DENIED:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_DISABLED:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_EXPIRED:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_LOCKED_OUT:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_RESTRICTION:
+ case FREERDP_ERROR_CONNECT_CLIENT_REVOKED:
+ case FREERDP_ERROR_CONNECT_LOGON_FAILURE:
+ case FREERDP_ERROR_CONNECT_LOGON_TYPE_NOT_GRANTED:
+ case FREERDP_ERROR_CONNECT_NO_OR_MISSING_CREDENTIALS:
+ case FREERDP_ERROR_CONNECT_PASSWORD_CERTAINLY_EXPIRED:
+ case FREERDP_ERROR_CONNECT_PASSWORD_EXPIRED:
+ case FREERDP_ERROR_CONNECT_PASSWORD_MUST_CHANGE:
+ case FREERDP_ERROR_CONNECT_WRONG_PASSWORD:
+ case FREERDP_ERROR_INSUFFICIENT_PRIVILEGES:
+ case FREERDP_ERROR_SECURITY_NEGO_CONNECT_FAILED:
+ case FREERDP_ERROR_SERVER_DENIED_CONNECTION:
+ case FREERDP_ERROR_SERVER_INSUFFICIENT_PRIVILEGES:
+ case FREERDP_ERROR_SERVER_FRESH_CREDENTIALS_REQUIRED:
+ *status = GUAC_PROTOCOL_STATUS_CLIENT_UNAUTHORIZED;
+ *message = "Authentication failure.";
+ break;
Review comment:
There are currently only two authentication-related status codes:
* `GUAC_PROTOCOL_STATUS_CLIENT_UNAUTHORIZED` - The credentials are wrong or the user has not yet been authorized (has not logged in).
* `GUAC_PROTOCOL_STATUS_CLIENT_FORBIDDEN` - The operation is simply denied (regardless of whether the credentials are correct).
If we define additional status codes to cover things like expired credentials, perhaps something like `GUAC_PROTOCOL_STATUS_CLIENT_EXPIRED` and `GUAC_PROTOCOL_STATUS_CLIENT_LOCKED`, we can provide more user-visible feedback. Before we do that, it'd be good to understand why there are different codes within RDP / FreeRDP for "password expired", "password _certainly_ expired", "password must change", "account expired", etc. as those statuses seem awfully ... identical.
As for administrator-visible feedback, I definitely think it's reasonable to add more logging around this.
##########
File path: src/protocols/rdp/error.c
##########
@@ -18,14 +18,90 @@
*/
#include "error.h"
-#include "rdp.h"
#include <freerdp/freerdp.h>
#include <guacamole/client.h>
#include <guacamole/protocol.h>
#include <guacamole/socket.h>
+#include <winpr/wtypes.h>
-void guac_rdp_client_abort(guac_client* client) {
+/**
+ * Translates the error code returned by freerdp_get_last_error() for the given
+ * RDP instance into a Guacamole status code and human-readable message. If no
+ * error was reported, a successful error code and message will be assigned.
+ *
+ * @param rdp_inst
+ * The FreeRDP client instance handling the RDP connection that failed.
+ *
+ * @param status
+ * Pointer to the variable that should receive the guac_protocol_status
+ * value equivalent to the error returned by freerdp_get_last_error().
+ *
+ * @param message
+ * Pointer to the variable that should receive a static human-readable
+ * message generally describing the error returned by
+ * freerdp_get_last_error().
+ */
+static void guac_rdp_translate_last_error(freerdp* rdp_inst,
+ guac_protocol_status* status, const char** message) {
+
+ UINT32 last_error = freerdp_get_last_error(rdp_inst->context);
+ switch (last_error) {
+
+ /* Normal disconnect */
+ case FREERDP_ERROR_NONE:
+ case FREERDP_ERROR_SUCCESS:
+ *status = GUAC_PROTOCOL_STATUS_SUCCESS;
+ *message = "Disconnected.";
+ break;
+
+ /* Authentication failure */
+ case FREERDP_ERROR_AUTHENTICATION_FAILED:
+ case FREERDP_ERROR_CONNECT_ACCESS_DENIED:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_DISABLED:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_EXPIRED:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_LOCKED_OUT:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_RESTRICTION:
+ case FREERDP_ERROR_CONNECT_CLIENT_REVOKED:
+ case FREERDP_ERROR_CONNECT_LOGON_FAILURE:
+ case FREERDP_ERROR_CONNECT_LOGON_TYPE_NOT_GRANTED:
+ case FREERDP_ERROR_CONNECT_NO_OR_MISSING_CREDENTIALS:
+ case FREERDP_ERROR_CONNECT_PASSWORD_CERTAINLY_EXPIRED:
+ case FREERDP_ERROR_CONNECT_PASSWORD_EXPIRED:
+ case FREERDP_ERROR_CONNECT_PASSWORD_MUST_CHANGE:
+ case FREERDP_ERROR_CONNECT_WRONG_PASSWORD:
+ case FREERDP_ERROR_INSUFFICIENT_PRIVILEGES:
+ case FREERDP_ERROR_SECURITY_NEGO_CONNECT_FAILED:
+ case FREERDP_ERROR_SERVER_DENIED_CONNECTION:
+ case FREERDP_ERROR_SERVER_INSUFFICIENT_PRIVILEGES:
+ case FREERDP_ERROR_SERVER_FRESH_CREDENTIALS_REQUIRED:
+ *status = GUAC_PROTOCOL_STATUS_CLIENT_UNAUTHORIZED;
+ *message = "Authentication failure.";
+ break;
Review comment:
New status codes, yes, I think it'd be best to save that for a different change.
Logging, I agree we should do this. I'll add that real quick.
##########
File path: src/protocols/rdp/error.c
##########
@@ -18,14 +18,90 @@
*/
#include "error.h"
-#include "rdp.h"
#include <freerdp/freerdp.h>
#include <guacamole/client.h>
#include <guacamole/protocol.h>
#include <guacamole/socket.h>
+#include <winpr/wtypes.h>
-void guac_rdp_client_abort(guac_client* client) {
+/**
+ * Translates the error code returned by freerdp_get_last_error() for the given
+ * RDP instance into a Guacamole status code and human-readable message. If no
+ * error was reported, a successful error code and message will be assigned.
+ *
+ * @param rdp_inst
+ * The FreeRDP client instance handling the RDP connection that failed.
+ *
+ * @param status
+ * Pointer to the variable that should receive the guac_protocol_status
+ * value equivalent to the error returned by freerdp_get_last_error().
+ *
+ * @param message
+ * Pointer to the variable that should receive a static human-readable
+ * message generally describing the error returned by
+ * freerdp_get_last_error().
+ */
+static void guac_rdp_translate_last_error(freerdp* rdp_inst,
+ guac_protocol_status* status, const char** message) {
+
+ UINT32 last_error = freerdp_get_last_error(rdp_inst->context);
+ switch (last_error) {
+
+ /* Normal disconnect */
+ case FREERDP_ERROR_NONE:
+ case FREERDP_ERROR_SUCCESS:
+ *status = GUAC_PROTOCOL_STATUS_SUCCESS;
+ *message = "Disconnected.";
+ break;
+
+ /* Authentication failure */
+ case FREERDP_ERROR_AUTHENTICATION_FAILED:
+ case FREERDP_ERROR_CONNECT_ACCESS_DENIED:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_DISABLED:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_EXPIRED:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_LOCKED_OUT:
+ case FREERDP_ERROR_CONNECT_ACCOUNT_RESTRICTION:
+ case FREERDP_ERROR_CONNECT_CLIENT_REVOKED:
+ case FREERDP_ERROR_CONNECT_LOGON_FAILURE:
+ case FREERDP_ERROR_CONNECT_LOGON_TYPE_NOT_GRANTED:
+ case FREERDP_ERROR_CONNECT_NO_OR_MISSING_CREDENTIALS:
+ case FREERDP_ERROR_CONNECT_PASSWORD_CERTAINLY_EXPIRED:
+ case FREERDP_ERROR_CONNECT_PASSWORD_EXPIRED:
+ case FREERDP_ERROR_CONNECT_PASSWORD_MUST_CHANGE:
+ case FREERDP_ERROR_CONNECT_WRONG_PASSWORD:
+ case FREERDP_ERROR_INSUFFICIENT_PRIVILEGES:
+ case FREERDP_ERROR_SECURITY_NEGO_CONNECT_FAILED:
+ case FREERDP_ERROR_SERVER_DENIED_CONNECTION:
+ case FREERDP_ERROR_SERVER_INSUFFICIENT_PRIVILEGES:
+ case FREERDP_ERROR_SERVER_FRESH_CREDENTIALS_REQUIRED:
+ *status = GUAC_PROTOCOL_STATUS_CLIENT_UNAUTHORIZED;
+ *message = "Authentication failure.";
+ break;
Review comment:
OK - I've split things apart more and added more verbose messages.
----------------------------------------------------------------
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