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

Always try and show pre rendered preview #20451

Merged
merged 2 commits into from
Aug 13, 2020
Merged

Always try and show pre rendered preview #20451

merged 2 commits into from
Aug 13, 2020

Conversation

exussum12
Copy link
Contributor

Currently if the following situation happens

Server generates preview
Server has command removed which allows a preview to be shown
Client asks for preview, gets a 404 error when preview exists
(Mime checked before preview)

This happens more often with documents, or video as the commands are not
native PHP, they require a binary on the server.

After the fix the following would happen

Server generates preview
Server has command removed which allows a preview to be shown
Client asks for preview, gets preview which has been generated
(Mime checked after preview)

This would also allow offline generation (for example a docker image
containing the extra binaries), allowing a reduction in attack surface
of the instance serving the preview data.

@gary-kim gary-kim added 3. to review Waiting for reviews enhancement labels Apr 13, 2020
@gary-kim gary-kim added this to the Nextcloud 19 milestone Apr 13, 2020
@rullzer rullzer mentioned this pull request Apr 13, 2020
Merged
59 tasks
rullzer
rullzer approved these changes Apr 14, 2020
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Fine by me.

@rullzer
Copy link
Member

rullzer commented Apr 14, 2020

@exussum12 seems the test fail. Mind to have a look?

This was referenced Apr 15, 2020
Merged
Merged
@exussum12 exussum12 force-pushed the AllowPreviewWhenGeneratorHasBeenRemoved branch from 5c6ea6d to 4ab6721 Compare April 18, 2020 19:09
@rullzer rullzer mentioned this pull request Apr 23, 2020
Merged
11 tasks
@juliushaertl
Copy link
Member

There still seems to be an issue with one test:

There was 1 failure:

1) Test\Preview\GeneratorTest::testInvalidMimeType
Failed asserting that exception of type "TypeError" matches expected exception "OCP\Files\NotFoundException". Message was: "Argument 1 passed to Mock_IAppData_5c52be9a::getFolder() must be of the type string, null given, called in /drone/src/lib/private/Preview/Generator.php on line 460" at
/drone/src/lib/private/Preview/Generator.php:460
/drone/src/lib/private/Preview/Generator.php:129
/drone/src/lib/private/Preview/Generator.php:106
/drone/src/tests/lib/Preview/GeneratorTest.php:278
.

@juliushaertl juliushaertl added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 24, 2020
@rullzer rullzer modified the milestones: Nextcloud 19, Nextcloud 20 Apr 30, 2020
@MorrisJobke MorrisJobke mentioned this pull request Aug 11, 2020
Merged
57 tasks
@exussum12 @MorrisJobke
Always try and show pre rendered preview
b12a390 
Currently if the following situation happens

Server generates preview
Server has command removed which allows a preview to be shown
Client asks for preview, gets a 404 error when preview exists
(Mime checked before preview)

This happens more often with documents, or video as the commands are not
native PHP, they require a binary on the server.

After the fix the following would happen

Server generates preview
Server has command removed which allows a preview to be shown
Client asks for preview, gets preview which has been generated
(Mime checked after preview)

This would also allow offline generation (for example a docker image
containing the extra binaries), allowing a reduction in attack surface
of the instance serving the preview data.

Signed-off-by: Scott Dutton <scott@exussum.co.uk>
@MorrisJobke MorrisJobke force-pushed the AllowPreviewWhenGeneratorHasBeenRemoved branch from 4ab6721 to b12a390 Compare August 13, 2020 20:50
@MorrisJobke
Copy link
Member

Rebased to check the tests again.

@MorrisJobke
Fix existing test and add a specific one for the new case
8e24956 
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Copy link
Member

I fixed the test and added one specifically for this new use case.

MorrisJobke
MorrisJobke approved these changes Aug 13, 2020
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Fine with me 👍

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Aug 13, 2020
@MorrisJobke
Copy link
Member

CI also likes it -> merge \o/

@MorrisJobke MorrisJobke merged commit 75c659c into nextcloud:master Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rullzer rullzer rullzer approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@icewind1991 icewind1991 icewind1991 approved these changes

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Milestone
Nextcloud 20
Development

Successfully merging this pull request may close these issues.
6 participants
@exussum12 @rullzer @juliushaertl @MorrisJobke @icewind1991 @gary-kim