owncloud / core

:cloud: ownCloud web server core (Files, DAV, etc.)
https://owncloud.com
GNU Affero General Public License v3.0
8.37k stars 2.06k forks source link

Group share + reshare parent inconsistent when child share entry is missing #13361

Closed PVince81 closed 9 years ago

PVince81 commented 9 years ago

Steps to reproduce:

  1. Setup 7.0.2
  2. Create three users "u1", "u2" and "u3"
  3. Add "u1" and "u2" in a new group "g1"
  4. Login as "u1"
  5. Create a folder + subfolder "projects/subproject"
  6. Share "projects" with "g1" and check oc_share:
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+
| 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 | /projects   |          31 | 1421252385 |        0 | NULL       | NULL  |         0 |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+-------------+-------------+------------+----------+------------+-------+-----------+

Notice that no child entry is created. (in versions >= 7.0.3 a child entry is automatically created)

  1. Upgrade to 7.0.3, see that oc_share is still the same.
  2. Upgrade to 7.0.4, still no child entry
  3. Login as "u2"
  4. Share "projects/subproject" with "u3"
  5. Login as "u3"
  6. Check that "subprojects" can be seen in both "All files" and "Shared with You" sections
  7. Check oc_share:
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+----------------+-------------+------------+----------+------------+-------+-----------+
| 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 | /projects      |          31 | 1421252385 |        0 | NULL       | NULL  |         0 |
|  2 |          0 | u3         | u2        |      1 | folder    | 10          | /10         |          10 | /subproject    |          31 | 1421252546 |        0 | NULL       | NULL  |         0 |
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+----------------+-------------+------------+----------+------------+-------+-----------+

Notice that no child entry was created for u2 and the parent points directly to the group entry. However, if you tried this on 7.0.4 without the upgrade path, the child entry would be there and the reshare would point to the child entry instead of the parent. This works properly here but not sure if the behavior is correct, as it might cause inconsistencies.

Additionally @LukasReschke has met an environment where the reshare entry receives "NULL" as parent instead of the group parent id. Still not clear how to reproduce this.

Whenever the parent is "NULL", the reshare is orphaned and logging in as "u3" will show the share under "Shared with you" but not under "All files"

Versions

Reproduced on v7.0.4

@schiesbn I cannot reproduce the case where "NULL" appears as parent, maybe you have a clue when such situation could happen ? I still raised this ticket to document the case for the parent linking inconsistency. I'd say best would be if the group child entry is auto-created on reshare, to make sure we link to the correct one. Unless you say it should always link the parent to the group share ? Can anything go wrong here ?

LukasReschke commented 9 years ago

Thanks for debugging this with me @PVince81 much appreciated!

LukasReschke commented 9 years ago

Additionally @LukasReschke has met an environment where the reshare entry receives "NULL" as parent instead of the group parent id. Still not clear how to reproduce this.

If somebody feels adventurous to debug this behaviour on my coworking space's instance I can provide this person with access to a clone of that instance on my host system.

PVince81 commented 9 years ago

This guy here has the same issue, parent is NULL for any newly created reshares inside that old group share: https://github.com/owncloud/core/issues/12475#issuecomment-70102074

PVince81 commented 9 years ago

I wonder if it is related to the order of the shares in the database. Because now we have three setups that have been reported to produce parent=NULL, but in all my local tests the parent wasn't set to null even though the group-child was missing.

PVince81 commented 9 years ago

I'm setting this to 8.0 because it has been reported several times already and produces broken reshares.

PVince81 commented 9 years ago

CC @DeepDiver1975 for the milestone assignment

PVince81 commented 9 years ago

I managed to reproduce the NULL parent issue. Here are the steps from scratch:

  1. Setup OC 7.0.2
  2. Create three users "u1", "u2", "u3"
  3. Add "u1" and "u2" into a new group "g1"
  4. Add "u1", "u2" and "u3" into a new group "g2" (this is what was missing from above)
  5. Login as "u1"
  6. Create a folder "Projekte"
  7. Share "Projekte" with "g1" (the database will have only a single entry here, because this is 7.0.2)
  8. Login as "u2"
  9. Create a folder "Projekte/Subprojekt 2015"
  10. Share "Projekte/Subprojekt 2015" with "u3"
  11. Unshare it again
  12. Upgrade to 7.0.3
  13. Share "Projekte/Subprojekt 2015" with "u3" again
  14. Check the database
+----+------------+------------+-----------+--------+-----------+-------------+-------------+-------------+------------------+-------------+------------+----------+------------+-------+-----------+
| 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 | /Projekte        |          31 | 1421346556 |        0 | NULL       | NULL  |         0 |
|  3 |          0 | u3         | u2        |   NULL | folder    | 14          | /14         |          14 | /Subprojekt 2015 |          31 | 1421346719 |        0 | NULL       | NULL  |         0 |

The group share didn't have an entry for u2, so when resharing, the parent became "NULL". Note that if you didn't create the group "g2", the parent would point to 1. Very strange!

@schiesbn

schiessle commented 9 years ago

The group share doesn't need a entry for u2. user specific entries are only needed if the file_target differs. Re-Sharing should have the group share as parent. I will have a look at it tomorrow

PVince81 commented 9 years ago

getItemsSharedWithBySource() doesn't return anything in this specific situation. The SQL query returns no result.

If after that I unassign "g2" from all users and redo the reshare, it works! Crazy!

PVince81 commented 9 years ago

@schiesbn great, thanks. Maybe something goes wrong when checking for group memberships.

There's then another bug when the parent is assigned to the group share instead of child (when no child is there): if the resharer users (here u2) unshares from self. It adds the missing child entry with 0 permisisons. But the reshare is still there! It might be safer to reconsider this and make child entries mandatory to avoid additional cases to cover / potential bugs.

schiessle commented 9 years ago

It might be safer to reconsider this and make child entries mandatory to avoid additional cases to cover / potential bugs.

This would mean that we would need to update all group shares if users get added to groups. I would like to avoid this.

PVince81 commented 9 years ago

Understood. Then maybe the reshare code should then take care of adding the "missing" group child instead of linking directly to the parent.

PVince81 commented 9 years ago

Fix is here: https://github.com/owncloud/core/pull/13423

It was a bug in getItemSharedWithUser where multiple groups get wrongly concatenated into the SQL "IN" statement...

PVince81 commented 9 years ago

Fixed by https://github.com/owncloud/core/pull/13423