Make notes path configurable #207

danielmorlock · 2018-06-15T22:18:58Z

I introduced a user setting where a user can set a custom path for its notes. See "Settings" -> "Additional Settings".

Feedback is welcome!

korelstar

Thanks for this nice PR! This will fix #57 😃

I had a deep look at your code, and it's very good! However, I found some problems -- I commented them at the respective code line. As a general hint, here are some notes on building secure apps, which should be considered: https://docs.nextcloud.com/server/13/developer_manual/general/security.html

In #57, @jancborchardt suggested to use "a bottom left settings area" like in the files app, contacts app or calendar app. I think this would be better than hiding this setting on the personal settings page. But it should be easy to move your template to the other position.

Looking forward for an update of your PR 👍

korelstar · 2018-06-21T19:29:20Z

js/package.json

-  "dependencies": {},
+  "dependencies": {
+    "bower": "^1.8.4",
+    "grunt": "^1.0.3"


I don't think that those dependencies are required. For me, it works without them.

korelstar · 2018-06-21T19:30:46Z

templates/settings/personal.php

@@ -0,0 +1,5 @@
+<form id="notesSettings" class="section">
+	<h2>Notes</h2>
+	<p class="settings-hint">Set the default path to store your notes.</p>


These strings must be internationalized. Please use:

<?php p($l->t('Set the default path to store your notes.')); ?>

korelstar · 2018-06-21T19:38:49Z

templates/settings/personal.php

+<form id="notesSettings" class="section">
+	<h2>Notes</h2>
+	<p class="settings-hint">Set the default path to store your notes.</p>
+	<input type="text" name="notesPath" value="<?=$_['notesPath']?>" placeholder="Notes" id="notesPath" style="width:100%"/>


The value must be sanitized. Currently, you could do a XSS attack! Please use p(...) for this.

korelstar · 2018-06-21T19:39:47Z

templates/settings/personal.php

+<form id="notesSettings" class="section">
+	<h2>Notes</h2>
+	<p class="settings-hint">Set the default path to store your notes.</p>
+	<input type="text" name="notesPath" value="<?=$_['notesPath']?>" placeholder="Notes" id="notesPath" style="width:100%"/>


It would be nice, if also pressing return would save the setting. Currently, only putting the focus to somewhere else does work.

korelstar · 2018-06-21T19:50:26Z

service/notesservice.php

@@ -295,7 +300,8 @@ private function getFileById ($folder, $id) {
     * @return Folder
     */
    private function getFolderForUser ($userId) {
-        $path = '/' . $userId . '/files/Notes';
+    	$notesPath = $this->config->getUserValue($userId, 'notes', 'notesPath');


Here, we need to have a fallback to the default value Notes if there is no such setting, yet.

korelstar · 2018-06-21T19:58:49Z

controller/settingscontroller.php

+    public function setNotesPath($notesPath) {
+	    $uid = $this->userSession->getUser()->getUID();
+
+	    $path = '/' . $uid . '/files' . $notesPath;


I think this should be '/files/'

korelstar · 2018-06-21T20:00:13Z

controller/settingscontroller.php

+    public function setNotesPath($notesPath) {
+	    $uid = $this->userSession->getUser()->getUID();
+
+	    $path = '/' . $uid . '/files' . $notesPath;


I think we should sanitize $notesPath before using it here. However, I wasn't able to attack this. Does the Files API checks for directory traversals?

I can't find anything about this in the dev docs. The closest info I've found is this: https://github.com/nextcloud/server/blob/stable13/lib/public/Files/Folder.php#L104
Since the path was not "manually" sanitized before my changes, I did not sanitize them, too and I assume that this is done internally. But you're right, this should be verified - but I think I'm not the right person to do so :)

I analyzed this and found out that the nextcloud-server sanitizes the path. Hence, it's not a security problem, but maybe a usability problem.

jancborchardt · 2018-06-21T21:27:00Z

Yup, like @korelstar said, this should be in a bottom left settings area. You can find the design documentation on that at https://docs.nextcloud.com/server/14/developer_manual/design/settings.html :)

danielmorlock · 2018-06-24T12:04:07Z

@jancborchardt I've introduced the app settings in the bottom left. Further I did some work on making the settings management more abstract in order to support possibly other app settings next to "notesPath".

korelstar · 2018-06-24T14:42:15Z

Thanks! That looks good to me:

The personal settings page is obsolete now and should be removed. I will do a code review again, then (if @jancborchardt has no change requests).

danielmorlock · 2018-06-24T15:05:59Z

@korelstar I've removed the obsolete settings page.

korelstar · 2018-07-01T19:28:45Z

Thanks, @danielmorlock !

One last thing:

After changing the setting, nothing (visible) happens. It would be helpful, if the notes app would reload in order to make the new path effective. Can you add this functionality, please? After that I will merge this PR (if @jancborchardt doesn't intervene until then).

korelstar

Sorry for bothering you, @danielmorlock . I made the changes by myself. I think it's fine, now.

…ings")

… and possibly more app settings.

jancborchardt · 2018-07-17T22:16:45Z

Is it also possible to make the setting apply without a reload? :)

korelstar · 2018-07-25T19:58:23Z

Indeed, that would be nicer. That would require some more work. I think you should open a new issue for that.

HarleyStone · 2019-09-26T13:10:59Z

In looking at the Android app, I'm unable to locate the Advanced options within settings. Currently running 0.27.1 Thanks.

stefan-niedermann · 2019-09-26T13:17:31Z

Hi @HarleyStone,

this option is only available via the web interface.

@korelstar is there a REST-API for this setting?

HarleyStone · 2019-09-26T13:38:54Z

Hi @HarleyStone,

this option is only available via the web interface.

@korelstar is there a REST-API for this setting?

Ahhh. I see now. Thanks.

korelstar · 2019-09-26T14:44:07Z

@korelstar is there a REST-API for this setting?

No. Do you think we need this? I would say, that a setting in the server app is sufficient, isn't it?

stefan-niedermann · 2019-09-26T15:13:31Z

Not sure. Noone asked until now and it is something you will usually do max. 1 time, so.... No.

If there was an API, i might have implemented a setting, but it's not worth the effort i think.

korelstar requested changes Jun 21, 2018

View reviewed changes

jancborchardt added enhancement New feature or request design Related to the design or user experience labels Jun 21, 2018

korelstar mentioned this pull request Jul 1, 2018

show error message if notes folder has errors #219

Merged

korelstar approved these changes Jul 3, 2018

View reviewed changes

Daniel Morlock and others added 10 commits July 3, 2018 21:43

Make notes folder configurable in user settings (see "Additional Sett…

cfd47e0

…ings")

Revert previous Makefile

4ef447b

Undo adding unrequired dependencies

f7fc6fe

Update settingscontroller.php

c01d7c2

Use "Notes" as default value.

11056ea

Use p() and $l->t() for escaping and intl.

d22aed1

Implement settings service and use app settings to manage "notesPath"…

56b07a9

… and possibly more app settings.

Restore origin makefile.

af58983

Remove obsolete settings page.

5b5ed68

reload after changing notes path

3f49200

korelstar force-pushed the master branch from aa9bd8e to 3f49200 Compare July 3, 2018 19:47

korelstar merged commit f254fd5 into nextcloud:master Jul 3, 2018

korelstar mentioned this pull request Jul 3, 2018

Setting to choose notes folder #57

Closed

korelstar added this to the 2.4.0 milestone Aug 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make notes path configurable #207

Make notes path configurable #207

danielmorlock commented Jun 15, 2018

korelstar left a comment

korelstar Jun 21, 2018 •

edited

Loading

korelstar Jun 21, 2018

korelstar Jun 21, 2018 •

edited

Loading

korelstar Jun 21, 2018 •

edited

Loading

korelstar Jun 21, 2018

korelstar Jun 21, 2018

korelstar Jun 21, 2018

danielmorlock Jun 22, 2018

korelstar Jul 1, 2018

jancborchardt commented Jun 21, 2018

danielmorlock commented Jun 24, 2018

korelstar commented Jun 24, 2018

danielmorlock commented Jun 24, 2018

korelstar commented Jul 1, 2018

korelstar left a comment

jancborchardt commented Jul 17, 2018

korelstar commented Jul 25, 2018

HarleyStone commented Sep 26, 2019

stefan-niedermann commented Sep 26, 2019

HarleyStone commented Sep 26, 2019

korelstar commented Sep 26, 2019

stefan-niedermann commented Sep 26, 2019

Make notes path configurable #207

Make notes path configurable #207

Conversation

danielmorlock commented Jun 15, 2018

korelstar left a comment

Choose a reason for hiding this comment

korelstar Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

korelstar Jun 21, 2018

Choose a reason for hiding this comment

korelstar Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

korelstar Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

korelstar Jun 21, 2018

Choose a reason for hiding this comment

korelstar Jun 21, 2018

Choose a reason for hiding this comment

korelstar Jun 21, 2018

Choose a reason for hiding this comment

danielmorlock Jun 22, 2018

Choose a reason for hiding this comment

korelstar Jul 1, 2018

Choose a reason for hiding this comment

jancborchardt commented Jun 21, 2018

danielmorlock commented Jun 24, 2018

korelstar commented Jun 24, 2018

danielmorlock commented Jun 24, 2018

korelstar commented Jul 1, 2018

korelstar left a comment

Choose a reason for hiding this comment

jancborchardt commented Jul 17, 2018

korelstar commented Jul 25, 2018

HarleyStone commented Sep 26, 2019

stefan-niedermann commented Sep 26, 2019

HarleyStone commented Sep 26, 2019

korelstar commented Sep 26, 2019

stefan-niedermann commented Sep 26, 2019

korelstar Jun 21, 2018 •

edited

Loading

korelstar Jun 21, 2018 •

edited

Loading

korelstar Jun 21, 2018 •

edited

Loading