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.

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;

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;

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;

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;

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;

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 <https://www.sqlite.org/lang_altertable.html#altertabrename>.
+-- 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.

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<CipherData>, 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")
     }

src/db/models/cipher.rs

@@ -32,7 +32,6 @@ pub struct Cipher {
 
     pub data: String,
 
-    pub favorite: bool,
     pub password_history: Option<String>,
     pub deleted_at: Option<NaiveDateTime>,
 }
@@ -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::<i64>(&**conn).ok().unwrap_or(0) != 0
+    }
+
+    // Updates whether this cipher is a favorite of the specified user.
+    pub fn set_favorite(&self, favorite: Option<bool>, 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<String> {
         folders_ciphers::table
             .inner_join(folders::table)

src/db/schemas/mysql/schema.rs

@@ -20,7 +20,6 @@ table! {
         notes -> Nullable<Text>,
         fields -> Nullable<Text>,
         data -> Text,
-        favorite -> Bool,
         password_history -> Nullable<Text>,
         deleted_at -> Nullable<Datetime>,
     }
@@ -55,6 +54,13 @@ table! {
     }
 }
 
+table! {
+    favorites (user_uuid, cipher_uuid) {
+        user_uuid -> Text,
+        cipher_uuid -> Text,
+    }
+}
+
 table! {
     folders (uuid) {
         uuid -> Text,

src/db/schemas/postgresql/schema.rs

@@ -20,7 +20,6 @@ table! {
         notes -> Nullable<Text>,
         fields -> Nullable<Text>,
         data -> Text,
-        favorite -> Bool,
         password_history -> Nullable<Text>,
         deleted_at -> Nullable<Timestamp>,
     }
@@ -55,6 +54,13 @@ table! {
     }
 }
 
+table! {
+    favorites (user_uuid, cipher_uuid) {
+        user_uuid -> Text,
+        cipher_uuid -> Text,
+    }
+}
+
 table! {
     folders (uuid) {
         uuid -> Text,

src/db/schemas/sqlite/schema.rs

@@ -20,7 +20,6 @@ table! {
         notes -> Nullable<Text>,
         fields -> Nullable<Text>,
         data -> Text,
-        favorite -> Bool,
         password_history -> Nullable<Text>,
         deleted_at -> Nullable<Timestamp>,
     }
@@ -55,6 +54,13 @@ table! {
     }
 }
 
+table! {
+    favorites (user_uuid, cipher_uuid) {
+        user_uuid -> Text,
+        cipher_uuid -> Text,
+    }
+}
+
 table! {
     folders (uuid) {
         uuid -> Text,

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");
     }