-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
View-only dav plugin using IShare attributes #33994
Conversation
In PR for view-only plugin & extra share permission, I initially took an approach (#33458) of extending Extended
New
@DeepDiver1975 @PVince81 do you have any input? |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
e477d52
to
ed83441
Compare
91ba4c9
to
3b24700
Compare
3b24700
to
054ac20
Compare
054ac20
to
22b55cd
Compare
c3229aa
to
0d479c5
Compare
* @property {string} description | ||
* @property {number[]} shareType | ||
* @property {number[]} incompatiblePermissions | ||
* @property {OC.Share.Types.ShareAttribute[]} incompatibleAttributes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add the actual doc text, what are these attributes about ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How and where is the description
property used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will submit fixes for the comments asap
apps/dav/lib/DAV/ViewOnlyPlugin.php
Outdated
use OCA\DAV\Meta\MetaFile; | ||
use OCP\Files\FileInfo; | ||
use OCP\ILogger; | ||
use Sabre\DAV\Exception\ServiceUnavailable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
apps/dav/lib/DAV/ViewOnlyPlugin.php
Outdated
*/ | ||
class ViewOnlyPlugin extends ServerPlugin { | ||
|
||
/** @var \Sabre\DAV\Server $server */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unnecessary FQN
* @param IShare $share | ||
* @return string|null | ||
*/ | ||
private function formatShareAttributes(IShare $share) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private method which is only used once? - unnecessary
*/ | ||
private function setShareAttributes(IShare $share, $formattedShareAttributes) { | ||
$newShareAttributes = $this->shareManager->newShare()->newAttributes(); | ||
if ($formattedShareAttributes != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strict comparison please
if ($formattedShareAttributes != null) { | ||
foreach ($formattedShareAttributes as $formattedAttr) { | ||
$newShareAttributes->setAttribute( | ||
$formattedAttr["scope"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single quotes please
public function __construct( | ||
IRootFolder $rootFolder, | ||
IUrlGenerator $urlGenerator, | ||
EventDispatcher $eventDispatcher, | ||
\OCP\Share\IManager $shareManager, | ||
NotificationPublisher $notificationPublisher | ||
NotificationPublisher $notificationPublisher, | ||
$userSession |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
9c9877b
to
33795df
Compare
I've squashed everything into two distinct commits:
|
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... |
stable10: #34951 |
seems no issues with upsert and JSON: #34952 |
Hi @mrow4a |
Add after core/core/js/shareitemmodel.js Line 76 in 7da3c0f
Add before core/core/js/shareitemmodel.js Line 889 in 7da3c0f
This will help me solve my tasks. |
I replace the required attribute with |
THX @LinneyS - mind opening a pull request? We can much easier discuss this then. THX a lot! |
@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. |
|
Would be also good to merge UX for Forbidden exception with view-only #33992
@PVince81 @DeepDiver1975 @pmaier1