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

Display remote shares in "Shared with you" section #9098

Closed
PVince81 opened this issue Jun 18, 2014 · 27 comments · Fixed by #19514
Closed

Display remote shares in "Shared with you" section #9098

PVince81 opened this issue Jun 18, 2014 · 27 comments · Fixed by #19514

Comments

@PVince81
Copy link
Contributor

Steps to reproduce

  1. Mount a remote share (server to server sharing)
  2. Go to "Shared with you" section

Expected result

Share has an entry

Actual result

List is empty

I'm not sure whether we should add this to the "Shared with you" section or whether it should be yet another section. Or should it appear under "Shared with link" ?

Note that for any cases the API calls will need to be adapted to also return the remote shares as they are currently not returned at all.

Also note that the remote share mounts do not appear either under the "External storage" section, and I think it is correct.

@owncloud/designers @schiesbn @icewind1991

Setting as "Enhancement", might not have enough time for OC 7.

@PVince81
Copy link
Contributor Author

Setting to OC 8.

FYI @karlitschek @craigpg

@PVince81 PVince81 added this to the ownCloud 8 milestone Jun 20, 2014
@jancborchardt
Copy link
Member

Yes, the external shares should appear in the »Shared with you« section to seamlessly integrate with your instance.

@PVince81
Copy link
Contributor Author

@schiesbn the OCS share API should be modified to also return remote shares for this to work.

@PVince81 PVince81 modified the milestones: 8.0-current, ownCloud 8 Dec 19, 2014
@MorrisJobke
Copy link
Contributor

@PVince81 @DeepDiver1975 @schiesbn I guess this won't make it into 8.0? Postpone to 8.1?

@PVince81
Copy link
Contributor Author

PVince81 commented Jul 6, 2015

Slightly related: adding a panel for pending/accepted shares #13495

@rullzer
Copy link
Contributor

rullzer commented Jul 24, 2015

Ok so there are a few issues here.

  • The getItems code is a real beast... just look at is...
  • In order for this to work we need to join with the oc_share_external table but a lot of info that is present in regular shares is not present in there. So the code won't work with at least some modifications.

@rullzer
Copy link
Contributor

rullzer commented Sep 25, 2015

Consider the following situation: userA@serverA shares a file with userB@serverB

The OCS Share API endpoint .../shares does only look into the oc_shares table. This means that you find remotes shares there but only for userA since he initiated the share and thus it is in his oc_share table.

For userB this means that there is an entry in the oc_share_external. Now we do have the OCS endpint .../remote_shares but this only lists the "open" shares and allows you to accept or decline those shares. I think this endpoint should stay the way it is.

The default OCS Share Endpoint should just also include entries from oc_share_external. However adding this now has the potential to break a lot of stuff. Since data that is now returned by the endpoint is

  1. not properly documented
  2. a whole lot of data which we do not know how it is used by the consumers of the endpoint

I think it is probably best to defer this to 9.0 so we can properly think about how to handle this. Else it will be a hacky solution that we have to support.

CC: @cmonteroluque

@PVince81
Copy link
Contributor Author

also @MTRichards probably a wanted feature ?
The other alternative is to implement another section to accept/reject shares: #13495
But it might not be 100% the same, might still be good to have them listed under "Shared with you" once accepted.

@schiessle
Copy link
Contributor

@PVince81 I think that's not the same. Shares are in the "Pending shares" list as long as you didn't accept them or if you unshared it from yourself. On the other hand the "shared with you" section should contain all shares you already accepted.

@ghost
Copy link

ghost commented Sep 25, 2015

@PVince81 I think the idea of having rejected/pending shares makes a lot of sense (and is definitely 9.0 at the earliest at this point). @rullzer Your concerns are very valid, but what worries me is that this situation may be more common than just for this endpoint and we could find ourselves too scared to make appropriate changes. I would believe that accepted shares should be in the OCS Share Endpoint under any circumstance and if that breaks some other code one could argue that this is a bug (caused by our implementation so far, if you will). I'd be inclined to adding the entries, but I will yield if the expectation is that this is too dangerous a change this late. @DeepDiver1975 @karlitschek @MTRichards what do you guys think?

@schiessle
Copy link
Contributor

@rullzer yes, at the moment the OCS accecpt/decline was designed to send accept/decline messages from the recipient to the sender. That's why the end point looks in the oc_share table and not in the shares_external table.

But maybe that's not a problem. If we have a nice interface to accept/decline shares and the "pending shares" menu we might extend this concept to local shares as well. So the "internal" accept/decline api would be located in the sharing 2.0 OCS API while the accept/decline API to send it over to the other server would stay in the OCS federated sharing API as it is today.

@rullzer
Copy link
Contributor

rullzer commented Sep 25, 2015

@schiesbn I'm not saying it is a problem. I just stated how things work currently. Which means we do not have a proper way to list incoming remote shares (once they are accepted).

I agree that with sharing 2.0 we could introduce the possibility to accept local shares as well. That should be a lot easier then if we have proper interfaces.

@cmonteroluque I fully agree that we need to make changes at some point. But I think in the current state the best we can go (to get this in) is a quick hack and then hope that nothing seriously breaks. With sharing 2.0 we need to break sharing partly anyway and then we can fix this in a proper way.

@rullzer
Copy link
Contributor

rullzer commented Sep 25, 2015

having said that. The "cleanest" hack I see is to create yet another OCS endpoint that just returns all the accepted remote shares.

This could then be an additional call that is made in the shared with you section. And we just merge the results there. Since it is a new and additional endpoint we don't have to fear for 3rdparty stuff breaking.

How does that sound?

@ghost
Copy link

ghost commented Sep 25, 2015

It sounds right. Over complicates the API but you're absolutely right that it reduces risk. I am ok with that. Thank you!

@rullzer
Copy link
Contributor

rullzer commented Sep 26, 2015

So after a night sleep I was thinking about the following modifications to the API. This so we don't have to break the OCS Share endpoints to much for version 2.

Current

../shares: List internal shares and outgoing remote shares, most behaviour is somewhat documented...
../remote_shares: Lists pending remote shares and allows to accept/decline incomming federated shares

New

../shares: stays the way it is for now. For sharing 2.0 and OCS Share API v2 this should include remote shares as well
../remote_shares: Lists incomming remote shares, and allows users to unshare them via the API
../pending_shares: List pending incomming remote shares. And allows the user to accept/decline them.

This would basically mean that we move the "current" ../remote_shares to ../pending_shares and introduce a new ../remote_shares endpoint. We should be able to savely do this since ../remote_shares was introduced in 8.2

The main advantage I see here is that for OCS Share v2 we could merge ../shares and ../remote_shares but we could even keep ../remote_shares to just list federated shares.
And ../pending_shares is more generic. Which would allow for accepting/rejecting incomming local shares as well (if we chose to implement that).

I'll try to come up with a quick PR for this this weekend. Should not be to big since most of the code is already available.

@rullzer
Copy link
Contributor

rullzer commented Sep 26, 2015

@cmonteroluque new endpoint in #19386

@PVince81 could you help out with the js stuff. since that currently is not really ready for 2 ajax calls :(

@ghost
Copy link

ghost commented Sep 26, 2015

@rullzer Thank you! I think the modifications are good. @karlitschek any objections? @carlaschroder fyi

@karlitschek
Copy link
Contributor

no objections :-) Makes total sense :-) 👍

@PVince81
Copy link
Contributor Author

Please note that if an endpoint is OCS and already public, you cannot change it directly in case there are already clients out there using it.

@PVince81
Copy link
Contributor Author

@carlaschroder
Copy link

@cmonteroluque @rullzer Is this a change that concerns developers or ownCloud admins? I read the whole discussion twice, and it seems to be for devs. Except, perhaps, a note somewhere that federated shares appearing in Shared With You is the correct behavior.

@PVince81
Copy link
Contributor Author

I'd vote for moving this to 9.0, especially regarding API changes.

@ghost
Copy link

ghost commented Sep 30, 2015

@PVince81 the issue is that it's a clear bug and the changes proposed are small. I hate leaving this as it is and remote shares is an 8.2 interface anyway. What's the API change for existing implementations? Not having released 8.2 already means we have more flexibility than if we had already shipped.

@MorrisJobke
Copy link
Contributor

Please note that if an endpoint is OCS and already public, you cannot change it directly in case there are already clients out there using it.

@PVince81 #19386 (comment)

@PVince81
Copy link
Contributor Author

PVince81 commented Oct 1, 2015

Ok, fine by me.

@rullzer
Copy link
Contributor

rullzer commented Oct 1, 2015

Start of fix in #19514

@rullzer
Copy link
Contributor

rullzer commented Oct 2, 2015

Ok this is almost done.

The following is now happening.

#19386:
GET remote_shares: list accepted incomming remote shares
GET remote_shares/<ID>: get info about remote share with id
DELETE remote_shares/<ID>: delete remote share with id
GET remote_shares/pending: list pending incomming remote shares
PUT remote_shares/pending/<ID>: accept remote share with id
DELETE remote_share/pending/<ID>: decline remote share with id

So it is all back under remote_shares since @nickvergessen pointed out that if we only have pending_shares we should have unique id's. Which is not true currently if we start mixing local and remote shares. So this seemed cleaner.

Once #19386 is merged #19514 can be merged which is the frontend side of things to actually display the remote share.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants