diff --git a/unix_integration/src/db.rs b/unix_integration/src/db.rs index 49a491bca..367f319e8 100644 --- a/unix_integration/src/db.rs +++ b/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); }) } } diff --git a/unix_integration/tests/cache_layer_test.rs b/unix_integration/tests/cache_layer_test.rs index 915fa8db2..083837eaa 100644 --- a/unix_integration/tests/cache_layer_test.rs +++ b/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); +}