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

Jump to bottom

Public dav endpoint v2 #32400

Merged
merged 4 commits into from
Jan 9, 2024
Merged

Public dav endpoint v2 #32400

merged 4 commits into from
Jan 9, 2024

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented May 15, 2022 •
edited
Loading

Fix #19700

Implements a new v2 endpoint which also allows GET

$ curl -k https://dev.domain.com/public.php/dav/files/Y9Hc4SPqg564gid -X PROPFIND
$ curl -k https://dev.domain.com/public.php/dav/files/Y9Hc4SPqg564gid/video.mp4 -X PROPFIND

Access a file directly

$ curl -k https://dev.domain.com/public.php/dav/files/Y9Hc4SPqg564gid/video.mp4 -X GET

If password-protected

$ curl -k -u 'anonymous:password' https://dev.domain.com/public.php/dav/files/Y9Hc4SPqg564gid
  • GET access to direct files
  • Bruteforce protected
  • Once authenticated (with share password), no need to send basic auth again

@nextcloud/server-backend any thoughts?

@skjnldsv skjnldsv added this to the Nextcloud 25 milestone May 15, 2022
@skjnldsv skjnldsv self-assigned this May 15, 2022
@skjnldsv skjnldsv force-pushed the feat/public-dav-v2 branch from e162aaa to 6233f4e Compare May 15, 2022 09:20
@skjnldsv skjnldsv force-pushed the feat/public-dav-v2 branch from 6233f4e to a991665 Compare May 15, 2022 09:23
@skjnldsv skjnldsv requested review from a team, PVince81, ArtificialOwl, juliusknorr, icewind1991 and CarlSchwan and removed request for a team May 15, 2022 09:24
@skjnldsv skjnldsv force-pushed the feat/public-dav-v2 branch from a991665 to 3caa9cb Compare May 15, 2022 09:32
@PVince81
Copy link
Member

@skjnldsv what happens if you still send basic auth for "https://dev.domain.com/public.php/dav/files/Y9Hc4SPqg564gid" ? will it then try to look for a folder "Y9Hc4SPqg564gid" inside the share ? just to double check that there is no conflict/confusion happening between both approaches

@skjnldsv
Copy link
Member Author

@skjnldsv what happens if you still send basic auth for "dev.domain.com/public.php/dav/files/Y9Hc4SPqg564gid" ? will it then try to look for a folder "Y9Hc4SPqg564gid" inside the share ? just to double check that there is no conflict/confusion happening between both approaches

It will ignore the username.
Basic auth is only used to authenticate on protected shares now

This was referenced Aug 12, 2022
Merged
Merged
@skjnldsv skjnldsv modified the milestones: Nextcloud 25, Nextcloud 26 Aug 18, 2022
github-advanced-security[bot]

This comment was marked as resolved.

Sign in to view

@skjnldsv skjnldsv requested a review from juliusknorr October 28, 2022 11:54
@blizzz blizzz mentioned this pull request Feb 1, 2023
Merged
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 29, 2023
View reviewed changes
@skjnldsv skjnldsv requested a review from juliusknorr December 29, 2023 09:16
come-nc
come-nc approved these changes Jan 8, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

A bit too much legacy calls and new to my liking, but otherwise seems sane.

provokateurin
provokateurin approved these changes Jan 8, 2024
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

OpenAPI is good (no breaking changes because it only fixes things that were wrong before).

@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 8, 2024
@skjnldsv skjnldsv enabled auto-merge January 8, 2024 12:10
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 8, 2024
View reviewed changes
@skjnldsv skjnldsv force-pushed the feat/public-dav-v2 branch 2 times, most recently from 8773fdf to d44cc38 Compare January 9, 2024 09:55
@skjnldsv
feat: public dav endpoint v2
7b6a650 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv
fix: public dav and files_sharing testing fixes
82b5a19 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv
fix: improve typing and use \OCP\Server::get
18399fc 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the feat/public-dav-v2 branch from d44cc38 to 54df3e2 Compare January 9, 2024 09:56
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 9, 2024
View reviewed changes
@skjnldsv
fix(psalm): update baseline and suppress unnecessary issues
9e4c9b9 
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the feat/public-dav-v2 branch from 54df3e2 to 9e4c9b9 Compare January 9, 2024 10:09
@skjnldsv skjnldsv disabled auto-merge January 9, 2024 11:08
@skjnldsv skjnldsv merged commit dc2066b into master Jan 9, 2024
49 checks passed
@skjnldsv skjnldsv deleted the feat/public-dav-v2 branch January 9, 2024 11:13
nickvergessen
nickvergessen reviewed Jan 10, 2024
View reviewed changes
apps/dav/appinfo/info.xml
@@ -86,8 +86,4 @@
<provider>OCA\DAV\CardDAV\Activity\Provider\Card</provider>
</providers>
</activity>

<public>
<webdav>appinfo/v1/publicwebdav.php</webdav>
Copy link
Member

Choose a reason for hiding this comment

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

Just a question to help me understand, the file still exists and public.php/webdav also still prints:

This is the WebDAV interface. It can only be accessed by WebDAV clients such as the Nextcloud desktop sync client.

But where is it registered now?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, hardcoded in public.php now

Copy link
Member Author

Choose a reason for hiding this comment

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

Similarly as how remote.php is doing it :)

I tried to get them closer together now

@skjnldsv skjnldsv mentioned this pull request Jan 19, 2024
Closed
8 tasks
@susnux susnux mentioned this pull request Jan 31, 2024
Merged
@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
@nfebe nfebe mentioned this pull request Aug 1, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@come-nc come-nc come-nc approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

@provokateurin provokateurin provokateurin approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

@icewind1991 icewind1991 Awaiting requested review from icewind1991

@Altahrim Altahrim Awaiting requested review from Altahrim

Assignees

@skjnldsv skjnldsv

Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: dav feature: sharing technical debt
Projects
None yet
Milestone
Nextcloud 29
Development

Successfully merging this pull request may close these issues.

Public dav endpoint doesn't allow GET of files
6 participants
@skjnldsv @PVince81 @nickvergessen @juliusknorr @provokateurin @come-nc