Mobile app fails to sync when user is in organizations with multiple collections #169

Closed
opened 2026-02-04 18:12:17 +03:00 by OVERLORD · 7 comments
Owner

Originally created by @aksdb on GitHub (Dec 27, 2018).

Description

I managed to setup a user that can no longer sync. The user is in an organization where he is only associated to one of two collections but has admin rights. Therefore he is still allowed to see all collections. The end result is, that the mobile app crashes immediately (right after sync and then right upon startup).
In a debug session on Android I found that the exception is apparently a duplicate key while creating the dictionary for the Collections the user can see. Indeed, one collection UUID is in there twice.

Root cause

From what I can tell, the root of that evil is in collections.rs: find_by_user_uuid. The result of that function is a vector containing the sum of two queries, therefore having the same UUID (possibly) twice in the resultset.

Solution

Either cleanup beforehand or use a HashSet instead of a Vec (I didn't want to simply change that, since my Rust knowledge is rudimentary and you may have a good reason to use a Vec instead of a
HashSet).

Remarks

I'm not sure why the .NET dictionary mapping is so fragile regarding duplicate keys, but I think the proper solution here is to not send duplicate keys in the first place.

Originally created by @aksdb on GitHub (Dec 27, 2018). #### Description I managed to setup a user that can no longer sync. The user is in an organization where he is only associated to one of two collections but has admin rights. Therefore he is still allowed to see all collections. The end result is, that the mobile app crashes immediately (right after sync and then right upon startup). In a debug session on Android I found that the exception is apparently a duplicate key while creating the dictionary for the Collections the user can see. Indeed, one collection UUID is in there twice. #### Root cause From what I can tell, the root of that evil is in `collections.rs`: `find_by_user_uuid`. The result of that function is a vector containing the *sum* of two queries, therefore having the same UUID (possibly) twice in the resultset. #### Solution Either cleanup beforehand or use a `HashSet` instead of a `Vec` (I didn't want to simply change that, since my Rust knowledge is rudimentary and you may have a good reason to use a `Vec` instead of a `HashSet`). #### Remarks I'm not sure why the .NET dictionary mapping is so fragile regarding duplicate keys, but I think the proper solution here is to not send duplicate keys in the first place.
OVERLORD added the bug label 2026-02-04 18:12:17 +03:00
Author
Owner

@dani-garcia commented on GitHub (Dec 27, 2018):

