OCS API for server-to-server sharing #12452

schiessle · 2014-11-26T12:01:10Z

OCS API for server2server sharing as part of #12285 .

This is work in progress, I just want to give people a chance to look at it early.

This is how the API will look like:

POST to create new server2server share

http://localhost/oc/ocs/v1.php/cloud/shares

post data: shareWith, token, name, remote, remote_id, owner

POST to notify that the share was accepted

http://localhost/oc/ocs/v1.php/cloud/shares/{shareId}/accept

post data: token

POST to notify that the share was declined

http://localhost/oc/ocs/v1.php/cloud/shares/{shareId}/decline

post data: token

POST to unshare server-to-server share

http://localhost/oc/ocs/v1.php/cloud/shares/{shareId}/unshare

post data: 'token'

Known Issue:

if a share was declined the encryption keys are not updated. Will be fixed once [encryption] reorganize folder structure (second try to make Jenkins happy) #12382 is merged. Doesn't make sense to try to fix this now.

karlitschek · 2014-11-26T12:18:01Z

The API looks good in general.
Is is possible to have the ocs call as core routes somehow? Would be good to have cleaner routes if this become some kind of standard.

LukasReschke · 2014-11-26T12:28:15Z

Mhm. Would be great to incorporate either OCS with the AppFramework somehow or switch to REST routes powered by AppFramework controllers. That way we would be able to use all advantages of the AppFramework also in OCS routes... - Let me file a technical debt issue about that.

schiessle · 2014-11-26T13:43:59Z

I agree, from a standardization point of view this would make sense. I gave it a try, I could register following routes in core:

http://localhost/oc/ocs/v1.php/cloud/shares

http://localhost/oc/ocs/v1.php/cloud/shares/{id}/accept

http://localhost/oc/ocs/v1.php/cloud/shares/{id}/decline

all the code will still be in files_sharing. This works as long as the app is enabled. If the app is disabled the API will return

<?xml version="1.0"?>
<ocs>
 <meta>
  <status>failure</status>
  <statuscode>998</statuscode>
  <message>Api method not found</message>
 </meta>
 <data/>
</ocs>

karlitschek · 2014-11-26T13:48:19Z

👍

DeepDiver1975 · 2014-11-29T08:26:12Z

get rid of that classpath
please get rid of that static functions
write unit tests and inject depending objects

Thx

schiessle · 2014-12-01T16:08:35Z

This is now ready for review.

To try it out you can try follwing:

curl -X POST http://localhost/oc/ocs/v1.php/cloud/shares -d remote=remote.de -d token=jfdhfk -d name=theName -d owner=user2 -d remote_id=4 -d shareWith=remoteUser

this should create a corresponding entry in oc_share_external. Accepted should be set to "0" so the share will not be visible. If you use values from a real server-2-server share the share will become visible once you set accepted to "1". Additional a activity should be created for "remoteUser" to notify him about the new share.

accept/decline can be tested with a normal link share. Just create a link share and use the 'id' and 'token' from oc_share to accept/decline the share, accept should just trigger a notification that the share was accepted, decline will trigger a activity and remove the share from oc_share.

curl -X POST http://localhost/oc/ocs/v1.php/cloud/shares/<id>/decline -d token=<token>

and

curl -X POST http://localhost/oc/ocs/v1.php/cloud/shares/<id>/accept -d token=<token>

LukasReschke · 2014-12-01T16:12:50Z

apps/files_sharing/api/server2server.php

