vw_small

commit 6cbb72406980bb0127b378a770195066bacaf02e
parent a2316ca091ebb7f6e4ed06d150d72474ef103839
Author: Jeremy Lin <jeremy.lin@gmail.com>
Date:   Fri, 29 Oct 2021 11:45:43 -0700

Fix conflict resolution logic for `read_only` and `hide_passwords` flags

For one of these flags to be in effect for a cipher, upstream requires all of
(rather than any of) the collections the cipher is in to have that flag set.

Also, some of the logic for loading access restrictions was wrong. I think
that only malicious clients that also had knowledge of the UUIDs of ciphers
they didn't have access to would have been able to take advantage of that.

Diffstat:
M
src/db/models/cipher.rs
 | 
45
++++++++++++++++++++++++---------------------

1 file changed, 24 insertions(+), 21 deletions(-)
diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs
@@ -343,36 +343,39 @@ impl Cipher {
         db_run! {conn: {
             // Check whether this cipher is in any collections accessible to the
             // user. If so, retrieve the access flags for each collection.
-            let query = ciphers::table
+            let rows = ciphers::table
                 .filter(ciphers::uuid.eq(&self.uuid))
                 .inner_join(ciphers_collections::table.on(
                     ciphers::uuid.eq(ciphers_collections::cipher_uuid)))
                 .inner_join(users_collections::table.on(
                     ciphers_collections::collection_uuid.eq(users_collections::collection_uuid)
                         .and(users_collections::user_uuid.eq(user_uuid))))
-                .select((users_collections::read_only, users_collections::hide_passwords));
-
-            // There's an edge case where a cipher can be in multiple collections
-            // with inconsistent access flags. For example, a cipher could be in
-            // one collection where the user has read-only access, but also in
-            // another collection where the user has read/write access. To handle
-            // this, we do a boolean OR of all values in each of the `read_only`
-            // and `hide_passwords` columns. This could ideally be done as part
-            // of the query, but Diesel doesn't support a max() or bool_or()
-            // function on booleans and this behavior isn't portable anyway.
-            if let Ok(vec) = query.load::<(bool, bool)>(conn) {
-                let mut read_only = false;
-                let mut hide_passwords = false;
-                for (ro, hp) in vec.iter() {
-                    read_only |= ro;
-                    hide_passwords |= hp;
-                }
+                .select((users_collections::read_only, users_collections::hide_passwords))
+                .load::<(bool, bool)>(conn)
+                .expect("Error getting access restrictions");
 
-                Some((read_only, hide_passwords))
-            } else {
+            if rows.is_empty() {
                 // This cipher isn't in any collections accessible to the user.
-                None
+                return None;
             }
+
+            // A cipher can be in multiple collections with inconsistent access flags.
+            // For example, a cipher could be in one collection where the user has
+            // read-only access, but also in another collection where the user has
+            // read/write access. For a flag to be in effect for a cipher, upstream
+            // requires all collections the cipher is in to have that flag set.
+            // Therefore, we do a boolean AND of all values in each of the `read_only`
+            // and `hide_passwords` columns. This could ideally be done as part of the
+            // query, but Diesel doesn't support a min() or bool_and() function on
+            // booleans and this behavior isn't portable anyway.
+            let mut read_only = true;
+            let mut hide_passwords = true;
+            for (ro, hp) in rows.iter() {
+                read_only &= ro;
+                hide_passwords &= hp;
+            }
+
+            Some((read_only, hide_passwords))
         }}
     }