Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

Fix url links to lucene result page with OC 9.1 #124

Closed
wants to merge 1 commit into from
Closed

Fix url links to lucene result page with OC 9.1 #124

wants to merge 1 commit into from

Conversation

flaracca
Copy link

@mention-bot
Copy link

@flaracca, thanks for your PR! By analyzing the history of the files in this pull request, we identified @butonic, @VicDeo and @icewind1991 to be potential reviewers.

@flaracca flaracca changed the title Fix url links to lucene result page Fix url links to lucene result page with OC 9.1 Oct 14, 2016
@mrow4a
Copy link

mrow4a commented Oct 15, 2016

@PVince81
Copy link
Contributor

Well it depends what we want the behavior to be... The URL http://domain.com/files/index.php?dir=%2FDocuments&file=Example.odt should still be valid in 9.1 (although I thought "file" shuld be "scrollto"). This would display the file within the file list.

But changing it to the Webdav URL would trigger a download instead of displaying it in the list.
In some use cases, for example people who have the documents app, it makes more sense to display the result in the original file list to allow the user to then click on it to open in the documents app. (yes, ideal would be direct open, something for later)

@PVince81
Copy link
Contributor

Even better would be to change it to a permalink: http://localhost/owncloud/index.php/f/$fileid
That URL will automatically redirect to the file list with the correct file/directory name depending on the currently logged in user.

See owncloud/core#24434

@butonic
Copy link
Contributor

butonic commented Oct 17, 2016

Currently all search results are user specific and searching in shared files is not yet implemented in search_lucene. Resolving a user specific path is nod yet necessardy. Nevertheless the link generation suffered bitrot.

For files the link should point to the containing directory and scroll to the file. For directories, the link should just point to the directory. Or in php:

        if ($this->mime_type === 'httpd/unix-directory') {
            $this->link = \OC::$server->getURLGenerator()->linkToRoute(
                'files.view.index',
                [
                    'dir' => $this->path,
                ]
            );
        } else {
            $this->link = \OC::$server->getURLGenerator()->linkToRoute(
                'files.view.index',
                [
                    'dir' => dirname($this->path),
                    'scrollto' => $this->name,
                ]
            );
        }

@flaracca could you test with the above snippet?
@PVince81 I don't like using the fileid in the url, because it leaks our internal ids ... see http://stackoverflow.com/a/12384744 and other answers in that thread. I'd like an opinion of @Peter-Prochaska on that.

@peterprochaska
Copy link

@butonic What are your security concerns with a fileID in the url?
If we check the permissions correct...
Sure, if there is another opportunity, then we should do that.

@PVince81
Copy link
Contributor

@butonic then we'd need to get rid of the permalinks feature completely, because that's how it works, see owncloud/core#24434 which was approved before.

Also note that the internal fileid can be found by inspecting the "tr" rows in the file list.

The permissions are checked, yes. Only users who have access to that file will get a view. The check is done through a call that basically says "tell me which path this fileid has for the currently logged in user" (because the path can be different for share recipients). If the user has no access to the file, it gets a "Not found" error.

@PVince81
Copy link
Contributor

also: permalinks only work for logged in users

@butonic
Copy link
Contributor

butonic commented Oct 17, 2016

just call me paranoid. In the back of my mind a voice said 'don't expose internal ids or you will never get rid of them'. I tried to find a better explation, eg a blog article but could only come up with stackoverflow. ANd the answers are all about security concerns. AFAICT we will need to identify files by storage id and fileid ... but thats a different topic. Nevermind, lets focus on getting the right link here ...

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants