Add api clients for talking to remote clouds by icewind1991 · Pull Request #6651 · nextcloud/server

icewind1991 · 2017-09-26T15:14:23Z

We're getting more and more places where we need to talk to remote nextcloud instances, instead of each app having to figure out the \stuff for themselves, it would be better to have some common stuff in core.

Todo:

figure out a proper way how to test this

nickvergessen · 2017-09-26T15:53:36Z

lib/private/Remote/Api/OCS.php

+	}
+
+	public function getUser($userId) {
+		return new User($this->request('get', '/ocs/v1.php/cloud/users/' . $userId));


Please use v2.php

this endpoint only works authenticated?!

It is authenticated: https://github.com/nextcloud/server/pull/6651/files#diff-bd9df1a4708539bdbb70c6ef51d3862dR66

Yeah lol, but you need valid credentials of the same or an admin user on the third party instance...
I think this is just an example, but it's a bad one I wouldn't ship.

this endpoint only works authenticated

Yes

nickvergessen · 2017-09-26T15:53:44Z

lib/private/Remote/Api/OCS.php

+	}
+
+	public function getCapabilities() {
+		return $this->request('get', '/ocs/v1.php/cloud/capabilities');


nickvergessen · 2017-09-26T15:56:13Z

lib/private/Remote/User.php

+	 * @return string
+	 */
+	public function getTwitter() {
+		return $this->data['twitter'];


This is 12+ only and will throw undefined index when being called against 11

nickvergessen · 2017-09-26T15:56:52Z

Good idea to abstract that away 👍

nickvergessen · 2017-09-26T15:57:40Z

As for testing, use functional tests and run the same server behind 2 addresses, like its done for the federation tests?

MorrisJobke · 2017-09-27T08:47:01Z

lib/private/Remote/Credentials.php

+	 * @param string $user
+	 * @param string $password
+	 */
+	public function __construct($user, $password) {


Should then be added to the stack trace cleaner to not leak passwords in the log file.

That is for exceptions that are thrown inside the method, since this method won't throw exceptions it's unneeded

codecov · 2017-10-02T15:10:30Z

Codecov Report

Merging #6651 into master will increase coverage by 2.41%.
The diff coverage is 54.02%.

@@             Coverage Diff              @@
##             master    #6651      +/-   ##
============================================
+ Coverage     50.43%   52.84%   +2.41%     
+ Complexity    24712    22883    -1829     
============================================
  Files          1522     1449      -73     
  Lines         86422    88732    +2310     
  Branches          0     1349    +1349     
============================================
+ Hits          43585    46894    +3309     
+ Misses        42837    41838     -999

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Remote/Api/ApiFactory.php	`0% <0%> (ø)`	`2 <2> (?)`
lib/private/Remote/Api/ApiCollection.php	`0% <0%> (ø)`	`3 <3> (?)`
lib/private/Remote/Credentials.php	`100% <100%> (ø)`	`3 <3> (?)`
lib/private/Remote/InstanceFactory.php	`100% <100%> (ø)`	`2 <2> (?)`
lib/private/Remote/User.php	`14.28% <14.28%> (ø)`	`15 <15> (?)`
lib/private/Server.php	`82.93% <30%> (+0.31%)`	`126 <3> (ø)`	⬇️
lib/private/Remote/Api/ApiBase.php	`52.5% <52.5%> (ø)`	`10 <10> (?)`
lib/private/Remote/Api/OCS.php	`53.33% <53.33%> (ø)`	`15 <15> (?)`
lib/private/Remote/Instance.php	`90.47% <90.47%> (ø)`	`13 <13> (?)`
lib/private/Template/JSResourceLocator.php	`0% <0%> (-50.91%)`	`22% <0%> (-1%)`
... and 953 more

icewind1991 · 2017-10-06T14:00:12Z

Added tests and plugic api,

ready to review @rullzer @nickvergessen

icewind1991 · 2017-10-18T11:14:35Z

Please review @rullzer @nickvergessen @schiessle

nickvergessen · 2017-10-18T12:26:16Z

Fine by me, although I would like to not have the interfaces in OCP for now, so we can test them in one release before finalizing them.

👍

icewind1991 · 2017-10-18T13:15:55Z

They are added to the public api since they will be part of the migration api which needs to be implemented by apps

LukasReschke · 2017-10-19T09:50:10Z

lib/private/Remote/Api/ApiBase.php

+				$response = $client->options($fullUrl, $options);
+				break;
+			default:
+				throw new \InvalidArgumentException('Invalid method ' . $method);


Add @throw to the PHPDoc

LukasReschke · 2017-10-19T09:50:38Z

lib/private/Remote/Api/ApiCollection.php

+class ApiCollection implements IApiCollection {
+	private $instance;
+	private $credentials;
+	private $clientService;


Type annotations here would be nice :)

LukasReschke · 2017-10-19T09:50:59Z

lib/private/Remote/Api/ApiFactory.php

+use OCP\Remote\IInstance;
+
+class ApiFactory implements IApiFactory {
+	private $clientService;


Type annotations here would be nice :)

LukasReschke · 2017-10-19T09:53:29Z

lib/private/Remote/Instance.php

+	/** @var IClientService */
+	private $clientService;
+
+	private $status;


Typehint missing

LukasReschke · 2017-10-19T09:54:59Z

lib/private/Remote/Instance.php

+			return $this->status;
+		}
+		$key = 'remote/' . $this->url . '/status';
+		$status = $this->cache->get($key);


Can we permanently store the result of this somewhere? Just because otherwise we're prone to downgrade attacks if an adversary blocks the 443 port.

LukasReschke · 2017-10-19T09:55:23Z

lib/private/Remote/Instance.php

+		return $status;
+	}
+
+	private function downloadStatus($url) {


PHPDocs would be nice :)

LukasReschke · 2017-10-19T09:55:32Z

lib/private/Remote/InstanceFactory.php

+
+class InstanceFactory implements IInstanceFactory {
+	private $cache;
+	private $clientService;


Typehints would be nice :)

LukasReschke · 2017-10-19T09:56:06Z

lib/public/Remote/IInstanceFactory.php

+ */
+interface IInstanceFactory {
+	/**
+	 * @param $url


LukasReschke · 2017-10-19T09:59:33Z

lib/private/Remote/Api/OCS.php

+	}
+
+	public function getUser($userId) {
+		return new User($this->request('get', 'cloud/users/' . $userId));


This seems pretty error-prone, especially since the User class is just accessing the array data. Can we have more issets in the User class and some sanity check on the response?

icewind1991 · 2017-10-19T13:38:44Z

@LukasReschke all done.

It now separately caches if a remote supports https and refuses to downgrade to http

LukasReschke · 2017-10-23T08:49:26Z

lib/private/Remote/Instance.php

 			$response = $this->downloadStatus('https://' . $this->getUrl() . '/status.php');
 			$protocol = 'https';
 			if (!$response) {
+				if ($status = $this->cache->get($httpsKey)) {


Please cache in IAppConfig or so 😇

Signed-off-by: Robin Appelman <robin@icewind.nl>

rullzer

Lets get this in!

icewind1991 added the 2. developing Work in progress label Sep 26, 2017

icewind1991 added this to the Nextcloud 13 milestone Sep 26, 2017

icewind1991 requested review from MorrisJobke, nickvergessen and rullzer September 26, 2017 15:14

nickvergessen requested changes Sep 26, 2017

View reviewed changes

MorrisJobke reviewed Sep 27, 2017

View reviewed changes

icewind1991 force-pushed the remote-cloud-client branch from dc50df6 to 6feb2b9 Compare October 2, 2017 15:10

icewind1991 force-pushed the remote-cloud-client branch 2 times, most recently from c50b453 to 7bd5bd6 Compare October 6, 2017 13:55

icewind1991 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 6, 2017

icewind1991 force-pushed the remote-cloud-client branch 2 times, most recently from d063ff1 to 9587796 Compare October 17, 2017 15:46

LukasReschke requested changes Oct 19, 2017

View reviewed changes

LukasReschke requested changes Oct 23, 2017

View reviewed changes

icewind1991 added 6 commits December 8, 2017 16:34

Add api clients for talking to remote clouds

ac2c26f

Signed-off-by: Robin Appelman <robin@icewind.nl>

Some tests for the remote cloud api

74b5ce8

Signed-off-by: Robin Appelman <robin@icewind.nl>

Add public api for remote api

5133a31

Signed-off-by: Robin Appelman <robin@icewind.nl>

add more typehints

8b01176

Signed-off-by: Robin Appelman <robin@icewind.nl>

validate user response

78a24e3

Signed-off-by: Robin Appelman <robin@icewind.nl>

refuse to use http if we know a remote has https

f1eb55f

Signed-off-by: Robin Appelman <robin@icewind.nl>

Add some more tests for ocs remote api

5ce69e7

Signed-off-by: Robin Appelman <robin@icewind.nl>

MorrisJobke force-pushed the remote-cloud-client branch from 41399b5 to 5ce69e7 Compare December 8, 2017 15:37

MorrisJobke approved these changes Dec 8, 2017

View reviewed changes

MorrisJobke mentioned this pull request Dec 8, 2017

13.0.0 beta 2 #7425

Merged

28 tasks

rullzer approved these changes Dec 11, 2017

View reviewed changes

rullzer merged commit e8acf44 into master Dec 11, 2017

rullzer deleted the remote-cloud-client branch December 11, 2017 14:28

LukasReschke mentioned this pull request Feb 24, 2021

Deprecate OCP\Remote? #25780

Closed

Uh oh!

Conversation

icewind1991 commented Sep 26, 2017 • 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!

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!

nickvergessen commented Sep 26, 2017

Uh oh!

nickvergessen commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

icewind1991 commented Oct 6, 2017

Uh oh!

icewind1991 commented Oct 18, 2017

Uh oh!

nickvergessen commented Oct 18, 2017

Uh oh!

icewind1991 commented Oct 18, 2017

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!

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!

icewind1991 commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rullzer left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

icewind1991 commented Sep 26, 2017 •

edited

Loading

codecov bot commented Oct 2, 2017 •

edited

Loading