mozilla / authenticator-rs

Rust library to interact with Security Keys, used by Firefox
https://crates.io/crates/authenticator
Mozilla Public License 2.0
273 stars 70 forks source link

Extract serialize_map! macros #338

Closed emlun closed 1 month ago

emlun commented 2 months ago

Prompted by https://github.com/mozilla/authenticator-rs/pull/337#discussion_r1643364304 - it's better to just not have to worry about correctly calculating the map length.

This introduces macros util::serialize_map! and util::serialize_map_optional!, which take care of computing the map length of and serializing heterogeneous maps. serialize_map! requires all entries to always be present (and thus generates a map of constant length), and serialize_map_optional! takes Option<T> entries and omits None values.

The serialize_map_optional! API is a little bit awkward because it needs the caller to specify a bunch of arbitrary identifiers (this is because the macro needs to bind the values to internal local variables in order to not evaluate the expressions multiple times, but macro_rules! can't generate a non-constant number of internal identifiers as far as I've been able to tell), but I think this is at least much better than the raw serde API as this makes it impossible for the declared map length and actual entries to go out of sync.

This includes regression tests covering all occurrences of code that was refactored to use the new macros. I have manually verified that the tests cover both the map length computations and the serialization of each individual entry of each struct.

The tests inflate the diff volume quite a lot; the diff of just the refactorization is +239, -366. :wink:

emlun commented 2 months ago

Here's an example macro expansion, just for fun:

impl Serialize for MakeCredentials {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        debug!("Serialize MakeCredentials");
        serialize_map_optional!(
            serializer,
            v1: &0x01 => Some(&self.client_data_hash),
            v2: &0x02 => Some(&self.rp),
            v3: &0x03 => Some(&self.user),
            v4: &0x04 => Some(&self.pub_cred_params),
            v5: &0x05 => (!self.exclude_list.is_empty()).then_some(&self.exclude_list),
            v6: &0x06 => self.extensions.has_content().then_some(&self.extensions),
            v7: &0x07 => self.options.has_some().then_some(&self.options),
            v8: &0x08 => &self.pin_uv_auth_param,
            v9: &0x09 => self.pin_uv_auth_param.as_ref().map(|p| p.pin_protocol.id()),
            va: &0x0a => &self.enterprise_attestation,
        )
    }
}

expands to:

impl Serialize for MakeCredentials {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        debug!("Serialize MakeCredentials");
        {
            let serializer = serializer;
            let v1 = (Some(&self.client_data_hash));
            let v2 = (Some(&self.rp));
            let v3 = (Some(&self.user));
            let v4 = (Some(&self.pub_cred_params));
            let v5 = ((!self.exclude_list.is_empty()).then_some(&self.exclude_list));
            let v6 = (self.extensions.has_content().then_some(&self.extensions));
            let v7 = (self.options.has_some().then_some(&self.options));
            let v8 = (&self.pin_uv_auth_param);
            let v9 = (self.pin_uv_auth_param.as_ref().map(|p| p.pin_protocol.id()));
            let va = (&self.enterprise_attestation);
            let map_len = 0usize
                + if v1.is_some() { 1usize } else { 0usize }
                + if v2.is_some() { 1usize } else { 0usize }
                + if v3.is_some() { 1usize } else { 0usize }
                + if v4.is_some() { 1usize } else { 0usize }
                + if v5.is_some() { 1usize } else { 0usize }
                + if v6.is_some() { 1usize } else { 0usize }
                + if v7.is_some() { 1usize } else { 0usize }
                + if v8.is_some() { 1usize } else { 0usize }
                + if v9.is_some() { 1usize } else { 0usize }
                + if va.is_some() { 1usize } else { 0usize };
            let mut map = serializer.serialize_map(core::option::Option::Some(map_len))?;
            if let core::option::Option::Some(v) = v1 {
                map.serialize_entry((&0x01), &v)?;
            }
            if let core::option::Option::Some(v) = v2 {
                map.serialize_entry((&0x02), &v)?;
            }
            if let core::option::Option::Some(v) = v3 {
                map.serialize_entry((&0x03), &v)?;
            }
            if let core::option::Option::Some(v) = v4 {
                map.serialize_entry((&0x04), &v)?;
            }
            if let core::option::Option::Some(v) = v5 {
                map.serialize_entry((&0x05), &v)?;
            }
            if let core::option::Option::Some(v) = v6 {
                map.serialize_entry((&0x06), &v)?;
            }
            if let core::option::Option::Some(v) = v7 {
                map.serialize_entry((&0x07), &v)?;
            }
            if let core::option::Option::Some(v) = v8 {
                map.serialize_entry((&0x08), &v)?;
            }
            if let core::option::Option::Some(v) = v9 {
                map.serialize_entry((&0x09), &v)?;
            }
            if let core::option::Option::Some(v) = va {
                map.serialize_entry((&0x0a), &v)?;
            }
            map.end()
        }
    }
}
emlun commented 2 months ago

I made serialize_map_optional! less awkward for most (all current) uses of it, by eliminating the need for the identifier argument for the first 10 entries. This comes at the cost of a bit of hardcoding, but I think the improved call site API is worth it.