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

Allow additional Sabre plugins in publicwebdav.php #35621

Merged
merged 2 commits into from
Dec 13, 2022
Merged

Allow additional Sabre plugins in publicwebdav.php #35621

merged 2 commits into from
Dec 13, 2022

Conversation

julien-nc
Copy link
Member

It could be useful to have additional Sabre plugins in publicwebdav.php. I just added something similar to https://github.com/nextcloud/server/blob/master/apps/dav/appinfo/v1/webdav.php#L80-L82

Concrete use case: in the context of https://github.com/nextcloud/integration_openproject , we want to allow upload to a public share from a browser (from an OpenProject page actually). For that, we need a Sabre plugin to allow the origin website via the CORS headers.

Any strong reason why the OCA\DAV\Connector\Sabre::addPlugin event is not dispatched in publicwebdav.php?

@julien-nc julien-nc added this to the Nextcloud 26 milestone Dec 6, 2022
@julien-nc julien-nc mentioned this pull request Dec 6, 2022
Closed
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 6, 2022
View reviewed changes
apps/dav/appinfo/v1/publicwebdav.php Fixed Show fixed Hide fixed
apps/dav/appinfo/v1/publicwebdav.php Fixed Show fixed Hide fixed
apps/dav/appinfo/v1/publicwebdav.php Fixed Show fixed Hide fixed
@PVince81
Copy link
Member

PVince81 commented Dec 6, 2022

@julien-nc indeed this is long overdue 😄

@julien-nc julien-nc force-pushed the enh/noid/sabre-plugin-publicwebdav branch from 2a982e3 to 22e02ef Compare December 6, 2022 13:33
@julien-nc julien-nc requested a review from PVince81 December 6, 2022 13:36
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 6, 2022
View reviewed changes
apps/dav/appinfo/v1/publicwebdav.php Fixed Show fixed Hide fixed
apps/dav/appinfo/v1/publicwebdav.php Fixed Show fixed Hide fixed
apps/dav/appinfo/v1/publicwebdav.php Fixed Show fixed Hide fixed
@julien-nc julien-nc force-pushed the enh/noid/sabre-plugin-publicwebdav branch from 22e02ef to c0703ed Compare December 6, 2022 14:00
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 6, 2022
View reviewed changes
apps/dav/appinfo/v1/publicwebdav.php Fixed Show fixed Hide fixed
apps/dav/appinfo/v1/publicwebdav.php Fixed Show fixed Hide fixed
apps/dav/appinfo/v1/publicwebdav.php Fixed Show fixed Hide fixed
PVince81
PVince81 approved these changes Dec 6, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

please check if it's the correct event dispatcher version / the latest non-deprecated one, if applicable

@julien-nc
Copy link
Member Author

@PVince81 Do you mean switching to more modern event management?
There's now a new SabrePublicPluginEvent and we dispatch it with IEventDispatcher::dispatchTyped.

@tcitworld

This comment was marked as resolved.

Sign in to view
@julien-nc

This comment was marked as resolved.

Sign in to view
@julien-nc
Copy link
Member Author

@PVince81 Are you fine with backporting this to 25, 24 and 23?

@tcitworld

This comment was marked as resolved.

Sign in to view
@PVince81
Copy link
Member

PVince81 commented Dec 7, 2022

@PVince81 Are you fine with backporting this to 25, 24 and 23?

technically this would count as API addition, so not sure
I'd be fine for 25 as it's still fresh and new, but other releases are a bit too old for that

@AndyScherzinger

@AndyScherzinger
Copy link
Member

@PVince81 Are you fine with backporting this to 25, 24 and 23?

I'd be fine with backporting since it is an addition and not breaking change, so I#d be okay with it

juliusknorr
juliusknorr approved these changes Dec 7, 2022
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@julien-nc
allow additional Sabre plugins in publicwebdav.php
da9ffe4 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
use modern event dispatch
2d860d2 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc force-pushed the enh/noid/sabre-plugin-publicwebdav branch from 1d9b4bc to 2d860d2 Compare December 12, 2022 13:04
@julien-nc
Copy link
Member Author

/backport to stable25

@julien-nc julien-nc merged commit 104a109 into master Dec 13, 2022
@julien-nc julien-nc deleted the enh/noid/sabre-plugin-publicwebdav branch December 13, 2022 10:20
@julien-nc
Copy link
Member Author

Drone failure is not related.

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Dec 13, 2022
Closed
@julien-nc
Copy link
Member Author

@PVince81 I agree with @AndyScherzinger, it does not break anything so it can be backported in 23 and 24.
But I'm not very objective since we need this for an app that supports 23 and 24...

technically this would count as API addition, so not sure

I see this as a "fix" for an inconsistency between public and private WebDav endpoints.

What do you think?

@PVince81
Copy link
Member

@julien-nc ok, fine

@julien-nc
Copy link
Member Author

/backport to stable24

@julien-nc
Copy link
Member Author

/backport to stable23

This was referenced Dec 13, 2022
Closed
Closed
@ChristophWurst
Copy link
Member

Drone failure is not related.

PHPDoc is needed for OCP\SabrePublicPluginEvent::SabrePublicPluginEvent

well well well

@ChristophWurst
Copy link
Member

Moreover autoloaders were not updated. Does this code even work?

ChristophWurst
ChristophWurst reviewed Dec 15, 2022
View reviewed changes
lib/public/SabrePublicPluginEvent.php
*/
namespace OCP;

class SabrePublicPluginEvent extends SabrePluginEvent {
Copy link
Member

Choose a reason for hiding this comment

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

The naming rule was not followed: https://docs.nextcloud.com/server/18/developer_manual/app/events.html#naming-scheme

@ChristophWurst ChristophWurst mentioned this pull request Dec 15, 2022
Merged
6 tasks
@julien-nc julien-nc mentioned this pull request Dec 15, 2022
Merged
@PVince81
Copy link
Member

need manual backport + include #35789

@PVince81
Copy link
Member

stable25: #35817
stable24: #35818
stable23: #35819

This was referenced Dec 20, 2022
Closed
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst left review comments

@PVince81 PVince81 PVince81 approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

Assignees
No one assigned
Labels
3. to review Waiting for reviews enhancement feature: dav
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.
7 participants
@julien-nc @PVince81 @tcitworld @AndyScherzinger @ChristophWurst @juliusknorr @skjnldsv