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

add file upload support for webdav #1743

Merged
merged 6 commits into from
May 2, 2022
Merged

Conversation

manzari
Copy link
Contributor

@manzari manzari commented May 10, 2020

Hello,
this is an UploadService implementation for nextcloud.

  • It uses the same settings as the FTP UploadService
  • urllib2 for requests
  • creates folders recursively

The requests for uploading files as well as the ones used for creating and deleting directories are documented here.
Also I can provide nextcloud accounts for testing, just pm me.

I hope you like it :)

@MichaIng
Copy link
Member

Many thanks for your PR. It seems to double with #1404 which adds Nextcloud upload support as well, along with generic WebDAV upload. Could you review whether this fully includes your suggested changes? Since @DavHau gave permissions to continue with his/her work, are you willing to continue with it, e.g. recreating your PR with WebDAV support added as well and rebasing on current dev branch?

@MichaIng MichaIng mentioned this pull request Mar 12, 2022
3 tasks
motioneye/uploadservices.py Outdated Show resolved Hide resolved
motioneye/uploadservices.py Show resolved Hide resolved
motioneye/uploadservices.py Show resolved Hide resolved
@MichaIng MichaIng linked an issue Mar 13, 2022 that may be closed by this pull request
@manzari
Copy link
Contributor Author

manzari commented Mar 13, 2022

Thank you for considering. I'm on the phone now. I'll look into #1404 beginning of the week

@MichaIng
Copy link
Member

MichaIng commented Mar 21, 2022

What I like about this PR compared to #1404 is that it only touches relevant code parts and that it does not add another 3rd party dependency.

The benefit of #1404 is that it adds a generic WebDAV upload service and Nextcloud only as a small abstraction of it. I'm not 100% sure whether there is anything special about Nextcloud's WebDAV endpoint, but I'm not aware of any and hence it should be similarly possible here simply by allowing to change the endpoint URL. This would increase the value massively.

Btw, not all Nextcloud instances run on the webroot, mine runs on /nextcloud base path. So I do not see any different option than a URL input and setting anyway. The other PR uses the same input field and text for both, generic WebDAV and Nextcloud. Generally I'm not sure whether it's worth it at all to have a dedicated Nextcloud option, since in its web interface, opening the Files app > Settings provides the exact WebDAV URL one needs as a copy field. A dedicated Nextcloud option may make it slightly easier by allowing to enter only the Nextcloud base URL, but on the other hand exactly this may cause confusion, when users copy the whole WebDAV URL: To make such info clear, two dedicated input texts would need to be shown for generic WebDAV (full endpoint URL) and Nextcloud (only base URL) would be required.

To put things together:

  • I suggest to add a generic WebDAV upload service only, which makes it much more valuable and not more difficult for Nextcloud users which have a trivial copy field in NC web interface for getting the full WebDAV endpoint URL.
  • This requires a setting for the full WebDAV endpoint URL. Since only one upload service can be added for each camera (is it?) theoretically the @upload_server setting could be reused, and then shown with a different text when "WebDAV" is selected, e.g. as a dedicated <tr> with different depends attribute but same input id.

@MichaIng MichaIng linked an issue Mar 21, 2022 that may be closed by this pull request
@manzari manzari changed the title add file upload support for nextcloud add file upload support for webdav Mar 24, 2022
@MichaIng
Copy link
Member

MichaIng commented Mar 24, 2022

Ignore the CodeFactor annotations, those are not related to this PR, not sure why it shows "failing" here which should only happen when new issues are added. I even disabled Bandit and tried to re-trigger the check, but nothing happens. It doubles with pre-commit anyway. (EDIT: Solved, probably a temporary issue with GitHub or CodeFactor) OOT: @cclauss I'm no big fans of ignoring Bandit rules code wide, shall we remove them and try to fix all annotations (from what I saw in CodeFactor not too difficult) respectively ignore the individual cases ("insecure use of /tmp", "0.0.0.0 binding") via inline comments? https://bandit.readthedocs.io/en/latest/config.html#exclusions

I'll test this PR now, @manzari many thanks for the rebase 👍.

@cclauss
Copy link
Member

cclauss commented Mar 25, 2022

I would encourage the community to get the code to pass all the bandit rules.

@MichaIng
Copy link
Member

The two mentioned ones cannot be solved, the /tmp one is a false positive, and the all interfaces binding is intended. But for everything else I agree. I'll open a PR the next days to remove all Bandit excludes and fix all annotations or add include excludes where its definitely a false positive or needs to be ignored to not break functionality/intended behaviour.

@MichaIng
Copy link
Member

MichaIng commented Apr 12, 2022

Apr 12 14:36:45 VM-Bullseye meyectl[5033]:    ERROR: webdav: a bytes-like object is required, not 'str'
Apr 12 14:36:45 VM-Bullseye meyectl[5033]: Traceback (most recent call last):
Apr 12 14:36:45 VM-Bullseye meyectl[5033]:   File "/usr/local/lib/python3.9/dist-packages/motioneye/uploadservices.py", line 924, in test_access
Apr 12 14:36:45 VM-Bullseye meyectl[5033]:     self._make_dirs(test_path)
Apr 12 14:36:45 VM-Bullseye meyectl[5033]:   File "/usr/local/lib/python3.9/dist-packages/motioneye/uploadservices.py", line 919, in _make_dirs
Apr 12 14:36:45 VM-Bullseye meyectl[5033]:     self._request(dir_url, 'MKCOL')
Apr 12 14:36:45 VM-Bullseye meyectl[5033]:   File "/usr/local/lib/python3.9/dist-packages/motioneye/uploadservices.py", line 898, in _request
Apr 12 14:36:45 VM-Bullseye meyectl[5033]:     base64string = b64encode(f'{self._username}:{self._password}')
Apr 12 14:36:45 VM-Bullseye meyectl[5033]:   File "/usr/lib/python3.9/base64.py", line 58, in b64encode
Apr 12 14:36:45 VM-Bullseye meyectl[5033]:     encoded = binascii.b2a_base64(s, newline=False)
Apr 12 14:36:45 VM-Bullseye meyectl[5033]: TypeError: a bytes-like object is required, not 'str'

