Share with group membership only #29241

jvillafanez · 2017-10-13T12:22:42Z

Description

Add an option to allow user to share only with the groups they belong to. If a user isn't a member of a group, that group won't appear as an option to share with it

Related Issue

#29075

Motivation and Context

Users can share with any group. This option restricts that behaviour in a similar way the "share with users in their groups" option does.

How Has This Been Tested?

Tested quickly with the sharing dialog. Other than that, consider this PR as untested for now. New unittests will be added soon.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2017-10-16T09:15:37Z

Codecov Report

Merging #29241 into master will increase coverage by <.01%.
The diff coverage is 76.92%.

@@             Coverage Diff              @@
##             master   #29241      +/-   ##
============================================
+ Coverage     59.39%    59.4%   +<.01%     
- Complexity    17165    17168       +3     
============================================
  Files          1027     1027              
  Lines         57223    57232       +9     
============================================
+ Hits          33990    33998       +8     
- Misses        23233    23234       +1

Impacted Files	Coverage Δ	Complexity Δ
settings/templates/panels/admin/filesharing.php	`100% <ø> (ø)`	`0 <0> (ø)`	⬇️
...files_sharing/lib/Controller/ShareesController.php	`16.22% <0%> (-0.08%)`	`93 <0> (ø)`
lib/private/Share/Share.php	`66.87% <100%> (+0.09%)`	`502 <2> (+2)`	⬆️
apps/files_sharing/lib/Capabilities.php	`100% <100%> (ø)`	`6 <0> (ø)`	⬇️
settings/Panels/Admin/FileSharing.php	`100% <100%> (ø)`	`6 <0> (ø)`	⬇️
lib/private/Share20/Manager.php	`97.25% <100%> (+0.01%)`	`198 <1> (+1)`	⬆️

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 67c8e05...b48e96e. Read the comment docs.

codecov · 2017-10-16T09:15:38Z

Codecov Report

Merging #29241 into master will increase coverage by 1.13%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##             master   #29241      +/-   ##
============================================
+ Coverage     59.39%   60.53%   +1.13%     
- Complexity    17165    17191      +26     
============================================
  Files          1027     1031       +4     
  Lines         57223    57284      +61     
============================================
+ Hits          33990    34677     +687     
+ Misses        23233    22607     -626

Impacted Files	Coverage Δ	Complexity Δ
settings/templates/panels/admin/filesharing.php	`100% <ø> (ø)`	`0 <0> (ø)`	⬇️
core/Migrations/Version20171026130750.php	`0% <0%> (ø)`	`2 <2> (?)`
lib/private/Share20/Manager.php	`97.25% <100%> (+0.01%)`	`198 <1> (+1)`	⬆️
apps/files_sharing/lib/Capabilities.php	`100% <100%> (ø)`	`6 <0> (ø)`	⬇️
settings/Panels/Admin/FileSharing.php	`100% <100%> (ø)`	`6 <0> (ø)`	⬇️
lib/private/Share/Share.php	`66.87% <100%> (+0.09%)`	`502 <2> (+2)`	⬆️
...files_sharing/lib/Controller/ShareesController.php	`89.03% <66.66%> (+72.73%)`	`93 <0> (ø)`	⬇️
apps/dav/lib/Server.php	`48.33% <0%> (-4.35%)`	`11% <0%> (-5%)`
lib/private/Installer.php	`46.92% <0%> (-3.79%)`	`89% <0%> (+3%)`
lib/private/legacy/api.php	`41.88% <0%> (-0.55%)`	`80% <0%> (+2%)`
... and 40 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 67c8e05...ad3f111. Read the comment docs.

jvillafanez · 2017-10-17T07:13:44Z

@phil-davis @individual-it could you check if the failing test is related with this PR's changes? I'm not sure if it's "just" a random failure or if it's something that needs to be fixed.

phil-davis · 2017-10-17T08:18:57Z

@jvillafanez I scanned through your code diffs and there is nothing touching the front-end that should make the test fail like it did (cannot even find the "share with" element on the page). So I restarted the job and we will see.

PVince81 · 2017-10-17T09:32:54Z

@jvillafanez can you also add integration tests ? Feel free to ask @SergioBertolinSG for some assistance.

jvillafanez · 2017-10-17T12:35:06Z

I'll check what we can add easily during today and tomorrow.

felixboehm · 2017-10-17T16:31:39Z

Need an assignment here, who continues work on this?

PVince81 · 2017-10-17T19:08:40Z

Assigned to @jvillafanez

jvillafanez · 2017-10-23T10:31:25Z

I've found a problem with the share API (probably known) that affects to all the sharing tests. The problem is that the response of the sharing API is different depending on the number of results it needs to return.

For only one group available in the results:

Array
(
    [exact] => Array
        (
            [users] => Array
                (
                )

            [groups] => Array
                (
                )

            [remotes] => Array
                (
                )

        )

    [users] => Array
        (
            [element] => Array
                (
                    [label] => Sharee1
                    [value] => Array
                        (
                            [shareType] => 0
                            [shareWith] => Sharee1
                        )

                )

        )

    [groups] => Array
        (
            [element] => Array
                (
                    [label] => ShareeGroup
                    [value] => Array
                        (
                            [shareType] => 1
                            [shareWith] => ShareeGroup
                        )

                )

        )

    [remotes] => Array
        (
        )

)

If there are several results:

Array
(
    [exact] => Array
        (
            [users] => Array
                (
                )

            [groups] => Array
                (
                )

            [remotes] => Array
                (
                )

        )

    [users] => Array
        (
            [element] => Array
                (
                    [label] => Sharee1
                    [value] => Array
                        (
                            [shareType] => 0
                            [shareWith] => Sharee1
                        )

                )

        )

    [groups] => Array
        (
            [element] => Array
                (
                    [0] => Array
                        (
                            [label] => ShareeGroup
                            [value] => Array
                                (
                                    [shareType] => 1
                                    [shareWith] => ShareeGroup
                                )

                        )

                    [1] => Array
                        (
                            [label] => ShareeGroup2
                            [value] => Array
                                (
                                    [shareType] => 1
                                    [shareWith] => ShareeGroup2
                                )

                        )

                )

        )

    [remotes] => Array
        (
        )

)

Note that if there is only one element available, that element is returned under the "element" key, but if there are several elements, the "element" key returns an array of elements. This will likely happen with the users, although I haven't checked.

This problem affects the new tests because we need at least 2 groups. In order to do so, we'll need to patch the underlying test code to accept both possibilities so the parsing is properly done (this will error prone since we can't distinguish both options in a clear way, checking if the key is an integer is ugly but there isn't any other easy way).

The best option would be to fix the problem in the sharing API, but that's out of the scope of this PR (it will also need to fix the tests).

jvillafanez · 2017-10-25T09:06:57Z

@SergioBertolinSG could you take over for the integration tests? Note my last comment.

phil-davis · 2017-10-25T11:00:29Z

And (I think) @individual-it put his hand up to add some UI tests for this. There are already UI tests of this sharing area, so it should be just a matter of setting the new system option and then testing that the sharing dropdown now contains just the expected items.

SergioBertolinSG · 2017-10-25T13:07:24Z

@jvillafanez I can take care after finishing the pending tasks I have (which are several). Is that ok? Otherwise you should continue.

jvillafanez · 2017-10-26T11:57:01Z

Waiting for automatic testing.... other than that, this should be ready

SergioBertolinSG

👍 for the behat tests

PVince81 · 2017-10-26T12:46:02Z

@jvillafanez seems to work fine.

One thing that bothers me is that the behavior will change on update.

To recreate the old behavior, one needs to check both checkboxes.

We should write a migration to automatically tick the second checkbox if the first one was checked on update to avoid surprising admins with a sudden change of behavior. Then the admin can decide to tweak if needed.

@jvillafanez does that make sense ?

jvillafanez · 2017-10-26T12:57:14Z

Migration incomming!

jvillafanez · 2017-10-26T13:26:59Z

Migration added (not tested yet)

I don't think we need to check the OC version as long as the migrations are run only once (which is what I expect). Under these circumstances, copy the value from one key to another should do the trick.

PVince81 · 2017-10-26T13:45:47Z

@jvillafanez migrations are run only once, yes. Keep in mind that migrations also run at install time so make sure it also works in that situation.

jvillafanez · 2017-10-27T08:00:51Z

Fresh install -> migration runs without doing anything (key is missing so nothing to do)
Update from 10.0.3 with checkbox active -> key is duplicated
Update from 9.1.6 with checkbox active -> key is duplicated

LGTM

PVince81

👍

PVince81 · 2017-10-27T08:11:51Z

@jvillafanez seems codecov is not happy, might be missing some test coverage ?

PVince81 · 2017-10-27T08:13:06Z

@jvillafanez I checked the report. Not all is coverable (ignore the migration), please write tests for the changes in ShareesController.

