Public share middleware & controller #9518

rullzer · 2018-05-18T10:45:42Z

Fixes #5986

@juliushaertl as discussed.

Todo:

~~integrate Do not logout on auth on public share page #9756~~
add since tags etc
Maybe nog change functions for publci sharing?
Move PublicFileShareController just to files_sharing
Move basic auth templates to core (so they are default by default as well)
add tests

Once merged:

Docs: Document new publisharecontroller documentation#785

juliusknorr · 2018-05-18T13:55:24Z

lib/public/AppFramework/PublicShareController.php

+	 * Show the authentication page
+	 * The form has to submit to the authenticate method route
+	 */
+	abstract public function showAuthenticate(string $token): TemplateResponse;


Shouldn't we have a general showAuthenticate implementation here as well, since most of the time, we will just render a passwort request form.

We could do that sure.. just server it from the core app I guess then.

juliusknorr · 2018-05-18T13:59:29Z

@rullzer The IShareManager is just to be used by files, right? So calendar or other apps still need to implement their own sharing logic.

rullzer · 2018-05-18T17:46:28Z

@juliushaertl that is correct.

Ok let me think how to make it more generic then.
And then maybe have a FilesPublicShareController for all the apps that want to share files that implements some extra stuff.

rullzer · 2018-05-18T19:57:09Z

@juliushaertl ok abstracted away some more ;) This should work for all types of shares I guess

rullzer · 2018-05-18T20:16:08Z

Ok so it probably should be even further split up. Some controllers just need to know you are properly authenticated (like preview endpoints). If not they will just 404... I'll look into that more next week.

codecov · 2018-05-23T09:17:18Z

Codecov Report

Merging #9518 into master will increase coverage by 0.02%.
The diff coverage is 65.47%.

@@             Coverage Diff              @@
##             master    #9518      +/-   ##
============================================
+ Coverage      52.1%   52.12%   +0.02%     
- Complexity    25910    25946      +36     
============================================
  Files          1642     1645       +3     
  Lines         95721    96130     +409     
  Branches       1289     1373      +84     
============================================
+ Hits          49871    50108     +237     
- Misses        45850    46021     +171     
- Partials          0        1       +1

Impacted Files	Coverage Δ	Complexity Δ
..._sharing/lib/Middleware/SharingCheckMiddleware.php	`83.78% <ø> (-3.98%)`	`15 <0> (-6)`
apps/files_sharing/js/public.js	`47.42% <0%> (ø)`	`0 <0> (ø)`	⬇️
core/templates/publicshareauth.php	`0% <0%> (ø)`	`0 <0> (?)`
apps/files_sharing/appinfo/routes.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/legacy/template/functions.php	`9.3% <0%> (ø)`	`0 <0> (ø)`	⬇️
...e/AppFramework/DependencyInjection/DIContainer.php	`83.18% <100%> (+0.3%)`	`54 <2> (ø)`	⬇️
...sharing/lib/Controller/PublicPreviewController.php	`30.3% <21.05%> (-6.07%)`	`22 <5> (+4)`
...s/files_sharing/lib/Controller/ShareController.php	`37.85% <37.5%> (-9.7%)`	`55 <24> (-9)`
.../public/AppFramework/AuthPublicShareController.php	`76.08% <76.08%> (ø)`	`13 <13> (?)`
lib/public/AppFramework/PublicShareController.php	`82.35% <82.35%> (ø)`	`8 <8> (?)`
... and 12 more

rullzer · 2018-05-23T09:24:58Z

@juliushaertl even more splitup now. It is a lot of abstraction and not maybe 100% efficient. But better to have it secure I guess ;)

juliusknorr

@rullzer Looks good to me now in terms of separation 👍

- Move basic auth templates to core (so they are default by default as well)

👍

juliusknorr · 2018-06-10T21:47:50Z

