Allow setting expire on user / group shares via API

[butonic]

We have users that have been using the API since OC5 to set expire dates on user shares. This PR restores that functionality. no change is made in the web ui. That is tracked in #11642

[nickvergessen]

If a user has a share entry due to the name collision on a group share, it seems to be not correctly updated here

[butonic]

@nickvergessen can you give an examples which method / parameter configuration? or just add a unit test that demonstrates the wrong update.

[nickvergessen]

Well @butonic shouldn't you add unit tests to prove it works when trying to introduce a new fancy functionality 😜
Talked to @PVince81 at the lunch break and he also sees multiple possible pitfalls with this: we need to make sure that the expire logic and other stuff is called for all shares now, as well as server2server shares.

This also cries for new additions to the test plans of QA

[MorrisJobke]

cc @rullzer

[karlitschek]

@schiesbn @MTRichards We need your input on the correct behavior here. Do we want to have expiration of internal shares? And if yes is it ok to have it only via the API. And if yes. Is this a long term maintainable code path if it doesn´t exist in the GUI?

[PVince81]

This was already requested here: #11642

[MTRichards]

Do we want to have expiration of internal shares?

As discussed in the past, yes - I would like it. It is not super urgent, but a nice to have - and since we removed it in a past oC version, I do want it back. As we focus on financial services, this is more important than it was.

And if yes is it ok to have it only via the API.

I am not super happy about only via the API, since this is not really useful to the end user. Only this use case can work with the API directly, everyone else would want / need the GUI. This might be a step 1 to get to the GUI again using the API, then OK.

And if yes. Is this a long term maintainable code path if it doesn´t exist in the GUI?

Not really, nor is it all that useful outside of this customer. See above.

[butonic]

@PVince81 @schiesbn aren't we planning on using the same API for web / ajax requests?

[rullzer]

@butonic yes we are! Basically the ajax endpoints have to die as they lead to code duplication and they even use different arguments and such. So once we have it in the API we can also implement it in the new shiny sharing dialog.

Having said that it is still a long way to go for the new sharing dialog. I'll have a look at the code later.

[rullzer]

I would suggest to move this to 8.2 as it might have implications we can't think of before the feature freeze. And only exposing it in the API might not be the best UX.

[PVince81]

One of the difficult parts is the cram this all in the existing share dropdown.
It would be better to have the share dropdown/popup redesign go in first #5873 or at least keep this enhancement in mind when redesigning the dialog.

[nickvergessen]

Setting to develop, because this needs tests prior to merge

[schiessle]

Back-end support is easy. The tough question is how to expose the feature to the user in the share dialog. Because we would need a date-picker for every share.

I would suggest to address this issue with 8.2, maybe with a re-design of the share dialog as suggested by @PVince81

[DeepDiver1975]

I would suggest to address this issue with 8.2, maybe with a re-design of the share dialog as suggested by @PVince81

that's for sure the right things to do ...

@butonic is it enough from a customer support issue if you apply this patch manually at the customers having this issue? THX

[MorrisJobke]

@butonic that needs some love

@rullzer @schiesbn Maybe you are also interrested in this.

[scrutinizer-notifier]

A new inspection was created.

[ghost]

💣 Test FAILed. 💣

[MorrisJobke]

@butonic Is this ready for review? If not: What needs to be done? If you aren't able to fix this, maybe add a checklist so somebody can take over this.

[PVince81]

@butonic are expiration dates supposed to be inherited when resharing ? Because it seems this never worked due to this bug: #11396 (comment)

[PVince81]

Also, setting the date is not enough. You'd also need to fix the expiration logic to kick in for user shares too, see https://github.com/owncloud/core/blob/master/lib/private/share/share.php#L1329

[PVince81]

and if you enable this, then there will be another bug (#11396 (comment)) that will expire the wrong shares, because "share.js" already sends expirationDate for user shares when it's not supposed to. Quite a mess 😦

[butonic]

Interesting. I think they should, but probably not by adding the expire date to the reshare in any form. if the parent share expires the reshare should no longer work. but it should be possible tho create a reshare with a shorter expiration period, eg share e folder 'Project X' to a group with expire date set to in 4 month. A member of that group creates the subfolder 'Project X/Task Y' and creates a public link with expire date set to in 45 days.

[PVince81]

so basically before this can be made to work we need to:

• fix Can't reshare link with default expiration date #11396 (comment) in a way that still allows setting expirationDate on user shares, but without the reshare bug (properly convert the inherited date from String to DateTime here: https://github.com/owncloud/core/blob/master/lib/private/share/share.php#L2214 but also clarify the expected behavior for reshares Allow setting expire on user / group shares via API #15459 (comment))
• fix expireItem() function to allow expiring user share
• IMPORTANT: run a repair step before this feature is introduced to make sure all the expiration dates that were set by that older bug are cleared first, see Repair step to remove bogus expiration dates from non-link shares #19132

[PVince81]

I think this should rather wait for the sharing code revamp that is planned for 9.0, see #13014.

Then at this time if we want to support user share expiration, even if there is no UI for it, it should get scheduled/planned/designed too @cmonteroluque

Also there is a chance that the re-share behavior is changed in #9058 which might obsolete what was mentionned above.

[ghost]

@PVince81 ok, thanks

[MorrisJobke]

I think this should rather wait for the sharing code revamp that is planned for 9.0, see #13014.

@rullzer @schiesbn @cmonteroluque What is the status of this?

[rullzer]

@MorrisJobke we are first porting as much existing behaviour as we can before we introduce new stuff. But once that is done this should not be to hard.

[PVince81]

@rullzer something to add to the next steps ?

I'm surprised this didn't conflict with the many changes done to the share code.

[rullzer]

@PVince81 yes worth to think about! Should be relatively easy now...

There is no conflict because it is all in the old code ;)

[karlitschek]

This PR is agains an outdated code base. Let's consider this again once we redo the sharing dialog.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.