[data_release/document_repository] Make upload directory configurable #5815

ridz1208 · 2019-11-29T23:53:35Z

Brief summary of changes

This gives the option to the users to configure where the default upload directory of data_release and document_repository modules will be. this also removes the burden on LORIS to ensure these locations are readable/writeable and follows the standard established by the other modules uploading data to loris.

testing

before checking out the branch, make sure you are able to upload a file to doc repo and data release. If it is not the case, you should resolve the issue as it is not due to this change.
check out this branch and ensure you are still able to upload and download files to the 2 modules (the sql patch for existing projects is designed to be backwards compatible)
change the upload paths from the configuration module and make sure the new paths exist on your machine and are apache writable
try uploading downloading other documents to both modules to ensure that the functionality is unaffected.

Important note for release

Projects should be encouraged to move their files out of the old directories and into new directories somewhere else on their data mount/drive.

@@ -94,5 +94,7 @@ INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (97,70,'/data-raisinbrea
 INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (98,93,'V1');
 INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (99,101,'');
 INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (102,19,'false');
+INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (103,102,'/var/www/loris/modules/document_repository/user_uploads/');


Maybe we can change the default to be outside of the module now that the install script doesn't create it.

PapillonMcGill · 2019-12-02T19:16:31Z

@ridz1208 Could we do the same thing with project/tables_sql/ and project/instruments/?

This way, in no situation, Apache would require writing inside the loris directory.

johnsaigle · 2019-12-02T20:05:46Z

modules/data_release/ajax/FileUpload.php

-$DB   = \Database::singleton();
-$user = \User::singleton();
+$DB     = \Database::singleton();
+$user   = \User::singleton();


Not a big deal but could you update these to use the Factory like the line below?

i could... and i wanted to... I just decided to avoid "unrelated changes". I'm gonna do it though

It's nice to leave code cleaner than you found it, especially when it's an open issue. I think you're safe here

sold man stop selling

ridz1208 · 2019-12-02T21:54:03Z

@PapillonMcGill The 2 directories you mentioned are much more complicated. Projects track and commit the data in these directories generally with git. it would be very tricky directory to decide which files get read from where if a project is using both LINST and tracked php instruments. same goes for tables_sql, some generated patches are commited to the repo

johnsaigle · 2019-12-03T18:46:46Z

tools/install.sh

@@ -152,8 +144,6 @@ elif [[ " ${redhat[*]} " =~ " $os_distro " ]]; then
 else
    echo "$os_distro Linux distribution detected. We currently do not support this. "
    echo "Please manually change subdirectory ownership and permissions to ensure the web server can read *and write* in the following: "
-    echo "../modules/data_release/user_uploads "
-    echo "../modules/document_repository/user_uploads "


Line 120 in this file should be removed

Changing group to 'www-data' or 'apache' to give permission to create directories in Document Repository module

driusan · 2019-12-04T16:08:28Z

SQL/0000-00-03-ConfigTables.sql

@@ -75,6 +75,8 @@ INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType,
 INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'publication_uploads', 'Path to uploaded publications', 1, 0, 'web_path', ID, 'Publications', 10 FROM ConfigSettings WHERE Name="paths";
 INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'publication_deletions', 'Path to deleted publications', 1, 0, 'web_path', ID, 'Deleted Publications', 11 FROM ConfigSettings WHERE Name="paths";
 INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'MINCToolsPath', 'Path to the MINC tools', 1, 0, 'web_path', ID, 'Path to the MINC tools', 12 FROM ConfigSettings WHERE Name="paths";
+INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'documentRepositoryPath', 'Path to uploaded document repository files', 1, 0, 'text', ID, 'Document Repository Upload Path', 13 FROM ConfigSettings WHERE Name="paths";


Shouldn't these be of type web_path, not text, so that the config module verifies they exist?

Yes, absolutely. That's a great catch.

ouuh i forgot about that

driusan · 2019-12-04T16:09:23Z

SQL/0000-00-03-ConfigTables.sql

@@ -190,6 +192,8 @@ INSERT INTO Config (ConfigID, Value) SELECT ID, "/data/uploads/" FROM ConfigSett
 INSERT INTO Config (ConfigID, Value) SELECT ID, "/data/publication_uploads/" FROM ConfigSettings WHERE Name="publication_uploads";
 INSERT INTO Config (ConfigID, Value) SELECT ID, "/data/publication_uploads/to_be_deleted/" FROM ConfigSettings WHERE Name="publication_deletions";
 INSERT INTO Config (ConfigID, Value) SELECT ID, "%MINCToolsPath%" FROM ConfigSettings WHERE Name="MINCToolsPath";
+INSERT INTO Config (ConfigID, Value) SELECT ID, "/data/document_repository_uploads/" FROM ConfigSettings WHERE Name="documentRepositoryPath";
+INSERT INTO Config (ConfigID, Value) SELECT ID, "/data/data_release_uploads/" FROM ConfigSettings WHERE Name="dataReleasePath";


Why don't we leave the default empty and update the code to have reasonable defaults if the value is null?

Do you have an idea about what those constants would be? Where would be a reasonable place on a linux server for these files to go? Maybe somewhere under /srv/?

I don't know what constants you're talking about.. I'm suggesting leaving them null.

@driusan but you suggested to update the code to have reasonable defaults. what do you mean by that ?

"defaults" was the wrong word.. I meant it should behave reasonably if the value is unset (ie. provide an error message pointing to the config setting if it's required or disable the feature if it's an optional thing similarly to how to the instrument_manager disables uploading if it doesn't have write access.)

okai so if i understand correctly;

in RB set the paths to "ideal" values /data/...

in 0000 conf files do not add an entry into the Config table and let projects set a validpath

in the patch for existing projects, maintain status quo and discuss in the release the recommendation of moving the directories.

agreed ?

Yes, though we'll need to provide some detail about also moving the existing files from one destination to another, not just changing the path.

I don't know what the status quo is so I can't comment on the last point, but that sounds right.

e.g. if there's modules/document_repository/user_uploads/admin/meme.jpg then the admin/ folder will need to be moved to the new location or else it will be inaccessible.

yes for sure but that would not be an issue in the current state because existing projects will have their path point to the old path.

it should only be a recommendation on our end

maltheism · 2019-12-05T17:55:54Z

@ridz1208 what about the genomic_browser module?

ridz1208 · 2019-12-05T19:27:54Z

@maltheism what about it ?

maltheism · 2019-12-05T19:37:10Z

@ridz1208 you can upload a file in that module and it complains about a directory if one doesn't exist. Seems similar to what this PR is trying to get rid of & solve.

ridz1208 · 2019-12-05T19:39:32Z

@maltheism no, genomics seems to have an already configurable path

ridz1208 · 2019-12-13T19:43:53Z

@johnsaigle @driusan please review

johnsaigle

Small wording changes for the errors. I'll approve after these changes are made.

modules/data_release/ajax/FileUpload.php

Co-Authored-By: John Saigle <4022790+johnsaigle@users.noreply.github.com>

ridz1208 · 2019-12-16T16:49:04Z

@driusan @johnsaigle back to you

stale

driusan · 2019-12-17T14:40:22Z

modules/document_repository/php/files.class.inc

@@ -205,7 +209,7 @@ class Files extends \NDB_Page
            $Notifier->notify($msg_data);
        }

-        $path = __DIR__ . "/../user_uploads/$dataDir";
+        $path = $path.$dataDir;


I don't think it's safe to assume that path is defined with a / at the end of it (same for the other places the filename is concatenated to it.) It should explicitly add it (and throw an exception or otherwise die beforehand it path is empty for security reasons.)

I just followed the assumption made in other uploading forms. If i make changes to this one do i make it to the others too ?

you mean other forms in this module or other forms in LORIS?

Loris. Like media for example

no, just fix these ones.

Hmm we could temporarily I suppose. A better solution would be to rework modules to use the new File Uploader class because that has all of that validation built in. But that's a more substantial change.

I think it would be a good idea to more consistently handle this with something like os.path.join (Python) or filepath.Join (Go), but that's beyond the scope of this PR.

Yeah I think I'm agreeing with you just doing the PHP equivalent. I think this works (adapted from some WordPress code):

return rtrim( $string, '/\\' ) . '/';

We could wrap it in a Utility function just so that we have a standard way of doing this. not necessary though.

Lets talk after i have questions

As far as I know PHP doesn't have a built in equivalent. I think we should write one with appropriate unit tests, but I don't think this PR is the right place to do it. That's why I think these ones should just be fixed locally and then another issue/PR can tackle doing it "right" throughout LORIS

ridz1208 · 2019-12-17T21:23:50Z

@driusan @johnsaigle is that sufficient ?

johnsaigle · 2019-12-17T21:41:30Z

modules/document_repository/php/files.class.inc

@@ -368,7 +373,7 @@ class Files extends \NDB_Page
        $fileSize     = $uploadedFile->getSize();
        $fileName     = $uploadedFile->getClientFileName();
        $fileType     = pathinfo($fileName, PATHINFO_EXTENSION);
-        $uploadPath   = "$base/modules/document_repository/user_uploads/$name/";
+        $uploadPath   = $path.$name."/";


I think this should be using the new Utility function too

…aces#5815) This gives the option to the users to configure where the default upload directory of data_release and document_repository modules will be. this also removes the burden on LORIS to ensure these locations are readable/writeable and follows the standard established by the other modules uploading data to loris.

ridz1208 added 3 commits November 29, 2019 18:42

SQL

af18ee5

code

cf9d628

RB

5f19132

ridz1208 added 2 commits November 29, 2019 18:55

typo

7a71b44

install mod

c894e11

ridz1208 mentioned this pull request Nov 29, 2019

[Core/Install] Improve UNIX permission usage #5323

Merged

PapillonMcGill approved these changes Dec 2, 2019

View reviewed changes

johnsaigle reviewed Dec 2, 2019

View reviewed changes

johnsaigle added the Blocking PR should be prioritized because it is blocking the progress of another task label Dec 2, 2019

johnsaigle mentioned this pull request Dec 2, 2019

[Doc Repo] Refactor to use new File Uploader class #5234

Merged

johnsaigle reviewed Dec 2, 2019

View reviewed changes

johnsaigle reviewed Dec 3, 2019

View reviewed changes

driusan previously requested changes Dec 4, 2019

View reviewed changes

adjustments

09fbda7

johnsaigle added the Needs formatting PR requires formatting fixes to meet our coding standards (PHPCS, phan, etc.) label Dec 9, 2019

phpcs

dae2448

ridz1208 removed the Needs formatting PR requires formatting fixes to meet our coding standards (PHPCS, phan, etc.) label Dec 13, 2019

johnsaigle suggested changes Dec 16, 2019

View reviewed changes

modules/data_release/ajax/FileUpload.php Outdated Show resolved Hide resolved

modules/data_release/ajax/FileUpload.php Outdated Show resolved Hide resolved

Update modules/data_release/ajax/FileUpload.php

6c2e50c

Co-Authored-By: John Saigle <4022790+johnsaigle@users.noreply.github.com>

Update modules/data_release/ajax/FileUpload.php

9d9c2f8

Co-Authored-By: John Saigle <4022790+johnsaigle@users.noreply.github.com>

johnsaigle approved these changes Dec 16, 2019

View reviewed changes

driusan reviewed Dec 17, 2019

View reviewed changes

ridz1208 added the State: Needs work PR awaiting additional work by the author to proceed label Dec 17, 2019

added check

97e08be

johnsaigle reviewed Dec 17, 2019

View reviewed changes

johnsaigle approved these changes Dec 17, 2019

View reviewed changes

oops

0eec4b8

ridz1208 force-pushed the 2019_11_29_config_for_uploads branch from 77f52eb to 0eec4b8 Compare December 17, 2019 21:57

johnsaigle removed the State: Needs work PR awaiting additional work by the author to proceed label Dec 18, 2019

driusan merged commit 6c290e4 into aces:master Dec 20, 2019

ridz1208 added this to the 23.0.0 milestone Jan 7, 2020

johnsaigle mentioned this pull request Jan 13, 2020

[Data release] Reactification of the module #5224

Merged

[data_release/document_repository] Make upload directory configurable #5815

[data_release/document_repository] Make upload directory configurable #5815

Conversation

ridz1208 commented Nov 29, 2019 • edited by johnsaigle Loading

Brief summary of changes

testing

Important note for release

Related

Choose a reason for hiding this comment

PapillonMcGill commented Dec 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ridz1208 commented Dec 2, 2019

johnsaigle Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsaigle Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maltheism commented Dec 5, 2019

ridz1208 commented Dec 5, 2019

maltheism commented Dec 5, 2019 • edited Loading

ridz1208 commented Dec 5, 2019

ridz1208 commented Dec 13, 2019

johnsaigle left a comment

Choose a reason for hiding this comment

ridz1208 commented Dec 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driusan Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

ridz1208 commented Dec 17, 2019

Choose a reason for hiding this comment

ridz1208 commented Nov 29, 2019 •

edited by johnsaigle

Loading

johnsaigle Dec 3, 2019 •

edited

Loading

johnsaigle Dec 6, 2019 •

edited

Loading

maltheism commented Dec 5, 2019 •

edited

Loading

driusan Dec 17, 2019 •

edited

Loading