From f83a8a36d16eb14c4d2f68f7edf7989bbf7973cb Mon Sep 17 00:00:00 2001 From: Jeremy Lin Date: Wed, 19 Aug 2020 02:16:27 -0700 Subject: [PATCH] Track favorites on a per-user basis Currently, favorites are tracked at the cipher level. For org-owned ciphers, this means that if one user sets it as a favorite, it automatically becomes a favorite for all other users that the cipher has been shared with. --- .../down.sql | 4 ++ .../up.sql | 9 +++ .../down.sql | 4 ++ .../up.sql | 9 +++ .../down.sql | 4 ++ .../up.sql | 64 +++++++++++++++++++ src/api/core/ciphers.rs | 7 +- src/db/models/cipher.rs | 48 +++++++++++++- src/db/schemas/mysql/schema.rs | 8 ++- src/db/schemas/postgresql/schema.rs | 8 ++- src/db/schemas/sqlite/schema.rs | 8 ++- src/main.rs | 13 +++- 12 files changed, 178 insertions(+), 8 deletions(-) create mode 100644 migrations/mysql/2020-08-02-025025_add_favorites_table/down.sql create mode 100644 migrations/mysql/2020-08-02-025025_add_favorites_table/up.sql create mode 100644 migrations/postgresql/2020-08-02-025025_add_favorites_table/down.sql create mode 100644 migrations/postgresql/2020-08-02-025025_add_favorites_table/up.sql create mode 100644 migrations/sqlite/2020-08-02-025025_add_favorites_table/down.sql create mode 100644 migrations/sqlite/2020-08-02-025025_add_favorites_table/up.sql diff --git a/migrations/mysql/2020-08-02-025025_add_favorites_table/down.sql b/migrations/mysql/2020-08-02-025025_add_favorites_table/down.sql new file mode 100644 index 0000000..9b79b63 --- /dev/null +++ b/migrations/mysql/2020-08-02-025025_add_favorites_table/down.sql @@ -0,0 +1,4 @@ +DROP TABLE favorites; + +ALTER TABLE ciphers +ADD COLUMN favorite BOOLEAN NOT NULL; diff --git a/migrations/mysql/2020-08-02-025025_add_favorites_table/up.sql b/migrations/mysql/2020-08-02-025025_add_favorites_table/up.sql new file mode 100644 index 0000000..904c15e --- /dev/null +++ b/migrations/mysql/2020-08-02-025025_add_favorites_table/up.sql @@ -0,0 +1,9 @@ +CREATE TABLE favorites ( + user_uuid CHAR(36) NOT NULL REFERENCES users(uuid), + cipher_uuid CHAR(36) NOT NULL REFERENCES ciphers(uuid), + + PRIMARY KEY (user_uuid, cipher_uuid) +); + +ALTER TABLE ciphers +DROP COLUMN favorite; diff --git a/migrations/postgresql/2020-08-02-025025_add_favorites_table/down.sql b/migrations/postgresql/2020-08-02-025025_add_favorites_table/down.sql new file mode 100644 index 0000000..9b79b63 --- /dev/null +++ b/migrations/postgresql/2020-08-02-025025_add_favorites_table/down.sql @@ -0,0 +1,4 @@ +DROP TABLE favorites; + +ALTER TABLE ciphers +ADD COLUMN favorite BOOLEAN NOT NULL; diff --git a/migrations/postgresql/2020-08-02-025025_add_favorites_table/up.sql b/migrations/postgresql/2020-08-02-025025_add_favorites_table/up.sql new file mode 100644 index 0000000..925eb2b --- /dev/null +++ b/migrations/postgresql/2020-08-02-025025_add_favorites_table/up.sql @@ -0,0 +1,9 @@ +CREATE TABLE favorites ( + user_uuid VARCHAR(40) NOT NULL REFERENCES users(uuid), + cipher_uuid VARCHAR(40) NOT NULL REFERENCES ciphers(uuid), + + PRIMARY KEY (user_uuid, cipher_uuid) +); + +ALTER TABLE ciphers +DROP COLUMN favorite; diff --git a/migrations/sqlite/2020-08-02-025025_add_favorites_table/down.sql b/migrations/sqlite/2020-08-02-025025_add_favorites_table/down.sql new file mode 100644 index 0000000..9b79b63 --- /dev/null +++ b/migrations/sqlite/2020-08-02-025025_add_favorites_table/down.sql @@ -0,0 +1,4 @@ +DROP TABLE favorites; + +ALTER TABLE ciphers +ADD COLUMN favorite BOOLEAN NOT NULL; diff --git a/migrations/sqlite/2020-08-02-025025_add_favorites_table/up.sql b/migrations/sqlite/2020-08-02-025025_add_favorites_table/up.sql new file mode 100644 index 0000000..c00a790 --- /dev/null +++ b/migrations/sqlite/2020-08-02-025025_add_favorites_table/up.sql @@ -0,0 +1,64 @@ +CREATE TABLE favorites ( + user_uuid TEXT NOT NULL REFERENCES users(uuid), + cipher_uuid TEXT NOT NULL REFERENCES ciphers(uuid), + + PRIMARY KEY (user_uuid, cipher_uuid) +); + +-- Drop the `favorite` column from the `ciphers` table, using the 12-step +-- procedure from . +-- Note that some steps aren't applicable and are omitted. + +-- 1. If foreign key constraints are enabled, disable them using PRAGMA foreign_keys=OFF. +-- +-- Diesel runs each migration in its own transaction. `PRAGMA foreign_keys` +-- is a no-op within a transaction, so this step must be done outside of this +-- file, before starting the Diesel migrations. + +-- 2. Start a transaction. +-- +-- Diesel already runs each migration in its own transaction. + +-- 4. Use CREATE TABLE to construct a new table "new_X" that is in the +-- desired revised format of table X. Make sure that the name "new_X" does +-- not collide with any existing table name, of course. + +CREATE TABLE new_ciphers( + uuid TEXT NOT NULL PRIMARY KEY, + created_at DATETIME NOT NULL, + updated_at DATETIME NOT NULL, + user_uuid TEXT REFERENCES users(uuid), + organization_uuid TEXT REFERENCES organizations(uuid), + atype INTEGER NOT NULL, + name TEXT NOT NULL, + notes TEXT, + fields TEXT, + data TEXT NOT NULL, + password_history TEXT, + deleted_at DATETIME +); + +-- 5. Transfer content from X into new_X using a statement like: +-- INSERT INTO new_X SELECT ... FROM X. + +INSERT INTO new_ciphers(uuid, created_at, updated_at, user_uuid, organization_uuid, atype, + name, notes, fields, data, password_history, deleted_at) +SELECT uuid, created_at, updated_at, user_uuid, organization_uuid, atype, + name, notes, fields, data, password_history, deleted_at +FROM ciphers; + +-- 6. Drop the old table X: DROP TABLE X. + +DROP TABLE ciphers; + +-- 7. Change the name of new_X to X using: ALTER TABLE new_X RENAME TO X. + +ALTER TABLE new_ciphers RENAME TO ciphers; + +-- 11. Commit the transaction started in step 2. + +-- 12. If foreign keys constraints were originally enabled, reenable them now. +-- +-- `PRAGMA foreign_keys` is scoped to a database connection, and Diesel +-- migrations are run in a separate database connection that is closed once +-- the migrations finish. diff --git a/src/api/core/ciphers.rs b/src/api/core/ciphers.rs index 7ea2dbe..53760ae 100644 --- a/src/api/core/ciphers.rs +++ b/src/api/core/ciphers.rs @@ -303,7 +303,6 @@ pub fn update_cipher_from_data( type_data["PasswordHistory"] = data.PasswordHistory.clone().unwrap_or(Value::Null); // TODO: ******* Backwards compat end ********** - cipher.favorite = data.Favorite.unwrap_or(false); cipher.name = data.Name; cipher.notes = data.Notes; cipher.fields = data.Fields.map(|f| f.to_string()); @@ -312,6 +311,7 @@ pub fn update_cipher_from_data( cipher.save(&conn)?; cipher.move_to_folder(data.FolderId, &headers.user.uuid, &conn)?; + cipher.set_favorite(data.Favorite, &headers.user.uuid, &conn)?; if ut != UpdateType::None { nt.send_cipher_update(ut, &cipher, &cipher.update_users_revision(&conn)); @@ -410,6 +410,11 @@ fn put_cipher(uuid: String, data: JsonUpcase, headers: Headers, conn None => err!("Cipher doesn't exist"), }; + // TODO: Check if only the folder ID or favorite status is being changed. + // These are per-user properties that technically aren't part of the + // cipher itself, so the user shouldn't need write access to change these. + // Interestingly, upstream Bitwarden doesn't properly handle this either. + if !cipher.is_write_accessible_to_user(&headers.user.uuid, &conn) { err!("Cipher is not write accessible") } diff --git a/src/db/models/cipher.rs b/src/db/models/cipher.rs index 3bf0ea6..5328d9d 100644 --- a/src/db/models/cipher.rs +++ b/src/db/models/cipher.rs @@ -32,7 +32,6 @@ pub struct Cipher { pub data: String, - pub favorite: bool, pub password_history: Option, pub deleted_at: Option, } @@ -51,7 +50,6 @@ impl Cipher { organization_uuid: None, atype, - favorite: false, name, notes: None, @@ -128,7 +126,7 @@ impl Cipher { "RevisionDate": format_date(&self.updated_at), "DeletedDate": self.deleted_at.map_or(Value::Null, |d| Value::String(format_date(&d))), "FolderId": self.get_folder_uuid(&user_uuid, &conn), - "Favorite": self.favorite, + "Favorite": self.is_favorite(&user_uuid, &conn), "OrganizationId": self.organization_uuid, "Attachments": attachments_json, "OrganizationUseTotp": true, @@ -337,6 +335,50 @@ impl Cipher { self.get_access_restrictions(&user_uuid, &conn).is_some() } + // Returns whether this cipher is a favorite of the specified user. + pub fn is_favorite(&self, user_uuid: &str, conn: &DbConn) -> bool { + let query = favorites::table + .filter(favorites::user_uuid.eq(user_uuid)) + .filter(favorites::cipher_uuid.eq(&self.uuid)) + .count(); + + query.first::(&**conn).ok().unwrap_or(0) != 0 + } + + // Updates whether this cipher is a favorite of the specified user. + pub fn set_favorite(&self, favorite: Option, user_uuid: &str, conn: &DbConn) -> EmptyResult { + if favorite.is_none() { + // No change requested. + return Ok(()); + } + + let (old, new) = (self.is_favorite(user_uuid, &conn), favorite.unwrap()); + match (old, new) { + (false, true) => { + User::update_uuid_revision(user_uuid, &conn); + diesel::insert_into(favorites::table) + .values(( + favorites::user_uuid.eq(user_uuid), + favorites::cipher_uuid.eq(&self.uuid), + )) + .execute(&**conn) + .map_res("Error adding favorite") + } + (true, false) => { + User::update_uuid_revision(user_uuid, &conn); + diesel::delete( + favorites::table + .filter(favorites::user_uuid.eq(user_uuid)) + .filter(favorites::cipher_uuid.eq(&self.uuid)) + ) + .execute(&**conn) + .map_res("Error removing favorite") + } + // Otherwise, the favorite status is already what it should be. + _ => Ok(()) + } + } + pub fn get_folder_uuid(&self, user_uuid: &str, conn: &DbConn) -> Option { folders_ciphers::table .inner_join(folders::table) diff --git a/src/db/schemas/mysql/schema.rs b/src/db/schemas/mysql/schema.rs index 36ee512..a353fa5 100644 --- a/src/db/schemas/mysql/schema.rs +++ b/src/db/schemas/mysql/schema.rs @@ -20,7 +20,6 @@ table! { notes -> Nullable, fields -> Nullable, data -> Text, - favorite -> Bool, password_history -> Nullable, deleted_at -> Nullable, } @@ -55,6 +54,13 @@ table! { } } +table! { + favorites (user_uuid, cipher_uuid) { + user_uuid -> Text, + cipher_uuid -> Text, + } +} + table! { folders (uuid) { uuid -> Text, diff --git a/src/db/schemas/postgresql/schema.rs b/src/db/schemas/postgresql/schema.rs index ec13d26..fcacb3e 100644 --- a/src/db/schemas/postgresql/schema.rs +++ b/src/db/schemas/postgresql/schema.rs @@ -20,7 +20,6 @@ table! { notes -> Nullable, fields -> Nullable, data -> Text, - favorite -> Bool, password_history -> Nullable, deleted_at -> Nullable, } @@ -55,6 +54,13 @@ table! { } } +table! { + favorites (user_uuid, cipher_uuid) { + user_uuid -> Text, + cipher_uuid -> Text, + } +} + table! { folders (uuid) { uuid -> Text, diff --git a/src/db/schemas/sqlite/schema.rs b/src/db/schemas/sqlite/schema.rs index ec13d26..fcacb3e 100644 --- a/src/db/schemas/sqlite/schema.rs +++ b/src/db/schemas/sqlite/schema.rs @@ -20,7 +20,6 @@ table! { notes -> Nullable, fields -> Nullable, data -> Text, - favorite -> Bool, password_history -> Nullable, deleted_at -> Nullable, } @@ -55,6 +54,13 @@ table! { } } +table! { + favorites (user_uuid, cipher_uuid) { + user_uuid -> Text, + cipher_uuid -> Text, + } +} + table! { folders (uuid) { uuid -> Text, diff --git a/src/main.rs b/src/main.rs index 2ce16a7..b9c8bcf 100644 --- a/src/main.rs +++ b/src/main.rs @@ -313,12 +313,23 @@ mod migrations { // Disable Foreign Key Checks during migration use diesel::RunQueryDsl; + + // FIXME: Per https://www.postgresql.org/docs/12/sql-set-constraints.html, + // "SET CONSTRAINTS sets the behavior of constraint checking within the + // current transaction", so this setting probably won't take effect for + // any of the migrations since it's being run outside of a transaction. + // Migrations that need to disable foreign key checks should run this + // from within the migration script itself. #[cfg(feature = "postgres")] diesel::sql_query("SET CONSTRAINTS ALL DEFERRED").execute(&connection).expect("Failed to disable Foreign Key Checks during migrations"); + + // Scoped to a connection/session. #[cfg(feature = "mysql")] diesel::sql_query("SET FOREIGN_KEY_CHECKS = 0").execute(&connection).expect("Failed to disable Foreign Key Checks during migrations"); + + // Scoped to a connection. #[cfg(feature = "sqlite")] - diesel::sql_query("PRAGMA defer_foreign_keys = ON").execute(&connection).expect("Failed to disable Foreign Key Checks during migrations"); + diesel::sql_query("PRAGMA foreign_keys = OFF").execute(&connection).expect("Failed to disable Foreign Key Checks during migrations"); embedded_migrations::run_with_output(&connection, &mut stdout()).expect("Can't run migrations"); }