cloud-py-api / nc_py_api

Nextcloud Python Framework
https://cloud-py-api.github.io/nc_py_api/
Other
93 stars 6 forks source link

Files FSNode user is not correctly scraped if Nextcloud is hosted in a sub-path #296

Closed vwbusguy closed 1 month ago

vwbusguy commented 1 month ago

Describe the bug

The user attribute of the FSNode objects returned from files.listdir is not correct if the Nextcloud instance is hosted in a sub-path. It currently evaluates this based on a assumption that the nextcloud instance is hosted on / and doesn't accommodate for an extra / if hosted in a subpath.

For example, for a Nextcloud instance hosted under /nextcloud, the user of the file will be "files".

FYI - I'm here at the Nextcloud Contributors conference this week and would be happy to connect and submit a PR to fix this while I'm here.

Steps/Code to Reproduce

Assuming nextcloud is installed at /nextcloud.

print("\n".join([f"User: {f.user} Path: {f.full_path} - Size: {f.info.size}" for f in sorted(nc.files.listdir(depth=-1), key = lambda x: x.info.size,reverse = True) if not f.is_dir]))

Expected Results

The .user is the actual user.

Actual Results

The .user attribute is "files".

image

Setup configuration

Nextcloud 30 nc-py-api==0.17.1

vwbusguy commented 1 month ago

user_path is similarly affected, being off by one in the case of Nextcloud being in a single subpath:

https://github.com/cloud-py-api/nc_py_api/blob/aef1ed29880a2eb8878feea9be1935a7f50542b5/nc_py_api/files/__init__.py#L226

bigcat88 commented 1 month ago

We would be happy to pull request a fix if you have time.

vwbusguy commented 1 month ago

So far the way I can think to do this most reliably is with a bit of regex:

path = 'nextcloud/files/admin/Photos/files/Nextcloud community.jpg'
user_path = re.sub(r'.*?files/([^/]+)/','',path,count=1)
print(user_path)
user = re.findall(r'files/([^/]+)/',path)[0]
print(user)

Returns:

Photos/files/Nextcloud community.jpg
admin

I tried to also accommodate for a user having a folder named "files" as well.

If this is acceptable, I'm happy to add this regex method in a PR. If these are called often, we'll probably want to use re.compile() as well so there's no performance degradation if these are called often.

bigcat88 commented 1 month ago

Aren't file paths in Nextcloud always relative to the site root?

It just seems to me at the moment that the bug is in filling the "full_path" variable, and not taking the value from there.

"full_path" should not contain "nextcloud" at the start, imho. I need a dev setup for that to take a look closer, maybe I am wrong.

vwbusguy commented 1 month ago

Technically, it does, in the sense that a /nextcloud serving of Nextcloud by default serves from a nextcloud/ folder in the Apache DocRoot (assuming LAMP and not a proxy_pass of some sort).

Yesterday was my first day using this API, so I'm probably not the best person to answer what full_path should be, but since it's also removing index.php in the URL from the parsing, your logic seems consistent to me.

It seems my PR fixed my particular problem but introduced a regression in the tests for the trashbin stuff for test paths that do not have /files/ in them, and I'm still not yet sure why.

vwbusguy commented 1 month ago

I talked with Sebastian and found that I needed to compensate for /trashbin and /versions in the pathing for that module as well. Submitting an update shortly.