apps/files_sharing/lib/Middleware/SharingCheckMiddleware.php

 			$token = $this->request->getParam('token');
 			$share = $this->shareManager->getShareByToken($token);
 			if ($share->getShareType() === \OCP\Share::SHARE_TYPE_LINK
 				&& !$this->isLinkSharingEnabled()) {


That check should probably also be in the PublicShareController, right?

mmm good point.

Yeah probably makes sense to not allow any public shares when it is disabled.

juliusknorr · 2018-06-10T21:59:39Z

lib/public/AppFramework/FilesPublicShareController.php

+use OCP\Share\IManager as ShareManager;
+use OCP\Share\IShare;
+
+abstract class FilesPublicShareController extends AuthPublicShareController {


I think this would probably make more sense to move this to the files_sharing app, since it is limited to be used with the ShareManager. At least I cannot think of how an app would require to use this.

In theory gallery could use it for example. BUt I agree. I'll move it. So the new code we add stays simple. If it turns out we duplicate it again in a lot (>4) places. Then we can think how to unify again.

rullzer · 2018-06-14T18:13:19Z

Failing test is unrelated

rullzer · 2018-06-19T19:40:59Z

@MorrisJobke yes that happens because you go directly to the auth page. So there is no state stored yet. Anyways let me fix that.

rullzer · 2018-06-19T19:42:07Z

@danxuliu ah you are right. Yes the tests need to be updated.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Now this is in core so the basics (that 99% of the app will want to use) looks always the same. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

* Removed tests that are now handled by the middleware * Updated tests Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

* They are handled now by the overal sharing public page middleware Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer · 2018-06-20T11:06:49Z

All happy!

MorrisJobke

Tested and works 👍

juliusknorr · 2018-06-20T15:34:24Z

apps/files_sharing/js/public.js

-				urlSpec.t = $('#dirToken').val();
-				return OC.generateUrl('/apps/files_sharing/ajax/publicpreview.php?') + $.param(urlSpec);
+				var token = $('#dirToken').val();
+				return OC.linkTo('files_sharing', '/publicpreview/'+token) + '?' + OC.buildQueryString(urlSpec);


Should be OC.generateUrl(OC.linkTo(...)) otherwise index.php prefix isn't added.

juliusknorr

Small issue with public previews on my instance, but besides that 👍

juliusknorr · 2018-06-20T15:35:09Z

apps/files_sharing/js/public.js

 		}
 		else if (previewSupported === 'true') {
-			$('#imgframe > video').attr('poster', OC.filePath('files_sharing', 'ajax', 'publicpreview.php') + '?' + OC.buildQueryString(params));
+			$('#imgframe > video').attr('poster', OC.linkTo('files_sharing', '/publicpreview/'+token) + '?' + OC.buildQueryString(params));


Should be OC.generateUrl(OC.linkTo(...)) otherwise index.php prefix isn't added.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

fixed

rullzer added enhancement 3. to review Waiting for reviews feature: sharing labels May 18, 2018

rullzer added this to the Nextcloud 14 milestone May 18, 2018

rullzer requested a review from juliusknorr May 18, 2018 10:45

rullzer added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 18, 2018

juliusknorr reviewed May 18, 2018

View reviewed changes

rullzer force-pushed the feature/5986/public_share_controller_middleware branch from cd160d7 to a30b48e Compare May 18, 2018 19:53

rullzer force-pushed the feature/5986/public_share_controller_middleware branch from a30b48e to f351b9d Compare May 23, 2018 09:17

rullzer force-pushed the feature/5986/public_share_controller_middleware branch from f351b9d to a68d26c Compare June 5, 2018 11:24

rullzer mentioned this pull request Jun 6, 2018

Do not logout on auth on public share page #9756

Closed

juliusknorr reviewed Jun 11, 2018

View reviewed changes

rullzer force-pushed the feature/5986/public_share_controller_middleware branch 5 times, most recently from da5ed84 to c855295 Compare June 14, 2018 12:16

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 14, 2018

rullzer requested review from MorrisJobke, icewind1991 and schiessle June 14, 2018 18:13

rullzer force-pushed the feature/5986/public_share_controller_middleware branch from ed23248 to ab4939a Compare June 19, 2018 19:46

rullzer added 14 commits June 20, 2018 08:53

Add the new PublicShareController and PublicShareMiddleware

f36ef8c

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Quick hack to make the files sharing app work with this properly

53a1b9a

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Move public preview endpoint over

366981f

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Move public auth page over

31392c2

Now this is in core so the basics (that 99% of the app will want to use) looks always the same. Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Don't allow public share pages if link sharing is disabled

20e5146

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Fix PublicPreviewControllerTests

ff3f63f

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Fix ShareControllerTest

9f658fe

* Removed tests that are now handled by the middleware * Updated tests Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Update SharingCheckMiddlewareTest

58376db

* They are handled now by the overal sharing public page middleware Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Add PublicShareMiddlewareTest

e733817

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Add PublicShareControllerTests

61e445d

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Add AuthPublicShareControllerTest

1bb8bc8

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Update token on regeneration for public login

6de656e

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Fix direct access to authen page

5455045

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Fix acceptance test

5805159

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer force-pushed the feature/5986/public_share_controller_middleware branch from ab4939a to 5805159 Compare June 20, 2018 06:57

MorrisJobke approved these changes Jun 20, 2018

View reviewed changes

juliusknorr reviewed Jun 20, 2018

View reviewed changes

juliusknorr previously requested changes Jun 20, 2018

View reviewed changes

Fix index.php links

a596b06

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

juliusknorr approved these changes Jun 20, 2018

View reviewed changes

MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 20, 2018

rullzer merged commit 8ebc3d9 into master Jun 21, 2018

rullzer deleted the feature/5986/public_share_controller_middleware branch June 21, 2018 08:09

juliusknorr mentioned this pull request Aug 23, 2018

Public board sharing nextcloud/deck#14

Open

Uh oh!

Public share middleware & controller #9518

Public share middleware & controller #9518

Uh oh!

Conversation

rullzer commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusknorr commented May 18, 2018

Uh oh!

rullzer commented May 18, 2018

Uh oh!

rullzer commented May 18, 2018

Uh oh!

rullzer commented May 18, 2018

Uh oh!

codecov bot commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rullzer commented May 23, 2018

Uh oh!

juliusknorr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rullzer commented Jun 14, 2018

Uh oh!

rullzer commented Jun 19, 2018

Uh oh!

rullzer commented Jun 19, 2018

Uh oh!

rullzer commented Jun 20, 2018

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliusknorr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rullzer commented May 18, 2018 •

edited

Loading

codecov bot commented May 23, 2018 •

edited

Loading