Rename existing files (#450)

* Fixed form mapper error where File objects were not being mapped correctly Will now rely on `FormData` methods for catching erroneous objects being passed in - it may be worth working towards a more robust mapper for future use, but for now, it is felt that this version is more than suitable for our needs. * Removed log statement Missed a log statement that is no longer required - this has been removed * Removed failing test * Added filedrag definitions This just makes my life easier - these are only useful for TS usage * Rename for upload of new files now possible - Added check for renaming in API endpoint as well as functionality to perform the rename - Cleaned up some code within `DocumentComponent`, namely: - made `el` property `readonly` - this was because I believed it was being overwritten somewhere (see next point) - the `updateProgress` function wasn't firing correctly and couldn't find the element it was supposed to be linked to - this has been removed (although it now means the code isn't as DRY) - Changed the `showerror` function in an attempt to make result consistent across calls - Added input and button for document renaming and implemented logic - as noted in the code - for some reason I wasn't able to modify the `original-name` data attribute within the component using `event.target` so I had to pass the button in directly - frankly, TypeScript appears to have dropped the ball somewhere on this - I would upgrade the version of TS, Babel, Webpack, etc. but I'm not insane enough to go down that rabbit-hole - this is deemed the lesser of two evils in my opinion. * Fixed typo in API endpoint * File renaming complete - Updated `datum::File` to check file name for marking datum as changed - Removed rename within file post request and placed within a put request - Updated `DocumentComponent` as required - Removed debug statements - Modified `edit.tt` as required * Typo fixed (thus fixing tests) * Changes as suggested Also applied perltidy using current config and modified output accordingly * File rename complete It appears that using `find_with_permission` on the `Fileval` gives a datum object. Due to the only reason you would be able to access the field in this manner for a rename would be if you already have permission, this is seen as an over-complication. * Updated templates * Updated logging to match and added check for window.test for use in test environments * Updated for IE11 removal and finished initial button and updates * Updated progress bar Also updated styling * Further UX changes Following feedback from UX testing, further changes complete and pushed * Created checks for renaming and valid names * Updated for correct renaming functionality * Updated file renaming * Fixes for bugs found in testing - Added old name to rename button event in order to revert the result on error - Exported `RenameEvent` to keep things DRY - Modified specifiers on multiple elements within `DocumentComponent` as events were being fired across the board rather than having required specificity - Removed error as this wasn't displaying correctly - Moved to async methods as IE11 is no longer in use to add clarity to code - Refactored out progress function and bound as required (DRY) - Discovered server is returning errors as strings, not as JSON objects (this will become a new ticket as a bug) so ensured that these are being parsed as required * Updated file name checks - Added check for invalid characters on upload - Allowed specific special characters on upload (underscore and dash) * Updated rename component * Fixed failing tests * Updated file upload to allow brackets in uploaded files Any invalid characters will be replaced when the file is saved to the record, but it is felt that the error still remain regardless. * Removed unnecessary changes * Updated comment on `file_check` * Revert on component * Further changes and bug fixes after testing on copy system * Removed additional CSS classes and used Bootstrap * Fix for spacing of rename button * Moved check out of other files and into endpoint with return value * Updated comments for file upload renaming Also removed extra check that's not required on `Datum::File` after `set_value` * Updates allowing for security considerations * Removed debug statements * Updated file checks * Re-added comments * Moved code for post to correct method * Fix for name checks on upload --------- Co-authored-by: root <[email protected]>
ctrlo · Oct 2, 2024 · 948394a · 948394a
1 parent 7c20ced
commit 948394a
Show file tree

Hide file tree

Showing 22 changed files with 588 additions and 123 deletions.
diff --git a/babel.config.js b/babel.config.js
@@ -1,19 +1,6 @@
 module.exports = {
   "presets": [
-    [
-      "@babel/preset-env",
-      {
-        "useBuiltIns": "usage",
-        "corejs": "3.8",
-        "targets": {
-          "edge": "17",
-          "firefox": "60",
-          "chrome": "67",
-          "safari": "11.1",
-          "ie": "11"
-        }
-      }
-    ],
+    "@babel/preset-env",
     "@babel/preset-typescript",
     "@babel/preset-react"
   ],

diff --git a/lib/GADS.pm b/lib/GADS.pm
@@ -1727,6 +1727,54 @@ post '/file/:id?' => require_login sub {
     }
 };
 
+put '/api/file/:id' => require_login sub {
+    my $id     = route_parameters->get('id');
+    my $is_new = param('is_new');
+
+    my $newname = param('filename')
+        or error __"Filename is required";
+
+    $filecheck->check_name($upload);
+
+    if ($is_new)
+    {
+        my $file = schema->resultset('Fileval')->find_with_permission($id, logged_in_user, new_file_only => 1)
+            or error __x"File ID {id} cannot be found", id => $id;
+
+        $file->single_rset->update({ name => $newname });
+
+        content_type 'application/json';
+
+        return encode_json({
+            id    => $file->single_id,
+            name  => $file->single_name,
+            is_ok => 1,
+        });
+    }
+    else
+    {
+        my $file = schema->resultset('Fileval')->find_with_permission($id, logged_in_user, 
+            rename_existing => 1)
+                or error __x"File ID {id} cannot be found", id => $id;
+
+        my $newFile = rset('Fileval')->create({
+            name           => $newname,
+            mimetype       => $file->single_mimetype,
+            content        => $file->single_content,
+            is_independent => 0,
+            edit_user_id   => logged_in_user->id,
+        });
+
+        content_type 'application/json';
+
+        return encode_json({
+            id    => $newFile->id,
+            name  => $newFile->name,
+            is_ok => 1,
+        });
+    }
+};
+
 # Use api route to ensure errors are returned as JSON
 post '/api/file/?' => require_login sub {
 
@@ -1745,10 +1793,17 @@ post '/api/file/?' => require_login sub {
 
     if (my $upload = upload('file'))
     {
-        my $mimetype = $filecheck->check_file($upload); # Borks on invalid file type
+        my $mimetype = $filecheck->check_file($upload, check_name => 0); # Borks on invalid file type
+        my $filename = $upload->filename;
+
+        # Remove any invalid characters from the new name - this will possibly be changed to an error going forward
+        # Due to dragging allowing (almost) any character it is decided that this would be best so users can input what
+        # they want, and the text be stripped on rename server-side
+        $filename =~ s/[^a-zA-Z0-9\._\-\(\)]//g;
+
         my $file;
         if (process( sub { $file = rset('Fileval')->create({
-            name           => $upload->filename,
+            name           => $filename,
             mimetype       => $mimetype,
             content        => $upload->content,
             is_independent => 0,
@@ -1757,7 +1812,7 @@ post '/api/file/?' => require_login sub {
         {
             return encode_json({
                 id       => $file->id,
-                filename => $upload->filename,
+                filename => $filename,
                 url      => "/file/".$file->id,
                 is_ok    => 1,
             });

diff --git a/lib/GADS/Datum/File.pm b/lib/GADS/Datum/File.pm
@@ -65,10 +65,13 @@ after set_value => sub {
         # single files.
         if (@values == 1 && @old == 1)
         {
-            my $old_content = $self->schema->resultset('Fileval')->find($old[0])->content;
+            my $old_value   = $self->schema->resultset('Fileval')->find($old[0]); # Only do one fetch here
+            my $old_content = $old_value->content;
+            my $old_name    = $old_value->name;
             $changed = 0 if $self->schema->resultset('Fileval')->search({
                 id      => $values[0],
                 content => $old_content,
+                name    => $old_name
             })->count;
         }
     }
@@ -247,6 +250,22 @@ has single_content => (
     },
 );
 
+has single_id => (
+    is      => 'rw',
+    lazy    => 1,
+    builder => sub {
+        $_[0]->_rset && $_[0]->_rset->id;
+    },
+);
+
+has single_rset => (
+    is      => 'rw',
+    lazy    => 1,
+    builder => sub {
+        $_[0]->_rset;
+    },
+);
+
 around 'clone' => sub {
     my $orig = shift;
     my $self = shift;

diff --git a/lib/GADS/Filecheck.pm b/lib/GADS/Filecheck.pm
@@ -26,7 +26,9 @@ sub is_image
 }
 
 sub check_file
-{   my ($self, $upload) = @_;
+{   my ($self, $upload, %options) = @_;
+
+    my $check_name = $options{check_name} // 1;
 
     my $info = $magic->info_from_filename($upload->tempname);
 
@@ -52,14 +54,21 @@ sub check_file
         unless $ext =~ /^(doc|docx|pdf|jpeg|jpg|png|wav|rtf|xls|xlsx|ods|ppt|pptx|odf|odg|odt|ott|sda|sdc|sdd|sdw|sxc|sxw|odp|sdp|csv|txt|msg|tif|svg)$/i;
 
     # As recommended at https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload
-    error __x"The filename {name} is not allowed. Filenames can only contain alphanumeric characters and a single dot",
-        name => $upload->filename
-            unless $upload->filename =~ /[-+_ a-zA-Z0-9]{1,200}\.[a-zA-Z0-9]{1,10}/;
+    # Brackets have been added to this - above recommendations do not explicitly state that brackets are not allowed - Ticket #1695
+    $self->check_name($upload->filename)
+        if($check_name);
 
     error __"Maximum file size is 5 MB"
         if $upload->size > 5 * 1024 * 1024;
 
     return $info->{mime_type};
 }
 
+sub check_name {
+    my ($self, $filename) = @_;
+    error __x"The filename {name} is not allowed. Filenames can only contain alphanumeric characters and a single dot",
+        name => $filename
+            unless $filename =~ /[-+_ a-zA-Z0-9\(\)]{1,200}\.[a-zA-Z0-9]{1,10}/;
+}
+
 1;
diff --git a/lib/GADS/Schema/ResultSet/Fileval.pm b/lib/GADS/Schema/ResultSet/Fileval.pm
@@ -19,7 +19,11 @@ sub independent
 }
 
 sub find_with_permission
-{   my ($self, $id, $user) = @_;
+{   my ($self, $id, $user, %options) = @_;
+
+    # Checks for whether the file is new or not, and therefore whether access is allowed
+    my $new_file_only = delete $options{new_file_only};
+    my $rename_existing = delete $options{rename_existing};
 
     my $fileval = $self->find($id) or return;
 
@@ -32,6 +36,9 @@ sub find_with_permission
     # This will control access to the file
     if ($file_rs && $file_rs->layout_id)
     {
+        error __"Access to this file is not allowed as it is not a new file"
+            if $new_file_only;
+
         my $layout = GADS::Layout->new(
             user        => $user,
             schema      => $self->result_source->schema,
@@ -63,6 +70,8 @@ sub find_with_permission
         $file->schema($self->result_source->schema);
     }
     else {
+        error __"Access to this file is not allowed"
+            if $new_file_only || $rename_existing;
         $file->schema($self->result_source->schema);
     }
 

diff --git a/package.json b/package.json
@@ -36,7 +36,6 @@
     "keycharm": "^0.3.0",
     "marked": "^9.1.1",
     "moment": "^2.24.0",
-    "npm-run-all": "^4.1.5",
     "popper.js": "^1.16.1",
     "propagating-hammerjs": "^1.4.0",
     "react": "^16.13.1",
@@ -60,6 +59,7 @@
     "@babel/preset-react": "^7.16.7",
     "@babel/preset-typescript": "^7.16.7",
     "@babel/runtime-corejs3": "^7.14.7",
+    "@jest/globals": "^29.7.0",
     "@types/jest": "^29.5.6",
     "@types/jquery": "^3.5.24",
     "@types/jstree": "^3.3.46",
@@ -84,7 +84,6 @@
     "jest": "^29.7.0",
     "jest-environment-jsdom": "^29.7.0",
     "mini-css-extract-plugin": "^2.7.2",
-    "npm-watch": "^0.6.0",
     "postcss-loader": "^3.0.0",
     "sass": "^1.23.7",
     "sass-loader": "^8.0.0",
@@ -99,7 +98,6 @@
   "browserslist": [
     "last 2 versions",
     "Firefox ESR",
-    "not dead",
-    "IE 11"
+    "not dead"
   ]
 }
diff --git a/src/frontend/components/button/_button.scss b/src/frontend/components/button/_button.scss
@@ -255,13 +255,15 @@ $btn-border-radius: 23px;
   border-bottom: 1px solid transparent;
   border-radius: 0;
   color: $brand-secundary;
+  background: $transparent;
 
   &::before {
     @extend %icon-font;
 
     content: "\E80a";
     margin-right: 0.75rem;
     color: $brand-secundary;
+    background: $transparent;
     transition: all 0.2s ease;
   }
 
@@ -272,6 +274,7 @@ $btn-border-radius: 23px;
     box-shadow: none;
     color: $brand-secundary;
     text-decoration: none;
+    background: $transparent;
   }
 }
 
@@ -623,3 +626,8 @@ $btn-border-radius: 23px;
   content: "\E807";
   color: $brand-danger;
 }
+
+.rename::before {
+  @extend %icon-font;
+  content: "\E80b";
+}