Proper message shown when private links accessed #28172

sharidas · 2017-06-20T14:28:40Z

When user tries to access private links which are
not accessible, then proper message is delivered
instead of Internal server error message. So is the
case when user is logged in and tries to access
private links not accessible.

Signed-off-by: Sujith H [email protected]

Description

This change addresses 2 causes:

When user logged in and tries to access private link which is not accessible. Instead of internal server error, now it shows up a message.
When user is not logged in and tries to access link which is not accessible

Related Issue

Motivation and Context

This change tries to polish up the message to be shown when non accessible private links are accessed by user under: logged in and non logged in state respectively.

How Has This Been Tested?

Created folder private which is accessed by another user under logged in and non logged in states. The messages are displayed on the browser.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

tomneedham · 2017-06-20T15:10:07Z

Logged out errors should use https://github.com/owncloud/core/blob/master/core/templates/error.php to be consistent and themeable

sharidas · 2017-06-20T15:59:36Z

@tomneedham Thanks for the review. I have changed logged out errors to error.php as suggested.

sharidas · 2017-06-21T05:25:12Z

Updating the screenshot :

PVince81 · 2017-06-21T07:40:29Z

@sharidas hmm weird, I seem to remember that this used to work. But now I tried with v9.1.0 when the feature was introduced and I'm also getting internal server error for non-existing files. Maybe back then I forgot to test this case and only tested inaccessible files.

But throwing NotFoundException should actually trigger the router to automatically display the generic "not found" error page. Maybe something has changed that.

tomneedham · 2017-06-21T08:34:00Z

Yes my opinion was that you should just be catching some ForbiddenException and NotFoundExceptions properly here, not adding these new if conditions that look quite fragile.

sharidas · 2017-06-21T10:44:23Z

Updated the PR again.

PVince81 · 2017-06-21T15:05:25Z

core/Controller/LoginController.php

+		 */
+
+		if (( $redirect_url !== null) and ($remember_login === null)) {
+			$param["error"] = "Sorry, your request could not be handled properly, please check your actions and try again.";


I don't understand why this is needed.

If a user tries to open a link to a file and requires being logged in, the they simply see the login page and the link itself is in the redirect URL. Once they are logged in, the login controller must redirect to that URL and then the ViewController takes over.

True, If the user is not logged in then automatically the redirect goes to login page.

can you remove this then ? why do we need it then ?

DeepDiver1975 · 2017-06-21T18:46:01Z

apps/files/lib/Controller/ViewController.php

@@ -299,6 +302,12 @@ public function showFile($fileId) {
 			}
 			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', $params));
 		}
+
+		if ( $this->userSession->isLoggedIn() and empty($files)) {
+			$param["error"] = "You don't have permissions to access this file/folder - Please contact the owner to share it with you.";


translation missing

Added translation to both ViewController and LoginController.

DeepDiver1975 · 2017-06-22T06:40:07Z

@sharidas which state is this pr now? 2 or 3 - cannot be both ;-)

sharidas · 2017-06-22T06:47:22Z

@DeepDiver1975 I have removed 2. Once I get any feedback I will add 2 ( if required ) and remove 3. Now made as 3.

pmaier1 · 2017-06-22T15:16:42Z

@sharidas yes, if not logged in a redirect to login page makes sense, of course. Maybe you could add a hint as in e.g. owncloud/oauth2#43 (comment) saying "You are trying to access a private link. Please log in first."

sharidas · 2017-06-23T10:05:49Z

Attaching screenshot of latest login screen message :

PVince81 · 2017-06-23T18:42:40Z

core/Controller/LoginController.php

