shareowner can now edit re-share permissions without any restriction

[karakayasemi]

This is the first PR of related issue. The other PRs do not feel right to me and I returned back to this one. Sorry for all the confusion.

I just made the UserSession nullable in the Manager constructor and adjusted tests. @PVince81 like you said in here #36166 (comment), whenever the session user is null, mentioned if condition will be disabled in this pr.

Description

Shareowner should have full permission on re-shares. For detecting re-share, this PR compares the session user UID with the shareowner UID as described in this comment: #36107 (comment).

Related Issue

• Fixes [10.3.0 alpha] Owner cannot edit group's permissions #36107

Motivation and Context

Fighting with bugs.

How Has This Been Tested?

Manually with steps in #36107, acceptance tests added and unit tests adjusted.

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:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

[codecov]

Codecov Report

Merging #36193 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #36193   +/-   ##
=======================================
  Coverage      54%      54%           
=======================================
  Files          63       63           
  Lines        7408     7408           
  Branches     1309     1309           
=======================================
  Hits         4001     4001           
  Misses       3021     3021           
  Partials      386      386

Flag	Coverage Δ
#javascript	54% <ø> (ø)	⬆️

---

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 2daddbd...c50db34. Read the comment docs.

[codecov]

Codecov Report

Merging #36193 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #36193   +/-   ##
=======================================
  Coverage      54%      54%           
=======================================
  Files          63       63           
  Lines        7408     7408           
  Branches     1309     1309           
=======================================
  Hits         4001     4001           
  Misses       3021     3021           
  Partials      386      386

Flag	Coverage Δ
#javascript	54% <ø> (ø)	⬆️

---

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 5d1ca21...c6d9c36. Read the comment docs.

[jvillafanez]

Code looks good if this is the final approach we want to take.

[karakayasemi]

No interface change, no confusing part in the code, I guess this is the best one. I would like to continue with this one.

[karakayasemi]

@micbar User object injected instead of UserSession, let's see what CI will say.

[jvillafanez]

I think there have been cases where the user isn't set yet inside the session during the dependency wiring. In addition, user in the session might change, which will likely cause bugs.

[micbar]

I think there have been cases where the user isn't set yet inside the session during the dependency wiring. In addition, user in the session might change, which will likely cause bugs.

So injecting the session would be better?

[karakayasemi]

Hmm, @jvillafanez is pointing out a right point. @micbar let's use UserSession if you agree.

[micbar]

Vincent and me wanted to have it more clear in the code why we have a session in ShareManager.

We don't need the session, we need the acting user.
The main underlying flaw her IMO is that the ShareManager Class has no visible context. Is it meant to be used as an admin / user / cli ?

[micbar]

But I see your point @jvillafanez

[jvillafanez]

I think we're back to square 1.
The reason we discarded this solution was because of problems in the CLI, because we don't really have a user session there. In practice, we'll have to login and logout with the users while doing things in the CLI. This is why we moved to extending the interface with the "acting user".

[karakayasemi]

Actually, the current behavior of ShareManager is logically correct. It does not allow reshares permission higher than the original share permission. But, we want to give privilege to share-owner in this specific case. To implement the desired behavior, we need to compromise from somewhere.

One of the options is changing the interface in a minor release. IMHO, in another interface implementation, our desired behavior can be unwanted. So, I would not identify this $actinguser parameter as common in interface wide. So, this option does not feel right to me.

On the injected IUserSession option, we will be able to reach our specific desire with being faithful to our interface contract. Share-owner will be able to edit own reshares with his permissions.
lf IUserSession or User is null, it indicates CLI or Admin usage and in this situation, ShareManager will work with share-owner permissions. In my POV, injecting UserSession seems the best option.

[PVince81]

👍 fine by me with the IUserSession approach

[jvillafanez]

Code looks good

[micbar]

@davitol Please test again during RC1 and test occ transfer-ownership with a lot of shares and reshares.

[HanaGemela]

Works as expected on the desktop sync client 2.5.4 and 2.6.0 RC1, macOS 10.14.5 and server 10.3.0RC1