Skip to content
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

OCS endpoint to list remote shares #19386

Merged
merged 5 commits into from
Oct 2, 2015
Merged

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Sep 26, 2015

Required for #9098

  • Move OCS endpoint ../remote_shares -> ../pending_shares
  • Introduce new endpoint ../remote_shares that allows listing of incoming remote shares.

@nickvergessen since you wrote the original ../remote_shares stuff are you fine with this?

I'm not really happy with this. But as stated in #9098 (comment) this is the cleanest hack I can think of. Comments?

CC: @cmonteroluque @DeepDiver1975

$shareInfo = $externalManager->getShare($params['id']);

if ($shareInfo === false) {
return new \OC_OCS_Result(null, 404, 'share does not exists');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/exists/exist/

@nickvergessen
Copy link
Contributor

Maybe remote_shares/pending or do want to use the same endpoint for normal shares in the future?

This needs a PR to fix the URLs in the Pyoc client

@rullzer
Copy link
Contributor Author

rullzer commented Sep 28, 2015

Maybe remote_shares/pending or do want to use the same endpoint for normal shares in the future?

Possible for sharing 2.0 yes.

@rullzer rullzer force-pushed the ocs_shareapi_extenstion branch from 9eb6858 to 8a9cec6 Compare September 28, 2015 19:12
@DeepDiver1975
Copy link
Member

Hmmm ... do we really want to change the behavior of already existing public routes?

This is a no go from my perspective - but maybe I missed something.

@rullzer
Copy link
Contributor Author

rullzer commented Sep 29, 2015

Hmmm ... do we really want to change the behavior of already existing public routes?

This is a no go from my perspective - but maybe I missed something.

This route was introduced in 8.2. So it is not yet officially released.

@DeepDiver1975
Copy link
Member

THX. Fine then

$getShare = $this->connection->prepare('
SELECT `remote`, `remote_id`, `share_token`, `name`
SELECT *
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to expose all of the data?
Thinking about password and share_token, I'm not sure they should be leaked like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I agree we should limit what we show. But we also show * in the listing of pending shares.. https://github.com/owncloud/core/blob/master/apps/files_sharing/lib/external/manager.php#L400-L430

So this doesn't introduce anything new.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay fine by me then

@MorrisJobke
Copy link
Contributor

@rullzer Ready to review or not?

@rullzer
Copy link
Contributor Author

rullzer commented Oct 1, 2015

@MorrisJobke not yet. I think I need to add some stuff. For #19501 to work properly.

@nickvergessen
Copy link
Contributor

btw while preparing a patch for the PyoC Client to change the URL, I realised we can't reuse the same endpoint for remote and normal shares! 💣
The problem is that they have a different unique id. For remote shares we need to accept oc_share_remote.id for normal shares oc_share.id So they can not reuse the same endpoint.

@rullzer
Copy link
Contributor Author

rullzer commented Oct 1, 2015

@nickvergessen warg... unique id's are needed then indeed...

@rullzer rullzer force-pushed the ocs_shareapi_extenstion branch from 9073b71 to 0e95eb6 Compare October 1, 2015 12:25
@rullzer
Copy link
Contributor Author

rullzer commented Oct 1, 2015

@nickvergessen I guess this is then the most logical solution.

@rullzer
Copy link
Contributor Author

rullzer commented Oct 1, 2015

Ok extra info added.. do now you actually know something about the remote shares.

Review time.

CC: @nickvergessen @MorrisJobke @DeepDiver1975

@rullzer rullzer force-pushed the ocs_shareapi_extenstion branch from 41e48b5 to 7477fa7 Compare October 1, 2015 15:21
* list incoming remote shares at 'remote_shares'
* get per share info at 'remote_shares/<ID>'
* delete remote share with a DELETE to 'remote_shares/<ID>'
Since we need a unique id to accept/reject shares for now we keep the
pending shares under remote_shares.

* remote_shares/pending lists pending shares
* PUT/DELETE to remote_shares/pending/<ID> will accept/reject the share
The data from the share_external is not to much. Thus we enrich this
data with info from the filecache.

This allows endpoints using this to actually show usefull information.

The filecache might not be up to date but that is a sacrifice we need to
make in terms of speed. Else the number of remote PROPFINDS grows
lineary with the number of remote shares wich will make this endpoint
practically unusable.
@rullzer rullzer force-pushed the ocs_shareapi_extenstion branch from 7477fa7 to 7ecd15d Compare October 2, 2015 05:53
@rullzer rullzer force-pushed the ocs_shareapi_extenstion branch from 7ecd15d to c924b67 Compare October 2, 2015 06:11
@rullzer
Copy link
Contributor Author

rullzer commented Oct 2, 2015

Ok cleaned up a bit more so we return a little less cluttered stuff.

@rullzer
Copy link
Contributor Author

rullzer commented Oct 2, 2015

OCI failure seems unrelated.

@nickvergessen
Copy link
Contributor

Didn't test, but looks good. 👍

@PVince81
Copy link
Contributor

PVince81 commented Oct 2, 2015

Works 👍

I get the JS code will need to be adjusted later.

@nickvergessen
Copy link
Contributor

PyOC patch owncloud/pyocclient#122

@rullzer
Copy link
Contributor Author

rullzer commented Oct 2, 2015

I get the JS code will need to be adjusted later.

@PVince81 Already done in #19514
But I did not want to have 1 huge PR.

@PVince81
Copy link
Contributor

PVince81 commented Oct 2, 2015

Ah yes, the JS code update is here #19514

DeepDiver1975 added a commit that referenced this pull request Oct 2, 2015
@DeepDiver1975 DeepDiver1975 merged commit daf9a63 into master Oct 2, 2015
@DeepDiver1975 DeepDiver1975 deleted the ocs_shareapi_extenstion branch October 2, 2015 10:13
@carlaschroder
Copy link

@rullzer Is this only for 8.2?

@nickvergessen
Copy link
Contributor

yes

@carlaschroder
Copy link

thanks @nickvergessen

@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants