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

Integrate files_sharing_webapppassword #61

Merged
merged 9 commits into from
Apr 19, 2023
Merged

Conversation

aleixq
Copy link
Contributor

@aleixq aleixq commented Apr 18, 2023

As discussed in #1 , although this feature will be better to be in nextcloud core (proposed in nextcloud/server#37716 ) This MR integrates https://gitlab.com/communia/files_sharing_webapppassword to allow to access to Share API to a list of allowed origins. It has a separated setting both in system config and app config than webdav endpoint so origins could be different.

For app config it must be defined in settings/admin/webapppassword
For system config it must be defined in files_sharing_webapppassword.origins

No automatic tests are made.

README.md Outdated

The setting is both used for the origin of the CORS headers for the WebDAV/CalDAV requests and
for the referrer check whether we want to generate a temporary app password.

Alos you can configure in the same way the files sharing part.

`'files_sharing_webapppassword.origins' => ['https://example.com'],` - array of allowed files sharing api origins
Copy link
Member

Choose a reason for hiding this comment

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

This setting would belong to a different app...

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@pbek
Copy link
Member

pbek commented Apr 18, 2023

For system config it must be defined in files_sharing_webapppassword.origins

Shouldn't that be something like: webapppassword.files_sharing_origins?

'url' => '/api/v1/shares/{id}',
'verb' => 'DELETE',
]

Copy link
Member

Choose a reason for hiding this comment

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

Aren't those Nextcloud routes? Why do they need to be included in webapppassword?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact it is reimplementing the files_sharing api in webapppassword namespace to be able to apply the filtering of origins.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see. This really should belong into Nextcloud core. 😅
Can you please mention that there are new sharing endpoints on the settings, like you did in the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, and restructured the settings to sections. It may include the copy url button and the real base url but this may add noise to this MR. maybe in next merge requests...

README.md Outdated

The setting is both used for the origin of the CORS headers for the WebDAV/CalDAV requests and
for the referrer check whether we want to generate a temporary app password.

Alos you can configure in the same way the files sharing part.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "Alos" => "Also"

@pbek
Copy link
Member

pbek commented Apr 19, 2023

Looking good, thank you!

@pbek pbek merged commit 3d3dccd into digital-blueprint:main Apr 19, 2023
@SINAKZM
Copy link

SINAKZM commented Jul 16, 2023

@aleixq @pbek hi i added my domain to ui parts as setting for sharing api not worked i get cors error also i tried another way add like define 'files_sharing_webapppassword.origins' => ['http://localhost:8080'] but i still get cors error . its working for webdav apis . i read your discussions about your app instead webapppassword but it is not yet released as you know. is there any reason for not working with only share apis?
i created an issue too . could you please guide me about this?

@aleixq
Copy link
Contributor Author

aleixq commented Jul 18, 2023

@aleixq @pbek hi i added my domain to ui parts as setting for sharing api not worked i get cors error also i tried another way add like define 'files_sharing_webapppassword.origins' => ['http://localhost:8080'] but i still get cors error . its working for webdav apis . i read your discussions about your app instead webapppassword but it is not yet released as you know. is there any reason for not working with only share apis? i created an issue too . could you please guide me about this?

replied in issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants