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.

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

Fixed issue with upload directories #5269

Merged
merged 3 commits into from
Jan 6, 2021

Conversation

jmcameron
Copy link
Collaborator

@jmcameron jmcameron commented Jan 5, 2021

Fix issue with upload directory always being under the bin directory. Now it obeys the .env setting for UPLOAD_DIR.

NOTES!

  • Only relative paths within the Bhima installation directory may be used
  • Currently it will reject any absolute paths or paths that start with '..' for UPLOAD_DIR.
  • Although it is probably possible to support absolute paths outside the Bhima installation, it will require updates to the app's 'app.use' function calls. Without those changes, paths outside of the Bhima directory cannot be accessed via HTTPS calls without ENOENT errors.
  • There are at least 3 ways to avoid conflicts with installation updates:
    • Create a new 'data' (or similar) directory inside the Bhima installation folder and add it to the .gitignore file.
    • Create a directory elsewhere and set up a symbolic/hard link to it within the Bhima directory
    • Revisit this issue and refactor to allow absolute paths.

TESTING:

  • In your .env file, try changing UPLOAD_DIR to an absolute path and then a path beginning with '..'. These should both fail.
    • NOTE that these failures are not handled very gracefully because this is an error that should be handled by the installer, not an end-user.
  • Test the current default for the upload directory (eg, 'client/uploads'
    • Try uploading a file (eg in the Patient Registry, chose a patient and upload a document)
    • Verify the upload worked and the file is downloadable. Do not delete it.
  • Test in a new directory:
    • Use 'bhima_test'
    • Create a folder within the Bhima installation directory (eg, 'data')
    • Change UPLOAD_DIR to the relative path for the new subdirectory the Bhima installation directory (eg, 'data').
    • Enable the debugging option in .env: DEBUG=app,errors,app:uploader
    • Monitor the client console for errors
    • Run 'yarn dev', notice in the server console the paths match what was given. Also notice that it is NOT in the bin directory!
    • Try uploading a file (eg in the Patient Registry, chose a patient and upload a document)
    • Verify the upload worked and the file is downloadable
    • Delete the new file
    • Exit 'yarn dev' and delete the temporary upload folder
  • Revert to the default UPLOAD directory (eg, 'client/uploads') and verify it still works and that the file uploaded earlier is still accessible. Delete the file, if desired.
  • Undo the debug settings in .env

@jmcameron jmcameron linked an issue Jan 5, 2021 that may be closed by this pull request
@jmcameron jmcameron requested a review from jniles January 5, 2021 20:03
if (path.isAbsolute(dir) || dir.startsWith('..')) {
throw new Error(`UPLOAD_DIR (${dir}) must be a relative path within the BHIMA software installation!`);
}
const rootDir = path.normalize(`${process.cwd()}/..`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't guaranteed to be the root directory in all circumstances. As explained in this SO answer, the process.cwd() is wherever the node call was made. So, it can changed, depending on where the executing terminal is in the file system.

Instead, you probably want __dirname. Here is a really good tutorial explaining how it works. This will never change, no matter who calls it or from where it is called.

@jniles
Copy link
Collaborator

jniles commented Jan 6, 2021

Sorry about the force push 😞

Ensures that `__dirname` is passed in as a variable.
@jniles
Copy link
Collaborator

jniles commented Jan 6, 2021

Got it working. Thanks so much for your help @jmcameron !

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 6, 2021

Build succeeded:

@bors bors bot merged commit f6f116c into Third-Culture-Software:master Jan 6, 2021
bors bot added a commit that referenced this pull request Jan 8, 2021
5271: Add better explanation of UPLOAD_DIR r=jniles a=jmcameron

Based on updates in PR #5269.


Co-authored-by: Jonathan Cameron <jmcameron@gmail.com>
@jmcameron jmcameron deleted the fix_upload_dir branch February 2, 2021 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UPLOAD_DIR doesn't seem to accept absolute paths
2 participants