From 979d010dc27376904f4435ff9dfd93b3dc52554e Mon Sep 17 00:00:00 2001 From: Jeremy Lin Date: Thu, 2 Jul 2020 21:51:20 -0700 Subject: [PATCH] Add support for hiding passwords in a collection Ref: https://github.com/bitwarden/server/pull/743 --- .../down.sql | 0 .../up.sql | 2 + .../down.sql | 0 .../up.sql | 2 + .../down.sql | 0 .../up.sql | 2 + src/api/core/organizations.rs | 15 +- src/db/models/cipher.rs | 151 ++++++++++++------ src/db/models/collection.rs | 8 +- src/db/models/organization.rs | 11 +- src/db/schemas/mysql/schema.rs | 1 + src/db/schemas/postgresql/schema.rs | 1 + src/db/schemas/sqlite/schema.rs | 1 + 13 files changed, 136 insertions(+), 58 deletions(-) create mode 100644 migrations/mysql/2020-07-01-214531_add_hide_passwords/down.sql create mode 100644 migrations/mysql/2020-07-01-214531_add_hide_passwords/up.sql create mode 100644 migrations/postgresql/2020-07-01-214531_add_hide_passwords/down.sql create mode 100644 migrations/postgresql/2020-07-01-214531_add_hide_passwords/up.sql create mode 100644 migrations/sqlite/2020-07-01-214531_add_hide_passwords/down.sql create mode 100644 migrations/sqlite/2020-07-01-214531_add_hide_passwords/up.sql diff --git a/migrations/mysql/2020-07-01-214531_add_hide_passwords/down.sql b/migrations/mysql/2020-07-01-214531_add_hide_passwords/down.sql new file mode 100644 index 00000000..e69de29b diff --git a/migrations/mysql/2020-07-01-214531_add_hide_passwords/up.sql b/migrations/mysql/2020-07-01-214531_add_hide_passwords/up.sql new file mode 100644 index 00000000..4587232e --- /dev/null +++ b/migrations/mysql/2020-07-01-214531_add_hide_passwords/up.sql @@ -0,0 +1,2 @@ +ALTER TABLE users_collections +ADD COLUMN hide_passwords BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/migrations/postgresql/2020-07-01-214531_add_hide_passwords/down.sql b/migrations/postgresql/2020-07-01-214531_add_hide_passwords/down.sql new file mode 100644 index 00000000..e69de29b diff --git a/migrations/postgresql/2020-07-01-214531_add_hide_passwords/up.sql b/migrations/postgresql/2020-07-01-214531_add_hide_passwords/up.sql new file mode 100644 index 00000000..4587232e --- /dev/null +++ b/migrations/postgresql/2020-07-01-214531_add_hide_passwords/up.sql @@ -0,0 +1,2 @@ +ALTER TABLE users_collections +ADD COLUMN hide_passwords BOOLEAN NOT NULL DEFAULT FALSE; diff --git a/migrations/sqlite/2020-07-01-214531_add_hide_passwords/down.sql b/migrations/sqlite/2020-07-01-214531_add_hide_passwords/down.sql new file mode 100644 index 00000000..e69de29b diff --git a/migrations/sqlite/2020-07-01-214531_add_hide_passwords/up.sql b/migrations/sqlite/2020-07-01-214531_add_hide_passwords/up.sql new file mode 100644 index 00000000..8beb0d33 --- /dev/null +++ b/migrations/sqlite/2020-07-01-214531_add_hide_passwords/up.sql @@ -0,0 +1,2 @@ +ALTER TABLE users_collections +ADD COLUMN hide_passwords BOOLEAN NOT NULL DEFAULT 0; -- FALSE diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index af43b588..8adacda3 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -374,7 +374,7 @@ fn get_collection_users(org_id: String, coll_id: String, _headers: AdminHeaders, .map(|col_user| { UserOrganization::find_by_user_and_org(&col_user.user_uuid, &org_id, &conn) .unwrap() - .to_json_read_only(col_user.read_only) + .to_json_user_access_restrictions(&col_user) }) .collect(); @@ -408,7 +408,9 @@ fn put_collection_users( continue; } - CollectionUser::save(&user.user_uuid, &coll_id, d.ReadOnly, &conn)?; + CollectionUser::save(&user.user_uuid, &coll_id, + d.ReadOnly, d.HidePasswords, + &conn)?; } Ok(()) @@ -452,6 +454,7 @@ fn get_org_users(org_id: String, _headers: AdminHeaders, conn: DbConn) -> JsonRe struct CollectionData { Id: String, ReadOnly: bool, + HidePasswords: bool, } #[derive(Deserialize)] @@ -523,7 +526,9 @@ fn send_invite(org_id: String, data: JsonUpcase, headers: AdminHeade match Collection::find_by_uuid_and_org(&col.Id, &org_id, &conn) { None => err!("Collection not found in Organization"), Some(collection) => { - CollectionUser::save(&user.uuid, &collection.uuid, col.ReadOnly, &conn)?; + CollectionUser::save(&user.uuid, &collection.uuid, + col.ReadOnly, col.HidePasswords, + &conn)?; } } } @@ -778,7 +783,9 @@ fn edit_user( match Collection::find_by_uuid_and_org(&col.Id, &org_id, &conn) { None => err!("Collection not found in Organization"), Some(collection) => { - CollectionUser::save(&user_to_edit.user_uuid, &collection.uuid, col.ReadOnly, &conn)?; + CollectionUser::save(&user_to_edit.user_uuid, &collection.uuid, + col.ReadOnly, col.HidePasswords, + &conn)?; } } } diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index d0eb4e85..7641cbf1 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -82,6 +82,15 @@ impl Cipher { let fields_json = self.fields.as_ref().and_then(|s| serde_json::from_str(s).ok()).unwrap_or(Value::Null); let password_history_json = self.password_history.as_ref().and_then(|s| serde_json::from_str(s).ok()).unwrap_or(Value::Null); + let (read_only, hide_passwords) = + match self.get_access_restrictions(&user_uuid, &conn) { + Some((ro, hp)) => (ro, hp), + None => { + error!("Cipher ownership assertion failure"); + (true, true) + }, + }; + // Get the data or a default empty value to avoid issues with the mobile apps let mut data_json: Value = serde_json::from_str(&self.data).unwrap_or_else(|_| json!({ "Fields":null, @@ -105,7 +114,15 @@ impl Cipher { } // TODO: ******* Backwards compat end ********** + // There are three types of cipher response models in upstream + // Bitwarden: "cipherMini", "cipher", and "cipherDetails" (in order + // of increasing level of detail). bitwarden_rs currently only + // supports the "cipherDetails" type, though it seems like the + // Bitwarden clients will ignore extra fields. + // + // Ref: https://github.com/bitwarden/server/blob/master/src/Core/Models/Api/Response/CipherResponseModel.cs let mut json_object = json!({ + "Object": "cipherDetails", "Id": self.uuid, "Type": self.atype, "RevisionDate": format_date(&self.updated_at), @@ -115,6 +132,8 @@ impl Cipher { "OrganizationId": self.organization_uuid, "Attachments": attachments_json, "OrganizationUseTotp": true, + + // This field is specific to the cipherDetails type. "CollectionIds": self.get_collections(user_uuid, &conn), "Name": self.name, @@ -123,8 +142,11 @@ impl Cipher { "Data": data_json, - "Object": "cipher", - "Edit": true, + // These values are true by default, but can be false if the + // cipher belongs to a collection where the org owner has enabled + // the "Read Only" or "Hide Passwords" restrictions for the user. + "Edit": !read_only, + "ViewPassword": !hide_passwords, "PasswordHistory": password_history_json, }); @@ -241,66 +263,97 @@ impl Cipher { } } - pub fn is_write_accessible_to_user(&self, user_uuid: &str, conn: &DbConn) -> bool { + /// Returns whether this cipher is directly owned by the user. + pub fn is_owned_by_user(&self, user_uuid: &str, conn: &DbConn) -> bool { ciphers::table .filter(ciphers::uuid.eq(&self.uuid)) - .left_join( - users_organizations::table.on(ciphers::organization_uuid - .eq(users_organizations::org_uuid.nullable()) - .and(users_organizations::user_uuid.eq(user_uuid))), - ) - .left_join(ciphers_collections::table) - .left_join( - users_collections::table - .on(ciphers_collections::collection_uuid.eq(users_collections::collection_uuid)), - ) - .filter(ciphers::user_uuid.eq(user_uuid).or( - // Cipher owner - users_organizations::access_all.eq(true).or( - // access_all in Organization - users_organizations::atype.le(UserOrgType::Admin as i32).or( - // Org admin or owner - users_collections::user_uuid.eq(user_uuid).and( - users_collections::read_only.eq(false), //R/W access to collection - ), - ), - ), - )) - .select(ciphers::all_columns) + .filter(ciphers::user_uuid.eq(&user_uuid)) .first::(&**conn) .ok() .is_some() } - pub fn is_accessible_to_user(&self, user_uuid: &str, conn: &DbConn) -> bool { + /// Returns whether this cipher is owned by an org in which the user has full access. + pub fn is_in_full_access_org(&self, user_uuid: &str, conn: &DbConn) -> bool { ciphers::table .filter(ciphers::uuid.eq(&self.uuid)) - .left_join( - users_organizations::table.on(ciphers::organization_uuid - .eq(users_organizations::org_uuid.nullable()) - .and(users_organizations::user_uuid.eq(user_uuid))), - ) - .left_join(ciphers_collections::table) - .left_join( - users_collections::table - .on(ciphers_collections::collection_uuid.eq(users_collections::collection_uuid)), - ) - .filter(ciphers::user_uuid.eq(user_uuid).or( - // Cipher owner - users_organizations::access_all.eq(true).or( - // access_all in Organization - users_organizations::atype.le(UserOrgType::Admin as i32).or( - // Org admin or owner - users_collections::user_uuid.eq(user_uuid), // Access to Collection - ), - ), - )) - .select(ciphers::all_columns) - .first::(&**conn) + .inner_join(ciphers_collections::table.on( + ciphers::uuid.eq(ciphers_collections::cipher_uuid))) + .inner_join(users_organizations::table.on( + ciphers::organization_uuid.eq(users_organizations::org_uuid.nullable()) + .and(users_organizations::user_uuid.eq(user_uuid)) + .and(users_organizations::status.eq(UserOrgStatus::Confirmed as i32)))) + // The user is an org admin or higher. + .filter(users_organizations::atype.le(UserOrgType::Admin as i32)) + // The user was granted full access to the org by an org owner/admin. + .or_filter(users_organizations::access_all.eq(true)) + .select(ciphers::uuid) + .first::(&**conn) .ok() .is_some() } + /// Returns the user's access restrictions to this cipher. A return value + /// of None means that this cipher does not belong to the user, and is + /// not in any collection the user has access to. Otherwise, the user has + /// access to this cipher, and Some(read_only, hide_passwords) represents + /// the access restrictions. + pub fn get_access_restrictions(&self, user_uuid: &str, conn: &DbConn) -> Option<(bool, bool)> { + // Check whether this cipher is directly owned by the user, or is in + // a collection that the user has full access to. If so, there are no + // access restrictions. + if self.is_owned_by_user(&user_uuid, &conn) || self.is_in_full_access_org(&user_uuid, &conn) { + return Some((false, false)); + } + + // 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 + .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. + match query.load::<(bool, bool)>(&**conn).ok() { + Some(vec) => { + let mut read_only = false; + let mut hide_passwords = false; + for (ro, hp) in vec.iter() { + read_only |= ro; + hide_passwords |= hp; + } + + Some((read_only, hide_passwords)) + }, + None => { + // This cipher isn't in any collections accessible to the user. + None + } + } + } + + pub fn is_write_accessible_to_user(&self, user_uuid: &str, conn: &DbConn) -> bool { + match self.get_access_restrictions(&user_uuid, &conn) { + Some((read_only, _hide_passwords)) => !read_only, + None => false, + } + } + + pub fn is_accessible_to_user(&self, user_uuid: &str, conn: &DbConn) -> bool { + self.get_access_restrictions(&user_uuid, &conn).is_some() + } + pub fn get_folder_uuid(&self, user_uuid: &str, conn: &DbConn) -> Option { folders_ciphers::table .inner_join(folders::table) diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index 616d9637..1d8b1740 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -199,6 +199,7 @@ pub struct CollectionUser { pub user_uuid: String, pub collection_uuid: String, pub read_only: bool, + pub hide_passwords: bool, } /// Database methods @@ -214,7 +215,7 @@ impl CollectionUser { } #[cfg(feature = "postgresql")] - pub fn save(user_uuid: &str, collection_uuid: &str, read_only: bool, conn: &DbConn) -> EmptyResult { + pub fn save(user_uuid: &str, collection_uuid: &str, read_only: bool, hide_passwords: bool, conn: &DbConn) -> EmptyResult { User::update_uuid_revision(&user_uuid, conn); diesel::insert_into(users_collections::table) @@ -222,16 +223,18 @@ impl CollectionUser { users_collections::user_uuid.eq(user_uuid), users_collections::collection_uuid.eq(collection_uuid), users_collections::read_only.eq(read_only), + users_collections::hide_passwords.eq(hide_passwords), )) .on_conflict((users_collections::user_uuid, users_collections::collection_uuid)) .do_update() .set(users_collections::read_only.eq(read_only)) + .set(users_collections::hide_passwords.eq(hide_passwords)) .execute(&**conn) .map_res("Error adding user to collection") } #[cfg(not(feature = "postgresql"))] - pub fn save(user_uuid: &str, collection_uuid: &str, read_only: bool, conn: &DbConn) -> EmptyResult { + pub fn save(user_uuid: &str, collection_uuid: &str, read_only: bool, hide_passwords: bool, conn: &DbConn) -> EmptyResult { User::update_uuid_revision(&user_uuid, conn); diesel::replace_into(users_collections::table) @@ -239,6 +242,7 @@ impl CollectionUser { users_collections::user_uuid.eq(user_uuid), users_collections::collection_uuid.eq(collection_uuid), users_collections::read_only.eq(read_only), + users_collections::hide_passwords.eq(hide_passwords), )) .execute(&**conn) .map_res("Error adding user to collection") diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index fac5cad0..2cce7021 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -310,10 +310,11 @@ impl UserOrganization { }) } - pub fn to_json_read_only(&self, read_only: bool) -> Value { + pub fn to_json_user_access_restrictions(&self, col_user: &CollectionUser) -> Value { json!({ "Id": self.uuid, - "ReadOnly": read_only + "ReadOnly": col_user.read_only, + "HidePasswords": col_user.hide_passwords, }) } @@ -324,7 +325,11 @@ impl UserOrganization { let collections = CollectionUser::find_by_organization_and_user_uuid(&self.org_uuid, &self.user_uuid, conn); collections .iter() - .map(|c| json!({"Id": c.collection_uuid, "ReadOnly": c.read_only})) + .map(|c| json!({ + "Id": c.collection_uuid, + "ReadOnly": c.read_only, + "HidePasswords": c.hide_passwords, + })) .collect() }; diff --git a/src/db/schemas/mysql/schema.rs b/src/db/schemas/mysql/schema.rs index 42943689..4b8fbc7d 100644 --- a/src/db/schemas/mysql/schema.rs +++ b/src/db/schemas/mysql/schema.rs @@ -141,6 +141,7 @@ table! { user_uuid -> Varchar, collection_uuid -> Varchar, read_only -> Bool, + hide_passwords -> Bool, } } diff --git a/src/db/schemas/postgresql/schema.rs b/src/db/schemas/postgresql/schema.rs index adcbbd3c..ec13d263 100644 --- a/src/db/schemas/postgresql/schema.rs +++ b/src/db/schemas/postgresql/schema.rs @@ -141,6 +141,7 @@ table! { user_uuid -> Text, collection_uuid -> Text, read_only -> Bool, + hide_passwords -> Bool, } } diff --git a/src/db/schemas/sqlite/schema.rs b/src/db/schemas/sqlite/schema.rs index adcbbd3c..ec13d263 100644 --- a/src/db/schemas/sqlite/schema.rs +++ b/src/db/schemas/sqlite/schema.rs @@ -141,6 +141,7 @@ table! { user_uuid -> Text, collection_uuid -> Text, read_only -> Bool, + hide_passwords -> Bool, } }