PVince81 · 2017-10-30T10:51:40Z

@jvillafanez please backport to stable10

jvillafanez · 2017-10-30T12:25:21Z

stable10 backport in #29391

individual-it · 2017-11-01T11:24:45Z

UI tests in #29406

lock · 2019-08-02T03:02:29Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

jvillafanez added 2 commits October 13, 2017 13:22

Add a checkbox to allow to share only to groups the user is member of

09bfad4

Adjust tests

4542166

jvillafanez added 2 - Developing feature:sharing settings:admin labels Oct 13, 2017

jvillafanez added this to the development milestone Oct 13, 2017

Adjust integration tests

b48e96e

jvillafanez added 3 - To Review and removed 2 - Developing labels Oct 16, 2017

felixboehm added the p2-high Escalation, on top of current planning, release blocker label Oct 17, 2017

PVince81 assigned jvillafanez Oct 17, 2017

Add integration tests and adjust some of the previous ones

7b5585a

SergioBertolinSG approved these changes Oct 26, 2017

View reviewed changes

Add migration to keep the same behvaiour between versions

0b409c8

PVince81 approved these changes Oct 27, 2017

View reviewed changes

PVince81 added the backport-request label Oct 27, 2017

Recover commented tests + adjust testGetGroups for the new config option

ad3f111

individual-it mentioned this pull request Oct 30, 2017

possibility to modify app settings through TestHelpers #29388

Merged

9 tasks

PVince81 merged commit e320296 into master Oct 30, 2017

PVince81 deleted the share_with_group_membership_only branch October 30, 2017 10:51

jvillafanez mentioned this pull request Oct 30, 2017

[stable10] Share with group membership only stable10 #29391

Merged

9 tasks

individual-it mentioned this pull request Nov 1, 2017

UI tests for restricting the sharing #29406

Merged

9 tasks

PVince81 removed the backport-request label Nov 3, 2017

ownclouders added the status/STALE label Dec 20, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share with group membership only #29241

Share with group membership only #29241

jvillafanez commented Oct 13, 2017

codecov bot commented Oct 16, 2017

codecov bot commented Oct 16, 2017 •

edited

Loading

jvillafanez commented Oct 17, 2017

phil-davis commented Oct 17, 2017

PVince81 commented Oct 17, 2017

jvillafanez commented Oct 17, 2017

felixboehm commented Oct 17, 2017

PVince81 commented Oct 17, 2017

jvillafanez commented Oct 23, 2017

jvillafanez commented Oct 25, 2017

phil-davis commented Oct 25, 2017

SergioBertolinSG commented Oct 25, 2017

jvillafanez commented Oct 26, 2017

SergioBertolinSG left a comment

PVince81 commented Oct 26, 2017

jvillafanez commented Oct 26, 2017

jvillafanez commented Oct 26, 2017

PVince81 commented Oct 26, 2017

jvillafanez commented Oct 27, 2017

PVince81 left a comment

PVince81 commented Oct 27, 2017

PVince81 commented Oct 27, 2017

PVince81 commented Oct 30, 2017

jvillafanez commented Oct 30, 2017

individual-it commented Nov 1, 2017

lock bot commented Aug 2, 2019

Share with group membership only #29241

Share with group membership only #29241

Conversation

jvillafanez commented Oct 13, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Oct 16, 2017

Codecov Report

codecov bot commented Oct 16, 2017 • edited Loading

Codecov Report

jvillafanez commented Oct 17, 2017

phil-davis commented Oct 17, 2017

PVince81 commented Oct 17, 2017

jvillafanez commented Oct 17, 2017

felixboehm commented Oct 17, 2017

PVince81 commented Oct 17, 2017

jvillafanez commented Oct 23, 2017

jvillafanez commented Oct 25, 2017

phil-davis commented Oct 25, 2017

SergioBertolinSG commented Oct 25, 2017

jvillafanez commented Oct 26, 2017

SergioBertolinSG left a comment

Choose a reason for hiding this comment

PVince81 commented Oct 26, 2017

jvillafanez commented Oct 26, 2017

jvillafanez commented Oct 26, 2017

PVince81 commented Oct 26, 2017

jvillafanez commented Oct 27, 2017

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Oct 27, 2017

PVince81 commented Oct 27, 2017

PVince81 commented Oct 30, 2017

jvillafanez commented Oct 30, 2017

individual-it commented Nov 1, 2017

lock bot commented Aug 2, 2019

codecov bot commented Oct 16, 2017 •

edited

Loading