Skip to content

Check expiry enforcement for all share types #40933

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.

Sign up for GitHub

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

Jump to bottom
Merged
come-nc merged 3 commits into from
Oct 23, 2023
Merged

Check expiry enforcement for all share types #40933

come-nc merged 3 commits into from
Oct 23, 2023

Conversation

@nfebe
Copy link
Contributor

@nfebe nfebe commented Oct 16, 2023

We should check that a maximum expiry date has been enforced for all shares and NOT JUST FOR internal shares before enforcing a UI max date, like in commit 9757e68

Resolves : no issue

@nfebe nfebe added 3. to review Waiting for reviews feature: sharing labels Oct 16, 2023
@nfebe nfebe added this to the Nextcloud 27.1.4 milestone Oct 16, 2023
@nfebe nfebe requested review from come-nc, sorbaugh and szaimen October 16, 2023 14:29
This was referenced Oct 16, 2023
Merged
Merged
@nfebe nfebe force-pushed the check-datemax-enforcement-pubshare branch from bbc12c4 to 8ef3748 Compare October 16, 2023 14:50
@nfebe nfebe requested review from artonge and susnux October 16, 2023 15:08
@nfebe nfebe force-pushed the check-datemax-enforcement-pubshare branch from 8ef3748 to 3f51b16 Compare October 16, 2023 15:28
come-nc
come-nc reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Enforcing now works for all three kind of shares.

But default seems broken for remote share it’s not applied?

@nfebe
Copy link
Contributor Author

nfebe commented Oct 16, 2023

Enforcing now works for all three kind of shares.

But default seems broken for remote share it’s not applied?

Added commit to fix it. Which also turns addresses and issue that existing of not being able to not set default if desired. Thanks @come-nc

@nfebe nfebe force-pushed the check-datemax-enforcement-pubshare branch 2 times, most recently from 2eda6db to 5840ca5 Compare October 17, 2023 07:57
@nickvergessen nickvergessen requested a review from come-nc October 17, 2023 09:56
@szaimen szaimen removed their request for review October 17, 2023 11:00
@come-nc
Copy link
Contributor

come-nc commented Oct 19, 2023

Hum, forced default expiration date now works for all cases, but for default value it uses the normal share default value for all 3 cases, even email and remote share, which should use their own default.

come-nc
come-nc reviewed Oct 19, 2023
View reviewed changes
come-nc
come-nc reviewed Oct 19, 2023
View reviewed changes
@nfebe nfebe force-pushed the check-datemax-enforcement-pubshare branch 5 times, most recently from 1fd800a to 420c0d9 Compare October 19, 2023 17:35
@nfebe nfebe requested a review from come-nc October 19, 2023 17:35
@come-nc
Copy link
Contributor

come-nc commented Oct 20, 2023

I had to run «npm run build» to fix the admin UI, so something is fishy in the commited dist. You will have to rebase on master and rebuild anyway.

Also, there is a problem:

  1. Set a default expiration date on all 3 types
  2. Force it for email share and not for the others
  3. Share with a user
  4. Date is enforced while it should not

Please test thoroughly requesting for review again.

come-nc
come-nc requested changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Enforcing is not working correctly (enforcing email enforces for local share, did not test other combinations)

@nfebe
Copy link
Contributor Author

nfebe commented Oct 20, 2023 •
edited
Loading

I had to run «npm run build» to fix the admin UI, so something is fishy in the commited dist. You will have to rebase on master and rebuild anyway.

Sorry @come-nc it appears, a merge conflict resolution overwrote some of the updates, during a rebase.

@nfebe
Copy link
Contributor Author

nfebe commented Oct 20, 2023 •
edited
Loading

Also, there is a problem:

Set a default expiration date on all 3 types
Force it for email share and not for the others
Share with a user
Date is enforced while it should not

Please test thoroughly requesting for review again.

About the defaultExpirationDate being enforced on all types, that was a known behavior which I added intentionally in :#40500

It is now clear that this variable was poorly named. If we have defaultRemoteExpirationDate, defaultInternalExpirationDate then it sounds like defaultExpirationDate is a general default and not defaultPublicExpirationDate so this did not appear like an off behavior but like a fallback default in case all the others are not set.

@nfebe nfebe force-pushed the check-datemax-enforcement-pubshare branch from 420c0d9 to e07d8e8 Compare October 20, 2023 09:41
@nfebe nfebe requested a review from come-nc October 20, 2023 09:44
come-nc
come-nc approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

🥳

