2756 - resolve invalid loading of dyngroups at startup (#2779)

* 2756 - resolve invalid loading of dyngroups at startup
* Add a "patch level" migration for domain one shot fixes

proto/src/constants.rs

@@ -149,6 +149,7 @@ pub const ATTR_OBJECTCLASS: &str = "objectclass";
 pub const ATTR_OTHER_NO_INDEX: &str = "other-no-index";
 pub const ATTR_PASSKEYS: &str = "passkeys";
 pub const ATTR_PASSWORD_IMPORT: &str = "password_import";
+pub const ATTR_PATCH_LEVEL: &str = "patch_level";
 pub const ATTR_PHANTOM: &str = "phantom";
 pub const ATTR_PRIMARY_CREDENTIAL: &str = "primary_credential";
 pub const ATTR_TOTP_IMPORT: &str = "totp_import";

server/lib/src/constants/entries.rs

@@ -145,6 +145,7 @@ pub enum Attribute {
     OtherNoIndex,
     PassKeys,
     PasswordImport,
+    PatchLevel,
     Phantom,
     PrimaryCredential,
     PrivateCookieKey,
@@ -341,6 +342,7 @@ impl TryFrom<String> for Attribute {
             ATTR_OTHER_NO_INDEX => Attribute::OtherNoIndex,
             ATTR_PASSKEYS => Attribute::PassKeys,
             ATTR_PASSWORD_IMPORT => Attribute::PasswordImport,
+            ATTR_PATCH_LEVEL => Attribute::PatchLevel,
             ATTR_PHANTOM => Attribute::Phantom,
             ATTR_PRIMARY_CREDENTIAL => Attribute::PrimaryCredential,
             ATTR_PRIVATE_COOKIE_KEY => Attribute::PrivateCookieKey,
@@ -512,6 +514,7 @@ impl From<Attribute> for &'static str {
             Attribute::OtherNoIndex => ATTR_OTHER_NO_INDEX,
             Attribute::PassKeys => ATTR_PASSKEYS,
             Attribute::PasswordImport => ATTR_PASSWORD_IMPORT,
+            Attribute::PatchLevel => ATTR_PATCH_LEVEL,
             Attribute::Phantom => ATTR_PHANTOM,
             Attribute::PrimaryCredential => ATTR_PRIMARY_CREDENTIAL,
             Attribute::PrivateCookieKey => ATTR_PRIVATE_COOKIE_KEY,
@@ -788,7 +791,7 @@ lazy_static! {
             Attribute::Description,
             Value::new_utf8s("System (local) info and metadata object.")
         ),
-        (Attribute::Version, Value::Uint32(19))
+        (Attribute::Version, Value::Uint32(20))
     );
 
     pub static ref E_DOMAIN_INFO_V1: EntryInitNew = entry_init!(

server/lib/src/constants/mod.rs

@@ -67,6 +67,7 @@ pub const DOMAIN_LEVEL_5: DomainVersion = 5;
 /// Domain Level introduced with 1.2.0.
 /// Deprcated as of 1.4.0
 pub const DOMAIN_LEVEL_6: DomainVersion = 6;
+pub const PATCH_LEVEL_1: u32 = 1;
 
 /// Domain Level introduced with 1.3.0.
 /// Deprcated as of 1.5.0
@@ -84,6 +85,7 @@ pub const DOMAIN_MIN_LEVEL: DomainVersion = DOMAIN_TGT_LEVEL;
 pub const DOMAIN_PREVIOUS_TGT_LEVEL: DomainVersion = DOMAIN_LEVEL_6;
 // The target supported domain functional level
 pub const DOMAIN_TGT_LEVEL: DomainVersion = DOMAIN_LEVEL_7;
+pub const DOMAIN_TGT_PATCH_LEVEL: u32 = PATCH_LEVEL_1;
 // The maximum supported domain functional level
 pub const DOMAIN_MAX_LEVEL: DomainVersion = DOMAIN_LEVEL_7;
 // The maximum supported domain functional level

server/lib/src/constants/schema.rs

@@ -690,6 +690,14 @@ pub static ref SCHEMA_ATTR_KEY_ACTION_IMPORT_JWS_ES256_DL6: SchemaAttribute = Sc
     ..Default::default()
 };
 
+pub static ref SCHEMA_ATTR_PATCH_LEVEL_DL7: SchemaAttribute = SchemaAttribute {
+    uuid: UUID_SCHEMA_ATTR_PATCH_LEVEL,
+    name: Attribute::PatchLevel.into(),
+    description: "".to_string(),
+    syntax: SyntaxType::Uint32,
+    ..Default::default()
+};
+
 // === classes ===
 
 pub static ref SCHEMA_CLASS_PERSON: SchemaClass = SchemaClass {
@@ -1074,6 +1082,7 @@ pub static ref SCHEMA_CLASS_DOMAIN_INFO_DL7: SchemaClass = SchemaClass {
         Attribute::DomainSsid.into(),
         Attribute::DomainLdapBasedn.into(),
         Attribute::LdapAllowUnixPwBind.into(),
+        Attribute::PatchLevel.into(),
     ],
     systemmust: vec![
         Attribute::Name.into(),

server/lib/src/constants/uuids.rs

@@ -301,6 +301,7 @@ pub const UUID_SCHEMA_ATTR_KEY_ACTION_IMPORT_JWS_ES256: Uuid =
     uuid!("00000000-0000-0000-0000-ffff00000173");
 pub const UUID_SCHEMA_CLASS_KEY_OBJECT_JWE_A128GCM: Uuid =
     uuid!("00000000-0000-0000-0000-ffff00000174");
+pub const UUID_SCHEMA_ATTR_PATCH_LEVEL: Uuid = uuid!("00000000-0000-0000-0000-ffff00000175");
 
 // System and domain infos
 // I'd like to strongly criticise william of the past for making poor choices about these allocations.

server/lib/src/repl/consumer.rs

@@ -296,6 +296,7 @@ impl<'a> QueryServerWriteTransaction<'a> {
             }
             ReplIncrementalContext::V1 {
                 domain_version,
+                domain_patch_level,
                 domain_uuid,
                 ranges,
                 schema_entries,
@@ -303,6 +304,7 @@ impl<'a> QueryServerWriteTransaction<'a> {
                 entries,
             } => self.consumer_apply_changes_v1(
                 *domain_version,
+                *domain_patch_level,
                 *domain_uuid,
                 ranges,
                 schema_entries,
@@ -316,6 +318,7 @@ impl<'a> QueryServerWriteTransaction<'a> {
     fn consumer_apply_changes_v1(
         &mut self,
         ctx_domain_version: DomainVersion,
+        ctx_domain_patch_level: u32,
         ctx_domain_uuid: Uuid,
         ctx_ranges: &BTreeMap<Uuid, ReplAnchoredCidRange>,
         ctx_schema_entries: &[ReplIncrementalEntryV1],
@@ -330,6 +333,13 @@ impl<'a> QueryServerWriteTransaction<'a> {
             return Err(OperationError::ReplDomainLevelUnsatisfiable);
         };
 
+        let domain_patch_level = self.get_domain_patch_level();
+
+        if ctx_domain_patch_level != domain_patch_level {
+            error!("Unable to proceed with consumer incremental - incoming domain patch level is not equal to our patch level. {} != {}", ctx_domain_patch_level, domain_patch_level);
+            return Err(OperationError::ReplDomainLevelUnsatisfiable);
+        };
+
         // Assert that the d_uuid matches the repl domain uuid.
         let db_uuid = self.be_txn.get_db_d_uuid()?;
 

server/lib/src/repl/proto.rs

@@ -752,6 +752,8 @@ pub enum ReplIncrementalContext {
     UnwillingToSupply,
     V1 {
         domain_version: DomainVersion,
+        #[serde(default)]
+        domain_patch_level: u32,
         domain_uuid: Uuid,
         // We need to send the current state of the ranges to populate into
         // the ranges so that lookups and ranges work properly, and the

server/lib/src/repl/supplier.rs

@@ -225,6 +225,7 @@ impl<'a> QueryServerReadTransaction<'a> {
 
         let schema = self.get_schema();
         let domain_version = self.d_info.d_vers;
+        let domain_patch_level = self.d_info.d_patch_level;
         let domain_uuid = self.d_info.d_uuid;
 
         let schema_entries: Vec<_> = schema_entries
@@ -249,6 +250,7 @@ impl<'a> QueryServerReadTransaction<'a> {
         // Build the incremental context.
         Ok(ReplIncrementalContext::V1 {
             domain_version,
+            domain_patch_level,
             domain_uuid,
             ranges,
             schema_entries,

server/lib/src/server/migrations.rs

@@ -40,8 +40,6 @@ impl QueryServer {
             .initialise_schema_core()
             .and_then(|_| write_txn.reload())?;
 
-        write_txn.reload()?;
-
         // This is what tells us if the domain entry existed before or not. This
         // is now the primary method of migrations and version detection.
         let db_domain_version = match write_txn.internal_search_uuid(UUID_DOMAIN_INFO) {
@@ -84,8 +82,20 @@ impl QueryServer {
         // No domain info was present, so neither was the rest of the IDM. We need to bootstrap
         // the base entries here.
         if db_domain_version == 0 {
+            // In this path because we create the dyn groups they are immediately added to the
+            // dyngroup cache and begin to operate.
             write_txn.initialise_idm()?;
-        }
+        } else {
+            // #2756 - if we *aren't* creating the base IDM entries, then we
+            // need to force dyn groups to reload since we're now at schema
+            // ready. This is done indirectly by ... reloading the schema again.
+            //
+            // This is because dyngroups don't load until server phase >= schemaready
+            // and the reload path for these is either a change in the dyngroup entry
+            // itself or a change to schema reloading. Since we aren't changing the
+            // dyngroup here, we have to go via the schema reload path.
+            write_txn.force_schema_reload();
+        };
 
         // Reload as init idm affects access controls.
         write_txn.reload()?;
@@ -95,11 +105,12 @@ impl QueryServer {
 
         // This is the start of domain info related migrations which we will need in future
         // to handle replication. Due to the access control rework, and the addition of "managed by"
-        // syntax, we need to ensure both node "fence" replication from each other. We do this
+        // syntax, we need to ensure both nodes "fence" replication from each other. We do this
         // by changing domain infos to be incompatible during this phase.
 
         // The reloads will have populated this structure now.
         let domain_info_version = write_txn.get_domain_version();
+        let domain_patch_level = write_txn.get_domain_patch_level();
         debug!(?db_domain_version, "After setting internal domain info");
 
         if domain_info_version < domain_target_level {
@@ -114,6 +125,10 @@ impl QueryServer {
                 .map(|()| {
                     warn!("Domain level has been raised to {}", domain_target_level);
                 })?;
+
+            // Reload if anything in migrations requires it - this triggers the domain migrations
+            // which in turn can trigger schema reloads etc.
+            write_txn.reload()?;
         } else {
             // This forces pre-release versions to re-migrate each start up. This solves
             // the domain-version-sprawl issue so that during a development cycle we can
@@ -127,12 +142,26 @@ impl QueryServer {
             // We did not already need a version migration as above
             if option_env!("KANIDM_PRE_RELEASE").is_some() && !cfg!(test) {
                 write_txn.domain_remigrate(DOMAIN_PREVIOUS_TGT_LEVEL)?;
+                write_txn.reload()?;
             }
         }
 
-        // Reload if anything in migrations requires it - this triggers the domain migrations
+        if domain_target_level >= DOMAIN_LEVEL_7 && domain_patch_level < DOMAIN_TGT_PATCH_LEVEL {
-        // which in turn can trigger schema reloads etc.
+            write_txn
-        write_txn.reload()?;
+                .internal_modify_uuid(
+                    UUID_DOMAIN_INFO,
+                    &ModifyList::new_purge_and_set(
+                        Attribute::PatchLevel,
+                        Value::new_uint32(DOMAIN_TGT_PATCH_LEVEL),
+                    ),
+                )
+                .map(|()| {
+                    warn!("Domain level has been raised to {}", domain_target_level);
+                })?;
+
+            // Run the patch migrations
+            write_txn.reload()?;
+        };
 
         // We are ready to run
         write_txn.set_phase(ServerPhase::Running);
@@ -622,8 +651,8 @@ impl<'a> QueryServerWriteTransaction<'a> {
         self.internal_modify(&filter, &modlist)?;
 
         // Now update schema
-
         let idm_schema_classes = [
+            SCHEMA_ATTR_PATCH_LEVEL_DL7.clone().into(),
             SCHEMA_CLASS_DOMAIN_INFO_DL7.clone().into(),
             SCHEMA_CLASS_SERVICE_ACCOUNT_DL7.clone().into(),
             SCHEMA_CLASS_SYNC_ACCOUNT_DL7.clone().into(),
@@ -673,6 +702,22 @@ impl<'a> QueryServerWriteTransaction<'a> {
         Ok(())
     }
 
+    /// Patch Application - This triggers a one-shot fixup task for issue #2756
+    /// to correct the content of dyngroups after the dyngroups are now loaded.
+    #[instrument(level = "info", skip_all)]
+    pub(crate) fn migrate_domain_patch_level_1(&mut self) -> Result<(), OperationError> {
+        admin_warn!("applying domain patch 1.");
+
+        debug_assert!(*self.phase >= ServerPhase::SchemaReady);
+
+        let filter = filter!(f_eq(Attribute::Class, EntryClass::DynGroup.into()));
+        let modlist = modlist!([m_pres(Attribute::Class, &EntryClass::DynGroup.into())]);
+
+        self.internal_modify(&filter, &modlist).map(|()| {
+            info!("forced dyngroups to re-calculate memberships");
+        })
+    }
+
     /// Migration domain level 7 to 8
     #[instrument(level = "info", skip_all)]
     pub(crate) fn migrate_domain_7_to_8(&mut self) -> Result<(), OperationError> {

server/lib/src/server/mod.rs

@@ -72,6 +72,7 @@ pub struct DomainInfo {
     pub(crate) d_name: String,
     pub(crate) d_display: String,
     pub(crate) d_vers: DomainVersion,
+    pub(crate) d_patch_level: u32,
     pub(crate) d_ldap_allow_unix_pw_bind: bool,
 }
 
@@ -192,6 +193,8 @@ pub trait QueryServerTransaction<'a> {
 
     fn get_domain_version(&self) -> DomainVersion;
 
+    fn get_domain_patch_level(&self) -> u32;
+
     fn get_domain_uuid(&self) -> Uuid;
 
     fn get_domain_name(&self) -> &str;
@@ -1067,6 +1070,10 @@ impl<'a> QueryServerTransaction<'a> for QueryServerReadTransaction<'a> {
         self.d_info.d_vers
     }
 
+    fn get_domain_patch_level(&self) -> u32 {
+        self.d_info.d_patch_level
+    }
+
     fn get_domain_uuid(&self) -> Uuid {
         self.d_info.d_uuid
     }
@@ -1213,6 +1220,10 @@ impl<'a> QueryServerTransaction<'a> for QueryServerWriteTransaction<'a> {
         self.d_info.d_vers
     }
 
+    fn get_domain_patch_level(&self) -> u32 {
+        self.d_info.d_patch_level
+    }
+
     fn get_domain_uuid(&self) -> Uuid {
         self.d_info.d_uuid
     }
@@ -1256,6 +1267,7 @@ impl QueryServer {
             // Start with our level as zero.
             // This will be reloaded from the DB shortly :)
             d_vers: DOMAIN_LEVEL_0,
+            d_patch_level: 0,
             d_name: domain_name.clone(),
             // we set the domain_display_name to the configuration file's domain_name
             // here because the database is not started, so we cannot pull it from there.
@@ -1786,25 +1798,37 @@ impl<'a> QueryServerWriteTransaction<'a> {
     /// Pulls the domain name from the database and updates the DomainInfo data in memory
     #[instrument(level = "debug", skip_all)]
     pub(crate) fn reload_domain_info_version(&mut self) -> Result<(), OperationError> {
-        let domain_info_version = self
+        let domain_info = self.internal_search_uuid(UUID_DOMAIN_INFO).map_err(|err| {
-            .internal_search_uuid(UUID_DOMAIN_INFO)
+            error!(?err, "Error getting domain info");
-            .and_then(|e| {
+            err
-                e.get_ava_single_uint32(Attribute::Version)
+        })?;
-                    .ok_or(OperationError::InvalidEntryState)
+
-            })
+        let domain_info_version = domain_info
-            .map_err(|err| {
+            .get_ava_single_uint32(Attribute::Version)
-                error!(?err, "Error getting domain version");
+            .ok_or_else(|| {
-                err
+                error!("domain info missing attribute version");
+                OperationError::InvalidEntryState
             })?;
 
+        let domain_info_patch_level = domain_info
+            .get_ava_single_uint32(Attribute::PatchLevel)
+            .unwrap_or(0);
+
         // We have to set the domain version here so that features which check for it
         // will now see it's been increased. This also prevents recursion during reloads
         // inside of a domain migration.
         let mut_d_info = self.d_info.get_mut();
         let previous_version = mut_d_info.d_vers;
+        let previous_patch_level = mut_d_info.d_patch_level;
         mut_d_info.d_vers = domain_info_version;
+        mut_d_info.d_patch_level = domain_info_patch_level;
 
-        if previous_version == domain_info_version || *self.phase < ServerPhase::DomainInfoReady {
+        // We must both be at the correct domain version *and* the correct patch level. If we are
+        // not, then we only proceed to migrate *if* our server boot phase is correct.
+        if (previous_version == domain_info_version
+            && previous_patch_level == domain_info_patch_level)
+            || *self.phase < ServerPhase::DomainInfoReady
+        {
             return Ok(());
         }
 
@@ -1830,6 +1854,12 @@ impl<'a> QueryServerWriteTransaction<'a> {
             self.migrate_domain_6_to_7()?;
         }
 
+        // Similar to the older system info migration handler, these allow "one shot" fixes
+        // to be issued and run by bumping the patch level.
+        if previous_patch_level <= PATCH_LEVEL_1 && domain_info_patch_level >= PATCH_LEVEL_1 {
+            self.migrate_domain_patch_level_1()?;
+        }
+
         if previous_version <= DOMAIN_LEVEL_7 && domain_info_version >= DOMAIN_LEVEL_8 {
             self.migrate_domain_7_to_8()?;
         }