Right, f'...' is a string, but b64encode doesn't allow strings: https://docs.python.org/3/library/base64.html#base64.b64encode
Since it also returns "bytes", the variable name base64string is misleading. However, since we want to add this as header, I guess we actually want a string, and best would be to use a method which takes and encodes a string in the first place.

EDIT: Solved!

@MichaIng
Copy link
Member

There is a general issue with the "Server Address" input here: The tooltip states to use the plain IP or hostname. But the code currently expects a protocol/scheme. Since WebDAV endpoints can be reachable via HTTP or HTTPS, we cannot auto-add the protocol/scheme like it is possible with other upload services.

I see two options:

  • Add a protocol/scheme input, so that the existing location input can be used. But to me so many inputs where a single endpoint URL is required sounds unreasonable, making things complicated.
  • Use a new input field which shall be the full URL of the DAV endpoint, including the root path. That makes most sense IMO, since on custom WebDAV server, like Nextcloud, can have a custom endpoint URL. E.g. with my Nextcloud instance it's https://my.domain.org/nextcloud/remote.php/dav/files/Micha/. This is shown in a copy box in Nextcloud UI, while with the current motionEye inputs, it is impossible to enter it, when respecting the input hints in the tooltips. I'm not sure how other common WebDAV implementations works, but a full endpoint URL should be shown or known with all of them.

@MichaIng
Copy link
Member

Upload works well when using the full endpoint URL with trailing slash as "Server Address" and full path with leading slash as "Location". The port is currently not used, but it should. However, to avoid all this confusion between URL, hostname, port, endpoint path, location etc etc, we really should use an "Endpoint URL" input field, with contains scheme/protocol, hostname, in case port, path to endpoint. The "Location" as dedicated input makes sense, if we want to be able to pre-create this root directory. But we could also have it added together with the endpoint URL, adding the info that this path needs to exist already.

Btw, since a simple PUT request is used here with basic authorization header, this actually works universally with webservers which allow PUT uploads, isn't it? Or are these MKCOL and DELETE commands only valid for WebDAV?

@cclauss cclauss requested review from cclauss and removed request for cclauss April 12, 2022 15:33
@zagrim
Copy link
Collaborator

zagrim commented Apr 13, 2022

Or are these MKCOL and DELETE commands only valid for WebDAV?

I'd suspect it to be somewhat restricted to WebDAV (which I'm not familiar with). At least MKCOL is not a valid HTTP request method (like PUT, DELETE etc)

@MichaIng MichaIng force-pushed the dev branch 2 times, most recently from 2269b85 to 7f9113f Compare April 20, 2022 18:05
@MichaIng
Copy link
Member

MichaIng commented Apr 20, 2022

Addressed reviews. Last thing is a formal step now, renaming the "server address" input to "WebDAV endpoint URL", which implies a new additional setting to be saved to the camera config. But the server address tooltip currently shows the wrong info that only the hostname or IP is required, while the full WebDAV endpoint URL is required.

manzari and others added 5 commits May 1, 2022 14:42
b64encode takes and returns only bytes, so the credentials need to be byte-encoded first and the returned bytes object decoded afterwards (ASCII works for base64 strings).

Additionally some %-formattings have been replaced with f-strings, fix some syntax errors and satisfy pre-commit black checks.
Remove unused port variable and GUI input.

Update urllib calls for new selective imports.

Always strip and re-add slashes from path elements so assure there is always exactly one slash between them.

Signed-off-by: MichaIng <[email protected]>
@MichaIng MichaIng force-pushed the dev branch 2 times, most recently from c98e218 to 66cd69e Compare May 1, 2022 13:14
@MichaIng
Copy link
Member

MichaIng commented May 1, 2022

I merged the endpoint URL setting with the one for S3 uploads, but wanted to show a different tooltip making use of the "depends" attribute. But sadly this doesn't work since in the JavaScript it is only interpreted for "tr" elements and I don't want to add the additional overhead for now to loop through all tooltips as well. To safe to the same setting, when using two dedicated input boxes, they need to have the same DOM ID. Not sure whether this can cause issues, as long as always one is hidden via display: none? I'll play around with it. Causes issues: Only the (empty) first hidden input field is read when applying, so the second input field has no effect. Also browsers show a warning in the console. So the only way to share the same setting currently is to use a more generic tooltip, respectively to extend it to contain common examples for S3 and WebDAV.

@MichaIng
Copy link
Member

MichaIng commented May 1, 2022

Okay, works well now 🙂. I'll merge in a few hours if no objections.

since the whole URL is required, not only the IP or hostname. Merge with S3 endpoint URL setting and extend the tooltip to show examples for both upload services.
@MichaIng MichaIng merged commit 8efa71f into motioneye-project:dev May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Feature request: Nextcloud as a storage target webdav nexcloud
4 participants