mirror of
https://github.com/dani-garcia/vaultwarden.git
synced 2026-03-01 11:19:52 +03:00
Mobile app fails to sync when user is in organizations with multiple collections #169
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
HashSetinstead of aVec(I didn't want to simply change that, since my Rust knowledge is rudimentary and you may have a good reason to use aVecinstead of aHashSet).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.
@dani-garcia commented on GitHub (Dec 27, 2018):
I'm not sure why that's happening, those two queries do two independent things:
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.
@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/syncthat only returns assigned collections either directly or via access all.@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 28, 2018):
At least I was able to reproduce my initial problem with a new, empty database. Steps I did:
Now when I login with test2@localhost, the result of
/synccontains four collections (one is included twice).I'm still trying to reproduce how I managed to have ciphers reference the same collection multiple times.
@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?
@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.
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):
Ok I found it. I'll fork, patch and open a PR.