+	}
+
+	private function isS2SEnabled() {
+		return \OCP\App::isEnabled('files_sharing');


What about \OCA\Files_Sharing\Helper::isOutgoingServer2serverShareEnabled and \OCA\Files_Sharing\Helper::isIncomingServer2serverShareEnabled?

schiessle · 2014-12-01T16:35:58Z

@LukasReschke good catch, I addressed all your comments.

LukasReschke · 2014-12-01T17:14:02Z

👎 Introduces a critical security bug. - Will elaborate in an inline comment.

LukasReschke · 2014-12-01T17:16:46Z

apps/files_sharing/api/server2server.php

+
+		if ($share) {
+			// userId must be set to the user who unshares
+			\OC_User::setUserId($share['uid_owner']);


💣 💣 💣

This introduces a security bug. - \OC_User::setUserId($foo) will also set the user session to the user:

/** * Sets user id for session and triggers emit */ public static function setUserId($uid) { \OC::$server->getSession()->set('user_id', $uid); }

Since neither $token nor $id are secrets (you have them if somebody sent you a public link) this means an unauthenticated adversary can login as one of those users without providing any credentials.

💣 💣 💣

The problem here is that while unshare we call the unshare hook to which various apps will listen. E.g. encryption to remove the corresponding share key. This hooks expect that unshare was triggered by a logged-in user getUser() for example to construct the correct path.

Since nobody login here there will be no session from a specific user. So what is the concrete problem if I set the user in the session for this call?

Also keep in mind that we only reach this point if the user who send the request provided a valid share ID and token (which could be seen as some kind of username/password in this context). Because the token is only known by you and the recipient of the server2server share.

Any other idea how to solve the problem?

The problem here is that while unshare we call the unshare hook to which various apps will listen. E.g. encryption to remove the corresponding share key. This hooks expect that unshare was triggered by a logged-in user getUser() for example to construct the correct path.
Since nobody login here there will be no session from a specific user. So what is the concrete problem if I set the user in the session for this call?

The problem is that you create a valid session for $share['uid_owner'] and neither the token nor the id are really secrets. If you have access to a public share you know the token and enumerating an ID is just a 20liner.

The problem here is:

You share the folder "bar" publicy (via link)

You send the link of "bar" to Bob

Bob accesses the decline URL

Bob receives a valid session (the one in set-cookie), with that session he is authenticated (just copy this in your browser and enjoy being logged-in)

Any other idea how to solve the problem?

No. But no magic with setUserId please. Also unsetting it after that is not what we should do.

Or as a practical example:

Login as user "admin"

Create a folder "foo"

Share "foo" publicy (http://localhost/core/index.php/s/aGCjLi78JOzFua4)

Delete all cookies

Send a decline request

➜ ~ curl -X POST http://localhost/core/ocs/v1.php/cloud/shares/1/decline -d token=aGCjLi78JOzFua4 -v * Hostname was NOT found in DNS cache * Trying ::1... * Connected to localhost (::1) port 80 (#0) > POST /core/ocs/v1.php/cloud/shares/1/decline HTTP/1.1 > User-Agent: curl/7.37.1 > Host: localhost > Accept: */* > Content-Length: 21 > Content-Type: application/x-www-form-urlencoded > * upload completely sent off: 21 out of 21 bytes < HTTP/1.1 200 OK < Date: Mon, 01 Dec 2014 18:18:10 GMT * Server Apache/2.4.9 (Unix) PHP/5.6.2 is not blacklisted < Server: Apache/2.4.9 (Unix) PHP/5.6.2 < X-Powered-By: PHP/5.6.2 < Set-Cookie: ocgns13bl46f=9b6krapplmghdc8n7fqfmndg06; path=/core; HttpOnly < Expires: Thu, 19 Nov 1981 08:52:00 GMT < Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0 < Pragma: no-cache < X-XSS-Protection: 1; mode=block < X-Content-Type-Options: nosniff < X-Frame-Options: Sameorigin < Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-eval'; style-src 'self' 'unsafe-inline'; frame-src *; img-src *; font-src 'self' data:; media-src *; connect-src * < X-Robots-Tag: none < Transfer-Encoding: chunked < Content-Type: text/xml; charset=UTF-8 < <?xml version="1.0"?> <ocs> <meta> <status>ok</status> <statuscode>100</statuscode> <message/> </meta> <data/> </ocs> * Connection #0 to host localhost left intact

6.Copy the cookie's value (9b6krapplmghdc8n7fqfmndg06) into your browser and have fun being logged-in as user "admin".

OK, that's bad... Of course you shouldn't get access to my ownCloud just because I shared a file with you. I will think about it... Does someone else has a idea how to address the problem?

schiessle · 2014-12-02T11:26:39Z

@LukasReschke I appreciate the discussion and I think we should definitely have this discussion. But I don't think that this PR is the right place and the right time for it. I think we should discuss this on devel@owncloud.org independent from this PR. This PR is a small part of a large feature for OC8 and we all know the deadlines. We shouldn't delay it because of a needed discussion about OCS and how this fits with the app framework.

schiessle · 2014-12-02T11:45:28Z

Tests pass locally, I create a new PR for Jenkins: #12541

LukasReschke · 2014-12-02T11:53:56Z

This PR is a small part of a large feature for OC8 and we all know the deadlines. We shouldn't delay it because of a needed discussion about OCS and how this fits with the app framework.

My problem with that is that merging this will require the API to be unchangeable for quite a time. Resulting in a lot of additional code smell that we can't clean up easily.

Then again: Why do we need to use OCS? - It's a pseudo-standard at best. Why can't we just use regular controllers and use apps/files_sharing/api/ instead ocs/v1.php/sharing/. It would only differ from an URL PoV and would make the code MUCH better. - Changing this would not even take a long time… This is not about "changing the whole OCS code", this is just about "don't add this to OCS but use proper REST controllers".

If we go the route with adding this we have to support the ocs routes even longer. Great stuff.

A sum-up can also be found at #12454 - I'll also send a heads-up to the ML about that.

I'll not give this implementation my 👍 just because I don't want to be the one held responsible for letting code in that we could have implemented better. - I'll do a security review later though…

karlitschek · 2014-12-02T11:58:16Z

@LukasReschke we use ocs already for some key apis and we will use it for more in the future. I don't understand the resistance here.
I'm happy to discuss the long term strategy with you at the next opportunity.

schiessle · 2014-12-02T12:01:05Z

Why can't we just use regular controllers and use apps/files_sharing/api/ instead ocs/v1.php/sharing/.

We could have URLs relative to the app, thats what the share API in apps/files_sharing/api/local.php does for example. The reason why we decided to have more general URLs is here: #12452 (comment)

LukasReschke · 2014-12-02T12:01:33Z

I don't understand the resistance here.

My resistance is pointed out at #12452 (comment) - the OCS implementation is just really really really bad compared to the other possibilities we have. And this will lead to bugs. It did in the past and it will pretty much in the future.

I just don't want to add more and more technical debt to the whole project.

I'm happy to discuss the long term strategy with you at the next opportunity.

Dito.

LukasReschke · 2014-12-02T12:04:55Z

@LukasReschke we use ocs already for some key apis and we will use it for more in the future. I don't understand the resistance here.

That said the critical security bug discovered in this PR would not have been possible with the AppFramework as session writing is blocked and @UseSession should make every developer suspect ;-)

LukasReschke · 2014-12-02T20:30:35Z

apps/files_sharing/api/server2server.php

+	 *
+	 * @param string $user
+	 * @param int $fileSource
+	 */


Missing return statement

LukasReschke · 2014-12-03T13:07:01Z

Looks okay from a security PoV now. However, I'll take a second look at this once we have some kind of GUI for this. - So no objections from my side beside the above stated statement.

schiessle · 2014-12-03T20:48:29Z

Thanks @LukasReschke ! With every piece the picture will become more clear, would be great if you could also review the following steps. Hopefully the unit tests are now also fixed, just waiting for Jenkins. Would be great to have a second reviewer... Maybe @DeepDiver1975 or @MorrisJobke ? Thanks!

scrutinizer-notifier · 2014-12-04T12:20:45Z

A new inspection was created.

scrutinizer-notifier · 2014-12-04T13:14:03Z

The inspection completed: 11 new issues, 44 updated code elements

ghost · 2014-12-04T14:25:32Z

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3746/
🚀 Test PASSed. 🚀

Edited by @MorrisJobke copied over from #12567 (comment)

schiessle · 2014-12-04T15:13:21Z

Test passed: #12567 (comment)

schiessle · 2014-12-08T10:10:38Z

Still need a second reviewer. Would be great to get this in to be able to continue with the other parts of server-to-server sharing

MorrisJobke · 2014-12-08T11:29:19Z

@schiesbn Review exchange? #12406 needs reviewers too ;)

I will now review this one.

MorrisJobke · 2014-12-08T13:11:59Z

Curl calls succeed and works as posted in the first comment. Code isn't the best, but I think that it impossible to refactor this to a proper state until january -> 👍

OCS API for server-to-server sharing

schiessle mentioned this pull request Nov 26, 2014

server2server sharing ng #12285

Closed

LukasReschke mentioned this pull request Nov 26, 2014

Make OCS routes work together with the AppFramework #12454

Open

schiessle force-pushed the server2server-ng-ocs branch 2 times, most recently from 622c767 to 431b756 Compare November 26, 2014 15:42

schiessle added the 2 - Developing label Nov 28, 2014

schiessle added this to the 8.0-current milestone Nov 28, 2014

schiessle force-pushed the server2server-ng-ocs branch from 2cfa269 to 1d9906a Compare November 28, 2014 17:20

schiessle force-pushed the server2server-ng-ocs branch from 1d9906a to 04eb1c7 Compare December 1, 2014 15:58

schiessle changed the title ~~[WIP] OCS API for server-to-server sharing~~ OCS API for server-to-server sharing Dec 1, 2014

schiessle added 3 - To Review and removed 2 - Developing labels Dec 1, 2014

LukasReschke reviewed Dec 1, 2014
View reviewed changes

schiessle force-pushed the server2server-ng-ocs branch from 04eb1c7 to 7d2260f Compare December 1, 2014 16:35

schiessle force-pushed the server2server-ng-ocs branch 4 times, most recently from 9c47274 to 89a87cb Compare December 1, 2014 17:10

LukasReschke reviewed Dec 1, 2014
View reviewed changes

schiessle mentioned this pull request Dec 2, 2014

OCS API for server-to-server sharing (ignore, just for Jenkins) #12541

Closed

schiessle force-pushed the server2server-ng-ocs branch from 0bf6dae to 83c1d00 Compare December 2, 2014 15:16

LukasReschke reviewed Dec 2, 2014
View reviewed changes

schiessle mentioned this pull request Dec 3, 2014

[ignore] Server2server ng ocs jenkins2 #12567

Closed

schiessle force-pushed the server2server-ng-ocs branch from 83c1d00 to a1a2e0c Compare December 3, 2014 11:09

schiessle force-pushed the server2server-ng-ocs branch 2 times, most recently from fcbd3d7 to 6a8ad18 Compare December 3, 2014 20:36

schiessle force-pushed the server2server-ng-ocs branch from 6a8ad18 to 2e2116e Compare December 3, 2014 21:57

OCS API for server-to-server sharing

698ecbf

schiessle force-pushed the server2server-ng-ocs branch from 2e2116e to 698ecbf Compare December 4, 2014 12:20

MorrisJobke added a commit that referenced this pull request Dec 8, 2014

Merge pull request #12452 from owncloud/server2server-ng-ocs

1362c0b

OCS API for server-to-server sharing

MorrisJobke merged commit 1362c0b into master Dec 8, 2014

MorrisJobke deleted the server2server-ng-ocs branch December 8, 2014 13:12

lock bot locked as resolved and limited conversation to collaborators Aug 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCS API for server-to-server sharing #12452

OCS API for server-to-server sharing #12452

schiessle commented Nov 26, 2014

karlitschek commented Nov 26, 2014

LukasReschke commented Nov 26, 2014

schiessle commented Nov 26, 2014

karlitschek commented Nov 26, 2014

DeepDiver1975 commented Nov 29, 2014

schiessle commented Dec 1, 2014

LukasReschke Dec 1, 2014

schiessle commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

LukasReschke Dec 1, 2014

schiessle Dec 1, 2014

LukasReschke Dec 1, 2014

LukasReschke Dec 1, 2014

schiessle Dec 1, 2014

schiessle commented Dec 2, 2014

schiessle commented Dec 2, 2014

LukasReschke commented Dec 2, 2014

karlitschek commented Dec 2, 2014

schiessle commented Dec 2, 2014

LukasReschke commented Dec 2, 2014

LukasReschke commented Dec 2, 2014

LukasReschke Dec 2, 2014

schiessle Dec 3, 2014

LukasReschke commented Dec 3, 2014

schiessle commented Dec 3, 2014

scrutinizer-notifier commented Dec 4, 2014

scrutinizer-notifier commented Dec 4, 2014

ghost commented Dec 4, 2014

schiessle commented Dec 4, 2014

schiessle commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

OCS API for server-to-server sharing #12452

OCS API for server-to-server sharing #12452

Conversation

schiessle commented Nov 26, 2014

karlitschek commented Nov 26, 2014

LukasReschke commented Nov 26, 2014

schiessle commented Nov 26, 2014

karlitschek commented Nov 26, 2014

DeepDiver1975 commented Nov 29, 2014

schiessle commented Dec 1, 2014

Choose a reason for hiding this comment

schiessle commented Dec 1, 2014

LukasReschke commented Dec 1, 2014

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

schiessle commented Dec 2, 2014

schiessle commented Dec 2, 2014

LukasReschke commented Dec 2, 2014

karlitschek commented Dec 2, 2014

schiessle commented Dec 2, 2014

LukasReschke commented Dec 2, 2014

LukasReschke commented Dec 2, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasReschke commented Dec 3, 2014

schiessle commented Dec 3, 2014

scrutinizer-notifier commented Dec 4, 2014

scrutinizer-notifier commented Dec 4, 2014

ghost commented Dec 4, 2014

schiessle commented Dec 4, 2014

schiessle commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014

MorrisJobke commented Dec 8, 2014