Complete system protected objects implementation. This allows class: system

to be tagged to types that should not be externally edited.

designs/system_protected_objects.rst

@@ -0,0 +1,60 @@
+System Protected Objects
+------------------------
+
+There are a number of system provided objects and functions that are important
+for correct operation of the server. It's important we protect these from certain
+types of modifications so that the system has a baseline of correct functionality.
+
+Some of this is protected by the migrations system built into the server, which
+will correct and "set" certain items to a known state on startup. However, like
+all IDM systems, downtime/restarts are rare, so we have to account for human
+error and provide constraints to maintain a healthy running system.
+
+What needs protecting?
+----------------------
+
+The current list (july 2019) is:
+
+* Core schema (enough to make attributetypes/classtypes/memberof/acp operate)
+* Anonymous
+* Admin
+* systeminfo
+
+Additionally, the scope of protection is limited - or rather, the scope of what is
+allowed.
+
+* New system protected items can only be created via internal operations, preventing
+accidental creation of these.
+* schema classtypes can have must/may altered on classes
+* schema attributetypes can have index altered on classes
+* anonymous can be locked
+* admin can be locked, and password changed (or other credentials changed).
+
+The plugin design
+-----------------
+
+This should be a plugin, as the hooks exist in the correct places to intercept and block
+the required operations, saving custom coding. The plugin will:
+
+* Block any create on "class": "system".
+* Block any delete on "class": "system".
+* Block modify to "class": "system" where the affect attr is NOT in the allowed mod set.
+
+The modify block will not be class aware, because schema protects us from other odd behaviours.
+An example - addition of account lock to a schema element. This would be "allowed" by this plugin
+because account lock will be in the allowed set for system, but schema would not allow addition
+of accountlock to a schema element thus protecting us. Because class will NEVER be in the
+allowed set for a system protected type, we can trust that these bounds won't be exceeded.
+
+Why not ACP?
+------------
+
+It was considered to provide default ACP's that would protect system items. This was rejected because:
+
+* it would require a "deny" acp type, and I do not wish to create this, as people could then create their own deny rules (always incorrect!)
+* There would be a lot of acp's involved in this protection (but acp's are expressive enough to provide it!)
+* The acp's would need a self-referencing acp to protect themselves from modification.
+* Having a seperate plugin to protect this will be faster than acp processing because we check less filters (But this is not a strong argument)
+* the plugin can provide targeted error messages about why they were denied, rather than a generic acp denied message.
+* the plugin can provide detailed testing of edge cases in a confined manner
+

src/lib/access.rs

@@ -15,6 +15,8 @@
 //   requirements (also search).
 //
 
+// TODO: Purge all reference to acp_allow/deny
+
 use concread::cowcell::{CowCell, CowCellReadTxn, CowCellWriteTxn};
 use std::collections::{BTreeMap, BTreeSet};
 

src/lib/constants.rs

@@ -138,7 +138,6 @@ pub static UUID_SCHEMA_ATTR_UUID: &'static str = "642a893b-fe1a-4fe1-805d-fb78e7
 pub static UUID_SCHEMA_ATTR_NAME: &'static str = "27be9127-5ba1-4c06-bce9-7250f2c7f630";
 pub static UUID_SCHEMA_ATTR_PRINCIPAL_NAME: &'static str = "64dda3ac-12cb-4000-9b30-97a92767ccab";
 pub static UUID_SCHEMA_ATTR_DESCRIPTION: &'static str = "a4da35a2-c5fb-4f8f-a341-72cd39ec9eee";
-pub static UUID_SCHEMA_ATTR_SYSTEM: &'static str = "ee28df1e-cf02-49ca-80b5-8310fb619377";
 pub static UUID_SCHEMA_ATTR_SECRET: &'static str = "0231c61a-0a43-4987-9293-8732ed9459fa";
 pub static UUID_SCHEMA_ATTR_MULTIVALUE: &'static str = "8a6a8bf3-7053-42e2-8cda-15af7a197513";
 pub static UUID_SCHEMA_ATTR_INDEX: &'static str = "2c5ff455-0709-4f67-a37c-35ff7e67bfff";
@@ -187,6 +186,7 @@ pub static UUID_SCHEMA_CLASS_ACCESS_CONTROL_MODIFY: &'static str =
     "fd860561-9d0a-4f12-be30-406834292d46";
 pub static UUID_SCHEMA_CLASS_ACCESS_CONTROL_CREATE: &'static str =
     "58c5c197-51d8-4c30-9a8e-b8a0bb0eaacd";
+pub static UUID_SCHEMA_CLASS_SYSTEM: &'static str = "ee28df1e-cf02-49ca-80b5-8310fb619377";
 
 // system supplementary
 pub static UUID_SCHEMA_ATTR_DISPLAYNAME: &'static str = "201bc966-954b-48f5-bf25-99ffed759861";
