Respect empty `expiryDate` value in server #44485

Fenn-CS · 2024-03-26T13:06:30Z

If expireDate is an empty string and not null then the server should not set a default.

Resolves : #44219

github-actions · 2024-04-30T01:45:06Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

Fenn-CS · 2024-04-30T10:14:30Z

Related changes : #44912

Fenn-CS · 2024-04-30T11:22:31Z

/backport to stable29

Fenn-CS · 2024-04-30T11:22:42Z

/backport to stable28

Fenn-CS · 2024-04-30T11:22:50Z

/backport to stable27

lib/private/Share20/Manager.php

nickvergessen · 2024-04-30T11:45:15Z

lib/private/Share20/Manager.php

@@ -487,25 +417,6 @@ protected function validateExpirationDateLink(IShare $share) {
 			}
 		}

-		// If expiredate is empty set a default one if there is a default


but you are removing the default functionality?

Yes :)... Since the users now have the opportunity to modify this settings before a share is created. i.e the user can see on the share creation menu, that an expiry date is required and can either allow the default or remove if not enforced.

Leaving this default functionality means, we always force shares to have expiry dates even when that's not the users intention.

The problem is that currently existing clients and app might rely on the default and not populate it via the UI or send it with each request.
So this is a breaking change that is impossible to discover and will lead to a lot of shares not expiring although they should.

I see. This breaking change is inevitable if we would solve #44219 though.

I am not sure but perhaps I could introduce checks for clients/app calls.

Another though is that Manager class is only part of the private API and maybe all we need to do is make sure the apps we maintain that uses the non public share interface are updated?

Another though is that Manager class is only part of the private API and maybe all we need to do is make sure the apps we maintain that uses the non public share interface are updated?

I meant apps like Android, iOS and our Desktop Client:

server/apps/files_sharing/lib/Controller/ShareAPIController.php

Line 574 in 490f545

string $expireDate = '',

server/apps/files_sharing/lib/Controller/ShareAPIController.php

Lines 648 to 656 in 490f545

//Expire date

if ($expireDate !== '') {

try {

$expireDateTime = $this->parseDate($expireDate);

$share->setExpirationDate($expireDateTime);

} catch (\Exception $e) {

throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD'));

}

}

That being said, the update also supports it, but basically you need to set it to '' (empty string) not null when updating a share and removing a expiration date.

Seems to work when updating, just not when creating, as the check is always executed then.
So instead of tampering with validateExpirationDateInternal like this, I would try to get the information into the share object that no expirationDate was sent and in

server/lib/private/Share20/Manager.php

Line 423 in ec5133b

