[7.0.4] Renaming a folder shared from multiple groups in v<=7.0.2 create multiple shares to the user in v>= 7.0.4

[GreenArchon]

Steps to reproduce:

1. Install owncloud 7.0.2 (or older)
2. Create users u1, u2 and groups g1, g2
3. Add user u2 to groups g1 and g2
4. From user u1, create a folder test1 and share it with groups g1 and g2 (possibly with different permissions)
5. Upgrade to 7.0.4 (or 7.0.3 or probably master)
6. From user u2, rename the folder test1 (ie to test2)

Expected behaviour:

Either:

• Nothing happens (except the folder being renamed)
• A share is created for u2 with the cumulative permissions of g1 and g2 (from [sharing] group shares #10107 I think)

Actual behaviour:

• 2 shares are created for the user, having (in oc_share) the parent, permissions, etc. columns pointing to g1 and g2's share, respectively.

Note: This is a follow-up from #12475 .

[LukasReschke]

@schiesbn @PVince81 FYI

[PVince81]

Ouch, I'm at step 4 and it already looks bad:

+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
| id | share_type | share_with | uid_owner | parent | item_type | item_source | item_target | file_source | file_target | permissions | stime      | accepted | expiration | token | mail_send |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
|  1 |          1 | g1         | u1        |   NULL | folder    | 9           | /9          |           9 | /test1      |          31 | 1422450372 |        0 | NULL       | NULL  |         0 |
|  2 |          1 | g2         | u1        |   NULL | folder    | 9           | /9          |           9 | /test1      |          17 | 1422450376 |        0 | NULL       | NULL  |         0 |
|  3 |          2 | u2         | u1        |      2 | folder    | 9           | /9          |           9 | /test1 (2)  |          31 | 1422450376 |        0 | NULL       | NULL  |         0 |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+

As you can see the user's child entry has no parent. It should link itself to either 1 or 2.
This is not a repairable situation.

[PVince81]

Ah no, sorry. I misread (lines were wrapped on my screen).
It does look fine: the child entry is linked with 2 (the share over g2). Not sure why it chose this one.
Let's continue.

[PVince81]

After the upgrade and rename, I see this:

+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
| id | share_type | share_with | uid_owner | parent | item_type | item_source | item_target | file_source | file_target | permissions | stime      | accepted | expiration | token | mail_send |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
|  1 |          1 | g1         | u1        |   NULL | folder    | 9           | /9          |           9 | /test1      |          31 | 1422450372 |        0 | NULL       | NULL  |         0 |
|  2 |          1 | g2         | u1        |   NULL | folder    | 9           | /9          |           9 | /test1      |          17 | 1422450376 |        0 | NULL       | NULL  |         0 |
|  3 |          2 | u2         | u1        |      2 | folder    | 9           | /9          |           9 | /test1 (2)  |          31 | 1422450376 |        0 | NULL       | NULL  |         0 |
|  4 |          2 | u2         | u1        |      1 | folder    | 9           | /9          |           9 | /test2      |          31 | 1422450372 |        0 | NULL       | NULL  |         0 |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+

Since I renamed "test1" and not "test1 (2)", a new entry is created which points to the share 1 instead of 2. I'd say this is expected.

For every share through a different group, the recipient receives a separate folder.
In this test case it was "test1" and "test1 (2)".

Ideally these shares should be combined and I'm not sure why they aren't, maybe because the combining code didn't exist back then in 7.0.2.

Let's try the same steps from scratch directly on 7.0.4.

[PVince81]

On 7.0.4, at step 4 (after sharing with the two groups, group1 read-write and group2 read-only), I see this now:

+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
| id | share_type | share_with | uid_owner | parent | item_type | item_source | item_target | file_source | file_target | permissions | stime      | accepted | expiration | token | mail_send |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
|  1 |          1 | g1         | u1        |   NULL | folder    | 15          | /15         |          15 | /test1      |          31 | 1422450769 |        0 | NULL       | NULL  |         0 |
|  2 |          2 | u2         | u1        |      1 | folder    | 15          | /15         |          15 | /test1      |          31 | 1422450769 |        0 | NULL       | NULL  |         0 |
|  3 |          1 | g2         | u1        |   NULL | folder    | 15          | /15         |          15 | /test1      |          17 | 1422450773 |        0 | NULL       | NULL  |         0 |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+

Almost the same as before, but now the child entry is linked to share 1 instead of 2, and there is no "test1 (2)".

Logging into the web UI as "u2" only shows a single folder "test1", which means that the shares have properly been merged.

I can upload files, which means that the broadest permissions have been given (read-write instead of read-only). So far so good.

After renaming "test1" to "test2", this is the DB:

+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
| id | share_type | share_with | uid_owner | parent | item_type | item_source | item_target | file_source | file_target | permissions | stime      | accepted | expiration | token | mail_send |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
|  1 |          1 | g1         | u1        |   NULL | folder    | 15          | /15         |          15 | /test1      |          31 | 1422450769 |        0 | NULL       | NULL  |         0 |
|  2 |          2 | u2         | u1        |      1 | folder    | 15          | /15         |          15 | /test2      |          31 | 1422450769 |        0 | NULL       | NULL  |         0 |
|  3 |          1 | g2         | u1        |   NULL | folder    | 15          | /15         |          15 | /test1      |          17 | 1422450773 |        0 | NULL       | NULL  |         0 |
|  4 |          2 | u2         | u1        |      3 | folder    | 15          | /15         |          15 | /test2      |          17 | 1422450773 |        0 | NULL       | NULL  |         0 |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+

So now it created a child entry for both group shares, having both renamed.
And then they are both also properly merged in the web UI.

I'll say that on 7.0.4 everything works as expected.

The problem is mostly about recovering from the old 7.0.2 DB entries and converting them somehow to the 7.0.4 structure.

@schiesbn what do you think ?

[MorrisJobke]

So the migration step is to check for a share that is shared to two groups/users, where one user is in both groups. Then I have to delete the share, that points in the parent item to one of these previously found shares?

Share to group abc and just test(who is in share) and a share to group abc and def (where user test is in both) are two valid cases that needs to be covered.

And of course tit needs to be the same file_source? Or needs to be them item:source the same? What about reshares?

@schiesbn @PVince81 Can you confirm this?

@PVince81 To clarify your questions: I created the shares on stable6 (to match real world experience) and now the task would be to clean up the shares properly. First of all those name (2) shares where both shares have the same permission should be dropped. Yes it could be bad to drop one of those shares, but this is the only way to fix this. I would write a repair step, where the admin can list all affected shares (it shouldn't be that much) and maybe list affected users to properly communicate that to the user. Would that be a good solution? Maybe also a "check for broken shares" command is a good first step. I volunteer for that task :)

[PVince81]

I don't fully understand everything that is happening here yet.

One question would be: would the repair step repair the previous situation (the one that we have before upgrade) or the one after the upgrade where it's already kind of broken ?
Ideally I suppose we might want both ?

[PVince81]

Now thinking of it, deleting such bogus child entries could cause additional issues if these child entries are also parent of reshares (if the receipient user reshared that specific folder, like "test (2)")
I guess you're right that this cannot be fixed automatically without a bit of breakage.
Or maybe add a check and don't delete such child entries and show an error "cannot automatically fix share of XYZ, it will need to be reshared manually".

To answer your question, I believe that they all have to be the same file_source/item_source because all these share entry actually originate from the same file/folder. However "file_target" is the name of the file/folder as seen in the recipient's view. That one can be changed more freely. Having all "file_target" values the same for the recipient should automatically have the folder displayed as a single entry.

[MorrisJobke]

@PVince81 I would like to fix the behaviour after the upgrade from 6 to pre-7.0.5 (because this cause the issue, if the linked PR is merged into 7.0.5). Then we need a tool to help the admin to fix this problem. I would first go for a tool that shows the different affected shares, then try to fix most of then and then list the admin the remaining shares, to directly contact the user to fix this somehow manually. Maybe this are three stages of one command, but as this affects huge installations it is hopefully worth the effort.

@DeepDiver1975 @schiesbn @PVince81 Opinions?

[PVince81]

Providing a tool to repair shares seems to be a lot of work. So far the "policy" was always to try and repair as much as possible automatically.

Do we want to start introducing GUI or CLI specialized tools for manually fixing shares (and also broken legacy storages ?) Or is a documentation with SQL queries enough ?

[PVince81]

The repair step before the upgrade sounds like a good idea.

[MorrisJobke]

@PVince81 I would like to just wrap some SQL statements into these commands.

[cdamken]

@PVince81

The repair step before the upgrade sounds like a good idea.

I'm agree, preventing is good 👍 but most users will see that the problem exist after upgrade. Or will be the solution in the upgrade script?

[MorrisJobke]

I justed talked to @schiesbn and he said that there was a valid reason to not group this after the upgrade. For the user it could produce weird behaviour if the user renamed both shares and moved it to different location. Then some random folders could be removed, because previously it was a share and it will be grouped together with another one.

@cdamken Is this a possible solution? Its from a technical point it's really hard to detect the shares that can be grouped. It's nowadays just a new feature, that shares a grouped. Doing it afterwards is just really hard.

[MorrisJobke]

@cdamken See my last comment with a possible solution for this

[cdamken]

@MorrisJobke It sounds reasonable. Thank you

[MorrisJobke]

Reason for close: see #13429 (comment)