I'm not sure why that's happening, those two queries do two independent things:

  • The first one returns all the collections that the user has access_all enabled for (These shouldn't have entries in users_collections)
  • The second one returns the entries in users_collections, for the cases where the user only has access to specific collections.

If there are duplicate entries, that must mean the user has the access_all field set to true, but also has entries in users_collections, which doesn't make sense, because when access_all is true, those users_collections are not created.

If you go to the organization from the web vault, edit the user that is giving you problems, make sure that the access all collections is checked and click save, does the problem still happen?

But anyway, I think the cleanest solution would be to change the second query to check that access_all is equal to false, then there should be no possibility of overlap.

@dani-garcia commented on GitHub (Dec 27, 2018): I'm not sure why that's happening, those two queries do two independent things: - The first one returns all the collections that the user has access_all enabled for (These shouldn't have entries in users_collections) - The second one returns the entries in users_collections, for the cases where the user only has access to specific collections. If there are duplicate entries, that must mean the user has the access_all field set to true, but also has entries in users_collections, which doesn't make sense, because when access_all is true, those users_collections are not created. If you go to the organization from the web vault, edit the user that is giving you problems, make sure that the access all collections is checked and click save, does the problem still happen? But anyway, I think the cleanest solution would be to change the second query to check that access_all is equal to false, then there should be no possibility of overlap.
Author
Owner

@mprasil commented on GitHub (Dec 27, 2018):

I think @aksdb was implying that the user gets access to collection twice via direct collection assignment and via the admin privileges that inherently give access to all collections. However I can't reproduce the described behaviour of /api/sync that only returns assigned collections either directly or via access all.

@mprasil commented on GitHub (Dec 27, 2018): I think @aksdb was implying that the user gets access to collection twice via direct collection assignment and via the admin privileges that inherently give access to all collections. However I can't reproduce the described behaviour of `/api/sync` that only returns assigned collections either directly or via access all.
Author
Owner

@aksdb commented on GitHub (Dec 27, 2018):

Strange. The user had individual collections and also the allow_all flag set. Changing anything in the web ui did not clear the allow_all flag. I now did that manually.
Now the problem seems to be a slightly different one, because apparently one cipher in the collection returns the same collection UUID twice in the JSON response. I will try to clean that up as well.
So I think the problem is not as generic as I thought at first.... either I have a data corruption due to version updates or there is a race condition somewhere. I will try to reproduce it with a clean DB.

Edit: it's not only one cipher. Apparently all ciphers in a specific collection report their collection ID twice.

@aksdb commented on GitHub (Dec 27, 2018): Strange. The user had individual collections and also the allow_all flag set. Changing anything in the web ui did not clear the allow_all flag. I now did that manually. Now the problem seems to be a slightly different one, because apparently one cipher in the collection returns the same collection UUID twice in the JSON response. I will try to clean that up as well. So I think the problem is not as generic as I thought at first.... either I have a data corruption due to version updates or there is a race condition somewhere. I will try to reproduce it with a clean DB. Edit: it's not only one cipher. Apparently all ciphers in a specific collection report their collection ID twice.
Author
Owner

@aksdb commented on GitHub (Dec 28, 2018):

At least I was able to reproduce my initial problem with a new, empty database. Steps I did:

  • New User: test1@localhost
  • New User: test2@localhost
  • Login with test1
  • Create Org1
    • Create Col2
    • Invite test2 with admin, access all; confirm
  • Create Org2
    • Invite test2 with user, access one collection; confirm
  • Create item for Org1/DefaultCollection
  • Create item for Org1/Col2
  • Create item for Org2/DefaultCollection
  • Edit test2 in Org2: access only to DefaultCollection

Now when I login with test2@localhost, the result of /sync contains four collections (one is included twice).

I'm still trying to reproduce how I managed to have ciphers reference the same collection multiple times.

@aksdb commented on GitHub (Dec 28, 2018): At least I was able to reproduce my initial problem with a new, empty database. Steps I did: * New User: test1@localhost * New User: test2@localhost * Login with test1 * Create Org1 * Create Col2 * Invite test2 with admin, access all; confirm * Create Org2 * Invite test2 with user, access one collection; confirm * Create item for Org1/DefaultCollection * Create item for Org1/Col2 * Create item for Org2/DefaultCollection * Edit test2 in Org2: access only to DefaultCollection Now when I login with test2@localhost, the result of `/sync` contains *four collections* (one is included twice). I'm still trying to reproduce how I managed to have ciphers reference the same collection multiple times.
Author
Owner

@mprasil commented on GitHub (Dec 28, 2018):

I've created a PR, that changes the function for fetching collections. Can you compile my branch and see if that fixes the sync?

@mprasil commented on GitHub (Dec 28, 2018): I've created a PR, that changes the function for fetching collections. Can you compile my branch and see if that fixes the sync?
Author
Owner

@aksdb commented on GitHub (Dec 28, 2018):

@mprasil The PR seems to remedy my initial problem. Cool!
One thing that it does differently from before (afaics) is that someone with role admin can still be restricted in collections he can see. I do actually find that better though, since the (admin) user can then still edit himself if he wants to see more collections. It's even cool that the admin can now restrict the items he sees.
Since I'm not running the official Bitwarden server, I cannot compare how that one handles it (ok, I could of course check the source code .... ) but I think it's not necessary for bitwarden_rs to behave exactly the same as the official server anyway.

My other problem still persists though, and I have not yet figured out how to reproduce it on a clean DB.

{
  "Ciphers": [
    {
      "Attachments": [],
      "CollectionIds": [
        "e1697705-11c0-45ad-a9c0-49d136a1807b",
        "e1697705-11c0-45ad-a9c0-49d136a1807b"
      ],
....

I think the only cause for this could be within the JOINs ... but since all join conditions are on the UUIDs, that doesn't make much sense. But I keep investigating :-)

@aksdb commented on GitHub (Dec 28, 2018): @mprasil The PR seems to remedy my initial problem. Cool! One thing that it does differently from before (afaics) is that someone with role admin can still be restricted in collections he can see. I do actually find that better though, since the (admin) user can then still edit himself if he wants to see more collections. It's even cool that the admin can now restrict the items he sees. Since I'm not running the official Bitwarden server, I cannot compare how that one handles it (ok, I could of course check the source code .... ) but I think it's not necessary for bitwarden_rs to behave exactly the same as the official server anyway. My other problem still persists though, and I have not yet figured out how to reproduce it on a clean DB. ```json { "Ciphers": [ { "Attachments": [], "CollectionIds": [ "e1697705-11c0-45ad-a9c0-49d136a1807b", "e1697705-11c0-45ad-a9c0-49d136a1807b" ], .... ``` I think the only cause for this could be within the JOINs ... but since all join conditions are on the UUIDs, that doesn't make much sense. But I keep investigating :-)
Author
Owner

@aksdb commented on GitHub (Dec 28, 2018):

Ok I found it. I'll fork, patch and open a PR.

@aksdb commented on GitHub (Dec 28, 2018): Ok I found it. I'll fork, patch and open a PR.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/vaultwarden#169