if ($fullId === null && $expirationDate === null && $defaultExpireDate) {

instead of only checking $expirationDate === null you also check whether it was sent with the empty string, and only set the default date when it was not sent as empty string.

Thanks @nickvergessen resorted to something similar, makes plenty of sense.

lib/public/Share/IShare.php

If `expireDate` is an empty string and not `null` then the server should not set a default. Signed-off-by: fenn-cs <fenn25.fn@gmail.com>

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>

- Verify that explicitly sending empty `expireDate` param to server overwrite default and sets not expiry date, if non is enforced. - Update tests to avoid converting empty string to date. Signed-off-by: fenn-cs <fenn25.fn@gmail.com>

Addressed

ShGKme · 2024-05-28T17:17:55Z

This breaks uploading in Talk which expects $expireDate to be string

blizzz · 2024-05-28T20:16:58Z

This breaks uploading in Talk which expects $expireDate to be string

Also on 27?

Antreesy · 2024-05-29T07:30:11Z

Also on 27?

Confirm for stable27

lib/public/Share/IShare.php

Fenn-CS added 2. developing Work in progress feature: sharing 27-feedback 28-feedback labels Mar 26, 2024

Fenn-CS self-assigned this Mar 26, 2024

Fenn-CS mentioned this pull request Mar 26, 2024

enh: Send empty expireDate when not expireDate set #44291

Merged

susnux added this to the Nextcloud 30 milestone Apr 18, 2024

susnux mentioned this pull request Apr 29, 2024

[Bug]: When creating email share expiry date is set by default without enforcement #44219

Closed

9 tasks

github-actions bot added the feedback-requested label Apr 30, 2024

Fenn-CS force-pushed the 44219-share-server-respect-empty-expiry-date branch 2 times, most recently from be0e0fc to dbb802a Compare April 30, 2024 10:54

Fenn-CS changed the title ~~WIP : Respect empty expiryDate value in server~~ Respect empty expiryDate value in server Apr 30, 2024

Fenn-CS marked this pull request as ready for review April 30, 2024 11:20

Fenn-CS added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 30, 2024

Fenn-CS requested review from nickvergessen, icewind1991, susnux, Altahrim and artonge April 30, 2024 11:20

backportbot bot added the backport-request label Apr 30, 2024

github-advanced-security bot found potential problems Apr 30, 2024

View reviewed changes

lib/private/Share20/Manager.php Fixed Show fixed Hide fixed

nickvergessen reviewed Apr 30, 2024

View reviewed changes

Fenn-CS force-pushed the 44219-share-server-respect-empty-expiry-date branch 2 times, most recently from 9effd09 to e6a6340 Compare April 30, 2024 12:31

Fenn-CS requested review from provokateurin and removed request for provokateurin May 23, 2024 12:46

Fenn-CS force-pushed the 44219-share-server-respect-empty-expiry-date branch from 30e59ba to a47d7b0 Compare May 23, 2024 12:49

come-nc reviewed May 23, 2024

View reviewed changes

lib/public/Share/IShare.php Outdated Show resolved Hide resolved

Fenn-CS force-pushed the 44219-share-server-respect-empty-expiry-date branch 2 times, most recently from 2a233b1 to 8dd6554 Compare May 23, 2024 13:08

Fenn-CS added 3 commits May 23, 2024 14:11

fix(shareManager): Respect empty expireDate in server

59a5b4e

If `expireDate` is an empty string and not `null` then the server should not set a default. Signed-off-by: fenn-cs <fenn25.fn@gmail.com>

refactor(shareManager): Use constructor property promotion

f5a433f

Signed-off-by: fenn-cs <fenn25.fn@gmail.com>

Fenn-CS force-pushed the 44219-share-server-respect-empty-expiry-date branch from 8dd6554 to d41d885 Compare May 23, 2024 13:11

come-nc approved these changes May 23, 2024

View reviewed changes

artonge merged commit 500a9cb into master May 23, 2024
155 checks passed

artonge deleted the 44219-share-server-respect-empty-expiry-date branch May 23, 2024 14:40

This was referenced May 23, 2024

[stable27] Respect empty expiryDate value in server #45481

Merged

[stable28] Respect empty expiryDate value in server #45482

Closed

[stable29] Respect empty expiryDate value in server #45483

Merged

backportbot bot removed the backport-request label May 23, 2024

ShGKme mentioned this pull request May 28, 2024

fix(attachments): allow null expireDate in createShare nextcloud/spreed#12432

Closed

4 tasks

This was referenced May 29, 2024

fix(ShareApiClients): Maintain meaning of '' and null for share api cli… #45562

Merged

[stable28] Respect empty expiryDate value in server #45604

Merged

nickvergessen reviewed Jul 18, 2024

View reviewed changes

lib/public/Share/IShare.php Show resolved Hide resolved

nickvergessen added the pending documentation This pull request needs an associated documentation update label Jul 18, 2024

Fenn-CS mentioned this pull request Jul 21, 2024

chore: Add notes for noExpirationDate flag addition to share API nextcloud/documentation#12051

Open

Fenn-CS removed the pending documentation This pull request needs an associated documentation update label Jul 21, 2024

blizzz mentioned this pull request Jul 24, 2024

30.0.0 beta 1 #46713

Merged

danxuliu mentioned this pull request Aug 7, 2024

If a default expiration date is set, public shares cannot be created via OCS without an expiration date #10178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect empty `expiryDate` value in server #44485

Respect empty `expiryDate` value in server #44485

Fenn-CS commented Mar 26, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024

Fenn-CS commented Apr 30, 2024

Fenn-CS commented Apr 30, 2024

Fenn-CS commented Apr 30, 2024

Fenn-CS commented Apr 30, 2024

nickvergessen Apr 30, 2024

Fenn-CS Apr 30, 2024

nickvergessen Apr 30, 2024

Fenn-CS Apr 30, 2024

nickvergessen Apr 30, 2024

Fenn-CS Apr 30, 2024

ShGKme commented May 28, 2024

blizzz commented May 28, 2024

Antreesy commented May 29, 2024

	//Expire date
	if ($expireDate !== '') {
	try {
	$expireDateTime = $this->parseDate($expireDate);
	$share->setExpirationDate($expireDateTime);
	} catch (\Exception $e) {
	throw new OCSNotFoundException($this->l->t('Invalid date, date format must be YYYY-MM-DD'));
	}
	}

Respect empty expiryDate value in server #44485

Respect empty expiryDate value in server #44485

Conversation

Fenn-CS commented Mar 26, 2024 • edited Loading

github-actions bot commented Apr 30, 2024

Fenn-CS commented Apr 30, 2024

Fenn-CS commented Apr 30, 2024

Fenn-CS commented Apr 30, 2024

Fenn-CS commented Apr 30, 2024

nickvergessen Apr 30, 2024

Choose a reason for hiding this comment

Fenn-CS Apr 30, 2024

Choose a reason for hiding this comment

nickvergessen Apr 30, 2024

Choose a reason for hiding this comment

Fenn-CS Apr 30, 2024

Choose a reason for hiding this comment

nickvergessen Apr 30, 2024

Choose a reason for hiding this comment

Fenn-CS Apr 30, 2024

Choose a reason for hiding this comment

ShGKme commented May 28, 2024

blizzz commented May 28, 2024

Antreesy commented May 29, 2024

Respect empty `expiryDate` value in server #44485

Respect empty `expiryDate` value in server #44485

Fenn-CS commented Mar 26, 2024 •

edited

Loading