Finish up todo audit

src/lib/entry.rs

@@ -332,10 +332,10 @@ impl<STATE> Entry<EntryNormalised, STATE> {
                     }
                 }
             } 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<HashMap<String, &SchemaAttribute>, _> = classes
+                let may: Result<HashMap<&String, &SchemaAttribute>, _> = classes
                     .iter()
                     // Join our class systemmmust + must + systemmay + may into one.
                     .flat_map(|cls| {
@@ -348,18 +348,17 @@ impl<STATE> Entry<EntryNormalised, STATE> {
                     .map(|s| {
                         // This should NOT fail - if it does, it means our schema is
                         // in an invalid state!
-                        Ok((
+                        Ok((s, schema_attributes.get(s).ok_or(SchemaError::Corrupted)?))
-                            s.clone(),
-                            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<schemaerror>> 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<STATE> Entry<EntryValid, STATE> {
             // 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;
             }

src/lib/event.rs

@@ -180,7 +180,7 @@ pub struct SearchEvent {
     pub filter: Filter<FilterValid>,
     // This is the original filter, for the purpose of ACI checking.
     pub filter_orig: Filter<FilterValid>,
-    // TODO: Add list of attributes to request
+    // TODO #83: Add list of attributes to request
 }
 
 impl SearchEvent {

src/lib/idm/account.rs

@@ -26,10 +26,11 @@ pub(crate) struct Account {
     // account expiry?
 }
 
-impl TryFrom<Entry<EntryValid, EntryCommitted>> for Account {
+impl Account {
-    type Error = OperationError;
+    // 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(
-    fn try_from(value: Entry<EntryValid, EntryCommitted>) -> Result<Self, Self::Error> {
+        value: Entry<EntryValid, EntryCommitted>,
+    ) -> Result<Self, OperationError> {
         // Check the classes
         if !value.attribute_value_pres("class", "account") {
             return Err(OperationError::InvalidAccountState(
@@ -52,7 +53,7 @@ impl TryFrom<Entry<EntryValid, EntryCommitted>> 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<Entry<EntryValid, EntryCommitted>> for Account {
             groups: groups,
         })
     }
-}
 
-impl Account {
     // Could this actually take a claims list and application instead?
     pub(crate) fn to_userauthtoken(&self, claims: Vec<Claim>) -> Option<UserAuthToken> {
         // 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 ...
     }

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<AuthCredential>,
     ) -> Result<AuthState, OperationError> {
         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()))
             }
         }

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")
     }};
 }
 

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,

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");

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,
 }

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),