From 7dff8c01dd86f69761f4822b8b0c41709f03f271 Mon Sep 17 00:00:00 2001 From: BlackDex Date: Sun, 31 Jan 2021 21:46:37 +0100 Subject: [PATCH] JSON Response updates and small fixes Updated several json response models. Also fixed a few small bugs. ciphers.rs: - post_ciphers_create: * Prevent cipher creation to organization without a collection. - update_cipher_from_data: * ~~Fixed removal of user_uuid which prevent user-owned shared-cipher to be not editable anymore when set to read-only.~~ * Cleanup the json_data by removing the `Response` key/values from several objects. - delete_all: * Do not delete all Collections during the Purge of an Organization (same as upstream). cipher.rs: - Cipher::to_json: * Updated json response to match upstream. * Return empty json object if there is no type_data instead of values which should not be set for the type_data. organizations.rs: * Added two new endpoints to prevent Javascript errors regarding tax organization.rs: - Organization::to_json: * Updated response model to match upstream - UserOrganization::to_json: * Updated response model to match upstream collection.rs: - Collection::{to_json, to_json_details}: * Updated the json response model, and added a detailed version used during the sync - hide_passwords_for_user: * Added this function to return if the passwords should be hidden or not for the user at the specific collection (used by `to_json_details`) Update 1: Some small changes after comments from @jjlin. Update 2: Fixed vault purge by user to make sure the cipher is not part of an organization. Resolves #971 Closes #990, Closes #991 --- src/api/core/ciphers.rs | 56 +++++++++++++++++++++-------- src/api/core/organizations.rs | 19 ++++++++++ src/db/models/cipher.rs | 67 ++++++++++++++++++++++------------- src/db/models/collection.rs | 32 ++++++++++++++++- src/db/models/organization.rs | 37 ++++++++++++++++--- 5 files changed, 167 insertions(+), 44 deletions(-) diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index 077689b6..a882be97 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -91,7 +91,9 @@ fn sync(data: Form, headers: Headers, conn: DbConn) -> JsonResult { let folders_json: Vec = folders.iter().map(Folder::to_json).collect(); let collections = Collection::find_by_user_uuid(&headers.user.uuid, &conn); - let collections_json: Vec = collections.iter().map(Collection::to_json).collect(); + let collections_json: Vec = collections.iter() + .map(|c| c.to_json_details(&headers.user.uuid, &conn)) + .collect(); let policies = OrgPolicy::find_by_user(&headers.user.uuid, &conn); let policies_json: Vec = policies.iter().map(OrgPolicy::to_json).collect(); @@ -225,6 +227,12 @@ fn post_ciphers_admin(data: JsonUpcase, headers: Headers, conn: fn post_ciphers_create(data: JsonUpcase, headers: Headers, conn: DbConn, nt: Notify) -> JsonResult { let mut data: ShareCipherData = data.into_inner().data; + // Check if there are one more more collections selected when this cipher is part of an organization. + // err if this is not the case before creating an empty cipher. + if data.Cipher.OrganizationId.is_some() && data.CollectionIds.is_empty() { + err!("You must select at least one collection."); + } + // This check is usually only needed in update_cipher_from_data(), but we // need it here as well to avoid creating an empty cipher in the call to // cipher.save() below. @@ -323,6 +331,11 @@ pub fn update_cipher_from_data( || cipher.is_write_accessible_to_user(&headers.user.uuid, &conn) { cipher.organization_uuid = Some(org_id); + // After some discussion in PR #1329 re-added the user_uuid = None again. + // TODO: Audit/Check the whole save/update cipher chain. + // Upstream uses the user_uuid to allow a cipher added by a user to an org to still allow the user to view/edit the cipher + // even when the user has hide-passwords configured as there policy. + // Removing the line below would fix that, but we have to check which effect this would have on the rest of the code. cipher.user_uuid = None; } else { err!("You don't have permission to add cipher directly to organization") @@ -366,6 +379,23 @@ pub fn update_cipher_from_data( } } + // Cleanup cipher data, like removing the 'Response' key. + // This key is somewhere generated during Javascript so no way for us this fix this. + // Also, upstream only retrieves keys they actually want to store, and thus skip the 'Response' key. + // We do not mind which data is in it, the keep our model more flexible when there are upstream changes. + // But, we at least know we do not need to store and return this specific key. + fn _clean_cipher_data(mut json_data: Value) -> Value { + if json_data.is_array() { + json_data.as_array_mut() + .unwrap() + .iter_mut() + .for_each(|ref mut f| { + f.as_object_mut().unwrap().remove("Response"); + }); + }; + json_data + } + let type_data_opt = match data.Type { 1 => data.Login, 2 => data.SecureNote, @@ -374,23 +404,22 @@ pub fn update_cipher_from_data( _ => err!("Invalid type"), }; - let mut type_data = match type_data_opt { - Some(data) => data, + let type_data = match type_data_opt { + Some(mut data) => { + // Remove the 'Response' key from the base object. + data.as_object_mut().unwrap().remove("Response"); + // Remove the 'Response' key from every Uri. + if data["Uris"].is_array() { + data["Uris"] = _clean_cipher_data(data["Uris"].clone()); + } + data + }, None => err!("Data missing"), }; - // TODO: ******* Backwards compat start ********** - // To remove backwards compatibility, just delete this code, - // and remove the compat code from cipher::to_json - type_data["Name"] = Value::String(data.Name.clone()); - type_data["Notes"] = data.Notes.clone().map(Value::String).unwrap_or(Value::Null); - type_data["Fields"] = data.Fields.clone().unwrap_or(Value::Null); - type_data["PasswordHistory"] = data.PasswordHistory.clone().unwrap_or(Value::Null); - // TODO: ******* Backwards compat end ********** - cipher.name = data.Name; cipher.notes = data.Notes; - cipher.fields = data.Fields.map(|f| f.to_string()); + cipher.fields = data.Fields.map(|f| _clean_cipher_data(f).to_string() ); cipher.data = type_data.to_string(); cipher.password_history = data.PasswordHistory.map(|f| f.to_string()); @@ -1064,7 +1093,6 @@ fn delete_all( Some(user_org) => { if user_org.atype == UserOrgType::Owner { Cipher::delete_all_by_organization(&org_data.org_id, &conn)?; - Collection::delete_all_by_organization(&org_data.org_id, &conn)?; nt.send_user_update(UpdateType::Vault, &user); Ok(()) } else { diff --git a/src/api/core/organizations.rs b/src/api/core/organizations.rs index c6840eda..56bc7a70 100644 --- a/src/api/core/organizations.rs +++ b/src/api/core/organizations.rs @@ -47,7 +47,9 @@ pub fn routes() -> Vec { list_policies_token, get_policy, put_policy, + get_organization_tax, get_plans, + get_plans_tax_rates, ] } @@ -1006,6 +1008,13 @@ fn put_policy(org_id: String, pol_type: i32, data: Json, _headers: A Ok(Json(policy.to_json())) } +#[allow(unused_variables)] +#[get("/organizations//tax")] +fn get_organization_tax(org_id: String, _headers: Headers, _conn: DbConn) -> EmptyResult { + // Prevent a 404 error, which also causes Javascript errors. + err!("Only allowed when not self hosted.") +} + #[get("/plans")] fn get_plans(_headers: Headers, _conn: DbConn) -> JsonResult { Ok(Json(json!({ @@ -1057,3 +1066,13 @@ fn get_plans(_headers: Headers, _conn: DbConn) -> JsonResult { "ContinuationToken": null }))) } + +#[get("/plans/sales-tax-rates")] +fn get_plans_tax_rates(_headers: Headers, _conn: DbConn) -> JsonResult { + // Prevent a 404 error, which also causes Javascript errors. + Ok(Json(json!({ + "Object": "list", + "Data": [], + "ContinuationToken": null + }))) +} diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index 0536d46b..d41ebfb8 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -83,7 +83,12 @@ impl Cipher { use crate::util::format_date; let attachments = Attachment::find_by_cipher(&self.uuid, conn); - let attachments_json: Vec = attachments.iter().map(|c| c.to_json(host)).collect(); + // When there are no attachments use null instead of an empty array + let attachments_json = if attachments.is_empty() { + Value::Null + } else { + attachments.iter().map(|c| c.to_json(host)).collect() + }; 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); @@ -97,28 +102,31 @@ impl Cipher { }, }; - // 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, - "Name": self.name, - "Notes":null, - "Password":null, - "PasswordHistory":null, - "PasswordRevisionDate":null, - "Response":null, - "Totp":null, - "Uris":null, - "Username":null - })); - - // TODO: ******* Backwards compat start ********** - // To remove backwards compatibility, just remove this entire section - // and remove the compat code from ciphers::update_cipher_from_data - if self.atype == 1 && data_json["Uris"].is_array() { - let uri = data_json["Uris"][0]["Uri"].clone(); - data_json["Uri"] = uri; + // Get the type_data or a default to an empty json object '{}'. + // If not passing an empty object, mobile clients will crash. + let mut type_data_json: Value = serde_json::from_str(&self.data).unwrap_or(json!({})); + + // NOTE: This was marked as *Backwards Compatibilty Code*, but as of January 2021 this is still being used by upstream + // Set the first element of the Uris array as Uri, this is needed several (mobile) clients. + if self.atype == 1 { + if type_data_json["Uris"].is_array() { + let uri = type_data_json["Uris"][0]["Uri"].clone(); + type_data_json["Uri"] = uri; + } else { + // Upstream always has an Uri key/value + type_data_json["Uri"] = Value::Null; + } } - // TODO: ******* Backwards compat end ********** + + // Clone the type_data and add some default value. + let mut data_json = type_data_json.clone(); + + // NOTE: This was marked as *Backwards Compatibilty Code*, but as of January 2021 this is still being used by upstream + // data_json should always contain the following keys with every atype + data_json["Fields"] = json!(fields_json); + data_json["Name"] = json!(self.name); + data_json["Notes"] = json!(self.notes); + data_json["PasswordHistory"] = json!(password_history_json); // There are three types of cipher response models in upstream // Bitwarden: "cipherMini", "cipher", and "cipherDetails" (in order @@ -137,6 +145,8 @@ impl Cipher { "Favorite": self.is_favorite(&user_uuid, conn), "OrganizationId": self.organization_uuid, "Attachments": attachments_json, + // We have UseTotp set to true by default within the Organization model. + // This variable together with UsersGetPremium is used to show or hide the TOTP counter. "OrganizationUseTotp": true, // This field is specific to the cipherDetails type. @@ -155,6 +165,12 @@ impl Cipher { "ViewPassword": !hide_passwords, "PasswordHistory": password_history_json, + + // All Cipher types are included by default as null, but only the matching one will be populated + "Login": null, + "SecureNote": null, + "Card": null, + "Identity": null, }); let key = match self.atype { @@ -165,7 +181,7 @@ impl Cipher { _ => panic!("Wrong type"), }; - json_object[key] = data_json; + json_object[key] = type_data_json; json_object } @@ -448,7 +464,10 @@ impl Cipher { pub fn find_owned_by_user(user_uuid: &str, conn: &DbConn) -> Vec { db_run! {conn: { ciphers::table - .filter(ciphers::user_uuid.eq(user_uuid)) + .filter( + ciphers::user_uuid.eq(user_uuid) + .and(ciphers::organization_uuid.is_null()) + ) .load::(conn).expect("Error loading ciphers").from_db() }} } diff --git a/src/db/models/collection.rs b/src/db/models/collection.rs index 4b506a97..4e75279d 100644 --- a/src/db/models/collection.rs +++ b/src/db/models/collection.rs @@ -49,12 +49,21 @@ impl Collection { pub fn to_json(&self) -> Value { json!({ + "ExternalId": null, // Not support by us "Id": self.uuid, "OrganizationId": self.org_uuid, "Name": self.name, "Object": "collection", }) } + + pub fn to_json_details(&self, user_uuid: &str, conn: &DbConn) -> Value { + let mut json_object = self.to_json(); + json_object["Object"] = json!("collectionDetails"); + json_object["ReadOnly"] = json!(!self.is_writable_by_user(user_uuid, conn)); + json_object["HidePasswords"] = json!(self.hide_passwords_for_user(user_uuid, conn)); + json_object + } } use crate::db::DbConn; @@ -236,6 +245,28 @@ impl Collection { } } } + + pub fn hide_passwords_for_user(&self, user_uuid: &str, conn: &DbConn) -> bool { + match UserOrganization::find_by_user_and_org(&user_uuid, &self.org_uuid, &conn) { + None => true, // Not in Org + Some(user_org) => { + if user_org.has_full_access() { + return false; + } + + db_run! { conn: { + users_collections::table + .filter(users_collections::collection_uuid.eq(&self.uuid)) + .filter(users_collections::user_uuid.eq(user_uuid)) + .filter(users_collections::hide_passwords.eq(true)) + .count() + .first::(conn) + .ok() + .unwrap_or(0) != 0 + }} + } + } + } } /// Database methods @@ -364,7 +395,6 @@ impl CollectionUser { diesel::delete(users_collections::table.filter( users_collections::user_uuid.eq(user_uuid) .and(users_collections::collection_uuid.eq(user.collection_uuid)) - )) .execute(conn) .map_res("Error removing user from collections")?; diff --git a/src/db/models/organization.rs b/src/db/models/organization.rs index 457330cf..455c2466 100644 --- a/src/db/models/organization.rs +++ b/src/db/models/organization.rs @@ -147,9 +147,10 @@ impl Organization { pub fn to_json(&self) -> Value { json!({ "Id": self.uuid, + "Identifier": null, // not supported by us "Name": self.name, - "Seats": 10, - "MaxCollections": 10, + "Seats": 10, // The value doesn't matter, we don't check server-side + "MaxCollections": 10, // The value doesn't matter, we don't check server-side "MaxStorageGb": 10, // The value doesn't matter, we don't check server-side "Use2fa": true, "UseDirectory": false, @@ -157,6 +158,9 @@ impl Organization { "UseGroups": false, "UseTotp": true, "UsePolicies": true, + "UseSso": false, // We do not support SSO + "SelfHost": true, + "UseApi": false, // not supported by us "BusinessName": null, "BusinessAddress1": null, @@ -274,9 +278,10 @@ impl UserOrganization { json!({ "Id": self.org_uuid, + "Identifier": null, // not supported by us "Name": org.name, - "Seats": 10, - "MaxCollections": 10, + "Seats": 10, // The value doesn't matter, we don't check server-side + "MaxCollections": 10, // The value doesn't matter, we don't check server-side "UsersGetPremium": true, "Use2fa": true, @@ -285,8 +290,30 @@ impl UserOrganization { "UseGroups": false, "UseTotp": true, "UsePolicies": true, - "UseApi": false, + "UseApi": false, // not supported by us "SelfHost": true, + "SsoBound": false, // We do not support SSO + "UseSso": false, // We do not support SSO + // TODO: Add support for Business Portal + // Upstream is moving Policies and SSO management outside of the web-vault to /portal + // For now they still have that code also in the web-vault, but they will remove it at some point. + // https://github.com/bitwarden/server/tree/master/bitwarden_license/src/ + "UseBusinessPortal": false, // Disable BusinessPortal Button + + // TODO: Add support for Custom User Roles + // See: https://bitwarden.com/help/article/user-types-access-control/#custom-role + // "Permissions": { + // "AccessBusinessPortal": false, + // "AccessEventLogs": false, + // "AccessImportExport": false, + // "AccessReports": false, + // "ManageAllCollections": false, + // "ManageAssignedCollections": false, + // "ManageGroups": false, + // "ManagePolicies": false, + // "ManageSso": false, + // "ManageUsers": false + // }, "MaxStorageGb": 10, // The value doesn't matter, we don't check server-side