+		 * user is trying to access files for which he needs to login.
+		 */
+		if (( $redirect_url !== null) and ($remember_login === null)) {
+			$parameters['accessLink'] = true;


call it "reason" or something ? that would make that parameter reusable to be able to display message in other cases. I'm pretty sure there are a few more we could cover.

sharidas · 2017-06-29T14:37:32Z

core/Controller/LoginController.php

+		if (( $redirect_url !== null) and ($remember_login === null) and
+			($this->userSession->isLoggedIn() === false) and
+			($this->userSession->getLoginName() === null) and
+			strpos(urldecode($redirect_url), $fpath) !== false) {


I tried with private link ( in my machine ), lets say with http://localhost/testing/index.php/f/17 and the result was appropriate. Screenshot as below:

For testing if I changed the URL to lets say http://localhost/testing/index.php/foobar or http://localhost/testing/index.php/f/1/c it directly takes me to the login page.
Lot of if's doesn't look nice, though. Any suggestions regarding this?

No idea, sorry. Might need digging into the auth / routing code.

PVince81 · 2017-07-24T06:25:18Z

core/Controller/LoginController.php

+		if (!empty($redirect_url)) {
+			$urlArray = explode('%2F', $redirect_url);
+			$fileId = array_pop($urlArray);
+			$link = $this->urlGenerator->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileId' => $fileId]);


hmm, why not just reuse the redirect url directly instead of regenerating the link this way ?

For example in my machine while testing, the redirect url has value: %2Ftesting%2Findex.php%2Ff%2F17. I was trying to access : http://localhost/testing/index.php/f/17. So if I pass the fileid 17 to linkToRouteAbsolute, the url matches with the http://localhost/testing/index.php/f/17. Inorder to extract the fileid I did the steps in the PR.

So my intention was to check if redirect url a substring of newly created link.

@sharidas I still don't understand why you don't directly use the value of "redirect_url". The purpose of this variable is to contain the actual target page to open, else this value is completely useless.

sharidas · 2017-07-24T11:52:02Z

core/Controller/LoginController.php

+
+		if ((!empty($redirect_url)) and ($remember_login === null) and
+			($this->userSession->isLoggedIn() === false) and
+			strpos(urldecode($this->urlGenerator->getAbsoluteURL(urldecode($redirect_url))),


Modified the code . A check is done if getAbsoluteURL($redirect_url) contains string getAbsoluteURL('index/f/').

sharidas · 2017-08-02T14:26:58Z

core/Controller/LoginController.php

+
+		if ((!empty($redirect_url)) and ($remember_login === null) and
+			($this->userSession->isLoggedIn() === false) and
+			(strpos(urldecode($this->urlGenerator->getAbsoluteURL(urldecode($redirect_url))),


This is one way to reuse redirect_url and check if url urldecode($this->urlGenerator->getAbsoluteURL('/index.php/f/') in it.

that looks funny ....

PVince81

Fine by me 👍

PVince81 · 2017-08-03T14:55:33Z

@DeepDiver1975 another case of "ran for 7 hours and still running"

DeepDiver1975 · 2017-08-03T16:22:43Z

apps/files/lib/Controller/ViewController.php

@@ -299,6 +302,13 @@ public function showFile($fileId) {
 			}
 			return new RedirectResponse($this->urlGenerator->linkToRoute('files.view.index', $params));
 		}
+
+		if ( $this->userSession->isLoggedIn() and empty($files)) {
+			$l = \OC::$server->getL10N("core");


inject l10n?

DeepDiver1975 · 2017-08-03T16:23:52Z

core/Controller/LoginController.php

+
+		if ((!empty($redirect_url)) and ($remember_login === null) and
+			($this->userSession->isLoggedIn() === false) and
+			(strpos(urldecode($this->urlGenerator->getAbsoluteURL(urldecode($redirect_url))),


that looks funny ....

DeepDiver1975 · 2017-08-03T16:24:45Z

core/Controller/LoginController.php

+		if ((!empty($redirect_url)) and ($remember_login === null) and
+			($this->userSession->isLoggedIn() === false) and
+			(strpos(urldecode($this->urlGenerator->getAbsoluteURL(urldecode($redirect_url))),
+				urldecode($this->urlGenerator->getAbsoluteURL('/index.php/f/'))) !== false)) {


why decode here? the result of getAbsoluteURL is already decoded

When user tries to access private links which are not accessible, then proper message is delivered instead of Internal server error message. So is the case when user is logged in and tries to access private links not accessible. Signed-off-by: Sujith H <[email protected]>

PVince81 · 2017-08-07T10:02:38Z

@sharidas please backport to stable10

PVince81 · 2017-08-15T16:56:05Z

@sharidas missing backport

sharidas · 2017-08-15T17:42:51Z

#28600 is the PR backported. Sorry for missing this link here.

lock · 2019-08-02T19:02:38Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

sharidas changed the title ~~Proper message shown when private links accessed~~ [WIP] Proper message shown when private links accessed Jun 20, 2017

sharidas force-pushed the private-link-access-task branch from 02e7286 to 3410403 Compare June 20, 2017 15:57

sharidas force-pushed the private-link-access-task branch from 3410403 to 993f454 Compare June 21, 2017 10:43

sharidas changed the title ~~[WIP] Proper message shown when private links accessed~~ Proper message shown when private links accessed Jun 21, 2017

sharidas added 2 - Developing 3 - To Review labels Jun 21, 2017

PVince81 reviewed Jun 21, 2017

View reviewed changes

DeepDiver1975 requested changes Jun 21, 2017

View reviewed changes

sharidas force-pushed the private-link-access-task branch from 993f454 to e95a2e6 Compare June 22, 2017 05:30

sharidas removed the 2 - Developing label Jun 22, 2017

sharidas force-pushed the private-link-access-task branch from e95a2e6 to 688645f Compare June 22, 2017 17:13

sharidas added 2 - Developing and removed 3 - To Review labels Jun 22, 2017

sharidas force-pushed the private-link-access-task branch from 688645f to 38b0c95 Compare June 23, 2017 07:13

sharidas added 3 - To Review and removed 2 - Developing labels Jun 23, 2017

PVince81 reviewed Jun 23, 2017

View reviewed changes

sharidas force-pushed the private-link-access-task branch from 38b0c95 to d77aee2 Compare June 29, 2017 14:29

sharidas commented Jun 29, 2017

View reviewed changes

sharidas force-pushed the private-link-access-task branch from d77aee2 to 0a199f4 Compare July 5, 2017 09:23

sharidas force-pushed the private-link-access-task branch 4 times, most recently from f668d38 to 47ad419 Compare July 18, 2017 12:31

felixboehm assigned sharidas Jul 18, 2017

felixboehm added this to the development milestone Jul 18, 2017

felixboehm added the p1-urgent Critical issue, need to consider hotfix with just that issue label Jul 18, 2017

sharidas force-pushed the private-link-access-task branch from 47ad419 to ad4231f Compare July 21, 2017 05:27

PVince81 reviewed Jul 24, 2017

View reviewed changes

sharidas force-pushed the private-link-access-task branch from ad4231f to a6ff748 Compare July 24, 2017 11:49

sharidas commented Jul 24, 2017

View reviewed changes

sharidas force-pushed the private-link-access-task branch from a6ff748 to 3b7aef8 Compare July 31, 2017 06:32

sharidas commented Aug 2, 2017

View reviewed changes

PVince81 approved these changes Aug 2, 2017

View reviewed changes

sharidas force-pushed the private-link-access-task branch from 3b7aef8 to 5d1054a Compare August 2, 2017 15:12

DeepDiver1975 requested changes Aug 3, 2017

View reviewed changes

sharidas force-pushed the private-link-access-task branch 2 times, most recently from 52c0daa to 05bc43f Compare August 4, 2017 05:25

sharidas force-pushed the private-link-access-task branch from 05bc43f to 2709e27 Compare August 4, 2017 08:13

PVince81 merged commit 71a4d83 into master Aug 7, 2017

PVince81 deleted the private-link-access-task branch August 7, 2017 10:02

DeepDiver1975 added the backport-request label Aug 7, 2017

PVince81 removed the backport-request label Aug 15, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper message shown when private links accessed #28172

Proper message shown when private links accessed #28172

sharidas commented Jun 20, 2017

tomneedham commented Jun 20, 2017

sharidas commented Jun 20, 2017

sharidas commented Jun 21, 2017

PVince81 commented Jun 21, 2017

tomneedham commented Jun 21, 2017

sharidas commented Jun 21, 2017

PVince81 Jun 21, 2017

sharidas Jun 21, 2017

PVince81 Jun 22, 2017

DeepDiver1975 Jun 21, 2017

sharidas Jun 22, 2017

DeepDiver1975 commented Jun 22, 2017

sharidas commented Jun 22, 2017

pmaier1 commented Jun 22, 2017

sharidas commented Jun 23, 2017

PVince81 Jun 23, 2017

sharidas Jun 29, 2017

PVince81 Jul 3, 2017

PVince81 Jul 24, 2017

sharidas Jul 24, 2017

PVince81 Jul 24, 2017

sharidas Jul 24, 2017

sharidas Aug 2, 2017

DeepDiver1975 Aug 3, 2017

PVince81 left a comment

PVince81 commented Aug 3, 2017

DeepDiver1975 Aug 3, 2017

DeepDiver1975 Aug 3, 2017

DeepDiver1975 Aug 3, 2017

PVince81 commented Aug 7, 2017

PVince81 commented Aug 15, 2017

sharidas commented Aug 15, 2017

lock bot commented Aug 2, 2019

Proper message shown when private links accessed #28172

Proper message shown when private links accessed #28172

Conversation

sharidas commented Jun 20, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

tomneedham commented Jun 20, 2017

sharidas commented Jun 20, 2017

sharidas commented Jun 21, 2017

PVince81 commented Jun 21, 2017

tomneedham commented Jun 21, 2017

sharidas commented Jun 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 commented Jun 22, 2017

sharidas commented Jun 22, 2017

pmaier1 commented Jun 22, 2017

sharidas commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Aug 7, 2017

PVince81 commented Aug 15, 2017

sharidas commented Aug 15, 2017

lock bot commented Aug 2, 2019