Adjust expiration logic for shares to account for share attributes registered with apps #36766

mrow4a · 2020-01-15T01:17:33Z

related issue: #36751

also last commit fixes #36813

rollback html and css structure changes from [For 10.4] Include expiration option for user and group shares #36573 that broke compatibility with advanced share attributes that get registered by apps (example: https://github.com/owncloud/richdocuments/blob/master/js/viewer/shareoptions.js#L222-L260 and https://github.com/ONLYOFFICE/onlyoffice-owncloud/blob/master/js/share.js#L322-L372)
make sure expiration code works as expected
adjust required unit and acceptance tests
add js test to share attributes v2 checking proper rendering

jvillafanez

Code looks good

lib/private/Share20/Manager.php

core/js/shareitemmodel.js

codecov · 2020-01-15T20:22:31Z

Codecov Report

Merging #36766 into master will decrease coverage by <.01%.
The diff coverage is 72.05%.

@@             Coverage Diff              @@
##             master   #36766      +/-   ##
============================================
- Coverage     64.68%   64.68%   -0.01%     
+ Complexity    19122    19114       -8     
============================================
  Files          1269     1269              
  Lines         74779    74801      +22     
  Branches       1320     1327       +7     
============================================
+ Hits          48373    48383      +10     
- Misses        26018    26027       +9     
- Partials        388      391       +3

Flag	Coverage Δ	Complexity Δ
#javascript	`54.1% <57.14%> (-0.02%)`	`0 <0> (ø)`
#phpunit	`65.85% <96.15%> (-0.01%)`	`19114 <3> (-8)`

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Share/Share.php	`71.15% <0%> (ø)`	`501 <0> (ø)`	⬇️
...es_sharing/lib/Controller/Share20OcsController.php	`93.78% <100%> (+0.04%)`	`207 <0> (+1)`	⬆️
core/js/sharedialoglinkexpirationview.js	`76.56% <100%> (ø)`	`0 <0> (?)`
lib/private/Share20/Manager.php	`96.3% <100%> (-0.03%)`	`273 <3> (-9)`
core/js/sharedialoglinkshareview.js	`83.59% <100%> (ø)`	`0 <0> (ø)`	⬇️
core/js/shareitemmodel.js	`80.31% <100%> (ø)`	`0 <0> (ø)`	⬇️
core/js/shareconfigmodel.js	`83.67% <100%> (ø)`	`0 <0> (ø)`	⬇️
core/js/sharedialogshareelistview.js	`69.69% <40%> (-3.55%)`	`0 <0> (ø)`

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 787145b...1cee3f3. Read the comment docs.

phil-davis · 2020-01-16T10:46:08Z

I rebased and force-pushed because this was getting weird drone errors. Let's see what an up-to-date CI run says.

micbar · 2020-01-16T11:32:32Z

@phil-davis We need to make sure, that this JS api does not break again.

@mrow4a
We target a new RC2 on monday.

mrow4a · 2020-01-16T19:38:45Z

@phil-davis could you have a look now?

phil-davis · 2020-01-17T05:04:25Z

Here is a strange behaviour:

have richdocuments Secure View enabled
share something with Secure View checkbox checked, (and an expiration date set, optional)
click the x to the right of the expiration date field, to clear the expiration date

Expected behaviour: the expiration date is cleared, and all permissions checkboxes remain as they are.

Actual behaviour: the expiration date is cleared, and the Secure View checkbox gets "automagically" unchecked.

This "normal" core scenario works fine:

have richdocuments Secure View enabled
share something with "can edit", "creat", "change", "delete" checkboxes checked, (and an expiration date set, optional)
click the x to the right of the expiration date field, to clear the expiration date

The expiration date is cleared, the permissions checkboxes remain as they are - correct.

@mrow4a

lib/private/Share20/Manager.php

phil-davis · 2020-01-17T05:28:51Z

https://drone.owncloud.com/owncloud/core/22714/46/13
acceptance test fails, here I run it locally:

make test-acceptance-api BEHAT_FEATURE=tests/acceptance/features/apiShareManagementBasic/createShare.feature:427
Running apiShareManagementBasic tests tagged ~@skipOnOcV10&&[email protected]&&[email protected]&&@api&&~@skip 
@api @TestAlsoOnExternalUserBackend @files_sharing-app-required
Feature: sharing

  Background:                                                                    # /home/phil/git/owncloud/core/tests/acceptance/features/apiShareManagementBasic/createShare.feature:4
    Given using old DAV path                                                     # FeatureContext::usingOldOrNewDavPath()
    And user "user0" has been created with default attributes and skeleton files # FeatureContext::userHasBeenCreatedWithDefaultAttributes()

  Scenario: Share a file by multiple channels and download from sub-folder and direct file share                                        # /home/phil/git/owncloud/core/tests/acceptance/features/apiShareManagementBasic/createShare.feature:427
    Given these users have been created with default attributes and skeleton files:                                                     # FeatureContext::theseUsersHaveBeenCreated()
      | username |
      | user1    |
      | user2    |
    And group "grp1" has been created                                                                                                   # FeatureContext::groupHasBeenCreated()
    And user "user1" has been added to group "grp1"                                                                                     # FeatureContext::userHasBeenAddedToGroup()
    And user "user2" has been added to group "grp1"                                                                                     # FeatureContext::userHasBeenAddedToGroup()
    And user "user0" has created folder "/common"                                                                                       # FeatureContext::userHasCreatedFolder()
    And user "user0" has created folder "/common/sub"                                                                                   # FeatureContext::userHasCreatedFolder()
    And user "user0" has shared folder "common" with group "grp1"                                                                       # FeatureContext::userHasSharedFileWithGroupUsingTheSharingApi()
    And user "user1" has shared file "textfile0.txt" with user "user2"                                                                  # FeatureContext::userHasSharedFileWithUserUsingTheSharingApi()
    And user "user1" has moved file "/textfile0.txt" to "/common/textfile0.txt"                                                         # FeatureContext::userHasMovedFile()
      Failed asserting that 500 matches expected 201.
    And user "user1" has moved file "/common/textfile0.txt" to "/common/sub/textfile0.txt"                                              # FeatureContext::userHasMovedFile()
    When user "user2" uploads file "filesForUpload/file_to_overwrite.txt" to "/textfile0 (2).txt" using the WebDAV API                  # FeatureContext::userUploadsAFileTo()
    And user "user2" downloads file "/common/sub/textfile0.txt" with range "bytes=0-8" using the WebDAV API                             # FeatureContext::userDownloadsFileWithRange()
    Then the downloaded content should be "BLABLABLA"                                                                                   # FeatureContext::downloadedContentShouldBe()
    And the downloaded content when downloading file "/textfile0 (2).txt" for user "user2" with range "bytes=0-8" should be "BLABLABLA" # FeatureContext::downloadedContentWhenDownloadingForUserWithRangeShouldBe()
    And user "user2" should see the following elements                                                                                  # FeatureContext::userShouldSeeTheElements()
      | /common/sub/textfile0.txt |
      | /textfile0%20(2).txt      |

--- Failed scenarios:

    /home/phil/git/owncloud/core/tests/acceptance/features/apiShareManagementBasic/createShare.feature:427

1 scenario (1 failed)
17 steps (10 passed, 1 failed, 6 skipped)
0m16.46s (17.70Mb)

and owncloud.log has:

{"reqId":"MELQdktltc7pEPCF19bn","level":4,"time":"2020-01-17T05:26:25+00:00","remoteAddr":"192.168.1.25","user":"user1","app":"webdav","method":"MOVE","url":"\/remote.php\/webdav\/textfile0.txt","message":"Exception: Argument 2 passed to OC\\Share20\\Manager::checkRequiredAttributes() must implement interface OCP\\Share\\IAttributes, null given, called in \/home\/phil\/git\/owncloud\/core\/lib\/private\/Share20\/Manager.php on line 387: {\"Exception\":\"TypeError\",\"Message\":\"Argument 2 passed to OC\\\\Share20\\\\Manager::checkRequiredAttributes() must implement interface OCP\\\\Share\\\\IAttributes, null given, called in \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/private\\\/Share20\\\/Manager.php on line 387\",\"Code\":0,\"Trace\":\"#0 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/private\\\/Share20\\\/Manager.php(387): OC\\\\Share20\\\\Manager->checkRequiredAttributes(Object(OC\\\\Share20\\\\ShareAttributes), NULL)\\n#1 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/private\\\/Share20\\\/Manager.php(297): OC\\\\Share20\\\\Manager->validatePermissions(Object(OC\\\\Share20\\\\Share))\\n#2 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/private\\\/Share20\\\/Manager.php(942): OC\\\\Share20\\\\Manager->generalChecks(Object(OC\\\\Share20\\\\Share))\\n#3 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/apps\\\/files_sharing\\\/lib\\\/Updater.php(82): OC\\\\Share20\\\\Manager->updateShare(Object(OC\\\\Share20\\\\Share))\\n#4 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/apps\\\/files_sharing\\\/lib\\\/Updater.php(34): OCA\\\\Files_Sharing\\\\Updater::moveShareToShare('\\\/common\\\/textfil...')\\n#5 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/private\\\/legacy\\\/hook.php(103): OCA\\\\Files_Sharing\\\\Updater::renameHook(Array)\\n#6 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/private\\\/Files\\\/View.php(903): OC_Hook::emit('OC_Filesystem', 'post_rename', Array)\\n#7 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/public\\\/Events\\\/EventEmitterTrait.php(50): OC\\\\Files\\\\View->OC\\\\Files\\\\{closure}(Array)\\n#8 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/private\\\/Files\\\/View.php(918): OC\\\\Files\\\\View->emittingCall(Object(Closure), Array, 'file', 'rename')\\n#9 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/apps\\\/dav\\\/lib\\\/Connector\\\/Sabre\\\/Directory.php(481): OC\\\\Files\\\\View->rename('\\\/textfile0.txt', '\\\/common\\\/textfil...')\\n#10 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Tree.php(164): OCA\\\\DAV\\\\Connector\\\\Sabre\\\\Directory->moveInto('textfile0.txt', 'textfile0.txt', Object(OCA\\\\DAV\\\\Connector\\\\Sabre\\\\File))\\n#11 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/CorePlugin.php(641): Sabre\\\\DAV\\\\Tree->move('textfile0.txt', 'common\\\/textfile...')\\n#12 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/composer\\\/sabre\\\/event\\\/lib\\\/WildcardEmitterTrait.php(96): Sabre\\\\DAV\\\\CorePlugin->httpMove(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#13 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(474): Sabre\\\\DAV\\\\Server->emit('method:MOVE', Array)\\n#14 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(251): Sabre\\\\DAV\\\\Server->invokeMethod(Object(Sabre\\\\HTTP\\\\Request), Object(Sabre\\\\HTTP\\\\Response))\\n#15 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/composer\\\/sabre\\\/dav\\\/lib\\\/DAV\\\/Server.php(319): Sabre\\\\DAV\\\\Server->start()\\n#16 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/apps\\\/dav\\\/appinfo\\\/v1\\\/webdav.php(65): Sabre\\\\DAV\\\\Server->exec()\\n#17 \\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/remote.php(165): require_once('\\\/home\\\/phil\\\/git\\\/...')\\n#18 {main}\",\"File\":\"\\\/home\\\/phil\\\/git\\\/owncloud\\\/core\\\/lib\\\/private\\\/Share20\\\/Manager.php\",\"Line\":1839}"}

Which is the same problem as in the unit tests.

mrow4a · 2020-01-18T17:07:18Z

@davitol @phil-davis thank you for your excellent work in helping to spot an issue and review ! This was very helpful

regarding issue with secure-view+expiration, good catch - fix for richdocuments Do not update permissions or attributes if other share fields gets updated richdocuments#313 and onlyoffice Do not update permissions or attributes if other share fields gets updated ONLYOFFICE/onlyoffice-owncloud#299 apps
regarding acceptance tests, I added note in https://github.com/owncloud/core/pull/36766/files#diff-e8d8565fb4fcb24d2ff551b5e127cf6aR94, and lets add some advanced acceptanec tests (if possible) later.

mrow4a · 2020-01-18T19:34:05Z

@davitol @phil-davis just tested with onlyoffice/collabora in reshare and some complicated scenarious also manually, and all good.

phil-davis · 2020-01-23T11:16:30Z

This LGTM now. I tried with richdocuments and the secure-view checkboxes work for me along with the other "ordinary" permissions checkboxes.

@micbar can someone review the code? If all OK then we could merge.

phil-davis · 2020-01-23T11:38:03Z

Note: issue #36813 is an existing problem when sharing with a user and a group that have the same name. Now that we have an expiration date field, that field has the same issue when there is this "duplicate" sharing. So that is not the fault of the new expiration date code.

micbar · 2020-01-23T16:08:04Z

@mrow4a Please create a changelog item #36766 (comment)

PVince81

See comments

core/css/share.css

PVince81 · 2020-01-23T16:22:55Z

core/js/sharedialogshareelistview.js

@@ -122,13 +120,15 @@
 		/** @type {Function} **/
 		_template: undefined,

+		_currentlyToggled: [],


this has been declared as indexed array but its usage looks like it's a hash array instead, so would need to be declared as {}

thanks! I overlooked it!

PVince81 · 2020-01-23T16:23:49Z

core/js/sharedialogshareelistview.js

+			var shareId = $li.data('share-id');
+
+			if (!_.isUndefined(this._currentlyToggled[shareId])) {
+				this._currentlyToggled.splice(shareId, 1);


if _currentlyToggled is meant to be used as a hash array (aka object), use delete(this._currentlyToggled[shareId]) to delete the key.

I'm a bit surprised the current approach even worked

Yes, good point, overlooked.

core/js/sharedialogshareelistview.js

core/js/tests/specs/sharedialogshareelistview.js

PVince81 · 2020-01-23T16:34:19Z

mostly minor comments, most critical is likely https://github.com/owncloud/core/pull/36766/files/2df82086ff878d95a8ec1e05fdf77cc20fb9a1a4#diff-e8d8565fb4fcb24d2ff551b5e127cf6a

phil-davis · 2020-01-23T16:50:10Z

@mrow4a Please create a changelog item #36766 (comment)

@micbar do we need to add changelog items for bugfixes to features that were never released yet?

There will already be a changelog for feature "user/group share expiration". This PR simply fixes that feature, before the feature ever got released.

micbar · 2020-01-23T17:00:04Z

@mrow4a Please create a changelog item #36766 (comment)

@micbar do we need to add changelog items for bugfixes to features that were never released yet?

There will already be a changelog for feature "user/group share expiration". This PR simply fixes that feature, before the feature ever got released.

Good question, we need to consider that we released 10.4.0RC1 already and the PR will show up zj the detailed git compare list anyway.

I don't have a strong opinion on that, we could kind of merge it into the Original item by adding this pr link.

That seems like the best solution.
@mrow4a just add it to https://github.com/owncloud/core/blob/master/changelog/unreleased/36573 Link to this pr at the bottom.

mrow4a · 2020-01-23T18:21:37Z

@micbar @phil-davis @PVince81 adjusted to the review

…gistered with apps

…tments to share expiration js code

phil-davis · 2020-01-24T05:43:57Z

I added a changelog for #36813 to the last commit. The last commit fixes that issue (tested manually).

phil-davis

LGTM and tested manually.
I will think about how I can add some automated UI acceptance tests for the sharing with both user and group scenario. But that could be added later (does not have to hold up merging this PR).

@PVince81 please review the code again.

PVince81

👍 looks fine now, thanks for fixing

phil-davis · 2020-01-24T08:51:03Z

Issue to make generally better acceptance test coverage of these UI sharing permissions/expiration date for when there are multiple shares of the the same resource: #36818

mrow4a self-assigned this Jan 15, 2020

mrow4a requested review from jvillafanez, micbar, felixheidecke and PVince81 January 15, 2020 01:17

mrow4a mentioned this pull request Jan 15, 2020

Cannot reshare a file when Secure view is enabled #36751

Closed

jvillafanez reviewed Jan 15, 2020

View reviewed changes

lib/private/Share20/Manager.php Outdated Show resolved Hide resolved

mrow4a commented Jan 15, 2020

View reviewed changes

core/js/shareitemmodel.js Show resolved Hide resolved

phil-davis reviewed Jan 15, 2020

View reviewed changes

core/js/shareitemmodel.js Outdated Show resolved Hide resolved

core/js/shareitemmodel.js Show resolved Hide resolved

micbar added p2-high Escalation, on top of current planning, release blocker Acceptance tests labels Jan 15, 2020

phil-davis self-assigned this Jan 16, 2020

phil-davis force-pushed the fix_36751 branch from 08017f6 to 4d9ac44 Compare January 16, 2020 10:45

mrow4a force-pushed the fix_36751 branch 3 times, most recently from 1a9726c to f6ee573 Compare January 16, 2020 19:26

phil-davis reviewed Jan 17, 2020

View reviewed changes

lib/private/Share20/Manager.php Outdated Show resolved Hide resolved

lib/private/Share20/Manager.php Outdated Show resolved Hide resolved

lib/private/Share20/Manager.php Outdated Show resolved Hide resolved

mrow4a force-pushed the fix_36751 branch 2 times, most recently from 1183987 to 1c1dc1b Compare January 18, 2020 16:53

mrow4a force-pushed the fix_36751 branch 2 times, most recently from 532a51c to 45a386d Compare January 18, 2020 19:10

mrow4a force-pushed the fix_36751 branch from 45a386d to 2e226d1 Compare January 18, 2020 20:55

phil-davis mentioned this pull request Jan 23, 2020

Problems setting sharing permissions in the webUI when user and group have same name #36813

Closed

PVince81 reviewed Jan 23, 2020

View reviewed changes

mrow4a force-pushed the fix_36751 branch 2 times, most recently from b1e9361 to 7ffd5bf Compare January 23, 2020 18:19

mrow4a force-pushed the fix_36751 branch from 7ffd5bf to 0e4ba37 Compare January 23, 2020 18:23

phil-davis requested review from PVince81 and phil-davis January 24, 2020 05:07

owncloud deleted a comment from update-docs bot Jan 24, 2020

mrow4a and others added 4 commits January 24, 2020 11:27

Adjust expiration logic for shares to account for share attributes re…

460d544

…gistered with apps

Add js test to share attributes v2 checking proper rendering

9e64e13

Make link, group and user expiration texts the same. Some minor adjus…

1e58d15

…tments to share expiration js code

fix share checkboxes when a group and a user have the same name

1cee3f3

phil-davis force-pushed the fix_36751 branch from 3d48ad6 to 1cee3f3 Compare January 24, 2020 05:42

phil-davis reviewed Jan 24, 2020

View reviewed changes

PVince81 approved these changes Jan 24, 2020

View reviewed changes

phil-davis mentioned this pull request Jan 24, 2020

UI acceptance tests for sharing with multiple user and groups #36818

Closed

phil-davis merged commit a9d7524 into master Jan 24, 2020

delete-merged-branch bot deleted the fix_36751 branch January 24, 2020 08:51

This was referenced Jan 25, 2020

'Reshare in secure-view is disabled' acceptance test failing owncloud/richdocuments#316

Closed

[Tests-only] SkipOnOcV10.3 20200125 #36825

Merged

mrow4a mentioned this pull request Jan 28, 2020

Cross deletion of 'expiration date for user' shows the calendar using Safari as browser #36735

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust expiration logic for shares to account for share attributes registered with apps #36766

Adjust expiration logic for shares to account for share attributes registered with apps #36766

mrow4a commented Jan 15, 2020 •

edited by phil-davis

Loading

jvillafanez left a comment

codecov bot commented Jan 15, 2020 •

edited

Loading

phil-davis commented Jan 16, 2020

micbar commented Jan 16, 2020

mrow4a commented Jan 16, 2020

phil-davis commented Jan 17, 2020

phil-davis commented Jan 17, 2020

mrow4a commented Jan 18, 2020 •

edited

Loading

mrow4a commented Jan 18, 2020

phil-davis commented Jan 23, 2020

phil-davis commented Jan 23, 2020

micbar commented Jan 23, 2020

PVince81 left a comment

PVince81 Jan 23, 2020

mrow4a Jan 23, 2020

PVince81 Jan 23, 2020

mrow4a Jan 23, 2020

PVince81 commented Jan 23, 2020

phil-davis commented Jan 23, 2020

micbar commented Jan 23, 2020 •

edited

Loading

mrow4a commented Jan 23, 2020

phil-davis commented Jan 24, 2020 •

edited

Loading

phil-davis left a comment

PVince81 left a comment

phil-davis commented Jan 24, 2020

Adjust expiration logic for shares to account for share attributes registered with apps #36766

Adjust expiration logic for shares to account for share attributes registered with apps #36766

Conversation

mrow4a commented Jan 15, 2020 • edited by phil-davis Loading

jvillafanez left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2020 • edited Loading

Codecov Report

phil-davis commented Jan 16, 2020

micbar commented Jan 16, 2020

mrow4a commented Jan 16, 2020

phil-davis commented Jan 17, 2020

phil-davis commented Jan 17, 2020

mrow4a commented Jan 18, 2020 • edited Loading

mrow4a commented Jan 18, 2020

phil-davis commented Jan 23, 2020

phil-davis commented Jan 23, 2020

micbar commented Jan 23, 2020

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 Jan 23, 2020

Choose a reason for hiding this comment

mrow4a Jan 23, 2020

Choose a reason for hiding this comment

PVince81 Jan 23, 2020

Choose a reason for hiding this comment

mrow4a Jan 23, 2020

Choose a reason for hiding this comment

PVince81 commented Jan 23, 2020

phil-davis commented Jan 23, 2020

micbar commented Jan 23, 2020 • edited Loading

mrow4a commented Jan 23, 2020

phil-davis commented Jan 24, 2020 • edited Loading

phil-davis left a comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

phil-davis commented Jan 24, 2020

mrow4a commented Jan 15, 2020 •

edited by phil-davis

Loading

codecov bot commented Jan 15, 2020 •

edited

Loading

mrow4a commented Jan 18, 2020 •

edited

Loading

micbar commented Jan 23, 2020 •

edited

Loading

phil-davis commented Jan 24, 2020 •

edited

Loading