@@ -198,6 +198,7 @@ pub static JSON_SCHEMA_ATTR_DISPLAYNAME: &'static str = r#"{
     "attrs": {
       "class": [
         "object",
+        "system",
         "attributetype"
       ],
       "description": [
@@ -218,9 +219,6 @@ pub static JSON_SCHEMA_ATTR_DISPLAYNAME: &'static str = r#"{
       "syntax": [
         "UTF8STRING"
       ],
-      "system": [
-        "true"
-      ],
       "uuid": [
         "201bc966-954b-48f5-bf25-99ffed759861"
       ]
@@ -236,6 +234,7 @@ pub static JSON_SCHEMA_ATTR_MAIL: &'static str = r#"
     "attrs": {
       "class": [
         "object",
+        "system",
         "attributetype"
       ],
       "description": [
@@ -256,9 +255,6 @@ pub static JSON_SCHEMA_ATTR_MAIL: &'static str = r#"
       "syntax": [
         "UTF8STRING"
       ],
-      "system": [
-        "true"
-      ],
       "uuid": [
         "fae94676-720b-461b-9438-bfe8cfd7e6cd"
       ]
@@ -275,6 +271,7 @@ pub static JSON_SCHEMA_ATTR_SSH_PUBLICKEY: &'static str = r#"
     "attrs": {
       "class": [
         "object",
+        "system",
         "attributetype"
       ],
       "description": [
@@ -293,9 +290,6 @@ pub static JSON_SCHEMA_ATTR_SSH_PUBLICKEY: &'static str = r#"
       "syntax": [
         "UTF8STRING"
       ],
-      "system": [
-        "true"
-      ],
       "uuid": [
         "52f2f13f-d35c-4cca-9f43-90a12c968f72"
       ]
@@ -312,6 +306,7 @@ pub static JSON_SCHEMA_ATTR_PASSWORD: &'static str = r#"
     "attrs": {
       "class": [
         "object",
+        "system",
         "attributetype"
       ],
       "description": [
@@ -330,9 +325,6 @@ pub static JSON_SCHEMA_ATTR_PASSWORD: &'static str = r#"
       "syntax": [
         "UTF8STRING"
       ],
-      "system": [
-        "true"
-      ],
       "uuid": [
         "a5121082-be54-4624-a307-383839b0366b"
       ]
@@ -350,6 +342,7 @@ pub static JSON_SCHEMA_CLASS_PERSON: &'static str = r#"
     "attrs": {
       "class": [
         "object",
+        "system",
         "classtype"
       ],
       "description": [
@@ -383,6 +376,7 @@ pub static JSON_SCHEMA_CLASS_GROUP: &'static str = r#"
     "attrs": {
       "class": [
         "object",
+        "system",
         "classtype"
       ],
       "description": [
@@ -413,6 +407,7 @@ pub static JSON_SCHEMA_CLASS_ACCOUNT: &'static str = r#"
     "attrs": {
       "class": [
         "object",
+        "system",
         "classtype"
       ],
       "description": [

src/lib/entry.rs

@@ -1104,12 +1104,6 @@ impl From<&SchemaAttribute> for Entry<EntryValid, EntryNew> {
         let name_v = vec![s.name.clone()];
         let desc_v = vec![s.description.clone()];
 
-        let system_v = vec![if s.system {
-            "true".to_string()
-        } else {
-            "false".to_string()
-        }];
-
         let secret_v = vec![if s.secret {
             "true".to_string()
         } else {
@@ -1131,14 +1125,17 @@ impl From<&SchemaAttribute> for Entry<EntryValid, EntryNew> {
         attrs.insert("name".to_string(), name_v);
         attrs.insert("description".to_string(), desc_v);
         attrs.insert("uuid".to_string(), uuid_v);
-        attrs.insert("system".to_string(), system_v);
         attrs.insert("secret".to_string(), secret_v);
         attrs.insert("multivalue".to_string(), multivalue_v);
         attrs.insert("index".to_string(), index_v);
         attrs.insert("syntax".to_string(), syntax_v);
         attrs.insert(
             "class".to_string(),
-            vec!["object".to_string(), "attributetype".to_string()],
+            vec![
+                "object".to_string(),
+                "system".to_string(),
+                "attributetype".to_string(),
+            ],
         );
 
         // Insert stuff.
@@ -1167,7 +1164,11 @@ impl From<&SchemaClass> for Entry<EntryValid, EntryNew> {
         attrs.insert("uuid".to_string(), uuid_v);
         attrs.insert(
             "class".to_string(),
-            vec!["object".to_string(), "classtype".to_string()],
+            vec![
+                "object".to_string(),
+                "system".to_string(),
+                "classtype".to_string(),
+            ],
         );
         attrs.insert("systemmay".to_string(), s.systemmay.clone());
         attrs.insert("systemmust".to_string(), s.systemmust.clone());

src/lib/error.rs

@@ -40,6 +40,7 @@ pub enum OperationError {
     NotAuthenticated,
     InvalidAuthState(&'static str),
     InvalidSessionState,
+    SystemProtectedObject,
 }
 
 #[derive(Serialize, Deserialize, Debug, PartialEq)]

src/lib/event.rs

@@ -170,6 +170,13 @@ impl Event {
         // to audits and logs to determine what happened.
         event.clone()
     }
+
+    pub fn is_internal(&self) -> bool {
+        match self.origin {
+            EventOrigin::Internal => true,
+            _ => false,
+        }
+    }
 }
 
 #[derive(Debug)]

src/lib/plugins/mod.rs

@@ -17,18 +17,19 @@ mod refint;
 trait Plugin {
     fn id() -> &'static str;
 
-    // TODO: These should all return OperationError::Unimplemented
-
     fn pre_create_transform(
         _au: &mut AuditScope,
         _qs: &QueryServerWriteTransaction,
         _cand: &mut Vec<Entry<EntryInvalid, EntryNew>>,
         _ce: &CreateEvent,
     ) -> Result<(), OperationError> {
-        Ok(())
+        debug!(
+            "plugin {} has an unimplemented pre_create_transform!",
+            Self::id()
+        );
+        Err(OperationError::Plugin)
     }
 
-    /*
     fn pre_create(
         _au: &mut AuditScope,
         _qs: &QueryServerWriteTransaction,
@@ -36,9 +37,9 @@ trait Plugin {
         _cand: &Vec<Entry<EntryValid, EntryNew>>,
         _ce: &CreateEvent,
     ) -> Result<(), OperationError> {
-        Ok(())
+        debug!("plugin {} has an unimplemented pre_create!", Self::id());
+        Err(OperationError::Plugin)
     }
-    */
 
     fn post_create(
         _au: &mut AuditScope,
@@ -47,7 +48,8 @@ trait Plugin {
         _cand: &Vec<Entry<EntryValid, EntryNew>>,
         _ce: &CreateEvent,
     ) -> Result<(), OperationError> {
-        Ok(())
+        debug!("plugin {} has an unimplemented post_create!", Self::id());
+        Err(OperationError::Plugin)
     }
 
     fn pre_modify(
@@ -56,7 +58,8 @@ trait Plugin {
         _cand: &mut Vec<Entry<EntryInvalid, EntryCommitted>>,
         _me: &ModifyEvent,
     ) -> Result<(), OperationError> {
-        Ok(())
+        debug!("plugin {} has an unimplemented pre_modify!", Self::id());
+        Err(OperationError::Plugin)
     }
 
     fn post_modify(
@@ -67,7 +70,8 @@ trait Plugin {
         _cand: &Vec<Entry<EntryValid, EntryCommitted>>,
         _ce: &ModifyEvent,
     ) -> Result<(), OperationError> {
-        Ok(())
+        debug!("plugin {} has an unimplemented post_modify!", Self::id());
+        Err(OperationError::Plugin)
     }
 
     fn pre_delete(
@@ -76,7 +80,8 @@ trait Plugin {
         _cand: &mut Vec<Entry<EntryInvalid, EntryCommitted>>,
         _de: &DeleteEvent,
     ) -> Result<(), OperationError> {
-        Ok(())
+        debug!("plugin {} has an unimplemented pre_delete!", Self::id());
+        Err(OperationError::Plugin)
     }
 
     fn post_delete(
@@ -86,14 +91,16 @@ trait Plugin {
         _cand: &Vec<Entry<EntryValid, EntryCommitted>>,
         _ce: &DeleteEvent,
     ) -> Result<(), OperationError> {
-        Ok(())
+        debug!("plugin {} has an unimplemented post_delete!", Self::id());
+        Err(OperationError::Plugin)
     }
 
     fn verify(
         _au: &mut AuditScope,
         _qs: &QueryServerReadTransaction,
     ) -> Vec<Result<(), ConsistencyError>> {
-        Vec::new()
+        debug!("plugin {} has an unimplemented verify!", Self::id());
+        vec![Err(ConsistencyError::Unknown)]
     }
 }
 
@@ -121,7 +128,6 @@ macro_rules! run_pre_create_transform_plugin {
     }};
 }
 
-/*
 macro_rules! run_pre_create_plugin {
     (
         $au:ident,
@@ -141,7 +147,6 @@ macro_rules! run_pre_create_plugin {
         r
     }};
 }
-*/
 
 macro_rules! run_post_create_plugin {
     (
@@ -270,16 +275,12 @@ impl Plugins {
         ce: &CreateEvent,
     ) -> Result<(), OperationError> {
         audit_segment!(au, || {
-            let res =
+            let res = run_pre_create_transform_plugin!(au, qs, cand, ce, base::Base);
-                run_pre_create_transform_plugin!(au, qs, cand, ce, base::Base).and_then(|_| {
-                    run_pre_create_transform_plugin!(au, qs, cand, ce, refint::ReferentialIntegrity)
-                });
 
             res
         })
     }
 
-    /*
     pub fn run_pre_create(
         au: &mut AuditScope,
         qs: &QueryServerWriteTransaction,
@@ -287,14 +288,11 @@ impl Plugins {
         ce: &CreateEvent,
     ) -> Result<(), OperationError> {
         audit_segment!(au, || {
-            let res = run_pre_create_plugin!(au, qs, cand, ce, base::Base).and_then(|_| {
+            let res = run_pre_create_plugin!(au, qs, cand, ce, protected::Protected);
-                run_pre_create_plugin!(au, qs, cand, ce, refint::ReferentialIntegrity)
-            });
 
             res
         })
     }
-    */
 
     pub fn run_post_create(
         au: &mut AuditScope,
@@ -303,10 +301,8 @@ impl Plugins {
         ce: &CreateEvent,
     ) -> Result<(), OperationError> {
         audit_segment!(au, || {
-            let res = run_post_create_plugin!(au, qs, cand, ce, base::Base).and_then(|_| {
+            let res = run_post_create_plugin!(au, qs, cand, ce, refint::ReferentialIntegrity)
-                run_post_create_plugin!(au, qs, cand, ce, refint::ReferentialIntegrity)
+                .and_then(|_| run_post_create_plugin!(au, qs, cand, ce, memberof::MemberOf));
-                    .and_then(|_| run_post_create_plugin!(au, qs, cand, ce, memberof::MemberOf))
-            });
 
             res
         })
@@ -319,9 +315,8 @@ impl Plugins {
         me: &ModifyEvent,
     ) -> Result<(), OperationError> {
         audit_segment!(au, || {
-            let res = run_pre_modify_plugin!(au, qs, cand, me, base::Base).and_then(|_| {
+            let res = run_pre_modify_plugin!(au, qs, cand, me, protected::Protected)
-                run_pre_modify_plugin!(au, qs, cand, me, refint::ReferentialIntegrity)
+                .and_then(|_| run_pre_modify_plugin!(au, qs, cand, me, base::Base));
-            });
 
             res
         })
@@ -336,18 +331,9 @@ impl Plugins {
     ) -> Result<(), OperationError> {
         audit_segment!(au, || {
             let res =
-                run_post_modify_plugin!(au, qs, pre_cand, cand, me, base::Base).and_then(|_| {
+                run_post_modify_plugin!(au, qs, pre_cand, cand, me, refint::ReferentialIntegrity)
-                    run_post_modify_plugin!(
-                        au,
-                        qs,
-                        pre_cand,
-                        cand,
-                        me,
-                        refint::ReferentialIntegrity
-                    )
                     .and_then(|_| {
                         run_post_modify_plugin!(au, qs, pre_cand, cand, me, memberof::MemberOf)
-                    })
                     });
 
             res
@@ -361,10 +347,7 @@ impl Plugins {
         de: &DeleteEvent,
     ) -> Result<(), OperationError> {
         audit_segment!(au, || {
-            let res = run_pre_delete_plugin!(au, qs, cand, de, base::Base).and_then(|_| {
+            let res = run_pre_delete_plugin!(au, qs, cand, de, protected::Protected);
-                run_pre_delete_plugin!(au, qs, cand, de, refint::ReferentialIntegrity)
-            });
-
             res
         })
     }
@@ -376,10 +359,8 @@ impl Plugins {
         de: &DeleteEvent,
     ) -> Result<(), OperationError> {
         audit_segment!(au, || {
-            let res = run_post_delete_plugin!(au, qs, cand, de, base::Base).and_then(|_| {
+            let res = run_post_delete_plugin!(au, qs, cand, de, refint::ReferentialIntegrity)
-                run_post_delete_plugin!(au, qs, cand, de, refint::ReferentialIntegrity)
+                .and_then(|_| run_post_delete_plugin!(au, qs, cand, de, memberof::MemberOf));
-                    .and_then(|_| run_post_delete_plugin!(au, qs, cand, de, memberof::MemberOf))
-            });
 
             res
         })

src/lib/plugins/protected.rs

@@ -1,2 +1,339 @@
-// Objects matching some filter condition should
+// System protected objects. Items matching specific requirements
-// be protected from modification / deletion
+// may only have certain modifications performed.
+use crate::plugins::Plugin;
+
+use crate::audit::AuditScope;
+use crate::entry::{Entry, EntryCommitted, EntryInvalid, EntryNew, EntryValid};
+use crate::error::OperationError;
+use crate::event::{CreateEvent, DeleteEvent, ModifyEvent};
+use crate::modify::Modify;
+use crate::server::{QueryServerTransaction, QueryServerWriteTransaction};
+use std::collections::HashSet;
+
+pub struct Protected {}
+
+impl Plugin for Protected {
+    fn id() -> &'static str {
+        "plugin_protected"
+    }
+
+    fn pre_create(
+        au: &mut AuditScope,
+        _qs: &QueryServerWriteTransaction,
+        // List of what we will commit that is valid?
+        cand: &Vec<Entry<EntryValid, EntryNew>>,
+        ce: &CreateEvent,
+    ) -> Result<(), OperationError> {
+        if ce.event.is_internal() {
+            audit_log!(
+                au,
+                "Internal operation, not enforcing system object protection"
+            );
+            return Ok(());
+        }
+
+        cand.iter().fold(Ok(()), |acc, cand| match acc {
+            Err(_) => acc,
+            Ok(_) => {
+                if cand.attribute_value_pres("class", "system") {
+                    Err(OperationError::SystemProtectedObject)
+                } else {
+                    acc
+                }
+            }
+        })
+    }
+
+    fn pre_modify(
+        au: &mut AuditScope,
+        _qs: &QueryServerWriteTransaction,
+        // Should these be EntryValid?
+        cand: &mut Vec<Entry<EntryInvalid, EntryCommitted>>,
+        me: &ModifyEvent,
+    ) -> Result<(), OperationError> {
+        if me.event.is_internal() {
+            audit_log!(
+                au,
+                "Internal operation, not enforcing system object protection"
+            );
+            return Ok(());
+        }
+        // Prevent adding class: system
+        me.modlist.iter().fold(Ok(()), |acc, m| {
+            if acc.is_err() {
+                acc
+            } else {
+                match m {
+                    Modify::Present(a, v) => {
+                        if a == "class" && v == "system" {
+                            Err(OperationError::SystemProtectedObject)
+                        } else {
+                            Ok(())
+                        }
+                    }
+                    _ => Ok(()),
+                }
+            }
+        })?;
+        // if class: system, check the mods are "allowed"
+
+        let system_pres = cand.iter().fold(false, |acc, c| {
+            if acc {
+                acc
+            } else {
+                c.attribute_value_pres("class", "system")
+            }
+        });
+
+        audit_log!(au, "class: system -> {}", system_pres);
+        // No system types being altered, return.
+        if system_pres == false {
+            return Ok(());
+        }
+
+        // Something altered is system, check if it's allowed.
+        me.modlist.iter().fold(Ok(()), |acc, m| {
+            // Already hit an error, move on.
+            if acc.is_err() {
+                acc
+            } else {
+                let a = match m {
+                    Modify::Present(a, _) => a,
+                    Modify::Removed(a, _) => a,
+                    Modify::Purged(a) => a,
+                };
+                // TODO: This should be a set or some kind of structure?
+                // I had issues statically allocating this though ... alternately
+                // we could make it a "configurable" type?
+                if a == "must" {
+                    Ok(())
+                } else if a == "may" {
+                    Ok(())
+                } else {
+                    Err(OperationError::SystemProtectedObject)
+                }
+            }
+        })
+    }
+
+    fn pre_delete(
+        au: &mut AuditScope,
+        _qs: &QueryServerWriteTransaction,
+        // Should these be EntryValid
+        cand: &mut Vec<Entry<EntryInvalid, EntryCommitted>>,
+        de: &DeleteEvent,
+    ) -> Result<(), OperationError> {
+        if de.event.is_internal() {
+            audit_log!(
+                au,
+                "Internal operation, not enforcing system object protection"
+            );
+            return Ok(());
+        }
+
+        cand.iter().fold(Ok(()), |acc, cand| match acc {
+            Err(_) => acc,
+            Ok(_) => {
+                if cand.attribute_value_pres("class", "system") {
+                    Err(OperationError::SystemProtectedObject)
+                } else {
+                    acc
+                }
+            }
+        })
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::constants::JSON_ADMIN_V1;
+    use crate::entry::{Entry, EntryInvalid, EntryNew};
+    use crate::error::OperationError;
+
+    static JSON_ADMIN_ALLOW_ALL: &'static str = r#"{
+        "valid": null,
+        "state": null,
+        "attrs": {
+            "class": [
+                "object",
+                "access_control_profile",
+                "access_control_modify",
+                "access_control_create",
+                "access_control_delete",
+                "access_control_search"
+            ],
+            "name": ["idm_admins_acp_allow_all_test"],
+            "uuid": ["bb18f746-a409-497d-928c-5455d4aef4f7"],
+            "description": ["Builtin IDM Administrators Access Controls."],
+            "acp_enable": ["true"],
+            "acp_receiver": [
+                "{\"Eq\":[\"uuid\",\"00000000-0000-0000-0000-000000000000\"]}"
+            ],
+            "acp_targetscope": [
+                "{\"Pres\":\"class\"}"
+            ],
+            "acp_search_attr": ["name", "class", "uuid"],
+            "acp_modify_class": ["system"],
+            "acp_modify_removedattr": ["class", "displayname", "may", "must"],
+            "acp_modify_presentattr": ["class", "displayname", "may", "must"],
+            "acp_create_class": ["object", "person", "system"],
+            "acp_create_attr": ["name", "class", "description", "displayname"]
+        }
+    }"#;
+
+    #[test]
+    fn test_pre_create_deny() {
+        // Test creating with class: system is rejected.
+        let acp: Entry<EntryInvalid, EntryNew> =
+            serde_json::from_str(JSON_ADMIN_ALLOW_ALL).expect("json parse failure");
+
+        let preload = vec![acp];
+
+        let e: Entry<EntryInvalid, EntryNew> = serde_json::from_str(
+            r#"{
+            "valid": null,
+            "state": null,
+            "attrs": {
+                "class": ["person", "system"],
+                "name": ["testperson"],
+                "description": ["testperson"],
+                "displayname": ["testperson"]
+            }
+        }"#,
+        )
+        .expect("json parse failure");
+
+        let create = vec![e.clone()];
+
+        run_create_test!(
+            Err(OperationError::SystemProtectedObject),
+            preload,
+            create,
+            Some(JSON_ADMIN_V1),
+            |_, _| {}
+        );
+    }
+
+    #[test]
+    fn test_pre_modify_system_deny() {
+        let acp: Entry<EntryInvalid, EntryNew> =
+            serde_json::from_str(JSON_ADMIN_ALLOW_ALL).expect("json parse failure");
+        // Test modify of class to a system is denied
+        let e: Entry<EntryInvalid, EntryNew> = serde_json::from_str(
+            r#"{
+            "valid": null,
+            "state": null,
+            "attrs": {
+                "class": ["person", "system"],
+                "name": ["testperson"],
+                "description": ["testperson"],
+                "displayname": ["testperson"]
+            }
+        }"#,
+        )
+        .expect("json parse failure");
+
+        let preload = vec![acp, e.clone()];
+
+        run_modify_test!(
+            Err(OperationError::SystemProtectedObject),
+            preload,
+            filter!(f_eq("name", "testperson")),
+            modlist!([m_purge("displayname"), m_pres("displayname", "system test"),]),
+            Some(JSON_ADMIN_V1),
+            |_, _| {}
+        );
+    }
+
+    #[test]
+    fn test_pre_modify_class_add_deny() {
+        let acp: Entry<EntryInvalid, EntryNew> =
+            serde_json::from_str(JSON_ADMIN_ALLOW_ALL).expect("json parse failure");
+        // Show that adding a system class is denied
+        let e: Entry<EntryInvalid, EntryNew> = serde_json::from_str(
+            r#"{
+            "valid": null,
+            "state": null,
+            "attrs": {
+                "class": ["person"],
+                "name": ["testperson"],
+                "description": ["testperson"],
+                "displayname": ["testperson"]
+            }
+        }"#,
+        )
+        .expect("json parse failure");
+
+        let preload = vec![acp, e.clone()];
+
+        run_modify_test!(
+            Err(OperationError::SystemProtectedObject),
+            preload,
+            filter!(f_eq("name", "testperson")),
+            modlist!([m_pres("class", "system"),]),
+            Some(JSON_ADMIN_V1),
+            |_, _| {}
+        );
+    }
+
+    #[test]
+    fn test_pre_modify_attr_must_may_allow() {
+        let acp: Entry<EntryInvalid, EntryNew> =
+            serde_json::from_str(JSON_ADMIN_ALLOW_ALL).expect("json parse failure");
+        // Show that adding a system class is denied
+        let e: Entry<EntryInvalid, EntryNew> = serde_json::from_str(
+            r#"{
+            "valid": null,
+            "state": null,
+            "attrs": {
+                "class": ["object", "classtype"],
+                "name": ["testclass"],
+                "uuid": ["cfcae205-31c3-484b-8ced-667d1709c5e3"],
+                "description": ["Test Class"]
+            }
+        }"#,
+        )
+        .expect("json parse failure");
+
+        let preload = vec![acp, e.clone()];
+
+        run_modify_test!(
+            Ok(()),
+            preload,
+            filter!(f_eq("name", "testclass")),
+            modlist!([m_pres("may", "name"), m_pres("must", "name"),]),
+            Some(JSON_ADMIN_V1),
+            |_, _| {}
+        );
+    }
+
+    #[test]
+    fn test_pre_delete_deny() {
+        let acp: Entry<EntryInvalid, EntryNew> =
+            serde_json::from_str(JSON_ADMIN_ALLOW_ALL).expect("json parse failure");
+        // Test deleting with class: system is rejected.
+        let e: Entry<EntryInvalid, EntryNew> = serde_json::from_str(
+            r#"{
+            "valid": null,
+            "state": null,
+            "attrs": {
+                "class": ["person", "system"],
+                "name": ["testperson"],
+                "description": ["testperson"],
+                "displayname": ["testperson"]
+            }
+        }"#,
+        )
+        .expect("json parse failure");
+
+        let preload = vec![acp, e.clone()];
+
+        run_delete_test!(
+            Err(OperationError::SystemProtectedObject),
+            preload,
+            filter!(f_eq("name", "testperson")),
+            Some(JSON_ADMIN_V1),
+            |_, _| {}
+        );
+    }
+}

src/lib/schema.rs

@@ -19,15 +19,6 @@ use concread::cowcell::{CowCell, CowCellReadTxn, CowCellWriteTxn};
 // In the future this will parse/read it's schema from the db
 // but we have to bootstrap with some core types.
 
-// TODO: system_info metadata object schema
-
-// TODO: system class to indicate the type is a system object?
-// just a class? Does the class imply protections?
-// probably just protection from delete and modify, except systemmay/systemmust/index?
-
-// TODO: Schema types -> Entry conversion
-// TODO: Entry -> Schema given class. This is for loading from the db.
-
 // TODO: prefix on all schema types that are system?
 
 #[allow(non_camel_case_types)]
@@ -132,7 +123,8 @@ pub struct SchemaAttribute {
     pub uuid: Uuid,
     // Perhaps later add aliases?
     pub description: String,
-    pub system: bool,
+    // TODO: Remove this field, it does nothing
+    // and will be replaced by a different specific schema element.
     pub secret: bool,
     pub multivalue: bool,
     pub index: Vec<IndexType>,
@@ -174,16 +166,6 @@ impl SchemaAttribute {
                 .ok_or(OperationError::InvalidSchemaState("missing description"))
         );
 
-        // system
-        let system = try_audit!(
-            audit,
-            value
-                .get_ava_single_bool("system")
-                .ok_or(OperationError::InvalidSchemaState(
-                    "missing or invalid system"
-                ))
-        );
-
         // secret
         let secret = try_audit!(
             audit,
@@ -219,7 +201,6 @@ impl SchemaAttribute {
             name: name.clone(),
             uuid: uuid.clone(),
             description: description.clone(),
-            system: system,
             secret: secret,
             multivalue: multivalue,
             index: index,
@@ -548,7 +529,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_CLASS)
                         .expect("unable to parse static uuid"),
                     description: String::from("The set of classes defining an object"),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -562,7 +542,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_UUID)
                         .expect("unable to parse static uuid"),
                     description: String::from("The universal unique id of the object"),
-                    system: true,
                     secret: false,
                     multivalue: false,
                     index: vec![IndexType::EQUALITY],
@@ -576,7 +555,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_NAME)
                         .expect("unable to parse static uuid"),
                     description: String::from("The shortform name of an object"),
-                    system: true,
                     secret: false,
                     multivalue: false,
                     index: vec![IndexType::EQUALITY],
@@ -589,7 +567,6 @@ impl SchemaInner {
                     name: String::from("principal_name"),
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_PRINCIPAL_NAME).expect("unable to parse static uuid"),
                     description: String::from("The longform name of an object, derived from name and domain. Example: alice@project.org"),
-                system: true,
                     secret: false,
                     multivalue: false,
                     index: vec![IndexType::EQUALITY],
@@ -603,38 +580,17 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_DESCRIPTION)
                         .expect("unable to parse static uuid"),
                     description: String::from("A description of an attribute, object or class"),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![],
                     syntax: SyntaxType::UTF8STRING,
                 },
             );
-            s.attributes.insert(
-                // FIXME: Rename to system_provided? Or should we eschew this in favour of class?
-                // system_provided attr seems easier to provide access controls on, and can be
-                // part of object ...
-                String::from("system"),
-                SchemaAttribute {
-                    name: String::from("system"),
-                    uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_SYSTEM)
-                        .expect("unable to parse static uuid"),
-                    description: String::from(
-                        "Is this object or attribute provided from the core system?",
-                    ),
-                    system: true,
-                    secret: false,
-                    multivalue: false,
-                    index: vec![],
-                    syntax: SyntaxType::BOOLEAN,
-                },
-            );
             s.attributes.insert(String::from("secret"), SchemaAttribute {
                 // FIXME: Rename from system to schema_private? system_private? attr_private? private_attr?
                 name: String::from("secret"),
                 uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_SECRET).expect("unable to parse static uuid"),
                 description: String::from("If true, this value is always hidden internally to the server, even beyond access controls."),
-            system: true,
                 secret: false,
                 multivalue: false,
                 index: vec![],
@@ -644,7 +600,6 @@ impl SchemaInner {
                 name: String::from("multivalue"),
                 uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_MULTIVALUE).expect("unable to parse static uuid"),
                 description: String::from("If true, this attribute is able to store multiple values rather than just a single value."),
-            system: true,
                 secret: false,
                 multivalue: false,
                 index: vec![],
@@ -660,7 +615,6 @@ impl SchemaInner {
                     description: String::from(
                         "Describe the indexes to apply to instances of this attribute.",
                     ),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![],
@@ -677,7 +631,6 @@ impl SchemaInner {
                     description: String::from(
                         "Describe the syntax of this attribute. This affects indexing and sorting.",
                     ),
-                    system: true,
                     secret: false,
                     multivalue: false,
                     index: vec![IndexType::EQUALITY],
@@ -694,7 +647,6 @@ impl SchemaInner {
                     description: String::from(
                         "A list of system provided optional attributes this class can store.",
                     ),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![],
@@ -711,7 +663,6 @@ impl SchemaInner {
                     description: String::from(
                         "A user modifiable list of optional attributes this class can store.",
                     ),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![],
@@ -728,7 +679,6 @@ impl SchemaInner {
                     description: String::from(
                         "A list of system provided required attributes this class must store.",
                     ),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![],
@@ -745,7 +695,6 @@ impl SchemaInner {
                     description: String::from(
                         "A user modifiable list of required attributes this class must store.",
                     ),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![],
@@ -761,7 +710,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_ACP_ENABLE)
                         .expect("unable to parse static uuid"),
                     description: String::from("A flag to determine if this ACP is active for application. True is enabled, and enforce. False is checked but not enforced."),
-                    system: true,
                     secret: false,
                     multivalue: false,
                     index: vec![IndexType::EQUALITY],
@@ -778,7 +726,6 @@ impl SchemaInner {
                     description: String::from(
                         "Who the ACP applies to, constraining or allowing operations.",
                     ),
-                    system: true,
                     secret: false,
                     multivalue: false,
                     index: vec![IndexType::EQUALITY, IndexType::SUBSTRING],
@@ -794,7 +741,6 @@ impl SchemaInner {
                     description: String::from(
                         "The effective targets of the ACP, IE what will be acted upon.",
                     ),
-                    system: true,
                     secret: false,
                     multivalue: false,
                     index: vec![IndexType::EQUALITY, IndexType::SUBSTRING],
@@ -808,7 +754,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_ACP_SEARCH_ATTR)
                         .expect("unable to parse static uuid"),
                     description: String::from("The attributes that may be viewed or searched by the reciever on targetscope."),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -824,7 +769,6 @@ impl SchemaInner {
                     description: String::from(
                         "The set of classes that can be created on a new entry.",
                     ),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -840,7 +784,6 @@ impl SchemaInner {
                     description: String::from(
                         "The set of attribute types that can be created on an entry.",
                     ),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -855,7 +798,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_ACP_MODIFY_REMOVEDATTR)
                         .expect("unable to parse static uuid"),
                     description: String::from("The set of attribute types that could be removed or purged in a modification."),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -869,7 +811,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_ACP_MODIFY_PRESENTATTR)
                         .expect("unable to parse static uuid"),
                     description: String::from("The set of attribute types that could be added or asserted in a modification."),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -883,7 +824,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_ACP_MODIFY_CLASS)
                         .expect("unable to parse static uuid"),
                     description: String::from("The set of class values that could be asserted or added to an entry. Only applies to modify::present operations on class."),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -898,7 +838,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_MEMBEROF)
                         .expect("unable to parse static uuid"),
                     description: String::from("reverse group membership of the object"),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -912,7 +851,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_DIRECTMEMBEROF)
                         .expect("unable to parse static uuid"),
                     description: String::from("reverse direct group membership of the object"),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -926,7 +864,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_MEMBER)
                         .expect("unable to parse static uuid"),
                     description: String::from("List of members of the group"),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -943,7 +880,6 @@ impl SchemaInner {
                     description: String::from(
                         "The systems internal migration version for provided objects",
                     ),
-                    system: true,
                     secret: true,
                     multivalue: false,
                     index: vec![IndexType::EQUALITY],
@@ -958,7 +894,6 @@ impl SchemaInner {
                     uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_DOMAIN)
                         .expect("unable to parse static uuid"),
                     description: String::from("A DNS Domain name entry."),
-                    system: true,
                     secret: false,
                     multivalue: true,
                     index: vec![IndexType::EQUALITY],
@@ -978,7 +913,6 @@ impl SchemaInner {
                     systemmust: vec![
                         String::from("class"),
                         String::from("name"),
-                        String::from("system"),
                         String::from("secret"),
                         String::from("multivalue"),
                         String::from("syntax"),
@@ -1187,6 +1121,19 @@ impl SchemaInner {
                     must: vec![],
                 },
             );
+            s.classes.insert(
+                String::from("system"),
+                SchemaClass {
+                    name: String::from("system"),
+                    uuid: Uuid::parse_str(UUID_SCHEMA_CLASS_SYSTEM)
+                        .expect("unable to parse static uuid"),
+                    description: String::from("A class denoting that a type is system generated and protected. It has special internal behaviour."),
+                    systemmay: vec![],
+                    may: vec![],
+                    systemmust: vec![],
+                    must: vec![],
+                },
+            );
 
             // TODO: Probably needs to do a collect.
             let r = s.validate(&mut au);
@@ -1468,7 +1415,6 @@ mod tests {
                         "class": ["object", "attributetype"],
                         "name": ["schema_attr_test"],
                         "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"],
-                        "system": ["false"],
                         "secret": ["false"],
                         "multivalue": ["false"],
                         "index": ["EQUALITY"],
@@ -1488,9 +1434,8 @@ mod tests {
                         "name": ["schema_attr_test"],
                         "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"],
                         "description": ["Test attr parsing"],
-                        "system": ["thoeunaouhnt"],
                         "secret": ["false"],
-                        "multivalue": ["false"],
+                        "multivalue": ["htouaoeu"],
                         "index": ["EQUALITY"],
                         "syntax": ["UTF8STRING"]
                     }
@@ -1508,7 +1453,6 @@ mod tests {
                         "name": ["schema_attr_test"],
                         "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"],
                         "description": ["Test attr parsing"],
-                        "system": ["true"],
                         "secret": ["false"],
                         "multivalue": ["false"],
                         "index": ["NTEHNOU"],
@@ -1528,7 +1472,6 @@ mod tests {
                         "name": ["schema_attr_test"],
                         "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"],
                         "description": ["Test attr parsing"],
-                        "system": ["true"],
                         "secret": ["false"],
                         "multivalue": ["false"],
                         "index": ["EQUALITY"],
@@ -1549,7 +1492,6 @@ mod tests {
                         "name": ["schema_attr_test"],
                         "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"],
                         "description": ["Test attr parsing"],
-                        "system": ["false"],
                         "secret": ["false"],
                         "multivalue": ["false"],
                         "syntax": ["UTF8STRING"]
@@ -1569,7 +1511,6 @@ mod tests {
                         "name": ["schema_attr_test"],
                         "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"],
                         "description": ["Test attr parsing"],
-                        "system": ["false"],
                         "secret": ["false"],
                         "multivalue": ["false"],
                         "index": ["EQUALITY"],
@@ -1742,7 +1683,6 @@ mod tests {
                 name: String::from("principal_name"),
                 uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_PRINCIPAL_NAME).expect("unable to parse static uuid"),
                 description: String::from("The longform name of an object, derived from name and domain. Example: alice@project.org"),
-                system: true,
                 secret: false,
                 multivalue: false,
                 index: vec![IndexType::EQUALITY],
@@ -1774,7 +1714,6 @@ mod tests {
             description: String::from(
                 "Who the ACP applies to, constraining or allowing operations.",
             ),
-            system: true,
             secret: false,
             multivalue: false,
             index: vec![IndexType::EQUALITY, IndexType::SUBSTRING],
@@ -1809,7 +1748,6 @@ mod tests {
             name: String::from("uuid"),
             uuid: Uuid::parse_str(UUID_SCHEMA_ATTR_UUID).expect("unable to parse static uuid"),
             description: String::from("The universal unique id of the object"),
-            system: true,
             secret: false,
             multivalue: false,
             index: vec![IndexType::EQUALITY],
@@ -1831,7 +1769,6 @@ mod tests {
             name: String::from("single_value"),
             uuid: Uuid::new_v4(),
             description: String::from(""),
-            system: true,
             secret: false,
             multivalue: false,
             index: vec![IndexType::EQUALITY],
@@ -1852,7 +1789,6 @@ mod tests {
             name: String::from("mv_string"),
             uuid: Uuid::new_v4(),
             description: String::from(""),
-            system: true,
             secret: false,
             multivalue: true,
             index: vec![IndexType::EQUALITY],
@@ -1868,7 +1804,6 @@ mod tests {
             name: String::from("mv_bool"),
             uuid: Uuid::new_v4(),
             description: String::from(""),
-            system: true,
             secret: false,
             multivalue: true,
             index: vec![IndexType::EQUALITY],
@@ -1889,7 +1824,6 @@ mod tests {
             name: String::from("sv_syntax"),
             uuid: Uuid::new_v4(),
             description: String::from(""),
-            system: true,
             secret: false,
             multivalue: false,
             index: vec![IndexType::EQUALITY],
@@ -1907,7 +1841,6 @@ mod tests {
             name: String::from("sv_index"),
             uuid: Uuid::new_v4(),
             description: String::from(""),
-            system: true,
             secret: false,
             multivalue: false,
             index: vec![IndexType::EQUALITY],
@@ -2018,7 +1951,6 @@ mod tests {
                 "class": ["object", "attributetype"],
                 "name": ["testattr"],
                 "description": ["testattr"],
-                "system": ["false"],
                 "secret": ["false"],
                 "multivalue": ["false"],
                 "syntax": ["UTF8STRING"],
@@ -2042,7 +1974,6 @@ mod tests {
                 "class": ["object", "attributetype"],
                 "name": ["testattr"],
                 "description": ["testattr"],
-                "system": ["false"],
                 "secret": ["false"],
                 "multivalue": ["zzzzz"],
                 "uuid": ["db237e8a-0079-4b8c-8a56-593b22aa44d1"],
@@ -2065,7 +1996,6 @@ mod tests {
                 "class": ["object", "attributetype"],
                 "name": ["testattr"],
                 "description": ["testattr"],
-                "system": ["false"],
                 "secret": ["false"],
                 "multivalue": ["true"],
                 "uuid": ["db237e8a-0079-4b8c-8a56-593b22aa44d1"],

src/lib/server.rs

@@ -698,14 +698,14 @@ impl<'a> QueryServerWriteTransaction<'a> {
 
         let norm_cand: Vec<Entry<_, _>> = try_audit!(au, res);
 
-        /*
+        // Run any pre-create plugins now with schema validated entries.
+        // This is important for normalisation of certain types IE class
+        // or attributes for these checks.
         let mut audit_plugin_pre = AuditScope::new("plugin_pre_create");
-        let plug_pre_res =
+        let plug_pre_res = Plugins::run_pre_create(&mut audit_plugin_pre, &self, &norm_cand, ce);
-            Plugins::run_pre_create(&mut audit_plugin_pre, &self, &norm_cand, ce);
         au.append_scope(audit_plugin_pre);
 
         let _ = try_audit!(au, plug_pre_res, "Create operation failed (plugin), {:?}");
-        */
 
         let mut audit_be = AuditScope::new("backend_create");
         // We may change from ce.entries later to something else?
@@ -2445,8 +2445,7 @@ mod tests {
                     "description": ["Test Attribute"],
                     "multivalue": ["false"],
                     "secret": ["false"],
-                    "syntax": ["UTF8STRING"],
+                    "syntax": ["UTF8STRING"]
-                    "system": ["false"]
                 }
             }"#,
             )