Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[data_release/document_repository] Make upload directory configurable #5815

Merged
merged 11 commits into from
Dec 20, 2019
Merged

[data_release/document_repository] Make upload directory configurable #5815

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
af18ee5
SQL
ridz1208 Nov 29, 2019
cf9d628
code
ridz1208 Nov 29, 2019
5f19132
RB
ridz1208 Nov 29, 2019
7a71b44
typo
ridz1208 Nov 29, 2019
c894e11
install mod
ridz1208 Nov 29, 2019
09fbda7
adjustments
ridz1208 Dec 6, 2019
dae2448
phpcs
ridz1208 Dec 13, 2019
6c2e50c
Update modules/data_release/ajax/FileUpload.php
ridz1208 Dec 16, 2019
9d9c2f8
Update modules/data_release/ajax/FileUpload.php
ridz1208 Dec 16, 2019
97e08be
added check
ridz1208 Dec 17, 2019
0eec4b8
oops
ridz1208 Dec 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions SQL/0000-00-03-ConfigTables.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, 'web_path', ID, 'Document Repository Upload Path', 13 FROM ConfigSettings WHERE Name="paths";
INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'dataReleasePath', 'Path to uploaded data release files', 1, 0, 'web_path', ID, 'Data release Upload Path', 14 FROM ConfigSettings WHERE Name="paths";


INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, Label, OrderNumber) VALUES ('gui', 'Settings related to the overall display of LORIS', 1, 0, 'GUI', 3);
Expand Down
9 changes: 9 additions & 0 deletions SQL/New_patches/2019-11-29-Add_upload_directory_configuration.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'documentRepositoryPath', 'Path to uploaded document repository files', 1, 0, 'text', cs1.ID, 'Document Repository Upload Path', MAX(cs2.OrderNumber)+1 FROM ConfigSettings cs1 JOIN ConfigSettings cs2 WHERE cs1.Name="paths" AND cs2.parent=cs1.ID;
INSERT INTO ConfigSettings (Name, Description, Visible, AllowMultiple, DataType, Parent, Label, OrderNumber) SELECT 'dataReleasePath', 'Path to uploaded data release files', 1, 0, 'text', cs1.ID, 'Data Release Upload Path', MAX(cs2.OrderNumber)+1 FROM ConfigSettings cs1 JOIN ConfigSettings cs2 WHERE cs1.Name="paths" AND cs2.parent=cs1.ID;

-- For backwards compatibility, check the previous base and default to same folder as previous setting
SELECT Value INTO @base FROM Config c JOIN ConfigSettings cs ON cs.ID=c.ConfigID WHERE cs.Name="base";

INSERT INTO Config (ConfigID, Value) SELECT ID, CONCAT(@base,"modules/document_repository/user_uploads/") FROM ConfigSettings WHERE Name="documentRepositoryPath";
INSERT INTO Config (ConfigID, Value) SELECT ID, CONCAT(@base,"modules/data_release/user_uploads/") FROM ConfigSettings WHERE Name="dataReleasePath";

9 changes: 1 addition & 8 deletions modules/data_release/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,8 @@ Note: At the moment, the only way to remove a user's permission to a specific

## Configurations

- Data release uploads are stored under the
`modules/data_release/user_uploads directory`, which can easily be symlinked
to another location if necessary. Note that this directory needs to be
writable by your web server.
- `dataReleasePath` designates the target location for release file uploads.

## Other notes:

- Uploads are stored under the `modules/data_release/user_uploads` directory which
can easily be symlinked to another location if necessary, ensure that it can be written
to by your web server.
- Remove permissions by deleting rows in the data_release_permissions table.
- Upload date will automatically be added during file upload.
19 changes: 10 additions & 9 deletions modules/data_release/ajax/FileUpload.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,36 +12,37 @@
* @link https://github.com/aces/Loris
*/

$DB = \Database::singleton();
$user = \User::singleton();
$factory = \NDB_Factory::singleton();
$DB = $factory->database();
$user = $factory->user();
$config = $factory->config();

if ($_POST['action'] == 'upload'
&& $user->hasPermission("data_release_upload")
) {
$fileName = $_FILES["file"]["name"];
$version = $_POST['version'];
$upload_date = date('Y-m-d');
$base_path = __DIR__ . "/../user_uploads/";

$factory = NDB_Factory::singleton();
$settings = $factory->settings();

$baseURL = $settings->getBaseURL();
$path = $config->getSetting('dataReleasePath');

if (!file_exists(__DIR__ . "/../user_uploads/")) {
if (!file_exists($path)) {
error_log(
"ERROR: File upload failed. Default user_uploads"
"ERROR: File upload failed. Default upload"
ridz1208 marked this conversation as resolved.
Show resolved Hide resolved
. " directory not found."
);
header("HTTP/1.1 500 Internal Server Error");
} elseif (!is_writable(__DIR__ . "/../user_uploads/")) {
} elseif (!is_writable($path)) {
error_log(
"File upload failed. Default user_uploads directory"
"File upload failed. Default upload directory"
ridz1208 marked this conversation as resolved.
Show resolved Hide resolved
. " does not appear to be writeable."
);
header("HTTP/1.1 500 Internal Server Error");
} else {
$target_path = $base_path . $fileName;
$target_path = $path . $fileName;
if (move_uploaded_file($_FILES["file"]["tmp_name"], $target_path)) {
$DB->insert(
'data_release',
Expand Down
10 changes: 7 additions & 3 deletions modules/data_release/ajax/GetFile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@
* @link https://github.com/aces/Loris
*/

$user =& User::singleton();

$factory = \NDB_Factory::singleton();
$db = $factory->database();
$user = $factory->user();
$config = $factory->config();
$path = $config->getSetting('dataReleasePath');

$File = $_GET['File'];
// Make sure that the user isn't trying to break out of the $path by
Expand All @@ -23,13 +28,12 @@
header("HTTP/1.1 400 Bad Request");
exit(4);
}
$FullPath = __DIR__ . "/../user_uploads/$File";
$FullPath = $path . $File;
if (!file_exists($FullPath)) {
error_log("ERROR: File $FullPath does not exist");
header("HTTP/1.1 404 Not Found");
exit(5);
}
$db =& Database::singleton();
$fileID = $db->pselectOne(
"SELECT ID FROM data_release WHERE "
. "file_name=:fn",
Expand Down
17 changes: 11 additions & 6 deletions modules/document_repository/php/files.class.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ class Files extends \NDB_Page
*/
public function handle(ServerRequestInterface $request) : ResponseInterface
{
$config = \NDB_Factory::singleton()->config();
$path = $config->getSetting("documentRepositoryPath");
switch ($request->getMethod()) {
case "POST":
if ($this->uploadDocFile($request)) {
Expand Down Expand Up @@ -107,7 +109,7 @@ class Files extends \NDB_Page
$name = \User::singleton()->getUsername();
$record = urldecode(basename($request->getUri()->getPath()));
if (!is_numeric($record)) {
$file = __DIR__ . "/../user_uploads/$name/$record";
$file = $path. "$name/$record";
return (new \LORIS\Http\Response())
->withHeader('Content-Type', 'application/octet-stream')
->withHeader(
Expand Down Expand Up @@ -171,14 +173,16 @@ class Files extends \NDB_Page
*/
function deleteFile($rid): void
{
$factory = \NDB_Factory::singleton();
$DB = $factory->database();
$user = $factory->user();
$config = $factory->config();
$path = $config->getSetting("documentRepositoryPath");
// create Database object
$DB = \Database::singleton();
$user = \User::singleton();
$Notifier = new \NDB_Notifier(
"document_repository",
"delete"
);
$factory = \NDB_Factory::singleton();
$baseURL = $factory->settings()->getBaseURL();
$fileName = $DB->pselectOne(
"SELECT File_name FROM document_repository
Expand All @@ -205,7 +209,7 @@ class Files extends \NDB_Page
$Notifier->notify($msg_data);
}

$path = __DIR__ . "/../user_uploads/$dataDir";
$path = $path.$dataDir;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loris. Like media for example

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, just fix these ones.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets talk after i have questions

Copy link
Collaborator

@driusan driusan Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (file_exists($path)) {
unlink($path);
Expand Down Expand Up @@ -349,6 +353,7 @@ class Files extends \NDB_Page
$baseURL = $factory->settings()->getBaseURL();
$config = $factory->config();
$base = $config->getSetting('base');
$path = $config->getSetting("documentRepositoryPath");
$name = \User::singleton()->getUsername();
$DB = \Database::singleton();
$category = $req['category']; // required
Expand All @@ -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."/";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be using the new Utility function too

// $category is a string representation of an ID, and so should be at
// least equal to zero.
if (intval($category) < 0) {
Expand Down
2 changes: 2 additions & 0 deletions raisinbread/RB_files/RB_Config.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,'/data/document_repository_uploads/');
INSERT INTO `Config` (`ID`, `ConfigID`, `Value`) VALUES (104,103,'/data/data_release_uploads/');
UNLOCK TABLES;
SET FOREIGN_KEY_CHECKS=1;
2 changes: 2 additions & 0 deletions raisinbread/RB_files/RB_ConfigSettings.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,5 +98,7 @@ INSERT INTO `ConfigSettings` (`ID`, `Name`, `Description`, `Visible`, `AllowMult
INSERT INTO `ConfigSettings` (`ID`, `Name`, `Description`, `Visible`, `AllowMultiple`, `DataType`, `Parent`, `Label`, `OrderNumber`) VALUES (99,'usePwnedPasswordsAPI','Whether to query the Have I Been Pwned password API on password changes to prevent the usage of common and breached passwords',1,0,'boolean',1,'Enable \"Pwned Password\" check',22);
INSERT INTO `ConfigSettings` (`ID`, `Name`, `Description`, `Visible`, `AllowMultiple`, `DataType`, `Parent`, `Label`, `OrderNumber`) VALUES (100,'EnvironmentFile','Name of the environment file that need to be sourced for the imaging pipeline',1,0,'text',69,'Name of the environment file',20);
INSERT INTO `ConfigSettings` (`ID`, `Name`, `Description`, `Visible`, `AllowMultiple`, `DataType`, `Parent`, `Label`, `OrderNumber`) VALUES (101,'MINCToolsPath','Path to the MINC tools',1,0,'web_path',26,'Path to the MINC tools',12);
INSERT INTO `ConfigSettings` (`ID`, `Name`, `Description`, `Visible`, `AllowMultiple`, `DataType`, `Parent`, `Label`, `OrderNumber`) VALUES (102,'documentRepositoryPath','Path to uploaded document repository files',1,0,'text',26,'Document Repository Upload Path',13);
INSERT INTO `ConfigSettings` (`ID`, `Name`, `Description`, `Visible`, `AllowMultiple`, `DataType`, `Parent`, `Label`, `OrderNumber`) VALUES (103,'dataReleasePath','Path to uploaded data release files',1,0,'text',26,'Data Release Upload Path',14);
UNLOCK TABLES;
SET FOREIGN_KEY_CHECKS=1;
2 changes: 2 additions & 0 deletions raisinbread/migration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,7 @@
2019-11-25-Default_value_for_session_submitted.sql

# NEW
2019-10-09_move_MINCToolsPath_configuration_to_Config_tables.sql
2019-11-29-Add_upload_directory_configuration.sql

# CLEAN-UP
11 changes: 0 additions & 11 deletions tools/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ mkdir -p ../smarty/templates_c
# Setting 770 permissions for templates_c
chmod 770 ../smarty/templates_c

# Changing group to 'www-data' or 'apache' to give permission to create directories in Document Repository module
# Detecting distribution
if ! os_distro=$(hostnamectl 2>/dev/null)
then
Expand All @@ -130,20 +129,12 @@ debian=("Debian" "Ubuntu")
redhat=("Red" "CentOS" "Fedora" "Oracle")

if [[ " ${debian[*]} " =~ " $os_distro " ]]; then
mkdir -p ../modules/document_repository/user_uploads
mkdir -p ../modules/data_release/user_uploads
sudo chown www-data.www-data ../modules/document_repository/user_uploads
sudo chown www-data.www-data ../modules/data_release/user_uploads
sudo chown www-data.www-data ../smarty/templates_c
# Make Apache the group for project directory, so that the web based install
# can write the config.xml file.
sudo chgrp www-data ../project
sudo chmod 770 ../project
elif [[ " ${redhat[*]} " =~ " $os_distro " ]]; then
mkdir -p ../modules/document_repository/user_uploads
mkdir -p ../modules/data_release/user_uploads
sudo chown apache.apache ../modules/document_repository/user_uploads
sudo chown apache.apache ../modules/data_release/user_uploads
sudo chown apache.apache ../smarty/templates_c
# Make Apache the group for project directory, so that the web based install
# can write the config.xml file.
Expand All @@ -152,8 +143,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 "
Copy link
Contributor

@johnsaigle johnsaigle Dec 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

echo "../smarty/templates_c "
echo ""
fi
Expand Down