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

[For 10.4] Include expiration option for user and group shares #36531

Closed
wants to merge 26 commits into from

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Dec 6, 2019

Description

This is a re-submit of PR #36324 with the commits squashed and rebased to the current master (that has "drop PHP 7.0" and phpunit7).

Please work on top of this in a separate branch for making acceptance tests or fixing bugs. Then we need to cherry-pick tests and bugs fixes into here in a controlled manner so that we do not write over each other's work.

Excerpt from the feature file:

  • As a user I want to set an expiration date for collaborators in order to stay in control of access permissions by giving time-based access instead of granting access forever or until I manually remove the collaborator.
  • As a user I want to automatically limit my responsibility for a guest account. Guests are bound to being a collaborator. If they are not a collaborator of any resource, they can't log in anymore.
  • As an admin I want to set a default expiration date without enforcement to give a recommendation to users and help them maintain control over their resources. Still, I don't want to force or annoy users as some may have reasons to collaborate on resources for a longer or unrestricted amount of time, e.g., in projects.
  • As an admin I have to follow corporate policies which dictate that data exchange between employees or also with externals may only take place for a certain maximum amount of time to prevent data leaks by human mistake or negligence.

Related Issue

How Has This Been Tested?

CI acceptance tests added for:

  • new capabilities added in capabilities API
  • setting/clearing the new user and group expiration settings in the admin sharing settings UI page
  • sharing with users and groups with/without expiration date on the UI
  • using the sharing API to directly share/reshare/change share with default or specified expiry dates

Screenshots:

Anmerkung 2019-11-11 133034

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@codecov
Copy link

codecov bot commented Dec 6, 2019

Codecov Report

Merging #36531 into master will decrease coverage by 0.01%.
The diff coverage is 68.93%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36531      +/-   ##
============================================
- Coverage     64.66%   64.64%   -0.02%     
- Complexity    19049    19078      +29     
============================================
  Files          1269     1269              
  Lines         74497    74668     +171     
  Branches       1311     1322      +11     
============================================
+ Hits          48170    48269      +99     
- Misses        25941    26007      +66     
- Partials        386      392       +6
Flag Coverage Δ Complexity Δ
#javascript 54.03% <55.76%> (+0.01%) 0 <0> (ø) ⬇️
#phpunit 65.82% <72.67%> (-0.02%) 19078 <14> (+29)
Impacted Files Coverage Δ Complexity Δ
lib/private/Share20/ProviderFactory.php 85.36% <0%> (-6.74%) 17 <1> (+1)
core/js/config.php 0% <0%> (ø) 0 <0> (ø) ⬇️
...ederatedfilesharing/lib/FederatedShareProvider.php 62.41% <0%> (-0.28%) 88 <1> (+1)
...es_sharing/lib/Controller/Share20OcsController.php 93.73% <100%> (-0.03%) 206 <0> (ø)
settings/Panels/Admin/FileSharing.php 98.66% <100%> (+0.11%) 7 <0> (ø) ⬇️
settings/templates/panels/admin/filesharing.php 19.23% <20%> (+0.18%) 0 <0> (ø) ⬇️
core/js/sharedialogshareelistview.js 73.79% <34.78%> (-7.36%) 0 <0> (ø)
core/js/shareitemmodel.js 80.26% <66.66%> (-0.33%) 0 <0> (ø)
apps/files_sharing/lib/Capabilities.php 93.15% <71.42%> (-5.19%) 12 <1> (+2)
lib/private/Share20/DefaultShareProvider.php 97.9% <72.72%> (-0.46%) 120 <1> (+2)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f118d23...17d3281. Read the comment docs.

@phil-davis
Copy link
Contributor Author

Fix for the "group expiration only" problem have been merged from PR #36535
Next is to get the remaining acceptance tests sorted out.

@individual-it
Copy link
Member

Without the administrator setting a "default" expiration date, the user cannot set any expiration date at all. This is different to public link shares, where the user can always set an expiration date, whether a default is set of not.
For my logic there should be 3 different options:

  1. enable expiration dates
  2. set default expiration date
  3. enforce default expiration date as maximum expiration date

currently 1&2 are combined

maybe a renaming of the option would also do

Add api test for setting expiration date for resharing to user and groups
@jvillafanez
Copy link
Member

Without the administrator setting a "default" expiration date, the user cannot set any expiration date at all. This is different to public link shares, where the user can always set an expiration date, whether a default is set of not.

Backend behaviour should be the same for link, user and group shares. It seems a frontend problem to me.

What I understand is:

  • "enable expiration dates" will show or hide the date picker
  • if there is as default expiration date set, the date picker should choose / show that option if possible, but it must allow the user to choose any date. If there isn't a default expiration date set, probably the easiest option is not to do anything, just show the date picker without any particular date selected.
  • if the default is enforced, the date picker shouldn't allow to choose dates further than the one set (if possible), but the user should be able to choose any date between the current date and the default date

There is a corner case: no default expiration set, but enforced. As far as I know, this shouldn't happen. Worst case should be to enter either "0" days or "-1" days, but other than immediate expiration, it shouldn't be a problem (we might need to double-check this)

@phil-davis
Copy link
Contributor Author

I have squashed and rebased this code in PR #36573 (I did not want to lose anything here)

We can do final reviews and discussion of outstanding issues there.

@phil-davis phil-davis closed this Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants