View-only dav plugin using IShare attributes #33994

mrow4a · 2018-12-31T16:30:27Z

Implement extra share permissions with IShare from Approach 3 here - Support for additional arbitrary advanced share permissions #33458
Implement view-only plugin using extra share permissions to restrict view-only permission for read-only files.
Allow apps to register extra share permissions in frontend
Clean the code
Add unit tests
Try JSON type in DB @DeepDiver1975
Make sure PR works with custom groups

Would be also good to merge UX for Forbidden exception with view-only #33992

@PVince81 @DeepDiver1975 @pmaier1

mrow4a · 2019-01-07T09:57:32Z

In PR for view-only plugin & extra share permission, I initially took an approach (#33458) of extending oc_share table with new string column (json) extra_share_permissions. However, I think of going with the approach of external tables. These are pros&cons:

Extended oc_share table:

currently adds new function setExtraPermissions/getExtraPermissions only to Share20\Share class - https://github.com/owncloud/core/pull/33994/files#diff-a19a495d2d4616d9b451f849126013a4. IShare is not touched as it would require all apps to adjust.
easy to implement, no additional queries needed (single query retrieves the information). Expensive as each share need to reserve string space in DB
requires to add additional API functions to IShare?, or keep with appraoch of if instanceof Share20\Share https://github.com/owncloud/core/pull/33994/files#diff-faddd9984dfaf09a2b85d8dc4867207aR887

New extra_share_permissions table:

ExtraShareManager used to retrieve extra share permissions for shares
-IShare or Share/IManager not extended, new classes used (Share/ExtraPermissions/IManager)
new table, reserves space in DB only for shares which have extra share permissions (by full-share-id/share-id).
might need some additional queries to DB to retrieve extra share permissions.

@DeepDiver1975 @PVince81 do you have any input?

codecov · 2019-01-07T16:47:54Z

Codecov Report

Merging #33994 into master will decrease coverage by 16.23%.
The diff coverage is 52.17%.

@@              Coverage Diff              @@
##             master   #33994       +/-   ##
=============================================
- Coverage     64.77%   48.54%   -16.24%     
=============================================
  Files          1198      109     -1089     
  Lines         69418    10523    -58895     
  Branches       1276     1279        +3     
=============================================
- Hits          44964     5108    -39856     
+ Misses        24085     5044    -19041     
- Partials        369      371        +2

Flag	Coverage Δ	Complexity Δ
#javascript	`53.08% <52.17%> (-0.01%)`	`0 <0> (ø)`
#phpunit	`38.63% <ø> (-27.49%)`	`0 <ø> (-18340)`

Impacted Files	Coverage Δ	Complexity Δ
core/js/shareitemmodel.js	`76.89% <42.85%> (-0.89%)`	`0 <0> (ø)`
core/js/sharedialogshareelistview.js	`75.63% <56.25%> (-3.22%)`	`0 <0> (ø)`
lib/private/Files/Storage/DAV.php	`59.45% <0%> (-21.64%)`	`0% <0%> (ø)`
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
apps/dav/lib/Upload/ChunkLocationProvider.php
... and 1073 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 2bc23df...007cc39. Read the comment docs.

codecov · 2019-01-07T16:47:55Z

Codecov Report

Merging #33994 into master will increase coverage by 0.03%.
The diff coverage is 71.82%.

@@             Coverage Diff              @@
##             master   #33994      +/-   ##
============================================
+ Coverage     65.34%   65.38%   +0.03%     
- Complexity    18508    18592      +84     
============================================
  Files          1209     1213       +4     
  Lines         70108    70408     +300     
  Branches       1280     1295      +15     
============================================
+ Hits          45815    46036     +221     
- Misses        23921    23998      +77     
- Partials        372      374       +2

Flag	Coverage Δ	Complexity Δ
#javascript	`52.85% <41.23%> (-0.2%)`	`0 <0> (ø)`
#phpunit	`66.83% <84.95%> (+0.07%)`	`18592 <69> (+84)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
apps/files_sharing/lib/AppInfo/Application.php	`50.48% <0%> (-0.5%)`	`17 <0> (ø)`
apps/files/download.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Files/Meta/MetaFileVersionNode.php	`89.83% <0%> (-6.54%)`	`29 <2> (+2)`
lib/private/legacy/files.php	`0% <0%> (ø)`	`68 <0> (+3)`	⬆️
lib/private/Files/Meta/MetaVersionCollection.php	`87.5% <0%> (ø)`	`16 <0> (ø)`	⬇️
core/Migrations/Version20181220085457.php	`0% <0%> (ø)`	`2 <2> (?)`
lib/private/Share20/DefaultShareProvider.php	`98.42% <100%> (+0.09%)`	`121 <7> (+7)`	⬆️
apps/files_sharing/lib/MountProvider.php	`95.83% <100%> (+0.44%)`	`25 <0> (+3)`	⬆️
apps/dav/lib/Connector/Sabre/ServerFactory.php	`95.89% <100%> (+0.11%)`	`10 <0> (ø)`	⬇️
apps/files_sharing/lib/Hooks.php	`100% <100%> (ø)`	`34 <12> (+19)`	⬆️
... and 14 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 c6bdf34...33795df. Read the comment docs.

core/Migrations/Version20181220085457.php

lib/private/Share20/DefaultShareProvider.php

lib/private/Share20/ExtraPermissions/Permissions.php

lib/private/Share20/ExtraPermissions/Permission.php

PVince81 · 2019-04-01T14:53:19Z

core/js/shareitemmodel.js

+ * @property {string} description
+ * @property {number[]} shareType
+ * @property {number[]} incompatiblePermissions
+ * @property {OC.Share.Types.ShareAttribute[]} incompatibleAttributes


please add the actual doc text, what are these attributes about ?

How and where is the description property used?

DeepDiver1975

will submit fixes for the comments asap

DeepDiver1975 · 2019-04-02T09:18:25Z

apps/dav/lib/DAV/ViewOnlyPlugin.php

+use OCA\DAV\Meta\MetaFile;
+use OCP\Files\FileInfo;
+use OCP\ILogger;
+use Sabre\DAV\Exception\ServiceUnavailable;


DeepDiver1975 · 2019-04-02T09:19:00Z

apps/dav/lib/DAV/ViewOnlyPlugin.php

+ */
+class ViewOnlyPlugin extends ServerPlugin {
+
+ /** @var \Sabre\DAV\Server $server */


unnecessary FQN

apps/dav/lib/DAV/ViewOnlyPlugin.php

DeepDiver1975 · 2019-04-02T09:31:08Z

apps/files_sharing/lib/Controller/Share20OcsController.php

+ * @param IShare $share
+ * @return string|null
+ */
+ private function formatShareAttributes(IShare $share) {


private method which is only used once? - unnecessary

DeepDiver1975 · 2019-04-02T09:32:49Z

apps/files_sharing/lib/Controller/Share20OcsController.php

+ */
+ private function setShareAttributes(IShare $share, $formattedShareAttributes) {
+ $newShareAttributes = $this->shareManager->newShare()->newAttributes();
+ if ($formattedShareAttributes != null) {


strict comparison please

DeepDiver1975 · 2019-04-02T09:33:19Z

apps/files_sharing/lib/Controller/Share20OcsController.php

+ if ($formattedShareAttributes != null) {
+ foreach ($formattedShareAttributes as $formattedAttr) {
+ $newShareAttributes->setAttribute(
+ $formattedAttr["scope"],


single quotes please

DeepDiver1975 · 2019-04-02T09:34:54Z

apps/files_sharing/lib/Hooks.php

 public function __construct(
 IRootFolder $rootFolder,
 IUrlGenerator $urlGenerator,
 EventDispatcher $eventDispatcher,
 \OCP\Share\IManager $shareManager,
- NotificationPublisher $notificationPublisher
+ NotificationPublisher $notificationPublisher,
+ $userSession


IUSerSession $userSession = null

Added additional share attributes for shares as an extension point for apps to provide their own extra attributes that can be used as extra permissions.

Added share attribute "download" that can be used to allow/restrict downloading of shared files or folders.

PVince81 · 2019-04-02T13:10:05Z

I've squashed everything into two distinct commits:

first commit adds support for extra share permissions
second commit uses said support for the view-only "download" permission

PVince81 · 2019-04-02T13:10:59Z

I'm a bit concerned that the JSON field might not work in all databases with upsert from this upcoming PR #34769.

we will see after merging and rebasing the other one...

PVince81 · 2019-04-02T13:47:10Z

stable10: #34951

PVince81 · 2019-04-02T14:52:16Z

seems no issues with upsert and JSON: #34952

LinneyS · 2019-04-09T12:15:15Z

Hi @mrow4a
I started developing extended permissions for the ONLYOFFICE editor.
But I need the following features: like incompatiblePermissions I would like to have required permissions.
For example, a new option is available only if there are rights to edit and is hidden if there are no rights to edit.

LinneyS · 2019-04-09T13:36:12Z

Add after

core/core/js/shareitemmodel.js

Line 76 in 7da3c0f

* @property {OC.Share.Types.ShareAttribute[]} incompatibleAttributes

* @property {number[]} requiredPermissions

Add before

core/core/js/shareitemmodel.js

Line 889 in 7da3c0f

if (compatible) {

for(var ii in attr.requiredPermissions) {
    if (!this._hasPermission(permissions, attr.requiredPermissions[ii])) {
        compatible = false;
    }
}

This will help me solve my tasks.

LinneyS · 2019-04-09T13:43:54Z

I replace the required attribute with incompatibleAttributes with the enabled: false field.
And it worked

DeepDiver1975 · 2019-04-09T14:09:12Z

THX @LinneyS - mind opening a pull request? We can much easier discuss this then. THX a lot!

mrow4a · 2019-04-09T19:41:29Z

@LinneyS what is such permission about? We need to be sure that it follows the logic for "permissions in owncloud". That means, that checked permission box adds some higher level of permission, and unchecked lowers the permission level.

phil-davis · 2019-05-28T11:22:51Z

backport-request label removed. The backport was done already, see #33994 (comment)

mrow4a mentioned this pull request Jan 7, 2019

Support for additional arbitrary advanced share permissions #33458

Closed

mrow4a self-assigned this Jan 7, 2019

DeepDiver1975 requested changes Jan 7, 2019

View reviewed changes

mrow4a force-pushed the secure_view_plugin branch 6 times, most recently from e477d52 to ed83441 Compare January 12, 2019 18:46

mrow4a changed the title ~~[WIP] View-only dav plugin using extra share permissions manager.~~ [WIP] View-only dav plugin using IShare extra share permissions Jan 12, 2019

mrow4a force-pushed the secure_view_plugin branch 2 times, most recently from 91ba4c9 to 3b24700 Compare January 16, 2019 22:13

mrow4a force-pushed the secure_view_plugin branch from 3b24700 to 054ac20 Compare January 20, 2019 17:37

mrow4a changed the title ~~[WIP] View-only dav plugin using IShare extra share permissions~~ View-only dav plugin using IShare extra share permissions Jan 20, 2019

mrow4a mentioned this pull request Jan 20, 2019

Add secure view using using IShare attributes owncloud/richdocuments#245

Merged

3 tasks

PVince81 requested review from VicDeo and jvillafanez January 25, 2019 16:14

mrow4a force-pushed the secure_view_plugin branch from 054ac20 to 22b55cd Compare January 31, 2019 07:22

skshetry mentioned this pull request Feb 1, 2019

federationSharing acceptance test failures #34186

Closed

mrow4a force-pushed the secure_view_plugin branch 8 times, most recently from c3229aa to 0d479c5 Compare February 3, 2019 22:52

PVince81 reviewed Apr 1, 2019

View reviewed changes

DeepDiver1975 requested changes Apr 2, 2019

View reviewed changes

mrow4a added 2 commits April 2, 2019 15:08

Add support for extra share attributes

a3e6a3e

Added additional share attributes for shares as an extension point for apps to provide their own extra attributes that can be used as extra permissions.

Add View-only share permission using share attributes

33795df

Added share attribute "download" that can be used to allow/restrict downloading of shared files or folders.

PVince81 force-pushed the secure_view_plugin branch from 9c9877b to 33795df Compare April 2, 2019 13:09

PVince81 approved these changes Apr 2, 2019

View reviewed changes

PVince81 added 4 - To release and removed 3 - To Review labels Apr 2, 2019

PVince81 assigned PVince81 and unassigned DeepDiver1975 and mrow4a Apr 2, 2019

PVince81 added the backport-request label Apr 2, 2019

PVince81 merged commit b22bf45 into master Apr 2, 2019

delete-merged-branch bot deleted the secure_view_plugin branch April 2, 2019 13:43

PVince81 mentioned this pull request Apr 2, 2019

[stable10] View-only dav plugin using IShare attributes #34951

Merged

PVince81 mentioned this pull request Apr 2, 2019

Use upsert for subshare updates #34952

Merged

PVince81 mentioned this pull request Apr 5, 2019

JS tests extra share attributes #34975

Closed

LinneyS mentioned this pull request Apr 10, 2019

Add the requiredPermissions property for extra share attributes #35001

Closed

mrow4a mentioned this pull request Apr 13, 2019

Share attribute required feature #35016

Merged

phil-davis removed the backport-request label May 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

View-only dav plugin using IShare attributes #33994

View-only dav plugin using IShare attributes #33994

mrow4a commented Dec 31, 2018 •

edited

Loading

mrow4a commented Jan 7, 2019

codecov bot commented Jan 7, 2019

codecov bot commented Jan 7, 2019 •

edited

Loading

PVince81 Apr 1, 2019

LinneyS Apr 10, 2019

DeepDiver1975 left a comment

DeepDiver1975 Apr 2, 2019

DeepDiver1975 Apr 2, 2019

DeepDiver1975 Apr 2, 2019

DeepDiver1975 Apr 2, 2019

DeepDiver1975 Apr 2, 2019

DeepDiver1975 Apr 2, 2019

PVince81 commented Apr 2, 2019

PVince81 commented Apr 2, 2019

PVince81 commented Apr 2, 2019

PVince81 commented Apr 2, 2019

LinneyS commented Apr 9, 2019 •

edited

Loading

LinneyS commented Apr 9, 2019

LinneyS commented Apr 9, 2019 •

edited

Loading

DeepDiver1975 commented Apr 9, 2019

mrow4a commented Apr 9, 2019

phil-davis commented May 28, 2019

View-only dav plugin using IShare attributes #33994

View-only dav plugin using IShare attributes #33994

Conversation

mrow4a commented Dec 31, 2018 • edited Loading

mrow4a commented Jan 7, 2019

codecov bot commented Jan 7, 2019

Codecov Report

codecov bot commented Jan 7, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeepDiver1975 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Apr 2, 2019

PVince81 commented Apr 2, 2019

PVince81 commented Apr 2, 2019

PVince81 commented Apr 2, 2019

LinneyS commented Apr 9, 2019 • edited Loading

LinneyS commented Apr 9, 2019

LinneyS commented Apr 9, 2019 • edited Loading

DeepDiver1975 commented Apr 9, 2019

mrow4a commented Apr 9, 2019

phil-davis commented May 28, 2019

mrow4a commented Dec 31, 2018 •

edited

Loading

codecov bot commented Jan 7, 2019 •

edited

Loading

LinneyS commented Apr 9, 2019 •

edited

Loading

LinneyS commented Apr 9, 2019 •

edited

Loading