Consolidate private with preliminary public api #338

marcelklehr · 2017-02-10T17:33:49Z

This basically changes access to the private bookmarks and tags API to everyone without CSRF tokens and with CORS support, removes the public controller, and adjusts the public controller tests to work with the bookmark controller.

This is a work-in-progress and a follow-up on #322

Fixes #330, #328, #2

marcelklehr · 2017-02-10T22:13:57Z

I merged public_controller#return_as_json() into bookmarkcontroller#getBookmarks. As a result there's some duplicate functionality / redundant parameters now. This is to keep compatibility with the client-side code. If you want, I can adjust the client-side code to use the new parameters and remove the old ones.

Nonetheless, all tests are passing now and I also added docs for the API to the README :)

blizzz · 2017-02-18T22:39:59Z

appinfo/routes.php

-		array('name' => 'public#return_as_json', 'url' => '/public/rest/v1/bookmark', 'verb' => 'GET'),
-		array('name' => 'public#new_bookmark', 'url' => '/public/rest/v1/bookmark', 'verb' => 'POST'),
-		array('name' => 'public#edit_bookmark', 'url' => '/public/rest/v1/bookmark/{id}', 'verb' => 'PUT'),
-		array('name' => 'public#delete_bookmark', 'url' => '/public/rest/v1/bookmark/{id}', 'verb' => 'DELETE'),


Totally removing them would kill compatibility. Better if we could deprecate them over a couple releases.

we don't need to keep the newly added CRUD methods, though, right? They haven't made it into any release yet, afaik

Yes, what wasn't there in 0.9.1 is vogelfrei.

blizzz · 2017-02-18T22:40:39Z

controller/rest/bookmarkcontroller.php

+		$sortby = "",
+		$search = array()
+	) {
+		if ($user == null) {


always use === as equal comparison parameter

marcelklehr · 2017-02-19T17:11:29Z

appinfo/routes.php

 		//Legacy Routes
+		array('name' => 'public#return_as_json', 'url' => '/public/rest/v1/bookmark', 'verb' => 'GET'),
+


Is this ok?

blizzz · 2017-04-29T13:37:52Z

I rebased this locally. It seems something is wrong (perhaps with the annotated checks), because I am being logged out when access the bookmarks page normally via web interface.

blizzz · 2017-04-29T13:50:07Z

there lies the issue i think

{
  …"message":"Undefined index: PHP_AUTH_USER at \/srv\/http\/nextcloud\/master\/lib\/private\/AppFramework\/Middleware\/Security\/CORSMiddleware.php#88"
…

not the index, but that we are logged out and have no provided parameters for logging in.

marcelklehr · 2017-04-29T13:52:15Z

So, the problem I presume is that the client doesn't send auth data? Shouldn't there be a session cookie being sent along?

blizzz · 2017-04-29T13:58:13Z

The CORS annotation makes sure that sessions are not being used, therefore the logout happens.

marcelklehr · 2017-04-29T14:12:17Z

So, how do I realize an API that should be able to be used from within nextcloud as well as from other sites? Do we need to create a proxy Controller without CORS?

to fix app ui logging out onload

blizzz · 2017-04-29T21:25:20Z

~~Perhaps taking away CORS? But then, it was added for a reason. Or not?~~

Do we need to create a proxy Controller without CORS?

Seems so, at least my thoughts go into the same direction.

marcelklehr · 2017-04-30T11:10:53Z

Without CORS it works, so there's some mistake in how I added CORS, I guess.

Update: The tutorial doesn't mention anything else, though: https://docs.nextcloud.com/server/12/developer_manual/app/tutorial.html#adding-a-restful-api-optional

marcelklehr · 2017-04-30T11:18:48Z

Perhaps we need something like this: https://github.com/nextcloud/passman/blob/f8c13471b78b76a0576dd29e8c32d9e1e1f2359d/middleware/apimiddleware.php

marcelklehr · 2017-05-01T19:31:16Z

BernhardPosselt on IRC pointed out to me as well that CORS drops sessions, so we do need two controllers. The question is if this pr makes sense at all in this case. The whole idea was to have only one API controller. In this PR I basically merged the two, but now we're exposing the same interface at two different routes... Might still be better than having two entirely different interfaces on the two endpoints... What do you say?

blizzz · 2017-05-01T19:51:03Z

Sidenote: just came across the added @NoCSRFRequired annotations. Those would make it vulnerable to CSFR attacks. For pure API functionality this is necessary to drop indeed, but not within our interface, because sessions are active.

If there is no use case for CORS (i am not aware of an request?), we can leave that out. The Passman middlewere reference would be about it?

Two different routes are OK imho, however I do not see them duplicated?

Actually, now I am thinking whether we should not keep a dedicated apicontroller, which would just inherit the internal one and just change the annotations, and call the parent. Probably there is a hook, though.

marcelklehr · 2017-05-01T20:00:45Z

I agree on the CSRF issue.

CORS is not important to me, either.

I agree on the approach and have started working on this.

marcelklehr · 2017-05-01T20:04:28Z

I think @cors imight be necessary for security, though, since NoCSRFRequired is added to API endpoints and all...?

and expose public api add /public/rest/v2/ next to v1

blizzz · 2017-05-05T19:31:32Z

I think @cors imight be necessary for security, though, since NoCSRFRequired is added to API endpoints and all...?

Not really for security. While it disables sessions (client would also need to reauthenticate), it also allows to be used across origins, i.e. from other websites/webapps. Sounds OK to me to include it nevertheless.

blizzz · 2017-05-05T19:42:08Z

The changes seem to work so far from the UI. However, when returning bookmarks, only a piece of the data is returned back.

Expected:

Returned:

You'll see it as the date in the list is represented as "Invalid date" in the UI.

marcelklehr · 2017-05-05T21:34:13Z

This is a symptom of merging the two APIs, it can be easily fixed by either modifying the client-side to include a parameter that tells the API all attributes it needs, or by modifying the API to deliver all those by default. Your call :)

blizzz · 2017-05-06T08:44:24Z

Just return all of them. It keeps things simple, and the only extra cost comes from the tags. This is fine, and nothing can go wrong by chance (e.g
updating a bookmark).

always return everything

blizzz

Looks good now and I went through the formalities. Bigger things: an undeclared method is used and a typo in a variable name. Other comments are basically about style/form.

blizzz · 2017-05-07T20:58:37Z

controller/rest/bookmarkcontroller.php

+		$tags = array(),
+		$conjunction = "or",
+		$sortby = "",
+		$search = array()


phpdoc (@param) annotation is missing for $user, $tags, $conjunction, $sortby, $search (yes, it was missing before already).

blizzz · 2017-05-07T20:59:39Z

controller/rest/bookmarkcontroller.php

+		}else {
+			$publicOnly = true;
+			if ($this->userManager->userExists($user) == false) {
+				return $this->newJsonErrorMessage("User could not be identified");


newJsonErrorMessage() does not exist here

blizzz · 2017-05-07T21:01:43Z

controller/rest/bookmarkcontroller.php

@@ -218,7 +276,7 @@ public function deleteBookmark($id = -1) {
 	}

 	/**
-	  @NoAdminRequired
+		@NoAdminRequired


blizzz · 2017-05-07T21:01:48Z

controller/rest/bookmarkcontroller.php

@@ -243,7 +301,7 @@ public function clickBookmark($url = "") {
 	}

 	/**
-	  @NoAdminRequired
+		@NoAdminRequired


blizzz · 2017-05-07T21:01:55Z

controller/rest/bookmarkcontroller.php

@@ -271,7 +329,7 @@ public function importBookmark() {
 	}

 	/**
-	  @NoAdminRequired
+		@NoAdminRequired


blizzz · 2017-05-07T21:06:43Z

controller/rest/internaltagscontroller.php

+
+use \OCA\Bookmarks\Controller\Lib\Bookmarks;
+use \OCP\AppFramework\Http\JSONResponse;
+use \OCP\AppFramework\Http;


blizzz · 2017-05-07T21:06:52Z

controller/rest/internaltagscontroller.php

+use \OCP\AppFramework\Http\JSONResponse;
+use \OCP\AppFramework\Http;
+use \OCP\AppFramework\ApiController;
+use OCA\Bookmarks\Controller\Rest\InternalTagsController;


blizzz · 2017-05-07T21:08:57Z

tests/bookmarkcontroller_test.php

@@ -19,91 +19,111 @@ class Test_PublicController_Bookmarks extends TestCase {
 	private $db;
 	private $userManager;
 	/** @var	PublicController */


should become BookmarkController

blizzz · 2017-05-07T21:10:03Z

tests/bookmarkcontroller_test.php

 	private $publicController;

 	protected function setUp() {
 		parent::setUp();

 		$this->userid = "testuser";
+		$this->otherUser = "otheruser";


declaration missing

blizzz · 2017-05-07T21:13:30Z

tests/bookmarkcontroller_test.php

@@ -19,91 +19,111 @@ class Test_PublicController_Bookmarks extends TestCase {
 	private $db;
 	private $userManager;
 	/** @var	PublicController */
+	private $controller;
 	private $publicController;


would need the /** @var BookmarkController */ typehint, too

blizzz · 2017-05-09T21:54:56Z

controller/rest/internalbookmarkcontroller.php

+
+	public function __construct($appName, IRequest $request, $userId, IDBConnection $db, IL10N $l10n, Bookmarks $bookmarks, Manager $userManager) {
+		parent::__construct($appName, $request);
+	  $this->publicController = new BookmarkController($appname, $request, $userId, $db, $l10n, $bookmarks, $userManager);


Oh, missed this before. Typo in $appname, the n must be upper case.

blizzz · 2017-05-10T19:01:41Z

controller/rest/bookmarkcontroller.php

@@ -57,27 +60,81 @@ public function legacyGetBookmarks($type = "bookmark", $tag = '', $page = 0, $so
 	 * @param string $tag
 	 * @param int $page
 	 * @param string $sort
+	 * @param string user


sorry, but the $'s are missing in the var names

blizzz · 2017-05-10T19:03:17Z

controller/rest/internalbookmarkcontroller.php

+	 * @param string $tag
+	 * @param int $page
+	 * @param string $sort
+	 * @param string user


here too 😇

blizzz · 2017-05-10T19:08:57Z

I leave the two cosmetics for a quick additional PR. And use the moment to merge this. Good work, thanks a lot @marcelklehr :) 🚀 🎸

marcelklehr added 9 commits February 10, 2017 18:25

First try at consolidating private and public api

db8331b

Fix tests: Create otherUser

dc015ff

Fix tests: private edit and private create

73b83b9

Fix tests for public access

e1f888a

Test sth out

c1c88d8

Fix tests: Inspect correct sub path of json

11dc4d5

Feature: Merge back in features of public querying

653a657

Merge branch 'master' into consolidate/private-public-api

9d6514d

Remove superfluous code

4e1e965

marcelklehr changed the title ~~Consolidate private and preliminary public api~~ Consolidate private with preliminary public api Feb 10, 2017

marcelklehr added 3 commits February 10, 2017 22:05

Make sure $filterTag is set

1a9a2d3

Add docs for tagcontroller

0145737

Fix mind-o pricateCreate and publicCreate tests

3c78610

blizzz reviewed Feb 18, 2017

View reviewed changes

marcelklehr added 2 commits February 19, 2017 01:24

Re-add old public API endpoint for compat

ddf3eee

Use === as equal comparison operator

5fdda34

marcelklehr commented Feb 19, 2017

View reviewed changes

Add preflight_cors route

e77e83d

to fix app ui logging out onload

marcelklehr added 2 commits April 30, 2017 12:06

Remove CORS

8bdb6ec

API#getBookmarks: send id by default

5ed599e

marcelklehr added 2 commits May 1, 2017 22:09

Add a proxy controller for internal use of API

31a945c

and expose public api add /public/rest/v2/ next to v1

Fix Syntax and Class imports

a838298

marcelklehr added 2 commits May 6, 2017 12:25

API: Remove select[] param

e6b6f05

always return everything

Update README API docs with new v2 URL

9477d62

marcelklehr mentioned this pull request May 7, 2017

[Enhancement] Have a true sync API #2

Closed

blizzz requested changes May 7, 2017

View reviewed changes

marcelklehr mentioned this pull request May 8, 2017

NextCloud 11 Support floccusaddon/floccus#7

Closed

Fix style issues and a missing method

ccb4b84

blizzz reviewed May 9, 2017

View reviewed changes

Fix typo and unnecessary import

b9c810c

blizzz approved these changes May 10, 2017

View reviewed changes

blizzz merged commit 8d68ecb into nextcloud:master May 10, 2017

marcelklehr mentioned this pull request May 10, 2017

enabled preflighted CORS method #328

Closed

		//Legacy Routes
		array('name' => 'public#return_as_json', 'url' => '/public/rest/v1/bookmark', 'verb' => 'GET'),

Consolidate private with preliminary public api #338

Consolidate private with preliminary public api #338

Conversation

marcelklehr commented Feb 10, 2017 • edited Loading

marcelklehr commented Feb 10, 2017 • edited Loading

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

blizzz commented Apr 29, 2017

blizzz commented Apr 29, 2017

marcelklehr commented Apr 29, 2017

blizzz commented Apr 29, 2017

marcelklehr commented Apr 29, 2017

blizzz commented Apr 29, 2017 • edited Loading

marcelklehr commented Apr 30, 2017 • edited Loading

marcelklehr commented Apr 30, 2017

marcelklehr commented May 1, 2017

blizzz commented May 1, 2017

marcelklehr commented May 1, 2017 • edited Loading

marcelklehr commented May 1, 2017

blizzz commented May 5, 2017

blizzz commented May 5, 2017

marcelklehr commented May 5, 2017 • edited Loading

blizzz commented May 6, 2017

blizzz left a 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

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

blizzz commented May 10, 2017

marcelklehr commented Feb 10, 2017 •

edited

Loading

marcelklehr commented Feb 10, 2017 •

edited

Loading

blizzz commented Apr 29, 2017 •

edited

Loading

marcelklehr commented Apr 30, 2017 •

edited

Loading

marcelklehr commented May 1, 2017 •

edited

Loading

marcelklehr commented May 5, 2017 •

edited

Loading