Allow sharing with guest users when sharing is limited by user groups #36384

VicDeo · 2019-11-06T13:40:04Z

Description

Allow sharing with guest users when Restrict users to only share with users in their groups option is enabled

Related Issue

Fixes https://github.com/owncloud/enterprise/issues/3578

Motivation and Context

Guests app should not be affected by the ordinary sharing group restriction

How Has This Been Tested?

Enable the guests app
Check Restrict users to only share with users in their groups on the settings page
Share any item with someuser@example.org (guest)

Expected

User is created, a share is added for this user. No errors

Actual

User is created, but the share is NOT added for this user. Sharing error popup in the web UI

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:

codecov · 2019-11-06T15:55:42Z

Codecov Report

Merging #36384 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #36384      +/-   ##
============================================
+ Coverage     64.87%   64.87%   +<.01%     
- Complexity    19791    19797       +6     
============================================
  Files          1271     1272       +1     
  Lines         74733    74748      +15     
  Branches       1309     1309              
============================================
+ Hits          48480    48495      +15     
  Misses        25867    25867              
  Partials        386      386

Flag	Coverage Δ	Complexity Δ
#javascript	`54% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.07% <100%> (ø)`	`19797 <5> (+6)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Helper/UserTypeHelper.php	`100% <100%> (ø)`	`5 <5> (?)`
lib/private/Share20/Manager.php	`97.15% <100%> (ø)`	`251 <0> (+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 236e05c...ceed1f1. Read the comment docs.

jvillafanez · 2019-11-07T10:28:20Z

We might need to consider to create a UserTypeDetection class or similar at least to move this code to a place. It isn't the first place we try to detect if a user is a guest, and it won't be the last.

micbar · 2019-11-07T16:20:31Z

looks promising.

lib/private/Share20/Manager.php

karakayasemi

I am not familiar with the Guest app concept, so I am not sure of the concept. In terms of code, rest of the things are good.

lib/private/Share20/Manager.php

karakayasemi

I know, it is not a big deal about performance, but if the user is a guest, no need to make any calculation about group intersection. Because of that, moving this check to one upper level is better in terms of performance.

jvillafanez · 2019-11-08T08:21:29Z

lib/private/Helper/UserTypeHelper.php

+	/** @var IConfig */
+	private $config;
+
+	public function __construct(IAppManager $appManager = null, IConfig $config = null) {


include a comment explaining why the appManager and config can be null.

jvillafanez · 2019-11-08T08:23:46Z

lib/private/Helper/UserTypeHelper.php

+	 */
+	public function isGuestUser($uid) {
+		$guestsAppEnabled = $this->appManager->isEnabledForUser('guests');
+		if ($guestsAppEnabled) {


should the method return true for the guest user even if the app is disabled? I find it weird that a user is reported as guest, but then, after disabling the app, the user isn't a guest any longer.

jvillafanez · 2019-11-08T08:25:03Z

lib/private/Share20/Manager.php

@@ -462,15 +463,19 @@ protected function validateExpirationDate(\OCP\Share\IShare $share) {
 	 * @throws \Exception
 	 */
 	protected function userCreateChecks(\OCP\Share\IShare $share) {
+		$userTypeHelper = new UserTypeHelper();


this should be injected

VicDeo added the 2 - Developing label Nov 6, 2019

VicDeo added this to the development milestone Nov 6, 2019

VicDeo self-assigned this Nov 6, 2019

VicDeo force-pushed the bugfix/e3578 branch from c2d62b5 to 8dd5585 Compare November 7, 2019 14:18

VicDeo added 3 - To Review and removed 2 - Developing labels Nov 7, 2019

VicDeo requested review from micbar and sharidas November 7, 2019 14:25

VicDeo changed the title ~~[WIP]~~ Allow sharing with guest users when Restrict users to only share with users in their groups option is enabled Nov 7, 2019

VicDeo changed the title ~~Allow sharing with guest users when Restrict users to only share with users in their groups option is enabled~~ Allow sharing with guest users when sharing is limited by user groups Nov 7, 2019

VicDeo requested review from jvillafanez and karakayasemi November 7, 2019 14:33

karakayasemi reviewed Nov 8, 2019

View reviewed changes

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

karakayasemi reviewed Nov 8, 2019

View reviewed changes

VicDeo force-pushed the bugfix/e3578 branch from 8dd5585 to 51892c2 Compare November 8, 2019 07:36

karakayasemi reviewed Nov 8, 2019

View reviewed changes

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

karakayasemi suggested changes Nov 8, 2019

View reviewed changes

VicDeo force-pushed the bugfix/e3578 branch from 51892c2 to d30d6a7 Compare November 8, 2019 08:12

Allow sharing with guests when sharing is restricted within a group

ceed1f1

VicDeo force-pushed the bugfix/e3578 branch from d30d6a7 to ceed1f1 Compare November 8, 2019 08:14

micbar approved these changes Nov 8, 2019

View reviewed changes

karakayasemi approved these changes Nov 8, 2019

View reviewed changes

jvillafanez reviewed Nov 8, 2019

View reviewed changes

VicDeo merged commit 49872a8 into master Nov 8, 2019

delete-merged-branch bot deleted the bugfix/e3578 branch November 8, 2019 10:02

davitol mentioned this pull request Nov 26, 2019

10.3.2. - Changelog test Results #36477

Closed

37 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow sharing with guest users when sharing is limited by user groups #36384

Allow sharing with guest users when sharing is limited by user groups #36384

VicDeo commented Nov 6, 2019 •

edited

Loading

codecov bot commented Nov 6, 2019 •

edited

Loading

jvillafanez commented Nov 7, 2019

micbar commented Nov 7, 2019

karakayasemi left a comment

karakayasemi left a comment

jvillafanez Nov 8, 2019

jvillafanez Nov 8, 2019

jvillafanez Nov 8, 2019

Allow sharing with guest users when sharing is limited by user groups #36384

Allow sharing with guest users when sharing is limited by user groups #36384

Conversation

VicDeo commented Nov 6, 2019 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Expected

Actual

Types of changes

Checklist:

codecov bot commented Nov 6, 2019 • edited Loading

Codecov Report

jvillafanez commented Nov 7, 2019

micbar commented Nov 7, 2019

karakayasemi left a comment

Choose a reason for hiding this comment

karakayasemi left a comment

Choose a reason for hiding this comment

jvillafanez Nov 8, 2019

Choose a reason for hiding this comment

jvillafanez Nov 8, 2019

Choose a reason for hiding this comment

jvillafanez Nov 8, 2019

Choose a reason for hiding this comment

VicDeo commented Nov 6, 2019 •

edited

Loading

codecov bot commented Nov 6, 2019 •

edited

Loading