-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add api clients for talking to remote clouds #6651
Conversation
lib/private/Remote/Api/OCS.php
Outdated
} | ||
|
||
public function getUser($userId) { | ||
return new User($this->request('get', '/ocs/v1.php/cloud/users/' . $userId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please use
v2.php
- this endpoint only works authenticated?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this endpoint only works authenticated
Yes
lib/private/Remote/Api/OCS.php
Outdated
} | ||
|
||
public function getCapabilities() { | ||
return $this->request('get', '/ocs/v1.php/cloud/capabilities'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v2.php
lib/private/Remote/User.php
Outdated
* @return string | ||
*/ | ||
public function getTwitter() { | ||
return $this->data['twitter']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 12+ only and will throw undefined index when being called against 11
Good idea to abstract that away 👍 |
As for testing, use functional tests and run the same server behind 2 addresses, like its done for the federation tests? |
* @param string $user | ||
* @param string $password | ||
*/ | ||
public function __construct($user, $password) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should then be added to the stack trace cleaner to not leak passwords in the log file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is for exceptions that are thrown inside the method, since this method won't throw exceptions it's unneeded
dc50df6
to
6feb2b9
Compare
Codecov Report
@@ 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
|
c50b453
to
7bd5bd6
Compare
Added tests and plugic api, ready to review @rullzer @nickvergessen |
d063ff1
to
9587796
Compare
Please review @rullzer @nickvergessen @schiessle |
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. 👍 |
They are added to the public api since they will be part of the migration api which needs to be implemented by apps |
$response = $client->options($fullUrl, $options); | ||
break; | ||
default: | ||
throw new \InvalidArgumentException('Invalid method ' . $method); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add @throw
to the PHPDoc
class ApiCollection implements IApiCollection { | ||
private $instance; | ||
private $credentials; | ||
private $clientService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type annotations here would be nice :)
use OCP\Remote\IInstance; | ||
|
||
class ApiFactory implements IApiFactory { | ||
private $clientService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type annotations here would be nice :)
lib/private/Remote/Instance.php
Outdated
/** @var IClientService */ | ||
private $clientService; | ||
|
||
private $status; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typehint missing
lib/private/Remote/Instance.php
Outdated
return $this->status; | ||
} | ||
$key = 'remote/' . $this->url . '/status'; | ||
$status = $this->cache->get($key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/private/Remote/Instance.php
Outdated
return $status; | ||
} | ||
|
||
private function downloadStatus($url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHPDocs would be nice :)
|
||
class InstanceFactory implements IInstanceFactory { | ||
private $cache; | ||
private $clientService; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typehints would be nice :)
*/ | ||
interface IInstanceFactory { | ||
/** | ||
* @param $url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string?
lib/private/Remote/Api/OCS.php
Outdated
} | ||
|
||
public function getUser($userId) { | ||
return new User($this->request('get', 'cloud/users/' . $userId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@LukasReschke all done. It now separately caches if a remote supports https and refuses to downgrade to http |
lib/private/Remote/Instance.php
Outdated
$status = $this->cache->get($key); | ||
if (!$status) { | ||
$response = $this->downloadStatus('https://' . $this->getUrl() . '/status.php'); | ||
$protocol = 'https'; | ||
if (!$response) { | ||
if ($status = $this->cache->get($httpsKey)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please cache in IAppConfig or so 😇
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
41399b5
to
5ce69e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets get this in!
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: