Resolve issue with order of operations causing group memberships to disappear (#1845)

unix_integration/src/db.rs

@@ -153,12 +153,18 @@ impl<'a> DbTxn<'a> {
                 self.sqlite_error("group_t create", &e);
             })?;
 
+        // We defer group foreign keys here because we now manually cascade delete these when
+        // required. This is because insert or replace into will always delete then add
+        // which triggers this. So instead we defer and manually cascade.
+        //
+        // However, on accounts, we CAN delete cascade because accounts will always redefine
+        // their memberships on updates so this is safe to cascade on this direction.
         self.conn
             .execute(
                 "CREATE TABLE IF NOT EXISTS memberof_t (
                 g_uuid TEXT,
                 a_uuid TEXT,
-                FOREIGN KEY(g_uuid) REFERENCES group_t(uuid) ON DELETE CASCADE,
+                FOREIGN KEY(g_uuid) REFERENCES group_t(uuid) DEFERRABLE INITIALLY DEFERRED,
                 FOREIGN KEY(a_uuid) REFERENCES account_t(uuid) ON DELETE CASCADE
             )
             ",
@@ -204,6 +210,12 @@ impl<'a> DbTxn<'a> {
     }
 
     pub fn clear_cache(&self) -> Result<(), ()> {
+        self.conn
+            .execute("DELETE FROM memberof_t", [])
+            .map_err(|e| {
+                self.sqlite_error("delete memberof_t", &e);
+            })?;
+
         self.conn.execute("DELETE FROM group_t", []).map_err(|e| {
             self.sqlite_error("delete group_t", &e);
         })?;
@@ -407,6 +419,7 @@ impl<'a> DbTxn<'a> {
             .map_err(|e| {
                 self.sqlite_error("prepare", &e);
             })?;
+
         stmt.execute([&account.uuid])
             .map(|r| {
                 debug!("delete memberships -> {:?}", r);
@@ -437,6 +450,16 @@ impl<'a> DbTxn<'a> {
     }
 
     pub fn delete_account(&self, a_uuid: &str) -> Result<(), ()> {
+        self.conn
+            .execute(
+                "DELETE FROM memberof_t WHERE a_uuid = :a_uuid",
+                params![a_uuid],
+            )
+            .map(|_| ())
+            .map_err(|e| {
+                self.sqlite_error("account_t memberof_t cascade delete", &e);
+            })?;
+
         self.conn
             .execute(
                 "DELETE FROM account_t WHERE uuid = :a_uuid",
@@ -444,7 +467,7 @@ impl<'a> DbTxn<'a> {
             )
             .map(|_| ())
             .map_err(|e| {
-                self.sqlite_error("memberof_t create", &e);
+                self.sqlite_error("account_t delete", &e);
             })
     }
 
@@ -731,11 +754,17 @@ impl<'a> DbTxn<'a> {
     }
 
     pub fn delete_group(&self, g_uuid: &str) -> Result<(), ()> {
+        self.conn
+            .execute("DELETE FROM memberof_t WHERE g_uuid = :g_uuid", [g_uuid])
+            .map(|_| ())
+            .map_err(|e| {
+                self.sqlite_error("group_t memberof_t cascade delete", &e);
+            })?;
         self.conn
             .execute("DELETE FROM group_t WHERE uuid = :g_uuid", [g_uuid])
             .map(|_| ())
             .map_err(|e| {
-                self.sqlite_error("memberof_t create", &e);
+                self.sqlite_error("group_t delete", &e);
             })
     }
 }

unix_integration/tests/cache_layer_test.rs

@@ -846,3 +846,47 @@ async fn test_cache_nxset_allow_overrides() {
         .expect("Failed to get from cache");
     assert!(gt.is_some());
 }
+
+/// Issue 1830. If cache items expire where we have an account and a group, and we
+/// refresh the group *first*, the group appears to drop it's members. This is because
+/// sqlite "INSERT OR REPLACE INTO" triggers a delete cascade of the foreign key elements
+/// which then makes the group appear empty.
+///
+/// We can reproduce this by retrieving an account + group, wait for expiry, then retrieve
+/// only the group.
+#[tokio::test]
+async fn test_cache_group_fk_deferred() {
+    let (cachelayer, _adminclient) = setup_test(fixture(test_fixture)).await;
+
+    cachelayer.attempt_online().await;
+    assert!(cachelayer.test_connection().await);
+
+    // Get the account then the group.
+    let ut = cachelayer
+        .get_nssaccount_name("testaccount1")
+        .await
+        .expect("Failed to get from cache");
+    assert!(ut.is_some());
+
+    let gt = cachelayer
+        .get_nssgroup_name("testgroup1")
+        .await
+        .expect("Failed to get from cache");
+    assert!(gt.is_some());
+    assert!(gt.unwrap().members.len() == 1);
+
+    // Invalidate all items.
+    cachelayer.mark_offline().await;
+    assert!(cachelayer.invalidate().await.is_ok());
+    cachelayer.attempt_online().await;
+    assert!(cachelayer.test_connection().await);
+
+    // Get the *group*. It *should* still have it's members.
+    let gt = cachelayer
+        .get_nssgroup_name("testgroup1")
+        .await
+        .expect("Failed to get from cache");
+    assert!(gt.is_some());
+    // And check we have members in the group, since we came from a userlook up
+    assert!(gt.unwrap().members.len() == 1);
+}