ecotaxa / ecotaxa_back

Backend of the EcoTaxa application
GNU General Public License v3.0
6 stars 5 forks source link

API: `/my_files/{sub_path}` returns relative path #56

Open moi90 opened 1 year ago

moi90 commented 1 year ago

I'm using /my_files/{sub_path} to see if a file is already uploaded (in order to not upload it again), in preparation for a subsequent import (file_import/{project_id}).

When I actually upload the file, the returned filename is absolute (e.g. /tmp/ecotaxa_user.XXX/XXX/LOKI_46-24hours_01.zip and file_import/{project_id} works.

However, /my_files/{sub_path} returns a path relative to the user directory:

{
  'path': 'XXX',
  'entries': [{'name': 'LOKI_46-24hours_01.zip', 'type': 'F', 'size': 0, 'mtime': '2023-02-16 19:06:50.901954'}]
}

Therefore, file_import/{project_id} does not work, because it assumes absolute paths or paths relative to the common import directory.

No such file or directory: '/ecotaxa_import_area/XXX/LOKI_46-24hours_01.zip'

I think for the /my_files/{sub_path} endpoint to be useful, it should return absolute paths.

grololo06 commented 1 year ago

Hello, I think that security "rules" forbid the disclosure of real paths when possible, so it should be fixed the other way, i.e. the upload should return a relative path. There is ongoing work around [up|down]load and this would allow more flexibility, e.g. having different storage places depending on users, etc.

grololo06 commented 1 year ago

The idea as of today is to offer a Google Drive-like storage per user.

moi90 commented 1 year ago

Yes, I agree that it is not very safe to disclose real server paths. Maybe a different kind of identifier would work? Like user:path/relative/to/user/dir and common:path/relative/to/common/dir? (Then one wouldn't lose the ability to select different storage locations.)

What do you mean by "google-drive-like"?

What is the time horizon for "doing it the proper way"? Could you maybe apply my pull request as a short-term fix until the issue is resolved properly? I would say that /my_files is pretty much broken at the moment.

grololo06 commented 1 year ago

Hello, In fact file_import works as well for my_files entries, but it's dirty. See here: https://github.com/ecotaxa/ecotaxa_back/blob/a0f96d91067571e486dfb8d8d3e68e26442a26c9/QA/py/tests/test_my_files.py#L72 for composing a 'working' path.

In the future implementation, there will just be no access to absolute directory of any kind, so it will be easier.

Your idea about prefixes looks good to me: https://github.com/ecotaxa/ecotaxa_back/issues/60 It could probably be implemented right now, even if only used in UT.

The release planning is not fixed, but I don't think you're blocked for any purpose, but feel free to tell me otherwise.

moi90 commented 1 year ago

Thanks for this code example! So basically, I have to construct source_path myself on the client side: source_path = "/tmp/ecotaxa_user.{CREATOR_USER_ID}/{TAG}/{DEST_FILE_NAME}".

https://github.com/ecotaxa/ecotaxa_back/issues/60 It could probably be implemented right now, even if only used in UT.

What does UT mean?

You're right, this issue does not block me, I will work around it.

grololo06 commented 1 year ago

Thanks for this code example! So basically, I have to construct source_path myself on the client side: source_path = "/tmp/ecotaxa_user.{CREATOR_USER_ID}/{TAG}/{DEST_FILE_NAME}".

As of today, yes.

60 It could probably be implemented right now, even if only used in UT.

What does UT mean?

Unit Test

You're right, this issue does not block me, I will work around it.

Oki great.