Improve handling of prevent download permission

[juliusknorr]

With nextcloud/server#32482 using the viewer only works for apps like Collabora as for other filetypes like images the download to the users browser would be blocked if the download permission is removed from a share.

The error message for that is quite generic that it failed to load the file. In oder to improve the user experience, it would make sense to implement support for this in the viewer app.

Suggested implementation

In addition to the existing list of mime types for a handler, Collabora could pass over a list of mime types for "secure view". With this list, the viewer could check on the share attribute to see if download is disabled and then either show a proper fitting error message or still load the file if the handler supports the current mime type.

Any objections @skjnldsv @Raudius @CarlSchwan ?

[juliusknorr]

This would also resolve nextcloud/files_pdfviewer#649

[skjnldsv]

I am not sure this is the Viewer's job here. 😕
What happens if I do a GET on the dav ressource which have download disabled? Do I get a 403?

[juliusknorr]

Where would you see the responsibility then? In the end Collabora registers a file action through viewer, so from my perspective this would need to be a capability of the viewer API to provide a check for that permission as it is the standard approach to handle displaying a file, especially for the cases where apps do not support a certain set of permissions like PDF or the built-in image/video handlers.

[juliusknorr]

And yes, it would just be a 403.

[skjnldsv]

Who is fetching the file? Your component?
If so you have to emit an error event at the root of your component

viewer/src/mixins/Mime.js

Lines 132 to 143 in 2e1ccbf

	mounted() {
	// detect error and let the viewer know
	this.$el.addEventListener('error', e => {
	console.error('Error loading', this.filename, e)
	this.$emit('error', e)
	})
	// update image size on window resize
	window.addEventListener('resize', debounce(() => {
	this.updateHeightWidth()
	}, 100))
	},

viewer/src/views/Viewer.vue

Lines 134 to 136 in 4d7fd3e

	class="viewer__file viewer__file--active"
	@error="currentFailed" />
	<Error v-else

Either a js error event or a vue event

[skjnldsv]

Regarding errors handling, we can heavily improve in the viewer side.
Like passing a proper message :)

[skjnldsv]

Maybe it would actually be cleaner to NOT handle the errors and let the apps do that themselves like you seems to have been doing in nextcloud/text#2776 ?

[juliusknorr]

My main concern was that we implement the same handling for most of the handlers then, photos/videos even within viewer so why not moving that shared logic one level up into the viewer

[skjnldsv]

So:

1. let's keep it like it is
2. properly throw from your components
3. let's improve our error messages in Viewer (maybe check if error.message is defined and use it)
4. let's improve dav request errors messages too (in viewer)

Looks good to you?
Anything not covered or missing?

[juliusknorr]

Sorry, didn't think of throwing a proper error message through dav already 🙈 Sounds like the best approach.

[skjnldsv]

No worries! Do you also rely on Viewer internal file fetching?
Do you fetch it again afterwards?

[juliusknorr]

Not against WebDAV, but there are specific routes to initiate the rendering. For text we get the file content with some text session specific metadata through OCS, for Collabora the file is then requested by the Collabora server.

[skjnldsv]

Ok, so I guess since you still wait for Viewer to init and fetch the file with PROPFIND, you will benefir from this proper error handling :)

[allexzander]

@juliushaertl Is this planned for one of the nearest releases?

[juliusknorr]

The PROPFIND actually doesn't throw an error since the file is still listed. The dav plugin only checks on GET/COPY https://github.com/nextcloud/server/blob/ecf6b7667b8457592d6334e88d2bf54aa4834238/apps/dav/lib/DAV/ViewOnlyPlugin.php#L57-L58

Wondering if we could do another HEAD request to the file in the viewer then, however that would be one additional request per file viewing :/

[juliusknorr]

@skjnldsv I'd still say that it should be the responsibility of the viewer to pick the right handler depending on the PROPFIND result.

The most simple solution would be to have a webdav property that viewer can request during the propfind that is performed on the file already. The viewer handlers could then just pass a secure view option set to true where viewer could use those and skip the ones that don't support it (like files_pdfviewer).

[skjnldsv]

@skjnldsv I'd still say that it should be the responsibility of the viewer to pick the right handler depending on the PROPFIND result.

Sure, but Viewer should not be app-aware. There should not be any custom app code in it, (we already have specific checks for public view, or previews which I don't like.) If a solution is implemented for this in Viewer, this must be an universal one, and not only thought for richdocument 👍

[juliusknorr]

Sure, but Viewer should not be app-aware. There should not be any custom app code in it, (we already have specific checks for public view, or previews which I don't like. If a solution is implemented for this in Viewer, this must be an universal one, and not only thought for richdocument 👍

All right, then we are on the same page I think :)

[juliusknorr]

Would be solved by #2395 as each handler can check the file properties through the enabled callback