Properly send expiration date with all share.php calls

[PVince81]

There are three share.php calls for link shares:

• when changing password
• when setting expiration date
• when switching "allow public upload"

This fix makes sure that ALL the required info is sent for each of these
calls.

It fixes an issue where the expiration date was not passed when changing
the password.

Fixes #11671

TODO:

• Fix/finish failing JS unit test

[PVince81]

Please review @schiesbn @nickvergessen @icewind1991

To test:
Make sure that any combination of "password", "allow public upload" and "expiration date" are all saved whenever one of them is changed. (the bug was that expiration date did NOT get saved when changing the password)

[karlitschek]

looks good 👍

[schiessle]

I would recommend to fix it on the server side. We should probably remember here the expire date in addition to the permissions and the share token https://github.com/owncloud/core/blob/master/lib/private/share/share.php#L621. This way it also works if the password get set from the OCS API or through a different code path.

[PVince81]

I think it's still valid to re-send the expiration date for every call, because we also need to re-send "allowUpload" and the password every time... so I suggest we keep the current mechanism.

From the OCS API if you send an empty expiration date it should probably clear the date anyway ?

[PVince81]

It seems that it is expected to be able to send an empty string for expiration date, this is the case for example if you uncheck "Set expiration date". So adding code to the server to keep the old date would break that case.

[schiessle]

But if I send only the password because I want to set a password, then I don't want to clear the expire date.

It is bad anyway to delete the share and then re-create it. But as long as it works this way we should make sure that we remember all additional values.

Resending "allowUpload" shoudn't be necessary, because as you can see at the code I referenced we remember the old permissions before we update the share

[schiessle]

It seems that it is expected to be able to send an empty string for expiration date

Sending an empty expire date is something different than sending no expire date

[PVince81]

Ideally it should work as follows:

1. If a value is set explicitly, for example "key=value", then set key to value
2. If an empty still is set explicitly, for example "key=", then that key needs to be cleared
3. If "key" is omitted, the value should be left unchanged.

You were probable referring to 3) but the current code works like 2).
We can eventually change the JS code to work like 2) once we're sure that the server call supports that for all three values: "expirationdate", "shareWith" (for the password) and "allowUpload"

[PVince81]

I mean the JS code, so far, always working like 2): it was sending an empty string for "expirationdate".
You are suggesting to not send expirationdate at all, is that correct ?

[schiessle]

This is how the code works today, server side. Beside that it seems to ignore the expire date. For example if you set a password with the share API we also don't send a expire date along with it. So this would most likely still be broken. If you fix it on the server side we can make sure that it works in all cases.

[PVince81]

I'd prefer keeping this a pure JS fix for now. It does fix the problem and isn't unclean either.

If we start touching the server and changing the APIs it will require much more regression testing.

[schiessle]

Did you tested it with the OCS API? I'm quite sure you will have the same problem if you set a password with it. That's why I would prefer to have the fix as deep as possible in the stack, otherwise we have to "fix" all entry points.

[PVince81]

The OCS API docs here http://doc.owncloud.org/server/7.0/developer_manual/core/ocs-share-api.html#update-share says that only one parameter is allowed at a time.

And when I try to set a new password it will create a new share, and this new share has lost the expiration date I initially set on it.
I tried again in the web UI and setting the password indeed creates a new share.
This is a known bug #10671, but thanks to the fix here the expiration data is still sent as well so it is still kept.

I did another test with share.php: if I only pass "allowPublicUpload" without expiration date, even though one is set, then that one will also be lost.
With the OCS API it seems to work fine: passing "publicUpload" does not lose the expiration date.

The problem with the OCS API is that the doc states that you can only pass one parameter at a time. This means that it is impossible to update all parameters if they all recreate a new share (unless that is only for the password).

I think the ideal approach would be as specified before, for both the OCS API and the internal share.php:

1. if a parameter is specified, set it.
2. if a parameter is not specified, leave the value that we had before
3. if a parameter is specified with no value, remove that value from the DB

This means that the following needs to be done to be consistent:

• fix share.php to not remove value if no parameter of the name was passed
• make OCS API more tolerant and allow passing multiple values
• optionally: change this PR to only send the required info

[PVince81]

Actually in proper REST terms, a PUT call is supposed to get the FULL object with all attributes. This means that if attributes are omitted, they should be cleared.
To be able to only change a few parameters, one needs to use the PATCH method that accepts a diff.
See http://restful-api-design.readthedocs.org/en/latest/methods.html#patch-vs-put for more info.

[PVince81]

It seems IE8 doesn't support the PATCH method.
For the web UI we could still go with the standard PUT and pass all values every time like this PR does... there aren't that many so it should be fine. The question is rather for the OCS API, to have some kind of consistency.

[MorrisJobke]

@PVince81 Any news here? Is this fixable for ownCloud 8? #11671 is the issue for this

[ghost]

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

[PVince81]

Or convince @schiesbn to accept this PR first and work on the rest later ? 😉

[DeepDiver1975]

Generally speaking I'm all in for fixing this properly on server side - but the discussion shows that this is far from being easy. Let's review and test this work around and try to keep in mind that we need to refactor the server side in OC8.x

[MorrisJobke]

Ping

[PVince81]

@schiesbn your call

[DeepDiver1975]

let's attack this issue in 8.1 again

[PVince81]

@schiesbn ok to merge now ?

[LukasReschke]

Rebase required.

[PVince81]

Rebased. Fortunately it was just a conflict in the test file.

[ghost]

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

[scrutinizer-notifier]

The inspection completed: 1 new issues

[MorrisJobke]

@PVince81 Can you coordinate this with @schiesbn once he is back? Thanks :)

[MorrisJobke]

@schiesbn @PVince81 Ping

[PVince81]

Deferred to 8.2

[ghost]

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

[PVince81]

I guess we can hold this until the share dropdown is using OCS or whatever API ? @schiesbn @rullzer

[PVince81]

Here is the PR that makes the dropdown use OCS Share calls: #17143

[rullzer]

@PVince81 fine with me. Reminds me that I need to look into that PR a bit more ;)

[MorrisJobke]

Obsoleted by share API move to OCS