@nfebe nfebe force-pushed the check-datemax-enforcement-pubshare branch from e07d8e8 to d6ff703 Compare October 20, 2023 12:36
@nfebe nfebe enabled auto-merge October 20, 2023 12:37
@nextcloud-command nextcloud-command force-pushed the check-datemax-enforcement-pubshare branch from d6ff703 to 4548fdb Compare October 20, 2023 12:51
@nfebe nfebe force-pushed the check-datemax-enforcement-pubshare branch from 4548fdb to a212ea9 Compare October 22, 2023 08:44
@nfebe
Copy link
Contributor Author

nfebe commented Oct 22, 2023

/compile amend /

nfebe added 2 commits October 22, 2023 15:47
@nfebe
Check expiry enforcement for all share types
92417cd 
We should check that a maximum expiry date has been enforced for
all shares and NOT JUST FOR internal shares before enforcing a UI max date,
like in commit 9757e68

Signed-off-by: fenn-cs <[email protected]>
@nfebe
Fix has expiration date logic
46308e9 
Current expiration date errorneously assumes that `defaultExpirationDate`
applies to all kinds of shares. But it only really applies to public shares despite
its name.

This commit, fixes that by paring expiration dates with the correct share types during
new share initialization and simplifying the `hasExpirationDate` (check) property.

Signed-off-by: fenn-cs <[email protected]>
@nfebe nfebe force-pushed the check-datemax-enforcement-pubshare branch from a212ea9 to 2ca8561 Compare October 22, 2023 14:48
@nfebe
Copy link
Contributor Author

nfebe commented Oct 22, 2023

/compile amend /

@nextcloud-command nextcloud-command force-pushed the check-datemax-enforcement-pubshare branch from 2ca8561 to eb0c923 Compare October 22, 2023 15:39
@nfebe nfebe force-pushed the check-datemax-enforcement-pubshare branch from eb0c923 to 2ca8561 Compare October 22, 2023 16:12
@nfebe
Copy link
Contributor Author

nfebe commented Oct 22, 2023

/compile amend /

@nfebe @nextcloud-command
Do not treat isDefaultExpireDateEnforced as fallback
8dfcf2e 
`isDefaultExpireDateEnforced` and its corresponding `defaultExpirationDate`
is currently treated as the enforcement fallback when share type enforcements are not
set.

However, `isDefaultExpireDateEnforced` and `defaultExpirationDate` are actually more like
`isDefaultPublicExpireDateEnforced` and `defaultPublicExpirationDate` and therefore only applies
to public shares.

It might be ideal to rename this variables all the way from the backend config to the way we use
them in the frontend code.

Signed-off-by: fenn-cs <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
@nextcloud-command nextcloud-command force-pushed the check-datemax-enforcement-pubshare branch from 2ca8561 to 8dfcf2e Compare October 22, 2023 16:47
@come-nc come-nc disabled auto-merge October 23, 2023 07:32
@come-nc come-nc merged commit 6114364 into master Oct 23, 2023
@come-nc come-nc deleted the check-datemax-enforcement-pubshare branch October 23, 2023 07:32
@nfebe nfebe linked an issue Oct 24, 2023 that may be closed by this pull request
[Bug]: New share dialog, can't unckek expire date #41075
Closed
8 tasks
@marcelklehr
Copy link
Member

marcelklehr commented Dec 13, 2023

Can we please backport this to stable27?

@susnux
Copy link
Contributor

susnux commented Dec 13, 2023

Looks like this is already on stable27? At least when I cherry pick it is empty. cc @fenn-cs

@nfebe
Copy link
Contributor Author

nfebe commented Dec 13, 2023

@susnux it is : #40928

@marcelklehr
Copy link
Member

marcelklehr commented Dec 13, 2023

#40928 says it's a backport of #40927 though and not of this #40933 ?

@nfebe
Copy link
Contributor Author

nfebe commented Dec 13, 2023 •
edited
Loading

@marcelklehr it has commits from both pr's.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artonge artonge artonge approved these changes

@come-nc come-nc come-nc approved these changes

@sorbaugh sorbaugh Awaiting requested review from sorbaugh

@susnux susnux Awaiting requested review from susnux

Assignees

No one assigned

Labels

3. to review Waiting for reviews feature: sharing

Projects

📁 Files team
Archived in project

Milestone

Nextcloud 28

Development

Successfully merging this pull request may close these issues.

[Bug]: New share dialog, can't unckek expire date [Bug]: share-expiration-calendar broken

7 participants

@nfebe @come-nc @marcelklehr @susnux @artonge @nickvergessen