From 96beb3070ec33c230d1b5b7d4d807d3324d5978a Mon Sep 17 00:00:00 2001 From: William Brown Date: Sun, 28 Jul 2019 20:18:25 +0900 Subject: [PATCH] Finish up todo audit --- src/lib/constants.rs | 74 +++++++++++++++++++------------------- src/lib/entry.rs | 18 +++++----- src/lib/event.rs | 2 +- src/lib/idm/account.rs | 15 ++++---- src/lib/idm/authsession.rs | 10 ++++-- src/lib/idm/macros.rs | 2 +- src/lib/idm/server.rs | 4 +-- src/lib/plugins/base.rs | 5 +-- src/lib/proto/v1/mod.rs | 2 -- src/lib/schema.rs | 4 +-- 10 files changed, 70 insertions(+), 66 deletions(-) diff --git a/src/lib/constants.rs b/src/lib/constants.rs index 81d720e53..ddb4b42d4 100644 --- a/src/lib/constants.rs +++ b/src/lib/constants.rs @@ -136,56 +136,56 @@ pub static JSON_ANONYMOUS_V1: &'static str = r#"{ // Core // Schema uuids start at 00000000-0000-0000-0000-ffff00000000 -pub static UUID_SCHEMA_ATTR_CLASS: &'static str = "00000000-0000-0000-0000-ffff00000000"; -pub static UUID_SCHEMA_ATTR_UUID: &'static str = "00000000-0000-0000-0000-ffff00000001"; -pub static UUID_SCHEMA_ATTR_NAME: &'static str = "00000000-0000-0000-0000-ffff00000002"; -pub static UUID_SCHEMA_ATTR_PRINCIPAL_NAME: &'static str = "00000000-0000-0000-0000-ffff00000003"; -pub static UUID_SCHEMA_ATTR_DESCRIPTION: &'static str = "00000000-0000-0000-0000-ffff00000004"; -pub static UUID_SCHEMA_ATTR_MULTIVALUE: &'static str = "00000000-0000-0000-0000-ffff00000005"; -pub static UUID_SCHEMA_ATTR_INDEX: &'static str = "00000000-0000-0000-0000-ffff00000006"; -pub static UUID_SCHEMA_ATTR_SYNTAX: &'static str = "00000000-0000-0000-0000-ffff00000007"; -pub static UUID_SCHEMA_ATTR_SYSTEMMAY: &'static str = "00000000-0000-0000-0000-ffff00000008"; -pub static UUID_SCHEMA_ATTR_MAY: &'static str = "00000000-0000-0000-0000-ffff00000009"; -pub static UUID_SCHEMA_ATTR_SYSTEMMUST: &'static str = "00000000-0000-0000-0000-ffff00000010"; -pub static UUID_SCHEMA_ATTR_MUST: &'static str = "00000000-0000-0000-0000-ffff00000011"; -pub static UUID_SCHEMA_ATTR_MEMBEROF: &'static str = "00000000-0000-0000-0000-ffff00000012"; -pub static UUID_SCHEMA_ATTR_MEMBER: &'static str = "00000000-0000-0000-0000-ffff00000013"; -pub static UUID_SCHEMA_ATTR_DIRECTMEMBEROF: &'static str = "00000000-0000-0000-0000-ffff00000014"; -pub static UUID_SCHEMA_ATTR_VERSION: &'static str = "00000000-0000-0000-0000-ffff00000015"; -pub static UUID_SCHEMA_ATTR_DOMAIN: &'static str = "00000000-0000-0000-0000-ffff00000016"; -pub static UUID_SCHEMA_ATTR_ACP_ENABLE: &'static str = "00000000-0000-0000-0000-ffff00000017"; -pub static UUID_SCHEMA_ATTR_ACP_RECEIVER: &'static str = "00000000-0000-0000-0000-ffff00000018"; +pub static UUID_SCHEMA_ATTR_CLASS: &'static str = "00000000-0000-0000-0000-ffff00000000"; +pub static UUID_SCHEMA_ATTR_UUID: &'static str = "00000000-0000-0000-0000-ffff00000001"; +pub static UUID_SCHEMA_ATTR_NAME: &'static str = "00000000-0000-0000-0000-ffff00000002"; +pub static UUID_SCHEMA_ATTR_PRINCIPAL_NAME: &'static str = "00000000-0000-0000-0000-ffff00000003"; +pub static UUID_SCHEMA_ATTR_DESCRIPTION: &'static str = "00000000-0000-0000-0000-ffff00000004"; +pub static UUID_SCHEMA_ATTR_MULTIVALUE: &'static str = "00000000-0000-0000-0000-ffff00000005"; +pub static UUID_SCHEMA_ATTR_INDEX: &'static str = "00000000-0000-0000-0000-ffff00000006"; +pub static UUID_SCHEMA_ATTR_SYNTAX: &'static str = "00000000-0000-0000-0000-ffff00000007"; +pub static UUID_SCHEMA_ATTR_SYSTEMMAY: &'static str = "00000000-0000-0000-0000-ffff00000008"; +pub static UUID_SCHEMA_ATTR_MAY: &'static str = "00000000-0000-0000-0000-ffff00000009"; +pub static UUID_SCHEMA_ATTR_SYSTEMMUST: &'static str = "00000000-0000-0000-0000-ffff00000010"; +pub static UUID_SCHEMA_ATTR_MUST: &'static str = "00000000-0000-0000-0000-ffff00000011"; +pub static UUID_SCHEMA_ATTR_MEMBEROF: &'static str = "00000000-0000-0000-0000-ffff00000012"; +pub static UUID_SCHEMA_ATTR_MEMBER: &'static str = "00000000-0000-0000-0000-ffff00000013"; +pub static UUID_SCHEMA_ATTR_DIRECTMEMBEROF: &'static str = "00000000-0000-0000-0000-ffff00000014"; +pub static UUID_SCHEMA_ATTR_VERSION: &'static str = "00000000-0000-0000-0000-ffff00000015"; +pub static UUID_SCHEMA_ATTR_DOMAIN: &'static str = "00000000-0000-0000-0000-ffff00000016"; +pub static UUID_SCHEMA_ATTR_ACP_ENABLE: &'static str = "00000000-0000-0000-0000-ffff00000017"; +pub static UUID_SCHEMA_ATTR_ACP_RECEIVER: &'static str = "00000000-0000-0000-0000-ffff00000018"; pub static UUID_SCHEMA_ATTR_ACP_TARGETSCOPE: &'static str = "00000000-0000-0000-0000-ffff00000019"; pub static UUID_SCHEMA_ATTR_ACP_SEARCH_ATTR: &'static str = "00000000-0000-0000-0000-ffff00000020"; -pub static UUID_SCHEMA_ATTR_ACP_CREATE_CLASS: &'static str ="00000000-0000-0000-0000-ffff00000021"; +pub static UUID_SCHEMA_ATTR_ACP_CREATE_CLASS: &'static str = "00000000-0000-0000-0000-ffff00000021"; pub static UUID_SCHEMA_ATTR_ACP_CREATE_ATTR: &'static str = "00000000-0000-0000-0000-ffff00000022"; pub static UUID_SCHEMA_ATTR_ACP_MODIFY_REMOVEDATTR: &'static str = - "00000000-0000-0000-0000-ffff00000023"; + "00000000-0000-0000-0000-ffff00000023"; pub static UUID_SCHEMA_ATTR_ACP_MODIFY_PRESENTATTR: &'static str = - "00000000-0000-0000-0000-ffff00000024"; -pub static UUID_SCHEMA_ATTR_ACP_MODIFY_CLASS: &'static str ="00000000-0000-0000-0000-ffff00000025"; + "00000000-0000-0000-0000-ffff00000024"; +pub static UUID_SCHEMA_ATTR_ACP_MODIFY_CLASS: &'static str = "00000000-0000-0000-0000-ffff00000025"; -pub static UUID_SCHEMA_CLASS_ATTRIBUTETYPE: &'static str = "00000000-0000-0000-0000-ffff00000026"; -pub static UUID_SCHEMA_CLASS_CLASSTYPE: &'static str = "00000000-0000-0000-0000-ffff00000027"; -pub static UUID_SCHEMA_CLASS_OBJECT: &'static str = "00000000-0000-0000-0000-ffff00000028"; +pub static UUID_SCHEMA_CLASS_ATTRIBUTETYPE: &'static str = "00000000-0000-0000-0000-ffff00000026"; +pub static UUID_SCHEMA_CLASS_CLASSTYPE: &'static str = "00000000-0000-0000-0000-ffff00000027"; +pub static UUID_SCHEMA_CLASS_OBJECT: &'static str = "00000000-0000-0000-0000-ffff00000028"; pub static UUID_SCHEMA_CLASS_EXTENSIBLEOBJECT: &'static str = - "00000000-0000-0000-0000-ffff00000029"; -pub static UUID_SCHEMA_CLASS_MEMBEROF: &'static str = "00000000-0000-0000-0000-ffff00000030"; + "00000000-0000-0000-0000-ffff00000029"; +pub static UUID_SCHEMA_CLASS_MEMBEROF: &'static str = "00000000-0000-0000-0000-ffff00000030"; -pub static UUID_SCHEMA_CLASS_RECYCLED: &'static str = "00000000-0000-0000-0000-ffff00000031"; -pub static UUID_SCHEMA_CLASS_TOMBSTONE: &'static str = "00000000-0000-0000-0000-ffff00000032"; -pub static UUID_SCHEMA_CLASS_SYSTEM_INFO: &'static str = "00000000-0000-0000-0000-ffff00000033"; +pub static UUID_SCHEMA_CLASS_RECYCLED: &'static str = "00000000-0000-0000-0000-ffff00000031"; +pub static UUID_SCHEMA_CLASS_TOMBSTONE: &'static str = "00000000-0000-0000-0000-ffff00000032"; +pub static UUID_SCHEMA_CLASS_SYSTEM_INFO: &'static str = "00000000-0000-0000-0000-ffff00000033"; pub static UUID_SCHEMA_CLASS_ACCESS_CONTROL_PROFILE: &'static str = - "00000000-0000-0000-0000-ffff00000034"; + "00000000-0000-0000-0000-ffff00000034"; pub static UUID_SCHEMA_CLASS_ACCESS_CONTROL_SEARCH: &'static str = - "00000000-0000-0000-0000-ffff00000035"; + "00000000-0000-0000-0000-ffff00000035"; pub static UUID_SCHEMA_CLASS_ACCESS_CONTROL_DELETE: &'static str = - "00000000-0000-0000-0000-ffff00000036"; + "00000000-0000-0000-0000-ffff00000036"; pub static UUID_SCHEMA_CLASS_ACCESS_CONTROL_MODIFY: &'static str = - "00000000-0000-0000-0000-ffff00000037"; + "00000000-0000-0000-0000-ffff00000037"; pub static UUID_SCHEMA_CLASS_ACCESS_CONTROL_CREATE: &'static str = - "00000000-0000-0000-0000-ffff00000038"; -pub static UUID_SCHEMA_CLASS_SYSTEM: &'static str = "00000000-0000-0000-0000-ffff00000039"; + "00000000-0000-0000-0000-ffff00000038"; +pub static UUID_SCHEMA_CLASS_SYSTEM: &'static str = "00000000-0000-0000-0000-ffff00000039"; // system supplementary pub static UUID_SCHEMA_ATTR_DISPLAYNAME: &'static str = "00000000-0000-0000-0000-ffff00000040"; diff --git a/src/lib/entry.rs b/src/lib/entry.rs index 2587ed8a2..87dd823db 100644 --- a/src/lib/entry.rs +++ b/src/lib/entry.rs @@ -332,10 +332,10 @@ impl Entry { } } } else { - // TODO: Again, we clone string here, but it's so we can check all + // 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 + let may: Result, _> = classes .iter() // Join our class systemmmust + must + systemmay + may into one. .flat_map(|cls| { @@ -348,18 +348,17 @@ impl Entry { .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((s, schema_attributes.get(s).ok_or(SchemaError::Corrupted)?)) }) .collect(); let may = may?; - // TODO: Error needs to say what is missing + // TODO #70: Error needs to say what is missing // We need to return *all* missing attributes, not just the first error - // we find. + // we find. This will probably take a rewrite of the function definition + // to return a result<_, vec> and for the schema errors to take + // information about what is invalid. It's pretty nontrivial. // Check that any other attributes are in may // for each attr on the object, check it's in the may+must set @@ -835,10 +834,11 @@ impl Entry { // In the future, if we make uuid a real entry type, then this // check can "go away" because uuid will never exist as an ava. // - // TODO: Remove this check when uuid becomes a real attribute. + // NOTE: 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 ... + // as a result, I think we need to keep this continue line to not cause issues. if k == "uuid" { continue; } diff --git a/src/lib/event.rs b/src/lib/event.rs index 54b43a3c8..2b65d0d51 100644 --- a/src/lib/event.rs +++ b/src/lib/event.rs @@ -180,7 +180,7 @@ pub struct SearchEvent { pub filter: Filter, // This is the original filter, for the purpose of ACI checking. pub filter_orig: Filter, - // TODO: Add list of attributes to request + // TODO #83: Add list of attributes to request } impl SearchEvent { diff --git a/src/lib/idm/account.rs b/src/lib/idm/account.rs index 4f6f64aa9..2c78143cf 100644 --- a/src/lib/idm/account.rs +++ b/src/lib/idm/account.rs @@ -26,10 +26,11 @@ pub(crate) struct Account { // account expiry? } -impl TryFrom> for Account { - type Error = OperationError; - - fn try_from(value: Entry) -> Result { +impl Account { + // TODO #71: We need a second try_from that doesn't do group resolve for test cases I think. + pub(crate) fn try_from_entry( + value: Entry, + ) -> Result { // Check the classes if !value.attribute_value_pres("class", "account") { return Err(OperationError::InvalidAccountState( @@ -52,7 +53,7 @@ impl TryFrom> for Account { ))? .clone(); - // TODO: Resolve groups!!!! + // TODO #71: Resolve groups!!!! let groups = Vec::new(); let uuid = value.get_uuid().clone(); @@ -64,9 +65,7 @@ impl TryFrom> for Account { groups: groups, }) } -} -impl Account { // Could this actually take a claims list and application instead? pub(crate) fn to_userauthtoken(&self, claims: Vec) -> Option { // This could consume self? @@ -105,7 +104,7 @@ mod tests { serde_json::from_str(JSON_ANONYMOUS_V1).expect("Json deserialise failure!"); let anon_e = unsafe { anon_e.to_valid_committed() }; - let anon_account = Account::try_from(anon_e).expect("Must not fail"); + let anon_account = Account::try_from_entry(anon_e).expect("Must not fail"); println!("{:?}", anon_account); // I think that's it? we may want to check anonymous mech ... } diff --git a/src/lib/idm/authsession.rs b/src/lib/idm/authsession.rs index 8bf189474..3b24ec728 100644 --- a/src/lib/idm/authsession.rs +++ b/src/lib/idm/authsession.rs @@ -1,3 +1,4 @@ +use crate::audit::AuditScope; use crate::constants::UUID_ANONYMOUS; use crate::error::OperationError; use crate::idm::account::Account; @@ -123,9 +124,9 @@ impl AuthSession { } // This should return a AuthResult or similar state of checking? - // TODO: This needs some logging .... pub fn validate_creds( &mut self, + au: &mut AuditScope, creds: &Vec, ) -> Result { if self.finished { @@ -136,6 +137,7 @@ impl AuthSession { match self.handler.validate(creds) { CredState::Success(claims) => { + audit_log!(au, "Successful cred handling"); self.finished = true; let uat = self .account @@ -143,9 +145,13 @@ impl AuthSession { .ok_or(OperationError::InvalidState)?; Ok(AuthState::Success(uat)) } - CredState::Continue(allowed) => Ok(AuthState::Continue(allowed)), + CredState::Continue(allowed) => { + audit_log!(au, "Request credential continuation: {:?}", allowed); + Ok(AuthState::Continue(allowed)) + } CredState::Denied(reason) => { self.finished = true; + audit_log!(au, "Credentials denied: {}", reason); Ok(AuthState::Denied(reason.to_string())) } } diff --git a/src/lib/idm/macros.rs b/src/lib/idm/macros.rs index 7c688e104..a748dff9f 100644 --- a/src/lib/idm/macros.rs +++ b/src/lib/idm/macros.rs @@ -8,7 +8,7 @@ macro_rules! entry_str_to_account { serde_json::from_str($entry_str).expect("Json deserialise failure!"); let e = unsafe { e.to_valid_committed() }; - Account::try_from(e).expect("Account conversion failure") + Account::try_from_entry(e).expect("Account conversion failure") }}; } diff --git a/src/lib/idm/server.rs b/src/lib/idm/server.rs index c9679dbb2..830a7bdc9 100644 --- a/src/lib/idm/server.rs +++ b/src/lib/idm/server.rs @@ -118,7 +118,7 @@ impl<'a> IdmServerWriteTransaction<'a> { // typing and functionality so we can assess what auth types can // continue, and helps to keep non-needed entry specific data // out of the LRU. - let account = Account::try_from(entry)?; + let account = Account::try_from_entry(entry)?; let auth_session = AuthSession::new(account, init.appid.clone()); // Get the set of mechanisms that can proceed. This is tied @@ -153,7 +153,7 @@ impl<'a> IdmServerWriteTransaction<'a> { // Process the credentials here as required. // Basically throw them at the auth_session and see what // falls out. - auth_session.validate_creds(&creds.creds).map(|aus| { + auth_session.validate_creds(au, &creds.creds).map(|aus| { AuthResult { // Is this right? sessionid: creds.sessionid, diff --git a/src/lib/plugins/base.rs b/src/lib/plugins/base.rs index 6414288cd..f3d22bce2 100644 --- a/src/lib/plugins/base.rs +++ b/src/lib/plugins/base.rs @@ -137,9 +137,10 @@ impl Plugin for Base { 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 + // TODO #69: Can we report which UUID exists? Probably yes, we do // internal search and report the UUID *OR* we alter internal_exists - // to return UUID sets. + // to return UUID sets. This can be done as an extension to #69 where the + // internal exists is actually a wrapper around a search for uuid internally // // But does it add value? How many people will try to custom define/add uuid? let mut au_qs = AuditScope::new("qs_exist"); diff --git a/src/lib/proto/v1/mod.rs b/src/lib/proto/v1/mod.rs index 5c2ba7246..03694ac90 100644 --- a/src/lib/proto/v1/mod.rs +++ b/src/lib/proto/v1/mod.rs @@ -270,8 +270,6 @@ pub enum AuthState { #[derive(Debug, Serialize, Deserialize)] pub struct AuthResponse { - // TODO: Consider moving to an AuthMessageResponse type, and leave the proto - // without the session id because it's not necesary to know. pub sessionid: Uuid, pub state: AuthState, } diff --git a/src/lib/schema.rs b/src/lib/schema.rs index 163f717f7..bcb39893f 100644 --- a/src/lib/schema.rs +++ b/src/lib/schema.rs @@ -19,7 +19,7 @@ use concread::cowcell::{CowCell, CowCellReadTxn, CowCellWriteTxn}; // In the future this will parse/read it's schema from the db // but we have to bootstrap with some core types. -// TODO: prefix on all schema types that are system? +// TODO #72: prefix on all schema types that are system? #[allow(non_camel_case_types)] #[derive(Debug, Clone, PartialEq)] @@ -377,7 +377,7 @@ impl SchemaAttribute { } } - // TODO: This clones everything, which is expensive! + // NOTE: This clones values, but it's hard to see a way around it. pub fn normalise_value(&self, v: &String) -> String { match self.syntax { SyntaxType::SYNTAX_ID => self.normalise_syntax(v),