diff --git a/src/lib/access.rs b/src/lib/access.rs index 4dfb2ed29..e7440b78d 100644 --- a/src/lib/access.rs +++ b/src/lib/access.rs @@ -15,13 +15,11 @@ // requirements (also search). // -// TODO: Purge all reference to acp_allow/deny - use concread::cowcell::{CowCell, CowCellReadTxn, CowCellWriteTxn}; use std::collections::{BTreeMap, BTreeSet}; use crate::audit::AuditScope; -use crate::entry::{Entry, EntryCommitted, EntryNew, EntryNormalised, EntryValid}; +use crate::entry::{Entry, EntryCommitted, EntryNew, EntryNormalised, EntryReduced, EntryValid}; use crate::error::OperationError; use crate::filter::{Filter, FilterValid}; use crate::modify::Modify; @@ -518,7 +516,7 @@ pub trait AccessControlsTransaction { se: &SearchEvent, // TODO: This could actually take a &mut Vec and modify in place! entries: Vec>, - ) -> Result>, OperationError> { + ) -> Result>, OperationError> { /* * Super similar to above (could even re-use some parts). Given a set of entries, * reduce the attribute sets on them to "what is visible". This is ONLY called on @@ -532,8 +530,9 @@ pub trait AccessControlsTransaction { // interface is beyond me .... let rec_entry: &Entry = match &se.event.origin { EventOrigin::Internal => { + audit_log!(audit, "IMPOSSIBLE STATE: Internal search in external interface?! Returning empty for safety."); // No need to check ACS - return Ok(entries); + return Ok(Vec::new()); } EventOrigin::User(e) => &e, }; @@ -574,7 +573,7 @@ pub trait AccessControlsTransaction { // CAN'T see instead. // For each entry - let allowed_entries: Vec> = entries + let allowed_entries: Vec> = entries .into_iter() .map(|e| { // Get the set of attributes you can see @@ -754,12 +753,11 @@ pub trait AccessControlsTransaction { // set that apply to the entry that is performing the operation let scoped_acp: Vec<&AccessControlModify> = related_acp .iter() - // TODO: I don't like that acm here is a - // && - .filter_map(|acm| { + .filter_map(|acm: &&AccessControlModify| { // TODO: We are continually compiling and using these // in a tight loop, so this is a possible oppurtunity - // to cache or handle these better. + // to cache or handle these filters better - filter compiler + // cache maybe? let f_val = acm.acp.targetscope.clone(); match f_val.resolve(&me.event) { Ok(f_res) => { @@ -1189,7 +1187,7 @@ mod tests { AccessControlSearch, AccessControls, AccessControlsTransaction, }; use crate::audit::AuditScope; - use crate::entry::{Entry, EntryInvalid, EntryNew}; + use crate::entry::{Entry, EntryCommitted, EntryInvalid, EntryNew, EntryReduced}; // use crate::server::QueryServerWriteTransaction; use crate::event::{CreateEvent, DeleteEvent, ModifyEvent, SearchEvent}; @@ -1785,10 +1783,14 @@ mod tests { .search_filter_entry_attributes(&mut audit, $se, res) .expect("operation failed"); - println!("expect --> {:?}", $expect); + // Help the type checker for the expect set. + let expect_set: Vec> = + $expect.into_iter().map(|e| e.to_reduced()).collect(); + + println!("expect --> {:?}", expect_set); println!("result --> {:?}", reduced); // should be ok, and same as expect. - assert!(reduced == $expect); + assert!(reduced == expect_set); }}; } diff --git a/src/lib/audit.rs b/src/lib/audit.rs index c957fdcec..009764b78 100644 --- a/src/lib/audit.rs +++ b/src/lib/audit.rs @@ -48,6 +48,7 @@ macro_rules! audit_segment { let end = Instant::now(); let diff = end.duration_since(start); + audit_log!($au, "duration -> {:?}", diff); $au.set_duration(diff); // Return the result. Hope this works! diff --git a/src/lib/be/mod.rs b/src/lib/be/mod.rs index 5d74d8d6f..d3046363f 100644 --- a/src/lib/be/mod.rs +++ b/src/lib/be/mod.rs @@ -53,8 +53,8 @@ pub trait BackendTransaction { ) -> Result>, OperationError> { // Do things // Alloc a vec for the entries. - // FIXME: Make this actually a good size for the result set ... - // FIXME: Actually compute indexes here. + // TODO: Make this actually a good size for the result set ... + // TODO: Actually compute indexes here. // So to make this use indexes, we can use the filter type and // destructure it to work out what we need to actually search (if // possible) to create the candidate set. @@ -101,6 +101,7 @@ pub trait BackendTransaction { let entries: Result>, _> = raw_entries .iter() .filter_map(|id_ent| { + // We need the matches here to satisfy the filter map let db_e = match serde_cbor::from_slice(id_ent.data.as_slice()) .map_err(|_| OperationError::SerdeCborError) { @@ -113,7 +114,11 @@ pub trait BackendTransaction { Ok(v) => v, Err(e) => return Some(Err(e)), }; - let e = Entry::from_dbentry(db_e, id); + let e = + match Entry::from_dbentry(db_e, id).ok_or(OperationError::CorruptedEntry) { + Ok(v) => v, + Err(e) => return Some(Err(e)), + }; if e.entry_match_no_index(&filt) { Some(Ok(e)) } else { @@ -224,17 +229,15 @@ pub trait BackendTransaction { } impl Drop for BackendReadTransaction { - // Abort - // TODO: Is this correct for RO txn? + // Abort - so far this has proven reliable to use drop here. fn drop(self: &mut Self) { if !self.committed { debug!("Aborting BE RO txn"); self.conn .execute("ROLLBACK TRANSACTION", NO_PARAMS) - // TODO: Can we do this without expect? I think we can't due - // to the way drop works. - // - // We may need to change how we do transactions to not rely on drop + // We can't do this without expect. + // We may need to change how we do transactions to not rely on drop if + // it becomes and issue :( .expect("Unable to rollback transaction! Can not proceed!!!"); } } @@ -244,9 +247,11 @@ impl BackendReadTransaction { pub fn new(conn: r2d2::PooledConnection) -> Self { // Start the transaction debug!("Starting BE RO txn ..."); - // TODO: Way to flag that this will be a read only? - // TODO: Can we do this without expect? I think we need to change the type - // signature here if we wanted to... + // I'm happy for this to be an expect, because this is a huge failure + // of the server ... but if it happens a lot we should consider making + // this a Result<> + // + // There is no way to flag this is an RO operation. conn.execute("BEGIN TRANSACTION", NO_PARAMS) .expect("Unable to begin transaction!"); BackendReadTransaction { @@ -263,7 +268,6 @@ impl BackendTransaction for BackendReadTransaction { } static DBV_ID2ENTRY: &'static str = "id2entry"; -// static DBV_INDEX: &'static str = "index"; impl Drop for BackendWriteTransaction { // Abort @@ -272,10 +276,6 @@ impl Drop for BackendWriteTransaction { debug!("Aborting BE WR txn"); self.conn .execute("ROLLBACK TRANSACTION", NO_PARAMS) - // TODO: Can we do this without expect? I think we can't due - // to the way drop works. - // - // We may need to change how we do transactions to not rely on drop .expect("Unable to rollback transaction! Can not proceed!!!"); } } @@ -291,9 +291,6 @@ impl BackendWriteTransaction { pub fn new(conn: r2d2::PooledConnection) -> Self { // Start the transaction debug!("Starting BE WR txn ..."); - // TODO: Way to flag that this will be a write? - // TODO: Can we do this without expect? I think we need to change the type - // signature here if we wanted to... conn.execute("BEGIN TRANSACTION", NO_PARAMS) .expect("Unable to begin transaction!"); BackendWriteTransaction { @@ -347,11 +344,7 @@ impl BackendWriteTransaction { }) .collect(); - // audit_log!(au, "serialising: {:?}", ser_entries); - let ser_entries = ser_entries?; - - // THIS IS PROBABLY THE BIT WHERE YOU NEED DB ABSTRACTION { let mut stmt = try_audit!( au, @@ -388,8 +381,10 @@ impl BackendWriteTransaction { // tell which function is calling it. either this one or restore. audit_segment!(au, || { if entries.is_empty() { - // TODO: Better error - // End the timer + audit_log!( + au, + "No entries provided to BE to create, invalid server call!" + ); return Err(OperationError::EmptyRequest); } @@ -411,6 +406,10 @@ impl BackendWriteTransaction { entries: &Vec>, ) -> Result<(), OperationError> { if entries.is_empty() { + audit_log!( + au, + "No entries provided to BE to modify, invalid server call!" + ); return Err(OperationError::EmptyRequest); } @@ -434,8 +433,10 @@ impl BackendWriteTransaction { let data = serde_cbor::to_vec(&db_e).map_err(|_| OperationError::SerdeCborError)?; Ok(IdEntry { - // TODO: Instead of getting these from the entry, we could lookup + // TODO: Instead of getting these from the server entry struct , we could lookup // uuid -> id in the index. + // + // relies on the uuid -> id index being correct (and implemented) id: id, data: data, }) @@ -447,7 +448,9 @@ impl BackendWriteTransaction { // audit_log!(au, "serialising: {:?}", ser_entries); // Simple: If the list of id's is not the same as the input list, we are missing id's - // TODO: This check won't be needed once I rebuild the entry state types. + // + // The entry state checks prevent this from really ever being triggered, but we + // still prefer paranoia :) if entries.len() != ser_entries.len() { return Err(OperationError::InvalidEntryState); } @@ -483,7 +486,10 @@ impl BackendWriteTransaction { // Perform a search for the entries --> This is a problem for the caller audit_segment!(au, || { if entries.is_empty() { - // TODO: Better error + audit_log!( + au, + "No entries provided to BE to delete, invalid server call!" + ); return Err(OperationError::EmptyRequest); } @@ -506,7 +512,6 @@ impl BackendWriteTransaction { let id_list = try_audit!(au, id_list); // Simple: If the list of id's is not the same as the input list, we are missing id's - // TODO: This check won't be needed once I rebuild the entry state types. if entries.len() != id_list.len() { return Err(OperationError::InvalidEntryState); } @@ -516,8 +521,6 @@ impl BackendWriteTransaction { // SQL doesn't say if the thing "does or does not exist anymore". As a result, // two deletes is a safe and valid operation. Given how we allocate ID's we are // probably okay with this. - - // TODO: ACTUALLY HANDLE THIS ERROR WILLIAM YOU LAZY SHIT. let mut stmt = try_audit!( au, self.conn.prepare("DELETE FROM id2entry WHERE id = :id"), @@ -571,11 +574,15 @@ impl BackendWriteTransaction { OperationError::SerdeJsonError ); - self.internal_create(audit, &entries) + self.internal_create(audit, &entries)?; + let vr = self.verify(); + if vr.len() == 0 { + Ok(()) + } else { + Err(OperationError::ConsistencyError(vr)) + } // TODO: run re-index after db is restored - // TODO; run db verify - // self.verify(audit) } pub fn commit(mut self) -> Result<(), OperationError> { @@ -611,7 +618,7 @@ impl BackendWriteTransaction { { // TODO: // conn.execute("PRAGMA journal_mode=WAL;", NO_PARAMS) - // + // This stores versions of components. For example: // ---------------------- // | id | version | @@ -694,7 +701,7 @@ impl Backend { // a single DB thread, else we cause consistency issues. builder1.max_size(1) } else { - // FIXME: Make this configurable + // TODO: Make this configurable builder1.max_size(8) }; // Look at max_size and thread_pool here for perf later @@ -717,7 +724,6 @@ impl Backend { } pub fn read(&self) -> BackendReadTransaction { - // TODO: Don't use expect let conn = self .pool .get() @@ -726,7 +732,6 @@ impl Backend { } pub fn write(&self) -> BackendWriteTransaction { - // TODO: Don't use expect let conn = self .pool .get() diff --git a/src/lib/constants.rs b/src/lib/constants.rs index 7ac718cfe..a15cf5f61 100644 --- a/src/lib/constants.rs +++ b/src/lib/constants.rs @@ -117,6 +117,8 @@ pub static JSON_IDM_SELF_ACP_READ_V1: &'static str = r#"{ } }"#; +pub static UUID_DOES_NOT_EXIST: &'static str = "00000000-0000-0000-0000-fffffffffffe"; + pub static UUID_ANONYMOUS: &'static str = "00000000-0000-0000-0000-ffffffffffff"; pub static JSON_ANONYMOUS_V1: &'static str = r#"{ "valid": { @@ -133,12 +135,12 @@ pub static JSON_ANONYMOUS_V1: &'static str = r#"{ }"#; // Core +// TODO: All these should be internal range uuids instead. pub static UUID_SCHEMA_ATTR_CLASS: &'static str = "aa0f193f-3010-4783-9c9e-f97edb14d8c2"; pub static UUID_SCHEMA_ATTR_UUID: &'static str = "642a893b-fe1a-4fe1-805d-fb78e7f83ee7"; 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_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"; pub static UUID_SCHEMA_ATTR_SYNTAX: &'static str = "85e8c2c7-3852-48dd-bfc9-d0982a50e2ef"; @@ -213,9 +215,6 @@ pub static JSON_SCHEMA_ATTR_DISPLAYNAME: &'static str = r#"{ "name": [ "displayname" ], - "secret": [ - "false" - ], "syntax": [ "UTF8STRING" ], @@ -249,9 +248,6 @@ pub static JSON_SCHEMA_ATTR_MAIL: &'static str = r#" "name": [ "mail" ], - "secret": [ - "false" - ], "syntax": [ "UTF8STRING" ], @@ -284,9 +280,6 @@ pub static JSON_SCHEMA_ATTR_SSH_PUBLICKEY: &'static str = r#" "name": [ "ssh_publickey" ], - "secret": [ - "false" - ], "syntax": [ "UTF8STRING" ], @@ -319,9 +312,6 @@ pub static JSON_SCHEMA_ATTR_PASSWORD: &'static str = r#" "name": [ "password" ], - "secret": [ - "true" - ], "syntax": [ "UTF8STRING" ], diff --git a/src/lib/core.rs b/src/lib/core.rs index 4db974542..d5b7505a5 100644 --- a/src/lib/core.rs +++ b/src/lib/core.rs @@ -221,13 +221,12 @@ fn auth( } } } - AuthState::Denied => { + AuthState::Denied(_) => { // Remove the auth-session-id req.session().remove("auth-session-id"); Ok(HttpResponse::Ok().json(ar)) } AuthState::Continue(_) => { - // TODO: Where do we get the auth-session-id from? // Ensure the auth-session-id is set match req .session() diff --git a/src/lib/entry.rs b/src/lib/entry.rs index 3e001b7c9..c82737361 100644 --- a/src/lib/entry.rs +++ b/src/lib/entry.rs @@ -153,6 +153,9 @@ pub struct EntryInvalid; #[derive(Clone, Copy, Debug, Deserialize, Serialize)] pub struct EntryNormalised; +#[derive(Clone, Copy, Debug, Deserialize, Serialize)] +pub struct EntryReduced; + #[derive(Debug, Serialize, Deserialize)] pub struct Entry { valid: VALID, @@ -177,7 +180,11 @@ impl Entry { } } - // FIXME: Can we consume protoentry? + // Could we consume protoentry? + // + // I think we could, but that would limit us to how protoentry works, + // where we are likely to actually change the Entry type here and how + // we store and represent types and data. pub fn from_proto_entry( audit: &mut AuditScope, e: &ProtoEntry, @@ -189,7 +196,6 @@ impl Entry { // Somehow we need to take the tree of e attrs, and convert // all ref types to our types ... - let map2: Result>, OperationError> = e .attrs .iter() @@ -245,12 +251,7 @@ impl Entry { }; // Now validate it! - // First look at the classes on the entry. - // Now, check they are valid classes - // - // FIXME: We could restructure this to be a map that gets Some(class) - // if found, then do a len/filter/check on the resulting class set? - + // We scope here to limit the time of borrow of ne. { // First, check we have class on the object .... if !ne.attribute_pres("class") { @@ -258,14 +259,14 @@ impl Entry { return Err(SchemaError::InvalidClass); } - let entry_classes = ne.classes(); + // Do we have extensible? + let extensible = ne.attribute_value_pres("class", "extensibleobject"); + + let entry_classes = ne.classes().ok_or(SchemaError::InvalidClass)?; let entry_classes_size = entry_classes.len(); - let classes: HashMap = entry_classes - .filter_map(|c| match schema_classes.get(c) { - Some(cls) => Some((c.clone(), cls)), - None => None, - }) + let classes: Vec<&SchemaClass> = entry_classes + .filter_map(|c| schema_classes.get(c)) .collect(); if classes.len() != entry_classes_size { @@ -273,32 +274,26 @@ impl Entry { return Err(SchemaError::InvalidClass); }; - let extensible = classes.contains_key("extensibleobject"); - // What this is really doing is taking a set of classes, and building an - // "overall" class that describes this exact object for checking + // "overall" class that describes this exact object for checking. IE we + // build a super must/may set from the small class must/may sets. // for each class // add systemmust/must and systemmay/may to their lists // add anything from must also into may // Now from the set of valid classes make a list of must/may - // FIXME: This is clone on read, which may be really slow. It also may - // be inefficent on duplicates etc. // // NOTE: We still need this on extensible, because we still need to satisfy - // our other must conditions too! - let must: Result, _> = classes + // our other must conditions as well! + let must: Result, _> = classes .iter() // Join our class systemmmust + must into one iter - .flat_map(|(_, cls)| cls.systemmust.iter().chain(cls.must.iter())) + .flat_map(|cls| cls.systemmust.iter().chain(cls.must.iter())) .map(|s| { // This should NOT fail - if it does, it means our schema is // in an invalid state! - Ok(( - s.clone(), - schema_attributes.get(s).ok_or(SchemaError::Corrupted)?, - )) + Ok(schema_attributes.get(s).ok_or(SchemaError::Corrupted)?) }) .collect(); @@ -306,11 +301,10 @@ impl Entry { // Check that all must are inplace // for each attr in must, check it's present on our ent - // FIXME: Could we iter over only the attr_name - for (attr_name, _attr) in must { - let avas = ne.get_ava(&attr_name); + for attr in must { + let avas = ne.get_ava(&attr.name); if avas.is_none() { - return Err(SchemaError::MissingMustAttribute(String::from(attr_name))); + return Err(SchemaError::MissingMustAttribute(attr.name.clone())); } } @@ -338,10 +332,13 @@ impl Entry { } } } else { + // TODO: Again, we clone string here, but it's so we can check all + // the values in "may" ar here - so we can't avoid this look up. What we + // could do though, is have &String based on the schemaattribute though?; let may: Result, _> = classes .iter() // Join our class systemmmust + must + systemmay + may into one. - .flat_map(|(_, cls)| { + .flat_map(|cls| { cls.systemmust .iter() .chain(cls.must.iter()) @@ -360,8 +357,9 @@ impl Entry { let may = may?; - // FIXME: Error needs to say what is missing - // We need to return *all* missing attributes. + // TODO: Error needs to say what is missing + // We need to return *all* missing attributes, not just the first error + // we find. // Check that any other attributes are in may // for each attr on the object, check it's in the may+must set @@ -646,28 +644,37 @@ impl Entry { self.state.id } - pub fn from_dbentry(db_e: DbEntry, id: u64) -> Self { + pub fn from_dbentry(db_e: DbEntry, id: u64) -> Option { let attrs = match db_e.ent { DbEntryVers::V1(v1) => v1.attrs, }; - // TODO: Tidy this! let uuid: String = match attrs.get("uuid") { Some(vs) => vs.first(), None => None, - } - .expect("NO UUID PRESENT CORRUPT") + }? .clone(); - Entry { + Some(Entry { valid: EntryValid { uuid: uuid }, state: EntryCommitted { id }, attrs: attrs, + }) + } + + #[cfg(test)] + pub fn to_reduced(self) -> Entry { + Entry { + valid: EntryReduced, + state: self.state, + attrs: self.attrs, } } - pub fn reduce_attributes(self, allowed_attrs: BTreeSet<&str>) -> Self { - // TODO: Make this inplace, not copying. + pub fn reduce_attributes( + self, + allowed_attrs: BTreeSet<&str>, + ) -> Entry { // Remove all attrs from our tree that are NOT in the allowed set. let Entry { @@ -688,7 +695,7 @@ impl Entry { .collect(); Entry { - valid: s_valid, + valid: EntryReduced, state: s_state, attrs: f_attrs, } @@ -808,21 +815,6 @@ impl Entry { ))) } - // FIXME: This should probably have an entry state for "reduced" - // and then only that state can provide the into_pe type, so that we - // can guarantee that all entries must have been security checked. - pub fn into_pe(&self) -> ProtoEntry { - // It's very likely that at this stage we'll need to apply - // access controls, dynamic attributes or more. - // As a result, this may not even be the right place - // for the conversion as algorithmically it may be - // better to do this from the outside view. This can - // of course be identified and changed ... - ProtoEntry { - attrs: self.attrs.clone(), - } - } - pub fn gen_modlist_assert( &self, schema: &SchemaTransaction, @@ -844,6 +836,9 @@ impl Entry { // check can "go away" because uuid will never exist as an ava. // // TODO: Remove this check when uuid becomes a real attribute. + // UUID is now a real attribute, but it also has an ava for db_entry + // conversion - so what do? If we remove it here, we could have CSN issue with + // repl on uuid conflict, but it probably shouldn't be an ava either ... if k == "uuid" { continue; } @@ -868,9 +863,29 @@ impl Entry { } } +impl Entry { + pub fn into_pe(&self) -> ProtoEntry { + // It's very likely that at this stage we'll need to apply + // access controls, dynamic attributes or more. + // As a result, this may not even be the right place + // for the conversion as algorithmically it may be + // better to do this from the outside view. This can + // of course be identified and changed ... + ProtoEntry { + attrs: self.attrs.clone(), + } + } +} + +// impl Entry { impl Entry { - /* WARNING: Should these TODO move to EntryValid only? */ - // TODO: Should this be Vec<&str>? + /* + * WARNING: Should these TODO move to EntryValid only? + * I've tried to do this once, but the issue is that there + * is a lot of code in normalised and other states that + * relies on the ability to get ava. I think we may not be + * able to do so "easily". + */ pub fn get_ava(&self, attr: &str) -> Option<&Vec> { self.attrs.get(attr) } @@ -904,11 +919,23 @@ impl Entry { } pub fn attribute_pres(&self, attr: &str) -> bool { - // FIXME: Do we need to normalise attr name? + // Note, we don't normalise attr name, but I think that's not + // something we should over-optimise on. self.attrs.contains_key(attr) } pub fn attribute_value_pres(&self, attr: &str, value: &str) -> bool { + // Yeah, this is techdebt, but both names of this fn are valid - we are + // checking if an attribute-value is equal to, or asserting it's present + // as a pair. So I leave both, and let the compiler work it out. + self.attribute_equality(attr, value) + } + + pub fn attribute_equality(&self, attr: &str, value: &str) -> bool { + // we assume based on schema normalisation on the way in + // that the equality here of the raw values MUST be correct. + // We also normalise filters, to ensure that their values are + // syntax valid and will correctly match here with our indexes. match self.attrs.get(attr) { Some(v_list) => match v_list.binary_search(&value.to_string()) { Ok(_) => true, @@ -918,35 +945,23 @@ impl Entry { } } - pub fn attribute_equality(&self, attr: &str, value: &str) -> bool { - // we assume based on schema normalisation on the way in - // that the equality here of the raw values MUST be correct. - // We also normalise filters, to ensure that their values are - // syntax valid and will correctly match here with our indexes. - - // FIXME: Make this binary_search - - self.attrs.get(attr).map_or(false, |v| { - v.iter() - .fold(false, |acc, av| if acc { acc } else { value == av }) - }) + pub fn attribute_substring(&self, attr: &str, subvalue: &str) -> bool { + match self.attrs.get(attr) { + Some(v_list) => { + v_list + .iter() + .fold(false, |acc, v| if acc { acc } else { v.contains(subvalue) }) + } + None => false, + } } - pub fn attribute_substring(&self, _attr: &str, _subvalue: &str) -> bool { - unimplemented!(); - } - - pub fn classes(&self) -> EntryClasses { + pub fn classes(&self) -> Option { // Get the class vec, if any? // How do we indicate "empty?" - // FIXME: Actually handle this error ... - let v = self - .attrs - .get("class") - .map(|c| c.len()) - .expect("INVALID STATE, NO CLASS FOUND"); + let v = self.attrs.get("class").map(|c| c.len())?; let c = self.attrs.get("class").map(|c| c.iter()); - EntryClasses { size: v, inner: c } + Some(EntryClasses { size: v, inner: c }) } pub fn avas(&self) -> EntryAvas { @@ -1001,22 +1016,20 @@ where // a list of syntax violations ... // If this already exists, we silently drop the event? Is that an // acceptable interface? - // Should value here actually be a &str? pub fn add_ava(&mut self, attr: &str, value: &str) { - // get_mut to access value // How do we make this turn into an ok / err? self.attrs .entry(attr.to_string()) .and_modify(|v| { // Here we need to actually do a check/binary search ... - // FIXME: Because map_err is lazy, this won't do anything on release - // TODO: Is there a way to avoid the double to_string here? match v.binary_search(&value.to_string()) { // It already exists, done! Ok(_) => {} Err(idx) => { // This cloning is to fix a borrow issue with the or_insert below. // Is there a better way? + // + // I think it's only run once anyway, so non-issue? v.insert(idx, value.to_string()) } } @@ -1025,31 +1038,26 @@ where } pub fn remove_ava(&mut self, attr: &str, value: &str) { - // TODO fix this to_string + // It would be great to remove these extra allocations, but they + // really don't cost much :( let mv = value.to_string(); - self.attrs - // TODO: Fix this to_string - .entry(attr.to_string()) - .and_modify(|v| { - // Here we need to actually do a check/binary search ... - // FIXME: Because map_err is lazy, this won't do anything on release - match v.binary_search(&mv) { - // It exists, rm it. - Ok(idx) => { - v.remove(idx); - } - // It does not exist, move on. - Err(_) => {} + self.attrs.entry(attr.to_string()).and_modify(|v| { + // Here we need to actually do a check/binary search ... + match v.binary_search(&mv) { + // It exists, rm it. + Ok(idx) => { + v.remove(idx); } - }); + // It does not exist, move on. + Err(_) => {} + } + }); } pub fn purge_ava(&mut self, attr: &str) { self.attrs.remove(attr); } - // FIXME: Should this collect from iter instead? - // TODO: Should this be a Vec<&str> /// Overwrite the existing avas. pub fn set_avas(&mut self, attr: &str, values: Vec) { // Overwrite the existing value @@ -1063,6 +1071,7 @@ where } // Should this be schemaless, relying on checks of the modlist, and the entry validate after? + // YES. Makes it very cheap. pub fn apply_modlist(&mut self, modlist: &ModifyList) { // -> Result, OperationError> { // Apply a modlist, generating a new entry that conforms to the changes. @@ -1104,12 +1113,6 @@ impl From<&SchemaAttribute> for Entry { let name_v = vec![s.name.clone()]; let desc_v = vec![s.description.clone()]; - let secret_v = vec![if s.secret { - "true".to_string() - } else { - "false".to_string() - }]; - let multivalue_v = vec![if s.multivalue { "true".to_string() } else { @@ -1125,7 +1128,6 @@ impl From<&SchemaAttribute> for Entry { attrs.insert("name".to_string(), name_v); attrs.insert("description".to_string(), desc_v); attrs.insert("uuid".to_string(), uuid_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); @@ -1233,6 +1235,21 @@ mod tests { assert!(!e.attribute_equality("nonexist", "william")); } + #[test] + fn test_entry_substring() { + let mut e: Entry = Entry::new(); + + e.add_ava("userid", "william"); + + assert!(e.attribute_substring("userid", "william")); + assert!(e.attribute_substring("userid", "will")); + assert!(e.attribute_substring("userid", "liam")); + assert!(e.attribute_substring("userid", "lli")); + assert!(!e.attribute_substring("userid", "llim")); + assert!(!e.attribute_substring("userid", "bob")); + assert!(!e.attribute_substring("userid", "wl")); + } + #[test] fn test_entry_apply_modlist() { // Test application of changes to an entry. diff --git a/src/lib/error.rs b/src/lib/error.rs index 90c0eefde..0ce1b64b3 100644 --- a/src/lib/error.rs +++ b/src/lib/error.rs @@ -4,8 +4,6 @@ pub enum SchemaError { NotImplemented, InvalidClass, - // FIXME: Is there a way to say what we are missing on error? - // Yes, add a string on the enum. MissingMustAttribute(String), InvalidAttribute, InvalidAttributeSyntax, @@ -18,6 +16,7 @@ pub enum OperationError { EmptyRequest, Backend, NoMatchingEntries, + CorruptedEntry, ConsistencyError(Vec>), SchemaViolation(SchemaError), Plugin, diff --git a/src/lib/event.rs b/src/lib/event.rs index f148e8852..0c827f78d 100644 --- a/src/lib/event.rs +++ b/src/lib/event.rs @@ -1,5 +1,5 @@ use crate::audit::AuditScope; -use crate::entry::{Entry, EntryCommitted, EntryInvalid, EntryNew, EntryValid}; +use crate::entry::{Entry, EntryCommitted, EntryInvalid, EntryNew, EntryReduced, EntryValid}; use crate::filter::{Filter, FilterValid}; use crate::proto::v1::Entry as ProtoEntry; use crate::proto::v1::{ @@ -30,7 +30,7 @@ use actix::prelude::*; use uuid::Uuid; // Should the event Result have the log items? -// FIXME: Remove seralising here - each type should +// TODO: Remove seralising here - each type should // have it's own result type! // TODO: Every event should have a uuid for logging analysis @@ -50,7 +50,7 @@ pub struct SearchResult { } impl SearchResult { - pub fn new(entries: Vec>) -> Self { + pub fn new(entries: Vec>) -> Self { SearchResult { entries: entries .iter() @@ -119,7 +119,7 @@ impl Event { let uat = uat.ok_or(OperationError::NotAuthenticated)?; let e = try_audit!(audit, qs.internal_search_uuid(audit, uat.uuid.as_str())); - // FIXME: Now apply claims from the uat into the Entry + // TODO: Now apply claims from the uat into the Entry // to allow filtering. Ok(Event { @@ -342,7 +342,7 @@ pub struct CreateEvent { // This may affect which plugins are run ... } -// FIXME: Should this actually be in createEvent handler? +// TODO: Should this actually be in createEvent handler? impl CreateEvent { pub fn from_request( audit: &mut AuditScope, @@ -679,7 +679,7 @@ pub struct WhoamiResult { } impl WhoamiResult { - pub fn new(e: Entry) -> Self { + pub fn new(e: Entry) -> Self { WhoamiResult { youare: e.into_pe(), } diff --git a/src/lib/idm/authsession.rs b/src/lib/idm/authsession.rs index 7aac4cde0..1691fe1e3 100644 --- a/src/lib/idm/authsession.rs +++ b/src/lib/idm/authsession.rs @@ -13,7 +13,7 @@ enum CredState { Success(Vec), Continue(Vec), // TODO: Should we have a reason in Denied so that we - Denied, + Denied(&'static str), } #[derive(Clone, Debug)] @@ -35,24 +35,27 @@ impl CredHandler { pub fn validate(&mut self, creds: &Vec) -> CredState { match self { CredHandler::Anonymous => { - creds.iter().fold(CredState::Denied, |acc, cred| { - // TODO: if denied, continue returning denied. - // TODO: if continue, contunue returning continue. - // How to do this correctly? + creds.iter().fold( + CredState::Denied("non-anonymous credential provided"), + |acc, cred| { + // TODO: if denied, continue returning denied. + // TODO: if continue, contunue returning continue. + // How to do this correctly? - // There is no "continuation" from this type. - match cred { - AuthCredential::Anonymous => { - // For anonymous, no claims will ever be issued. - CredState::Success(Vec::new()) + // There is no "continuation" from this type. + match cred { + AuthCredential::Anonymous => { + // For anonymous, no claims will ever be issued. + CredState::Success(Vec::new()) + } + _ => { + // Should we have a reason in Denied so that we can say why denied? + acc + // CredState::Denied + } } - _ => { - // Should we have a reason in Denied so that we can say why denied? - acc - // CredState::Denied - } - } - }) + }, + ) } } } @@ -137,9 +140,9 @@ impl AuthSession { Ok(AuthState::Success(uat)) } CredState::Continue(allowed) => Ok(AuthState::Continue(allowed)), - CredState::Denied => { + CredState::Denied(reason) => { self.finished = true; - Ok(AuthState::Denied) + Ok(AuthState::Denied(reason.to_string())) } } // Also send an async message to self to log the auth as provided. diff --git a/src/lib/idm/server.rs b/src/lib/idm/server.rs index aed69c519..02c514e36 100644 --- a/src/lib/idm/server.rs +++ b/src/lib/idm/server.rs @@ -13,12 +13,12 @@ use uuid::Uuid; // use lru::LruCache; pub struct IdmServer { - // Need an auth-session table to save in progress authentications - // sessions: + // There is a good reason to keep this single thread - it + // means that limits to sessions can be easily applied and checked to + // variaous accounts, and we have a good idea of how to structure the + // in memory caches related to locking. // - // TODO: This should be Lru - // TODO: AuthSession should be per-session mutex to keep locking on the - // cell low to allow more concurrent auths. + // TODO: This needs a mark-and-sweep gc to be added. sessions: CowCell>, // Need a reference to the query server. qs: QueryServer, @@ -60,7 +60,6 @@ impl IdmServer { } impl<'a> IdmServerWriteTransaction<'a> { - // TODO: This should be something else, not the proto token! pub fn auth( &mut self, au: &mut AuditScope, diff --git a/src/lib/plugins/base.rs b/src/lib/plugins/base.rs index 63cd7966e..6414288cd 100644 --- a/src/lib/plugins/base.rs +++ b/src/lib/plugins/base.rs @@ -1,8 +1,10 @@ use crate::plugins::Plugin; -use std::collections::BTreeMap; +use std::collections::BTreeSet; +use std::ops::Bound::Included; use uuid::Uuid; use crate::audit::AuditScope; +use crate::constants::{UUID_ADMIN, UUID_ANONYMOUS, UUID_DOES_NOT_EXIST}; use crate::entry::{Entry, EntryCommitted, EntryInvalid, EntryNew}; use crate::error::{ConsistencyError, OperationError}; use crate::event::{CreateEvent, ModifyEvent}; @@ -11,13 +13,6 @@ use crate::server::{ QueryServerReadTransaction, QueryServerTransaction, QueryServerWriteTransaction, }; -// TO FINISH -/* -Add normalisation step -Add filter normaliser to search. -Add principal name generation -*/ - // This module has some special properties around it's operation, namely that it // has to make a certain number of assertions *early* in the entry lifecycle around // names and uuids since these have such signifigance to every other part of the @@ -43,9 +38,9 @@ impl Plugin for Base { fn pre_create_transform( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &mut Vec>, - _ce: &CreateEvent, + ce: &CreateEvent, ) -> Result<(), OperationError> { // For each candidate for entry in cand.iter_mut() { @@ -59,17 +54,16 @@ impl Plugin for Base { // If they have a name, but no principal name, derive it. // if they don't have uuid, create it. - // TODO: get_ava should have a str version for effeciency? let c_uuid: String = match entry.get_ava("uuid") { Some(u) => { // Actually check we have a value, could be empty array ... - // TODO: Should this be left to schema to assert the value? if u.len() > 1 { audit_log!(au, "Entry defines uuid attr, but multiple values."); return Err(OperationError::Plugin); }; // Schema of the value v, is checked in the filter generation. Neat! + // That way we don't need to check it here either. // Should this be forgiving and just generate the UUID? // NO! If you tried to specify it, but didn't give it, then you made @@ -90,7 +84,7 @@ impl Plugin for Base { } // Now, every cand has a UUID - create a cand uuid set from it. - let mut cand_uuid: BTreeMap<&String, ()> = BTreeMap::new(); + let mut cand_uuid: BTreeSet<&str> = BTreeSet::new(); // As we insert into the set, if a duplicate is found, return an error // that a duplicate exists. @@ -101,26 +95,53 @@ impl Plugin for Base { .first() .ok_or(OperationError::Plugin)?; audit_log!(au, "Entry valid UUID: {:?}", entry); - match cand_uuid.insert(uuid_ref, ()) { - Some(v) => { - audit_log!(au, "uuid duplicate found in create set! {:?}", v); + match cand_uuid.insert(uuid_ref.as_str()) { + false => { + audit_log!(au, "uuid duplicate found in create set! {:?}", uuid_ref); return Err(OperationError::Plugin); } - None => {} + true => {} } } + // Check that the system-protected range is not in the cand_uuid, unless we are + // an internal operation. + if !ce.event.is_internal() { + // The internal set is bounded by: UUID_ADMIN -> UUID_ANONYMOUS + // Sadly we need to allocate these to strings to make references, sigh. + // let uuid_admin: String = UUID_ADMIN.to_string(); + // let uuid_anon: String = UUID_ANONYMOUS.to_string(); + let overlap: usize = cand_uuid.range(UUID_ADMIN..UUID_ANONYMOUS).count(); + if overlap != 0 { + audit_log!( + au, + "uuid from protected system UUID range found in create set! {:?}", + overlap + ); + return Err(OperationError::Plugin); + } + } + + if cand_uuid.contains(UUID_DOES_NOT_EXIST) { + audit_log!( + au, + "uuid \"does not exist\" found in create set! {:?}", + UUID_DOES_NOT_EXIST + ); + return Err(OperationError::Plugin); + } + // Now from each element, generate a filter to search for all of them // - // NOTE: We don't exclude recycled or tombstones here! - - let filt_in = filter_all!(FC::Or(cand_uuid.keys().map(|u| f_eq("uuid", u)).collect(),)); + // IMPORTANT: We don't exclude recycled or tombstones here! + let filt_in = filter_all!(FC::Or(cand_uuid.iter().map(|u| f_eq("uuid", u)).collect(),)); // If any results exist, fail as a duplicate UUID is present. // TODO: Can we report which UUID exists? Probably yes, we do - // internal searh and report the UUID *OR* we alter internal_exists + // internal search and report the UUID *OR* we alter internal_exists // to return UUID sets. - + // + // But does it add value? How many people will try to custom define/add uuid? let mut au_qs = AuditScope::new("qs_exist"); let r = qs.internal_exists(&mut au_qs, filt_in); au.append_scope(au_qs); @@ -143,7 +164,7 @@ impl Plugin for Base { fn pre_modify( au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, _cand: &mut Vec>, me: &ModifyEvent, ) -> Result<(), OperationError> { @@ -225,12 +246,44 @@ impl Plugin for Base { mod tests { // #[macro_use] // use crate::plugins::Plugin; + use crate::constants::JSON_ADMIN_V1; use crate::entry::{Entry, EntryInvalid, EntryNew}; use crate::error::OperationError; use crate::modify::{Modify, ModifyList}; use crate::server::QueryServerTransaction; use crate::server::QueryServerWriteTransaction; + 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", "uuid"] + } + }"#; + // check create where no uuid #[test] fn test_pre_create_no_uuid() { @@ -572,4 +625,71 @@ mod tests { |_, _| {} ); } + + #[test] + fn test_protected_uuid_range() { + // Test an external create, it should fail. + // Testing internal create is not super needed, due to migrations at start + // up testing this every time we run :P + let acp: Entry = + serde_json::from_str(JSON_ADMIN_ALLOW_ALL).expect("json parse failure"); + + let preload = vec![acp]; + + let e: Entry = serde_json::from_str( + r#"{ + "valid": null, + "state": null, + "attrs": { + "class": ["person", "system"], + "name": ["testperson"], + "uuid": ["00000000-0000-0000-0000-f0f0f0f0f0f0"], + "description": ["testperson"], + "displayname": ["testperson"] + } + }"#, + ) + .expect("json parse failure"); + + let create = vec![e.clone()]; + + run_create_test!( + Err(OperationError::Plugin), + preload, + create, + Some(JSON_ADMIN_V1), + |_, _| {} + ); + } + + #[test] + fn test_protected_uuid_does_not_exist() { + // Test that internal create of "does not exist" will fail. + let preload = Vec::new(); + + let e: Entry = serde_json::from_str( + r#"{ + "valid": null, + "state": null, + "attrs": { + "class": ["person", "system"], + "name": ["testperson"], + "uuid": ["00000000-0000-0000-0000-fffffffffffe"], + "description": ["testperson"], + "displayname": ["testperson"] + } + }"#, + ) + .expect("json parse failure"); + + let create = vec![e.clone()]; + + run_create_test!( + Err(OperationError::Plugin), + preload, + create, + None, + |_, _| {} + ); + } } diff --git a/src/lib/plugins/macros.rs b/src/lib/plugins/macros.rs index 3bc0bbbd5..6b68ba2e4 100644 --- a/src/lib/plugins/macros.rs +++ b/src/lib/plugins/macros.rs @@ -16,7 +16,7 @@ macro_rules! setup_test { qs.initialise_helper($au).expect("init failed!"); if !$preload_entries.is_empty() { - let qs_write = qs.write(); + let mut qs_write = qs.write(); qs_write .internal_create($au, $preload_entries) .expect("Failed to preload entries"); @@ -56,7 +56,7 @@ macro_rules! run_create_test { let mut au_test = AuditScope::new("create_test"); { - let qs_write = qs.write(); + let mut qs_write = qs.write(); let r = qs_write.create(&mut au_test, &ce); debug!("r: {:?}", r); assert!(r == $expect); @@ -110,7 +110,7 @@ macro_rules! run_modify_test { let mut au_test = AuditScope::new("modify_test"); { - let qs_write = qs.write(); + let mut qs_write = qs.write(); let r = qs_write.modify(&mut au_test, &me); $check(&mut au_test, &qs_write); assert!(r == $expect); @@ -162,7 +162,7 @@ macro_rules! run_delete_test { let mut au_test = AuditScope::new("delete_test"); { - let qs_write = qs.write(); + let mut qs_write = qs.write(); let r = qs_write.delete(&mut au_test, &de); $check(&mut au_test, &qs_write); assert!(r == $expect); diff --git a/src/lib/plugins/memberof.rs b/src/lib/plugins/memberof.rs index 319c27f15..9f54fdc9a 100644 --- a/src/lib/plugins/memberof.rs +++ b/src/lib/plugins/memberof.rs @@ -50,10 +50,11 @@ where .flatten() .collect(); - // Sort - // TODO: promote groups to head of the affected_uuids set! - // this could be assisted by indexing in the future by providing a custom compare - // algo!!! + // IDEA: promote groups to head of the affected_uuids set! + // + // This isn't worth doing - it's only used in create/delete, it would not + // really make a large performance difference. Better to target improvements + // in the apply_memberof fn. affected_uuids.sort(); // Remove dups affected_uuids.dedup(); @@ -63,7 +64,7 @@ where fn apply_memberof( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, affected_uuids: Vec<&String>, ) -> Result<(), OperationError> { audit_log!(au, " => entering apply_memberof"); @@ -174,7 +175,7 @@ impl Plugin for MemberOf { fn post_create( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &Vec>, _ce: &CreateEvent, ) -> Result<(), OperationError> { @@ -187,7 +188,7 @@ impl Plugin for MemberOf { fn post_modify( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, pre_cand: &Vec>, cand: &Vec>, _me: &ModifyEvent, @@ -198,16 +199,17 @@ impl Plugin for MemberOf { .zip(cand.iter()) .filter(|(pre, post)| { // This is the base case to break cycles in recursion! - pre != post - && ( - // AND if it was a group, or will become a group. + ( + // If it was a group, or will become a group. post.attribute_value_pres("class", "group") || pre.attribute_value_pres("class", "group") ) + // And the group has changed ... + && pre != post + // Then memberof should be updated! }) // Flatten the pre-post tuples. We no longer care if it was // pre-post - // TODO: Could this be more effecient? .flat_map(|(pre, post)| vec![pre, post]) .inspect(|e| { audit_log!(au, "group reporting change: {:?}", e); @@ -229,7 +231,7 @@ impl Plugin for MemberOf { fn pre_delete( _au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, cand: &mut Vec>, _de: &DeleteEvent, ) -> Result<(), OperationError> { @@ -256,7 +258,7 @@ impl Plugin for MemberOf { fn post_delete( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &Vec>, _ce: &DeleteEvent, ) -> Result<(), OperationError> { diff --git a/src/lib/plugins/mod.rs b/src/lib/plugins/mod.rs index f0e1c8d96..b44578ddb 100644 --- a/src/lib/plugins/mod.rs +++ b/src/lib/plugins/mod.rs @@ -19,7 +19,7 @@ trait Plugin { fn pre_create_transform( _au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, _cand: &mut Vec>, _ce: &CreateEvent, ) -> Result<(), OperationError> { @@ -32,7 +32,7 @@ trait Plugin { fn pre_create( _au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, // List of what we will commit that is valid? _cand: &Vec>, _ce: &CreateEvent, @@ -43,7 +43,7 @@ trait Plugin { fn post_create( _au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, // List of what we commited that was valid? _cand: &Vec>, _ce: &CreateEvent, @@ -54,7 +54,7 @@ trait Plugin { fn pre_modify( _au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, _cand: &mut Vec>, _me: &ModifyEvent, ) -> Result<(), OperationError> { @@ -64,7 +64,7 @@ trait Plugin { fn post_modify( _au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, // List of what we modified that was valid? _pre_cand: &Vec>, _cand: &Vec>, @@ -76,7 +76,7 @@ trait Plugin { fn pre_delete( _au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, _cand: &mut Vec>, _de: &DeleteEvent, ) -> Result<(), OperationError> { @@ -86,7 +86,7 @@ trait Plugin { fn post_delete( _au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, // List of what we delete that was valid? _cand: &Vec>, _ce: &DeleteEvent, @@ -270,7 +270,7 @@ macro_rules! run_verify_plugin { impl Plugins { pub fn run_pre_create_transform( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &mut Vec>, ce: &CreateEvent, ) -> Result<(), OperationError> { @@ -283,7 +283,7 @@ impl Plugins { pub fn run_pre_create( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &Vec>, ce: &CreateEvent, ) -> Result<(), OperationError> { @@ -296,7 +296,7 @@ impl Plugins { pub fn run_post_create( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &Vec>, ce: &CreateEvent, ) -> Result<(), OperationError> { @@ -310,7 +310,7 @@ impl Plugins { pub fn run_pre_modify( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &mut Vec>, me: &ModifyEvent, ) -> Result<(), OperationError> { @@ -324,7 +324,7 @@ impl Plugins { pub fn run_post_modify( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, pre_cand: &Vec>, cand: &Vec>, me: &ModifyEvent, @@ -342,7 +342,7 @@ impl Plugins { pub fn run_pre_delete( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &mut Vec>, de: &DeleteEvent, ) -> Result<(), OperationError> { @@ -354,7 +354,7 @@ impl Plugins { pub fn run_post_delete( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &Vec>, de: &DeleteEvent, ) -> Result<(), OperationError> { diff --git a/src/lib/plugins/protected.rs b/src/lib/plugins/protected.rs index 29b7b559c..e5c628aa4 100644 --- a/src/lib/plugins/protected.rs +++ b/src/lib/plugins/protected.rs @@ -19,7 +19,7 @@ impl Plugin for Protected { fn pre_create( au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, // List of what we will commit that is valid? cand: &Vec>, ce: &CreateEvent, @@ -46,7 +46,7 @@ impl Plugin for Protected { fn pre_modify( au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, // Should these be EntryValid? cand: &mut Vec>, me: &ModifyEvent, @@ -118,7 +118,7 @@ impl Plugin for Protected { fn pre_delete( au: &mut AuditScope, - _qs: &QueryServerWriteTransaction, + _qs: &mut QueryServerWriteTransaction, // Should these be EntryValid cand: &mut Vec>, de: &DeleteEvent, diff --git a/src/lib/plugins/refint.rs b/src/lib/plugins/refint.rs index 00f44539d..a5d98c216 100644 --- a/src/lib/plugins/refint.rs +++ b/src/lib/plugins/refint.rs @@ -74,7 +74,7 @@ impl Plugin for ReferentialIntegrity { // be in cand AND db" to simply "is it in the DB?". fn post_create( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &Vec>, _ce: &CreateEvent, ) -> Result<(), OperationError> { @@ -102,7 +102,7 @@ impl Plugin for ReferentialIntegrity { fn post_modify( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, _pre_cand: &Vec>, _cand: &Vec>, me: &ModifyEvent, @@ -131,7 +131,7 @@ impl Plugin for ReferentialIntegrity { fn post_delete( au: &mut AuditScope, - qs: &QueryServerWriteTransaction, + qs: &mut QueryServerWriteTransaction, cand: &Vec>, _ce: &DeleteEvent, ) -> Result<(), OperationError> { diff --git a/src/lib/proto/v1/actors.rs b/src/lib/proto/v1/actors.rs index 97ab924ed..da2cbc8da 100644 --- a/src/lib/proto/v1/actors.rs +++ b/src/lib/proto/v1/actors.rs @@ -143,7 +143,7 @@ impl Handler for QueryServerV1 { fn handle(&mut self, msg: CreateRequest, _: &mut Self::Context) -> Self::Result { let mut audit = AuditScope::new("create"); let res = audit_segment!(&mut audit, || { - let qs_write = self.qs.write(); + let mut qs_write = self.qs.write(); let crt = match CreateEvent::from_request(&mut audit, msg, &qs_write) { Ok(c) => c, @@ -171,7 +171,7 @@ impl Handler for QueryServerV1 { fn handle(&mut self, msg: ModifyRequest, _: &mut Self::Context) -> Self::Result { let mut audit = AuditScope::new("modify"); let res = audit_segment!(&mut audit, || { - let qs_write = self.qs.write(); + let mut qs_write = self.qs.write(); let mdf = match ModifyEvent::from_request(&mut audit, msg, &qs_write) { Ok(m) => m, Err(e) => { @@ -197,7 +197,7 @@ impl Handler for QueryServerV1 { fn handle(&mut self, msg: DeleteRequest, _: &mut Self::Context) -> Self::Result { let mut audit = AuditScope::new("delete"); let res = audit_segment!(&mut audit, || { - let qs_write = self.qs.write(); + let mut qs_write = self.qs.write(); let del = match DeleteEvent::from_request(&mut audit, msg, &qs_write) { Ok(d) => d, @@ -270,7 +270,7 @@ impl Handler for QueryServerV1 { // Make an event from the whoami request. This will process the event and // generate a selfuuid search. - // FIXME: This current handles the unauthenticated check, and will + // TODO: This current handles the unauthenticated check, and will // trigger the failure, but if we can manage to work out async // then move this to core.rs, and don't allow Option to get // this far. diff --git a/src/lib/proto/v1/mod.rs b/src/lib/proto/v1/mod.rs index 599b410ba..bac8da178 100644 --- a/src/lib/proto/v1/mod.rs +++ b/src/lib/proto/v1/mod.rs @@ -68,7 +68,7 @@ pub struct UserAuthToken { /* ===== low level proto types ===== */ -// FIXME: We probably need a proto entry to transform our +// TODO: We probably need a proto entry to transform our // server core entry into. We also need to get from proto // entry to our actual entry. // @@ -265,7 +265,7 @@ pub enum AuthState { // for the client to view. Success(UserAuthToken), // Something was bad, your session is terminated and no cookie. - Denied, + Denied(String), // Continue to auth, allowed mechanisms listed. Continue(Vec), } diff --git a/src/lib/schema.rs b/src/lib/schema.rs index 422425043..e05899d6a 100644 --- a/src/lib/schema.rs +++ b/src/lib/schema.rs @@ -123,9 +123,6 @@ pub struct SchemaAttribute { pub uuid: Uuid, // Perhaps later add aliases? pub description: String, - // 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, pub syntax: SyntaxType, @@ -144,7 +141,6 @@ impl SchemaAttribute { } // uuid - // TODO: Alloc and free of string here? let uuid = try_audit!( audit, Uuid::parse_str(value.get_uuid().as_str()) @@ -166,13 +162,6 @@ impl SchemaAttribute { .ok_or(OperationError::InvalidSchemaState("missing description")) ); - // secret - let secret = try_audit!( - audit, - value - .get_ava_single_bool("secret") - .ok_or(OperationError::InvalidSchemaState("missing secret")) - ); // multivalue let multivalue = try_audit!( audit, @@ -201,7 +190,6 @@ impl SchemaAttribute { name: name.clone(), uuid: uuid.clone(), description: description.clone(), - secret: secret, multivalue: multivalue, index: index, syntax: syntax, @@ -272,7 +260,6 @@ impl SchemaAttribute { } fn validate_utf8string_insensitive(&self, v: &String) -> Result<(), SchemaError> { - // FIXME: Is there a way to do this that doesn't involve a copy? let t = v.to_lowercase(); if &t == v { Ok(()) @@ -390,7 +377,7 @@ impl SchemaAttribute { } } - // FIXME: This clones everything, which is expensive! + // TODO: This clones everything, which is expensive! pub fn normalise_value(&self, v: &String) -> String { match self.syntax { SyntaxType::SYNTAX_ID => self.normalise_syntax(v), @@ -430,7 +417,6 @@ impl SchemaClass { } // uuid - // TODO: Alloc and free of string here? let uuid = try_audit!( audit, Uuid::parse_str(value.get_uuid().as_str()) @@ -529,7 +515,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"), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -542,7 +527,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"), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UUID, @@ -555,7 +539,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"), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -567,7 +550,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"), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_PRINCIPAL, @@ -580,33 +562,20 @@ 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"), - secret: false, multivalue: true, index: vec![], syntax: SyntaxType::UTF8STRING, }, ); - 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."), - secret: false, - multivalue: false, - index: vec![], - syntax: SyntaxType::BOOLEAN, - }); s.attributes.insert(String::from("multivalue"), SchemaAttribute { 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."), - secret: false, multivalue: false, index: vec![], syntax: SyntaxType::BOOLEAN, }); s.attributes.insert( - // FIXME: Rename to index_attribute? attr_index? String::from("index"), SchemaAttribute { name: String::from("index"), @@ -615,14 +584,12 @@ impl SchemaInner { description: String::from( "Describe the indexes to apply to instances of this attribute.", ), - secret: false, multivalue: true, index: vec![], syntax: SyntaxType::INDEX_ID, }, ); s.attributes.insert( - // FIXME: Rename to attr_syntax? String::from("syntax"), SchemaAttribute { name: String::from("syntax"), @@ -631,14 +598,12 @@ impl SchemaInner { description: String::from( "Describe the syntax of this attribute. This affects indexing and sorting.", ), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::SYNTAX_ID, }, ); s.attributes.insert( - // FIXME: Rename to attribute_systemmay? String::from("systemmay"), SchemaAttribute { name: String::from("systemmay"), @@ -647,14 +612,12 @@ impl SchemaInner { description: String::from( "A list of system provided optional attributes this class can store.", ), - secret: false, multivalue: true, index: vec![], syntax: SyntaxType::UTF8STRING_INSENSITIVE, }, ); s.attributes.insert( - // FIXME: Rename to attribute_may? schema_may? String::from("may"), SchemaAttribute { name: String::from("may"), @@ -663,14 +626,12 @@ impl SchemaInner { description: String::from( "A user modifiable list of optional attributes this class can store.", ), - secret: false, multivalue: true, index: vec![], syntax: SyntaxType::UTF8STRING_INSENSITIVE, }, ); s.attributes.insert( - // FIXME: Rename to attribute_systemmust? schema_systemmust? String::from("systemmust"), SchemaAttribute { name: String::from("systemmust"), @@ -679,14 +640,12 @@ impl SchemaInner { description: String::from( "A list of system provided required attributes this class must store.", ), - secret: false, multivalue: true, index: vec![], syntax: SyntaxType::UTF8STRING_INSENSITIVE, }, ); s.attributes.insert( - // FIXME: Rename to attribute_must? schema_must? String::from("must"), SchemaAttribute { name: String::from("must"), @@ -695,7 +654,6 @@ impl SchemaInner { description: String::from( "A user modifiable list of required attributes this class must store.", ), - secret: false, multivalue: true, index: vec![], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -710,7 +668,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."), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::BOOLEAN, @@ -726,7 +683,6 @@ impl SchemaInner { description: String::from( "Who the ACP applies to, constraining or allowing operations.", ), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY, IndexType::SUBSTRING], syntax: SyntaxType::JSON_FILTER, @@ -741,7 +697,6 @@ impl SchemaInner { description: String::from( "The effective targets of the ACP, IE what will be acted upon.", ), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY, IndexType::SUBSTRING], syntax: SyntaxType::JSON_FILTER, @@ -754,7 +709,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."), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -769,7 +723,6 @@ impl SchemaInner { description: String::from( "The set of classes that can be created on a new entry.", ), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -784,7 +737,6 @@ impl SchemaInner { description: String::from( "The set of attribute types that can be created on an entry.", ), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -798,7 +750,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."), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -811,7 +762,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."), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -824,7 +774,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."), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -838,7 +787,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"), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::REFERENCE_UUID, @@ -851,7 +799,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"), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::REFERENCE_UUID, @@ -864,7 +811,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"), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::REFERENCE_UUID, @@ -880,7 +826,6 @@ impl SchemaInner { description: String::from( "The systems internal migration version for provided objects", ), - secret: true, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -894,7 +839,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."), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -913,7 +857,6 @@ impl SchemaInner { systemmust: vec![ String::from("class"), String::from("name"), - String::from("secret"), String::from("multivalue"), String::from("syntax"), String::from("description"), @@ -952,11 +895,7 @@ impl SchemaInner { description: String::from( "A system created class that all objects must contain", ), - systemmay: vec![ - // TODO: Owner? Responsible? Contact? - String::from("description"), - String::from("name"), - ], + systemmay: vec![String::from("description"), String::from("name")], may: vec![], systemmust: vec![ String::from("class"), @@ -1135,7 +1074,6 @@ impl SchemaInner { }, ); - // TODO: Probably needs to do a collect. let r = s.validate(&mut au); if r.len() == 0 { Ok(s) @@ -1150,7 +1088,7 @@ impl SchemaInner { pub fn validate(&self, audit: &mut AuditScope) -> Vec> { let mut res = Vec::new(); - // TODO: Does this need to validate anything further at all? The UUID + // Does this need to validate anything further at all? The UUID // will be checked as part of the schema migration on startup, so I think // just that all the content is sane is fine. for class in self.classes.values() { @@ -1262,7 +1200,8 @@ impl<'a> SchemaWriteTransaction<'a> { // purge all old attributes. self.inner.attributes.clear(); // Update with new ones. - // TODO: Do we need to check for dups? + // Do we need to check for dups? + // No, they'll over-write each other ... but we do need name uniqueness. attributetypes.into_iter().for_each(|a| { self.inner.attributes.insert(a.name.clone(), a); }); @@ -1276,7 +1215,8 @@ impl<'a> SchemaWriteTransaction<'a> { // purge all old attributes. self.inner.classes.clear(); // Update with new ones. - // TODO: Do we need to check for dups? + // Do we need to check for dups? + // No, they'll over-write each other ... but we do need name uniqueness. attributetypes.into_iter().for_each(|a| { self.inner.classes.insert(a.name.clone(), a); }); @@ -1415,7 +1355,6 @@ mod tests { "class": ["object", "attributetype"], "name": ["schema_attr_test"], "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"], - "secret": ["false"], "multivalue": ["false"], "index": ["EQUALITY"], "syntax": ["UTF8STRING"] @@ -1434,7 +1373,6 @@ mod tests { "name": ["schema_attr_test"], "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"], "description": ["Test attr parsing"], - "secret": ["false"], "multivalue": ["htouaoeu"], "index": ["EQUALITY"], "syntax": ["UTF8STRING"] @@ -1453,7 +1391,6 @@ mod tests { "name": ["schema_attr_test"], "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"], "description": ["Test attr parsing"], - "secret": ["false"], "multivalue": ["false"], "index": ["NTEHNOU"], "syntax": ["UTF8STRING"] @@ -1472,7 +1409,6 @@ mod tests { "name": ["schema_attr_test"], "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"], "description": ["Test attr parsing"], - "secret": ["false"], "multivalue": ["false"], "index": ["EQUALITY"], "syntax": ["TNEOUNTUH"] @@ -1492,7 +1428,6 @@ mod tests { "name": ["schema_attr_test"], "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"], "description": ["Test attr parsing"], - "secret": ["false"], "multivalue": ["false"], "syntax": ["UTF8STRING"] } @@ -1511,7 +1446,6 @@ mod tests { "name": ["schema_attr_test"], "uuid": ["66c68b2f-d02c-4243-8013-7946e40fe321"], "description": ["Test attr parsing"], - "secret": ["false"], "multivalue": ["false"], "index": ["EQUALITY"], "syntax": ["UTF8STRING"] @@ -1683,7 +1617,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"), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_PRINCIPAL, @@ -1714,7 +1647,6 @@ mod tests { description: String::from( "Who the ACP applies to, constraining or allowing operations.", ), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY, IndexType::SUBSTRING], syntax: SyntaxType::JSON_FILTER, @@ -1748,7 +1680,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"), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UUID, @@ -1769,7 +1700,6 @@ mod tests { name: String::from("single_value"), uuid: Uuid::new_v4(), description: String::from(""), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING_INSENSITIVE, @@ -1789,7 +1719,6 @@ mod tests { name: String::from("mv_string"), uuid: Uuid::new_v4(), description: String::from(""), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::UTF8STRING, @@ -1804,7 +1733,6 @@ mod tests { name: String::from("mv_bool"), uuid: Uuid::new_v4(), description: String::from(""), - secret: false, multivalue: true, index: vec![IndexType::EQUALITY], syntax: SyntaxType::BOOLEAN, @@ -1824,7 +1752,6 @@ mod tests { name: String::from("sv_syntax"), uuid: Uuid::new_v4(), description: String::from(""), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::SYNTAX_ID, @@ -1841,7 +1768,6 @@ mod tests { name: String::from("sv_index"), uuid: Uuid::new_v4(), description: String::from(""), - secret: false, multivalue: false, index: vec![IndexType::EQUALITY], syntax: SyntaxType::INDEX_ID, @@ -1951,7 +1877,6 @@ mod tests { "class": ["object", "attributetype"], "name": ["testattr"], "description": ["testattr"], - "secret": ["false"], "multivalue": ["false"], "syntax": ["UTF8STRING"], "uuid": ["db237e8a-0079-4b8c-8a56-593b22aa44d1"], @@ -1974,7 +1899,6 @@ mod tests { "class": ["object", "attributetype"], "name": ["testattr"], "description": ["testattr"], - "secret": ["false"], "multivalue": ["zzzzz"], "uuid": ["db237e8a-0079-4b8c-8a56-593b22aa44d1"], "syntax": ["UTF8STRING"] @@ -1996,7 +1920,6 @@ mod tests { "class": ["object", "attributetype"], "name": ["testattr"], "description": ["testattr"], - "secret": ["false"], "multivalue": ["true"], "uuid": ["db237e8a-0079-4b8c-8a56-593b22aa44d1"], "syntax": ["UTF8STRING"] @@ -2118,7 +2041,7 @@ mod tests { "attrs": { "class": ["extensibleobject"], "uuid": ["db237e8a-0079-4b8c-8a56-593b22aa44d1"], - "secret": ["zzzz"] + "multivalue": ["zzzz"] } }"#, ) @@ -2136,7 +2059,7 @@ mod tests { "attrs": { "class": ["extensibleobject"], "uuid": ["db237e8a-0079-4b8c-8a56-593b22aa44d1"], - "secret": ["true"] + "multivalue": ["true"] } }"#, ) @@ -2160,7 +2083,7 @@ mod tests { ); // test syntax of bool - let f_bool = filter_all!(f_eq("secret", "zzzz")); + let f_bool = filter_all!(f_eq("multivalue", "zzzz")); assert_eq!( f_bool.validate(&schema), Err(SchemaError::InvalidAttributeSyntax) @@ -2174,14 +2097,14 @@ mod tests { // Test the recursive structures validate let f_or_empty = filter_all!(f_or!([])); assert_eq!(f_or_empty.validate(&schema), Err(SchemaError::EmptyFilter)); - let f_or = filter_all!(f_or!([f_eq("secret", "zzzz")])); + let f_or = filter_all!(f_or!([f_eq("multivalue", "zzzz")])); assert_eq!( f_or.validate(&schema), Err(SchemaError::InvalidAttributeSyntax) ); let f_or_mult = filter_all!(f_and!([ f_eq("class", "attributetype"), - f_eq("secret", "zzzzzzz"), + f_eq("multivalue", "zzzzzzz"), ])); assert_eq!( f_or_mult.validate(&schema), diff --git a/src/lib/server.rs b/src/lib/server.rs index 15e98000f..6ffa11db7 100644 --- a/src/lib/server.rs +++ b/src/lib/server.rs @@ -16,9 +16,11 @@ use crate::constants::{ JSON_IDM_ADMINS_V1, JSON_IDM_SELF_ACP_READ_V1, JSON_SCHEMA_ATTR_DISPLAYNAME, JSON_SCHEMA_ATTR_MAIL, JSON_SCHEMA_ATTR_PASSWORD, JSON_SCHEMA_ATTR_SSH_PUBLICKEY, JSON_SCHEMA_CLASS_ACCOUNT, JSON_SCHEMA_CLASS_GROUP, JSON_SCHEMA_CLASS_PERSON, - JSON_SYSTEM_INFO_V1, + JSON_SYSTEM_INFO_V1, UUID_DOES_NOT_EXIST, +}; +use crate::entry::{ + Entry, EntryCommitted, EntryInvalid, EntryNew, EntryNormalised, EntryReduced, EntryValid, }; -use crate::entry::{Entry, EntryCommitted, EntryInvalid, EntryNew, EntryNormalised, EntryValid}; use crate::error::{ConsistencyError, OperationError, SchemaError}; use crate::event::{ CreateEvent, DeleteEvent, Event, EventOrigin, ExistsEvent, ModifyEvent, ReviveRecycledEvent, @@ -49,7 +51,7 @@ pub trait QueryServerTransaction { &self, au: &mut AuditScope, se: &SearchEvent, - ) -> Result>, OperationError> { + ) -> Result>, OperationError> { /* * This just wraps search, but it's for the external interface * so as a result it also reduces the entry set's attributes at @@ -81,34 +83,15 @@ pub trait QueryServerTransaction { // exists in the server, not arbitrary attr names. // // This normalises and validates in a single step. - /* - let vf = match se.filter.validate(self.get_schema()) { - Ok(f) => f, - // TODO: Do something with this error - Err(e) => return Err(OperationError::SchemaViolation(e)), - }; - - audit_log!(au, "search: valid filter -> {:?}", vf); - */ + // + // NOTE: Filters are validated in event conversion. // Now resolve all references. let vfr = try_audit!(au, se.filter.resolve(&se.event)); - // TODO: Assert access control allows the filter and requested attrs. - - /* - let mut audit_plugin_pre = AuditScope::new("plugin_pre_search"); - let plug_pre_res = Plugins::run_pre_search(&mut audit_plugin_pre); - au.append_scope(audit_plugin_pre); - - match plug_pre_res { - Ok(_) => {} - Err(e) => { - audit_log!(au, "Search operation failed (plugin), {:?}", e); - return Err(e); - } - } - */ + // NOTE: We currently can't build search plugins due to the inability to hand + // the QS wr/ro to the plugin trait. However, there shouldn't be a need for search + // plugis, because all data transforms should be in the write path. let mut audit_be = AuditScope::new("backend_search"); let res = self @@ -125,8 +108,6 @@ pub trait QueryServerTransaction { // ACP application. There is a second application to reduce the // attribute set on the entries! // - // TODO: Make a search_ext that applies search_filter_entry_attributes - // and does Entry -> EntryReduced. let mut audit_acp = AuditScope::new("access_control_profiles"); let access = self.get_accesscontrols(); let acp_res = access.search_filter_entries(&mut audit_acp, se, res); @@ -134,34 +115,12 @@ pub trait QueryServerTransaction { au.append_scope(audit_acp); let acp_res = try_audit!(au, acp_res); - /* - let mut audit_plugin_post = AuditScope::new("plugin_post_search"); - let plug_post_res = Plugins::run_post_search(&mut audit_plugin_post); - au.append_scope(audit_plugin_post); - - match plug_post_res { - Ok(_) => {} - Err(e) => { - audit_log!(au, "Search operation failed (plugin), {:?}", e); - return Err(e); - } - } - */ - Ok(acp_res) } fn exists(&self, au: &mut AuditScope, ee: &ExistsEvent) -> Result { let mut audit_be = AuditScope::new("backend_exists"); - // How to get schema? - /* - let vf = match ee.filter.validate(self.get_schema()) { - Ok(f) => f, - Err(e) => return Err(OperationError::SchemaViolation(e)), - }; - */ - let vfr = try_audit!(au, ee.filter.resolve(&ee.event)); let res = self @@ -173,7 +132,7 @@ pub trait QueryServerTransaction { res } - // TODO: Should this actually be names_to_uuids and we do batches? + // Should this actually be names_to_uuids and we do batches? // In the initial design "no", we can always write a batched // interface later. // @@ -218,7 +177,7 @@ pub trait QueryServerTransaction { return Err(OperationError::InvalidDBState); } - // TODO: fine for 0/1 case, but check len for >= 2 to eliminate that case. + // error should never be triggered due to the len checks above. let e = res.first().ok_or(OperationError::NoMatchingEntries)?; // Get the uuid from the entry. Again, check it exists, and only one. let uuid_res: String = e.get_uuid().to_string(); @@ -380,13 +339,13 @@ pub trait QueryServerTransaction { value: &String, ) -> Result { let schema = self.get_schema(); - // TODO: Normalise the attr, else lookup with fail .... let schema_name = schema .get_attributes() .get("name") .expect("Schema corrupted"); - // TODO: Should we return the normalise attr? + // Should we return the normalise attr? + // no, I think that it's not up to us to normalise this all the time. let temp_a = schema_name.normalise_value(attr); // Lookup the attr @@ -399,31 +358,19 @@ pub trait QueryServerTransaction { // So, if possible, resolve the value // to a concrete uuid. Ok(_) => { - // TODO: Should this check existance? - // Could this be a security risk for disclosure? - // So it would only reveal if a name/uuid did/did not exist - // because this pre-acp check, but inversely, this means if we - // fail fast here, we would not hae a situation where we would create - // then ref-int would invalidate the structure immediately. - // - // I can see a situation where you would modify, and then immediately - // have the mod removed because it would fail the refint (IE add - // raw uuid X, then immediately it's removed) - // - // This would never be the case with resolved uuid's though, because - // they are inside the txn. So do we just ignore this as an edge case? - // - // For now, refint will fight the raw uuid's, and will be tested to - // assume they don't exist on create/mod/etc.. If this check was added - // then refint may not need post_create handlers. + // It's a uuid - we do NOT check for existance, because that + // could be revealing or disclosing - it is up to acp to assert + // if we can see the value or not, and it's not up to us to + // assert the filter value exists. Ok(value.clone()) } Err(_) => { // it's not a uuid, try to resolve it. - // TODO: If this errors, should we actually pass - // back a place holder "no such uuid" and then - // fail an exists check later? - Ok(self.name_to_uuid(audit, value)?) + // if the value is NOT found, we map to "does not exist" to allow + // the value to continue being evaluated, which of course, will fail + // all subsequent filter tests because it ... well, doesn't exist. + self.name_to_uuid(audit, value) + .or_else(|_| Ok(UUID_DOES_NOT_EXIST.to_string())) } } } @@ -434,9 +381,9 @@ pub trait QueryServerTransaction { } } None => { - // Welp, you'll break when we hit schema validation soon anyway. - // Just clone in this case ... - // TODO: Honestly, we could just return the schema error here ... + // No attribute of this name exists - clone the value anyway, because + // the schema check will fail soon, and it's beyond this functions + // purpose to validate your content anyway. Ok(value.clone()) } } @@ -530,12 +477,14 @@ impl QueryServerReadTransaction { pub struct QueryServerWriteTransaction<'a> { committed: bool, - // be_write_txn: BackendWriteTransaction, - // schema_write: SchemaWriteTransaction, - // read: QueryServerReadTransaction, be_txn: BackendWriteTransaction, schema: SchemaWriteTransaction<'a>, accesscontrols: AccessControlsWriteTransaction<'a>, + // We store a set of flags that indicate we need a reload of + // schema or acp, which is tested by checking the classes of the + // changing content. + changed_schema: bool, + changed_acp: bool, } impl<'a> QueryServerTransaction for QueryServerWriteTransaction<'a> { @@ -596,21 +545,23 @@ impl QueryServer { be_txn: self.be.write(), schema: self.schema.write(), accesscontrols: self.accesscontrols.write(), + changed_schema: false, + changed_acp: false, } } pub(crate) fn initialise_helper(&self, audit: &mut AuditScope) -> Result<(), OperationError> { - let ts_write_1 = self.write(); + let mut ts_write_1 = self.write(); ts_write_1 .initialise_schema_core(audit) .and_then(|_| ts_write_1.commit(audit))?; - let ts_write_2 = self.write(); + let mut ts_write_2 = self.write(); ts_write_2 .initialise_schema_idm(audit) .and_then(|_| ts_write_2.commit(audit))?; - let ts_write_3 = self.write(); + let mut ts_write_3 = self.write(); ts_write_3 .initialise_idm(audit) .and_then(|_| ts_write_3.commit(audit)) @@ -623,7 +574,7 @@ impl QueryServer { } impl<'a> QueryServerWriteTransaction<'a> { - pub fn create(&self, au: &mut AuditScope, ce: &CreateEvent) -> Result<(), OperationError> { + pub fn create(&mut self, au: &mut AuditScope, ce: &CreateEvent) -> Result<(), OperationError> { // The create event is a raw, read only representation of the request // that was made to us, including information about the identity // performing the request. @@ -637,7 +588,7 @@ impl<'a> QueryServerWriteTransaction<'a> { let candidates: Vec> = ce.entries.iter().map(|er| er.clone()).collect(); - // TODO: Normalise but DO NOT validate the entries. + // Normalise but DO NOT validate the entries. let norm_cand: Result>, _> = candidates .into_iter() .map(|e| { @@ -671,7 +622,7 @@ impl<'a> QueryServerWriteTransaction<'a> { let mut audit_plugin_pre_transform = AuditScope::new("plugin_pre_create_transform"); let plug_pre_transform_res = Plugins::run_pre_create_transform( &mut audit_plugin_pre_transform, - &self, + self, &mut candidates, ce, ); @@ -702,7 +653,7 @@ impl<'a> QueryServerWriteTransaction<'a> { // 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 = Plugins::run_pre_create(&mut audit_plugin_pre, &self, &norm_cand, ce); + let plug_pre_res = 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), {:?}"); @@ -725,7 +676,7 @@ impl<'a> QueryServerWriteTransaction<'a> { // Run any post plugins let mut audit_plugin_post = AuditScope::new("plugin_post_create"); - let plug_post_res = Plugins::run_post_create(&mut audit_plugin_post, &self, &norm_cand, ce); + let plug_post_res = Plugins::run_post_create(&mut audit_plugin_post, self, &norm_cand, ce); au.append_scope(audit_plugin_post); if plug_post_res.is_err() { @@ -733,13 +684,37 @@ impl<'a> QueryServerWriteTransaction<'a> { return plug_post_res; } + // We have finished all plugs and now have a successful operation - flag if + // schema or acp requires reload. + self.changed_schema = norm_cand.iter().fold(false, |acc, e| { + if acc { + acc + } else { + e.attribute_value_pres("class", "classtype") + || e.attribute_value_pres("class", "attributetype") + } + }); + self.changed_acp = norm_cand.iter().fold(false, |acc, e| { + if acc { + acc + } else { + e.attribute_value_pres("class", "access_control_profile") + } + }); + audit_log!( + au, + "Schema reload: {:?}, ACP reload: {:?}", + self.changed_schema, + self.changed_acp + ); + // We are complete, finalise logging and return audit_log!(au, "Create operation success"); res } - pub fn delete(&self, au: &mut AuditScope, de: &DeleteEvent) -> Result<(), OperationError> { + pub fn delete(&mut self, au: &mut AuditScope, de: &DeleteEvent) -> Result<(), OperationError> { // Do you have access to view all the set members? Reduce based on your // read permissions and attrs // THIS IS PRETTY COMPLEX SEE THE DESIGN DOC @@ -801,7 +776,7 @@ impl<'a> QueryServerWriteTransaction<'a> { // Pre delete plugs let mut audit_plugin_pre = AuditScope::new("plugin_pre_delete"); let plug_pre_res = - Plugins::run_pre_delete(&mut audit_plugin_pre, &self, &mut candidates, de); + Plugins::run_pre_delete(&mut audit_plugin_pre, self, &mut candidates, de); au.append_scope(audit_plugin_pre); if plug_pre_res.is_err() { @@ -832,7 +807,7 @@ impl<'a> QueryServerWriteTransaction<'a> { // Post delete plugs let mut audit_plugin_post = AuditScope::new("plugin_post_delete"); - let plug_post_res = Plugins::run_post_delete(&mut audit_plugin_post, &self, &del_cand, de); + let plug_post_res = Plugins::run_post_delete(&mut audit_plugin_post, self, &del_cand, de); au.append_scope(audit_plugin_post); if plug_post_res.is_err() { @@ -840,6 +815,30 @@ impl<'a> QueryServerWriteTransaction<'a> { return plug_post_res; } + // We have finished all plugs and now have a successful operation - flag if + // schema or acp requires reload. + self.changed_schema = del_cand.iter().fold(false, |acc, e| { + if acc { + acc + } else { + e.attribute_value_pres("class", "classtype") + || e.attribute_value_pres("class", "attributetype") + } + }); + self.changed_acp = del_cand.iter().fold(false, |acc, e| { + if acc { + acc + } else { + e.attribute_value_pres("class", "access_control_profile") + } + }); + audit_log!( + au, + "Schema reload: {:?}, ACP reload: {:?}", + self.changed_schema, + self.changed_acp + ); + // Send result audit_log!(au, "Delete operation success"); res @@ -906,7 +905,7 @@ impl<'a> QueryServerWriteTransaction<'a> { // Should this take a revive event? pub fn revive_recycled( - &self, + &mut self, au: &mut AuditScope, re: &ReviveRecycledEvent, ) -> Result<(), OperationError> { @@ -934,7 +933,7 @@ impl<'a> QueryServerWriteTransaction<'a> { self.impersonate_modify_valid(au, re.filter.clone(), re.filter.clone(), m_valid, &re.event) } - pub fn modify(&self, au: &mut AuditScope, me: &ModifyEvent) -> Result<(), OperationError> { + pub fn modify(&mut self, au: &mut AuditScope, me: &ModifyEvent) -> Result<(), OperationError> { // Get the candidates. // Modify applies a modlist to a filter, so we need to internal search // then apply. @@ -1021,7 +1020,7 @@ impl<'a> QueryServerWriteTransaction<'a> { // Pre mod plugins let mut audit_plugin_pre = AuditScope::new("plugin_pre_modify"); let plug_pre_res = - Plugins::run_pre_modify(&mut audit_plugin_pre, &self, &mut candidates, me); + Plugins::run_pre_modify(&mut audit_plugin_pre, self, &mut candidates, me); au.append_scope(audit_plugin_pre); if plug_pre_res.is_err() { @@ -1029,7 +1028,7 @@ impl<'a> QueryServerWriteTransaction<'a> { return plug_pre_res; } - // TODO: There is a potential optimisation here, where if + // NOTE: There is a potential optimisation here, where if // candidates == pre-candidates, then we don't need to store anything // because we effectively just did an assert. However, like all // optimisations, this could be premature - so we for now, just @@ -1065,7 +1064,7 @@ impl<'a> QueryServerWriteTransaction<'a> { let mut audit_plugin_post = AuditScope::new("plugin_post_modify"); let plug_post_res = Plugins::run_post_modify( &mut audit_plugin_post, - &self, + self, &pre_candidates, &norm_cand, me, @@ -1077,6 +1076,38 @@ impl<'a> QueryServerWriteTransaction<'a> { return plug_post_res; } + // We have finished all plugs and now have a successful operation - flag if + // schema or acp requires reload. Remember, this is a modify, so we need to check + // pre and post cands. + self.changed_schema = + norm_cand + .iter() + .chain(pre_candidates.iter()) + .fold(false, |acc, e| { + if acc { + acc + } else { + e.attribute_value_pres("class", "classtype") + || e.attribute_value_pres("class", "attributetype") + } + }); + self.changed_acp = norm_cand + .iter() + .chain(pre_candidates.iter()) + .fold(false, |acc, e| { + if acc { + acc + } else { + e.attribute_value_pres("class", "access_control_profile") + } + }); + audit_log!( + au, + "Schema reload: {:?}, ACP reload: {:?}", + self.changed_schema, + self.changed_acp + ); + // return audit_log!(au, "Modify operation success"); res @@ -1087,7 +1118,7 @@ impl<'a> QueryServerWriteTransaction<'a> { // only, allowing certain plugin by passes etc. pub fn internal_create( - &self, + &mut self, audit: &mut AuditScope, entries: Vec>, ) -> Result<(), OperationError> { @@ -1101,7 +1132,7 @@ impl<'a> QueryServerWriteTransaction<'a> { } pub fn internal_delete( - &self, + &mut self, audit: &mut AuditScope, filter: Filter, ) -> Result<(), OperationError> { @@ -1116,7 +1147,7 @@ impl<'a> QueryServerWriteTransaction<'a> { } pub fn internal_modify( - &self, + &mut self, audit: &mut AuditScope, filter: Filter, modlist: ModifyList, @@ -1135,7 +1166,7 @@ impl<'a> QueryServerWriteTransaction<'a> { } pub fn impersonate_modify_valid( - &self, + &mut self, audit: &mut AuditScope, f_valid: Filter, f_intent_valid: Filter, @@ -1150,7 +1181,7 @@ impl<'a> QueryServerWriteTransaction<'a> { } pub fn impersonate_modify( - &self, + &mut self, audit: &mut AuditScope, filter: Filter, filter_intent: Filter, @@ -1185,7 +1216,7 @@ impl<'a> QueryServerWriteTransaction<'a> { } pub fn internal_migrate_or_create_str( - &self, + &mut self, audit: &mut AuditScope, e_str: &str, ) -> Result<(), OperationError> { @@ -1200,7 +1231,7 @@ impl<'a> QueryServerWriteTransaction<'a> { } pub fn internal_migrate_or_create( - &self, + &mut self, audit: &mut AuditScope, e: Entry, ) -> Result<(), OperationError> { @@ -1209,11 +1240,10 @@ impl<'a> QueryServerWriteTransaction<'a> { // attributes in the situation. // If not exist, create from Entry B // - // TODO: WARNING: this requires schema awareness for multivalue types! - // We need to either do a schema aware merge, or we just overwrite those - // few attributes. - // // This will extra classes an attributes alone! + // + // NOTE: gen modlist IS schema aware and will handle multivalue + // correctly! let filt = match e.filter_from_attrs(&vec![String::from("uuid")]) { Some(f) => f, None => return Err(OperationError::FilterGeneration), @@ -1250,7 +1280,7 @@ impl<'a> QueryServerWriteTransaction<'a> { // Should this take a be_txn? pub fn internal_assert_or_create( - &self, + &mut self, audit: &mut AuditScope, e: Entry, ) -> Result<(), OperationError> { @@ -1264,7 +1294,9 @@ impl<'a> QueryServerWriteTransaction<'a> { None => return Err(OperationError::FilterGeneration), }; - // Does it exist? (TODO: Should be search, not exists ...) + // Does it exist? we use search here, not exists, so that if the entry does exist + // we can compare it is identical, which avoids a delete/create cycle that would + // trigger csn/repl each time we start up. match self.internal_search(audit, filt.clone()) { Ok(results) => { if results.len() == 0 { @@ -1290,7 +1322,7 @@ impl<'a> QueryServerWriteTransaction<'a> { } } - pub fn initialise_schema_core(&self, audit: &mut AuditScope) -> Result<(), OperationError> { + pub fn initialise_schema_core(&mut self, audit: &mut AuditScope) -> Result<(), OperationError> { // Load in all the "core" schema, that we already have in "memory". let entries = self.schema.to_entries(); @@ -1310,7 +1342,7 @@ impl<'a> QueryServerWriteTransaction<'a> { r } - pub fn initialise_schema_idm(&self, audit: &mut AuditScope) -> Result<(), OperationError> { + pub fn initialise_schema_idm(&mut self, audit: &mut AuditScope) -> Result<(), OperationError> { // List of IDM schemas to init. let idm_schema: Vec<&str> = vec![ JSON_SCHEMA_ATTR_DISPLAYNAME, @@ -1325,17 +1357,17 @@ impl<'a> QueryServerWriteTransaction<'a> { let mut audit_si = AuditScope::new("start_initialise_schema_idm"); let r: Result, _> = idm_schema .iter() + // Each item individually logs it's result .map(|e_str| self.internal_migrate_or_create_str(&mut audit_si, e_str)) .collect(); audit.append_scope(audit_si); assert!(r.is_ok()); - // TODO: Should we log the set of failures some how? r.map(|_| ()) } // This function is idempotent - pub fn initialise_idm(&self, audit: &mut AuditScope) -> Result<(), OperationError> { + pub fn initialise_idm(&mut self, audit: &mut AuditScope) -> Result<(), OperationError> { // First, check the system_info object. This stores some server information // and details. It's a pretty static thing. let mut audit_si = AuditScope::new("start_system_info"); @@ -1512,16 +1544,20 @@ impl<'a> QueryServerWriteTransaction<'a> { } pub fn commit(mut self, audit: &mut AuditScope) -> Result<(), OperationError> { - // TODO: This could be faster if we cache the set of classes changed + // This could be faster if we cache the set of classes changed // in an operation so we can check if we need to do the reload or not // // Reload the schema from qs. - self.reload_schema(audit)?; + if self.changed_schema { + self.reload_schema(audit)?; + } // Determine if we need to update access control profiles // based on any modifications that have occured. // IF SCHEMA CHANGED WE MUST ALSO RELOAD!!! IE if schema had an attr removed - // that we rely on we MUST fail this! - self.reload_accesscontrols(audit)?; + // that we rely on we MUST fail this here!! + if self.changed_schema || self.changed_acp { + self.reload_accesscontrols(audit)?; + } // Now destructure the transaction ready to reset it. let QueryServerWriteTransaction { @@ -1529,6 +1565,8 @@ impl<'a> QueryServerWriteTransaction<'a> { be_txn, schema, accesscontrols, + changed_schema, + changed_acp, } = self; assert!(!committed); // Begin an audit. @@ -1536,12 +1574,8 @@ impl<'a> QueryServerWriteTransaction<'a> { let r = schema.validate(audit); if r.len() == 0 { - // TODO: At this point, if validate passes, we probably actually want - // to perform a schema reload BEFORE we be commit. Because the be holds - // all the data, we need everything to be consistent *first* as the be - // is the last point we can really backout! - // Alternate, we attempt to reload during batch ops, but this seems - // costly. + // Schema has been validated, so we can go ahead and commit it with the be + // because both are consistent. schema .commit() .and_then(|_| accesscontrols.commit().and_then(|_| be_txn.commit())) @@ -1570,7 +1604,7 @@ mod tests { #[test] fn test_qs_create_user() { run_test!(|server: &QueryServer, audit: &mut AuditScope| { - let server_txn = server.write(); + let mut server_txn = server.write(); let filt = filter!(f_eq("name", "testperson")); let admin = server_txn .internal_search_uuid(audit, UUID_ADMIN) @@ -1619,23 +1653,23 @@ mod tests { run_test!(|server: &QueryServer, audit: &mut AuditScope| { { // Setup and abort. - let server_txn = server.write(); + let mut server_txn = server.write(); assert!(server_txn.initialise_schema_core(audit).is_ok()); } { - let server_txn = server.write(); + let mut server_txn = server.write(); assert!(server_txn.initialise_schema_core(audit).is_ok()); assert!(server_txn.initialise_schema_core(audit).is_ok()); assert!(server_txn.commit(audit).is_ok()); } { // Now do it again in a new txn, but abort - let server_txn = server.write(); + let mut server_txn = server.write(); assert!(server_txn.initialise_schema_core(audit).is_ok()); } { // Now do it again in a new txn. - let server_txn = server.write(); + let mut server_txn = server.write(); assert!(server_txn.initialise_schema_core(audit).is_ok()); assert!(server_txn.commit(audit).is_ok()); } @@ -1647,7 +1681,7 @@ mod tests { fn test_qs_modify() { run_test!(|server: &QueryServer, audit: &mut AuditScope| { // Create an object - let server_txn = server.write(); + let mut server_txn = server.write(); let e1: Entry = serde_json::from_str( r#"{ @@ -1778,7 +1812,7 @@ mod tests { // Test modifying an entry and adding an extra class, that would cause the entry // to no longer conform to schema. run_test!(|server: &QueryServer, audit: &mut AuditScope| { - let server_txn = server.write(); + let mut server_txn = server.write(); let e1: Entry = serde_json::from_str( r#"{ @@ -1855,7 +1889,7 @@ mod tests { fn test_qs_delete() { run_test!(|server: &QueryServer, audit: &mut AuditScope| { // Create - let server_txn = server.write(); + let mut server_txn = server.write(); let e1: Entry = serde_json::from_str( r#"{ @@ -1939,7 +1973,7 @@ mod tests { #[test] fn test_qs_tombstone() { run_test!(|server: &QueryServer, audit: &mut AuditScope| { - let server_txn = server.write(); + let mut server_txn = server.write(); let admin = server_txn .internal_search_uuid(audit, UUID_ADMIN) .expect("failed"); @@ -2024,7 +2058,7 @@ mod tests { #[test] fn test_qs_recycle_simple() { run_test!(|server: &QueryServer, audit: &mut AuditScope| { - let server_txn = server.write(); + let mut server_txn = server.write(); let admin = server_txn .internal_search_uuid(audit, UUID_ADMIN) .expect("failed"); @@ -2165,7 +2199,7 @@ mod tests { fn test_qs_recycle_advanced() { run_test!(|server: &QueryServer, audit: &mut AuditScope| { // Create items - let server_txn = server.write(); + let mut server_txn = server.write(); let admin = server_txn .internal_search_uuid(audit, UUID_ADMIN) .expect("failed"); @@ -2210,7 +2244,7 @@ mod tests { #[test] fn test_qs_name_to_uuid() { run_test!(|server: &QueryServer, audit: &mut AuditScope| { - let server_txn = server.write(); + let mut server_txn = server.write(); let e1: Entry = serde_json::from_str( r#"{ @@ -2248,7 +2282,7 @@ mod tests { #[test] fn test_qs_uuid_to_name() { run_test!(|server: &QueryServer, audit: &mut AuditScope| { - let server_txn = server.write(); + let mut server_txn = server.write(); let e1: Entry = serde_json::from_str( r#"{ @@ -2289,7 +2323,7 @@ mod tests { #[test] fn test_qs_clone_value() { run_test!(|server: &QueryServer, audit: &mut AuditScope| { - let server_txn = server.write(); + let mut server_txn = server.write(); let e1: Entry = serde_json::from_str( r#"{ "valid": null, @@ -2369,7 +2403,7 @@ mod tests { ) .expect("json failure"); - let server_txn = server.write(); + let mut server_txn = server.write(); // Add a new class. let ce_class = CreateEvent::new_internal(vec![e_cd.clone()]); assert!(server_txn.create(audit, &ce_class).is_ok()); @@ -2381,7 +2415,7 @@ mod tests { server_txn.commit(audit).expect("should not fail"); // Start a new write - let server_txn = server.write(); + let mut server_txn = server.write(); // Add the class to an object // should work let ce_work = CreateEvent::new_internal(vec![e1.clone()]); @@ -2391,7 +2425,7 @@ mod tests { server_txn.commit(audit).expect("should not fail"); // Start a new write - let server_txn = server.write(); + let mut server_txn = server.write(); // delete the class let de_class = unsafe { DeleteEvent::new_internal_invalid(filter!(f_eq("name", "testclass"))) }; @@ -2400,7 +2434,7 @@ mod tests { server_txn.commit(audit).expect("should not fail"); // Start a new write - let server_txn = server.write(); + let mut server_txn = server.write(); // Trying to add now should fail let ce_fail = CreateEvent::new_internal(vec![e1.clone()]); assert!(server_txn.create(audit, &ce_fail).is_err()); @@ -2444,14 +2478,13 @@ mod tests { "uuid": ["cfcae205-31c3-484b-8ced-667d1709c5e3"], "description": ["Test Attribute"], "multivalue": ["false"], - "secret": ["false"], "syntax": ["UTF8STRING"] } }"#, ) .expect("json failure"); - let server_txn = server.write(); + let mut server_txn = server.write(); // Add a new attribute. let ce_attr = CreateEvent::new_internal(vec![e_ad.clone()]); assert!(server_txn.create(audit, &ce_attr).is_ok()); @@ -2463,7 +2496,7 @@ mod tests { server_txn.commit(audit).expect("should not fail"); // Start a new write - let server_txn = server.write(); + let mut server_txn = server.write(); // Add the attr to an object // should work let ce_work = CreateEvent::new_internal(vec![e1.clone()]); @@ -2473,7 +2506,7 @@ mod tests { server_txn.commit(audit).expect("should not fail"); // Start a new write - let server_txn = server.write(); + let mut server_txn = server.write(); // delete the attr let de_attr = unsafe { DeleteEvent::new_internal_invalid(filter!(f_eq("name", "testattr"))) }; @@ -2482,7 +2515,7 @@ mod tests { server_txn.commit(audit).expect("should not fail"); // Start a new write - let server_txn = server.write(); + let mut server_txn = server.write(); // Trying to add now should fail let ce_fail = CreateEvent::new_internal(vec![e1.clone()]); assert!(server_txn.create(audit, &ce_fail).is_err());