[GitHub] [guacamole-server] mike-jumper commented on a change in pull request #242: GUACAMOLE-474: Allow file uploads and downloads to be disabled

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #242:
URL: https://github.com/apache/guacamole-server/pull/242#discussion_r432283863



##########
File path: src/protocols/rdp/channels/rdpdr/rdpdr-fs-messages.c
##########
@@ -98,9 +98,13 @@ void guac_rdpdr_fs_process_create(guac_rdp_common_svc* svc,
         /* Create \Download if it doesn't exist */
         file = guac_rdp_fs_get_file((guac_rdp_fs*) device->data, file_id);
         if (file != NULL && strcmp(file->absolute_path, "\\") == 0) {
-            int download_id =
-                guac_rdp_fs_open((guac_rdp_fs*) device->data, "\\Download",
-                    GENERIC_READ, 0, FILE_OPEN_IF, FILE_DIRECTORY_FILE);
+            
+            /* Only create Download folder if downloads are enabled. */
+            if(!((guac_rdp_fs*) devices->data)->disable_download) {

Review comment:
       Whoops - need that space after `if`.

##########
File path: src/common-ssh/sftp.c
##########
@@ -787,6 +787,13 @@ static int guac_common_ssh_sftp_get_handler(guac_user* user,
     /* Otherwise, send file contents */
     else {
 
+        /* If downloads are disabled, log and return. */
+        if (filesystem->disable_download) {
+            guac_user_log(user, GUAC_LOG_INFO,
+                    "File downloads have been disabled.");

Review comment:
       In line with the other download failure messages (`Unable to read file "..."`, `Unable to read directory "...": (reason)`), I suggest phrasing this message as a failed/refused operation and including the name of the file.
   
   The current message (`File downloads have been disabled.`) is not immediately clear that it indicates that a request isn't being serviced, and could be interpreted as a simple acknowledgement of the download setting.

##########
File path: src/protocols/rdp/fs.c
##########
@@ -77,11 +79,15 @@ void guac_rdp_fs_free(guac_rdp_fs* fs) {
 }
 
 guac_object* guac_rdp_fs_alloc_object(guac_rdp_fs* fs, guac_user* user) {
-
+    
     /* Init filesystem */
     guac_object* fs_object = guac_user_alloc_object(user);
     fs_object->get_handler = guac_rdp_download_get_handler;
-    fs_object->put_handler = guac_rdp_upload_put_handler;
+    
+    /* Assign handler only if uploads are not disabled. */
+    if(!fs->disable_upload)

Review comment:
       `if(` -> `if (`
   
   ;)




----------------------------------------------------------------
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