Merge pull request #246 from radicle-dev/xla/239-adjust-org-id-validation

Adjust org id validations

Author: xla

client/examples/project_registration.rs

@@ -1,6 +1,7 @@
 //! Register a project on the ledger
 use futures::compat::{Compat, Future01CompatExt};
 use futures::future::FutureExt;
+use std::convert::TryFrom;
 
 use radicle_registry_client::*;
 
@@ -19,7 +20,7 @@ async fn go() -> Result<(), Error> {
     let client = Client::create(node_host).await?;
 
     let project_name = ProjectName::from_string("radicle-registry".to_string()).unwrap();
-    let org_id = OrgId::from_string("Monadic".to_string()).unwrap();
+    let org_id = OrgId::try_from("monadic").unwrap();
 
     // Choose some random project hash and create a checkpoint
     let project_hash = H256::random();

client/tests/end_to_end.rs

@@ -28,7 +28,7 @@ async fn register_project() {
     .result
     .unwrap();
 
-    let org_id = random_string32();
+    let org_id = random_org_id();
     let register_org_message = message::RegisterOrg {
         org_id: org_id.clone(),
     };

core/src/lib.rs

@@ -36,6 +36,9 @@ pub use bytes128::Bytes128;
 pub mod string32;
 pub use string32::String32;
 
+mod org_id;
+pub use org_id::OrgId;
+
 mod error;
 pub use error::RegistryError;
 
@@ -58,16 +61,14 @@ pub type ProjectId = (ProjectName, OrgId);
 /// The name a project is registered with.
 pub type ProjectName = String32;
 
-pub type OrgId = String32;
-
 /// Org
 ///
 /// Different from [state::Org] in which this type gathers
-/// both the id and the other Org fields, respectively stored
+/// both the [`OrgId`] and the other [`Org`] fields, respectively stored
 /// as an Org's storage key and data, into one complete type.
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct Org {
-    // Id of an Org.
+    // Unique id of an Org.
     pub id: OrgId,
 
     /// See [state::Org::account_id]

core/src/org_id.rs

@@ -0,0 +1,225 @@
+// Radicle Registry
+// Copyright (C) 2019 Monadic GmbH <[email protected]>
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License version 3 as
+// published by the Free Software Foundation.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+/// `OrgId` is the unique identifier for organisations.
+///
+/// https://github.com/radicle-dev/registry-spec/blob/master/body.tex#L110
+use alloc::prelude::v1::*;
+use core::convert::{From, Into, TryFrom};
+use parity_scale_codec as codec;
+
+use crate::string32;
+
+#[derive(codec::Encode, Clone, Debug, Eq, PartialEq)]
+pub struct OrgId(String);
+
+impl OrgId {
+    fn from_string(input: String) -> Result<Self, InvalidOrgIdError> {
+        // Must be at least 1 character.
+        if input.is_empty() {
+            return Err(InvalidOrgIdError("must be at least 1 character"));
+        }
+        // Must be no longer than 32.
+        if input.len() > 32 {
+            return Err(InvalidOrgIdError("must not exceed 32 characters"));
+        }
+        // Must only contain a-z, 0-9 and '-' characters.
+        if !input
+            .chars()
+            .all(|c| c.is_ascii_digit() || c.is_ascii_lowercase() || c == '-')
+        {
+            return Err(InvalidOrgIdError("must only include a-z, 0-9 and '-'"));
+        }
+
+        // Must not start with a '-'.
+        if input.starts_with('-') {
+            return Err(InvalidOrgIdError("must not start with a '-'"));
+        }
+        // Must not end with a '-'.
+        if input.ends_with('-') {
+            return Err(InvalidOrgIdError("must not end with a '-'"));
+        }
+        // Must not contain sequences of more than one '-'.
+        if input.contains("--") {
+            return Err(InvalidOrgIdError(
+                "must not have more than one consecutive '-'",
+            ));
+        }
+
+        let id = Self(input);
+
+        Ok(id)
+    }
+}
+
+impl codec::Decode for OrgId {
+    fn decode<I: codec::Input>(input: &mut I) -> Result<Self, codec::Error> {
+        let decoded: String = String::decode(input)?;
+
+        match OrgId::try_from(decoded) {
+            Ok(id) => Ok(id),
+            Err(err) => Err(codec::Error::from(err.what())),
+        }
+    }
+}
+
+impl Into<String> for OrgId {
+    fn into(self) -> String {
+        self.0
+    }
+}
+
+impl TryFrom<String> for OrgId {
+    type Error = InvalidOrgIdError;
+
+    fn try_from(input: String) -> Result<Self, Self::Error> {
+        Self::from_string(input)
+    }
+}
+
+impl TryFrom<&str> for OrgId {
+    type Error = InvalidOrgIdError;
+
+    fn try_from(input: &str) -> Result<Self, Self::Error> {
+        Self::from_string(input.to_string())
+    }
+}
+
+impl TryFrom<string32::String32> for OrgId {
+    type Error = InvalidOrgIdError;
+
+    fn try_from(input: string32::String32) -> Result<Self, Self::Error> {
+        Self::from_string(input.into())
+    }
+}
+
+impl core::str::FromStr for OrgId {
+    type Err = InvalidOrgIdError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        Self::from_string(s.to_string())
+    }
+}
+
+#[cfg(feature = "std")]
+impl core::fmt::Display for OrgId {
+    fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
+        write!(f, "OrgId({})", self.0)
+    }
+}
+
+/// Error type when conversion from an inordinate input failed.
+#[derive(codec::Encode, Clone, Debug, Eq, PartialEq)]
+pub struct InvalidOrgIdError(&'static str);
+
+impl InvalidOrgIdError {
+    /// Error description
+    ///
+    /// This function returns an actual error str.
+    pub fn what(&self) -> &'static str {
+        self.0
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::fmt::Display for InvalidOrgIdError {
+    fn fmt(&self, f: &mut core::fmt::Formatter) -> std::fmt::Result {
+        write!(f, "InvalidOrgIdError({})", self.0)
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for InvalidOrgIdError {
+    fn description(&self) -> &str {
+        self.0
+    }
+}
+
+impl From<&'static str> for InvalidOrgIdError {
+    #[cfg(feature = "std")]
+    fn from(s: &'static str) -> Self {
+        Self(s)
+    }
+
+    #[cfg(not(feature = "std"))]
+    fn from(s: &'static str) -> Self {
+        InvalidOrgIdError(s)
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use super::OrgId;
+    use parity_scale_codec::{Decode, Encode};
+
+    #[test]
+    fn id_too_short() {
+        assert!(OrgId::from_string("".into()).is_err());
+    }
+
+    #[test]
+    fn id_too_long() {
+        let input = std::iter::repeat("X").take(33).collect::<String>();
+        let too_long = OrgId::from_string(input);
+        assert!(too_long.is_err());
+    }
+
+    #[test]
+    fn id_invalid_characters() {
+        let invalid_characters = OrgId::from_string("AZ+*".into());
+        assert!(invalid_characters.is_err());
+    }
+
+    #[test]
+    fn id_invalid_prefix() {
+        let invalid_prefix = OrgId::from_string("-radicle".into());
+        assert!(invalid_prefix.is_err());
+    }
+
+    #[test]
+    fn id_invalid_suffix() {
+        let invalid_suffix = OrgId::from_string("radicle-".into());
+        assert!(invalid_suffix.is_err());
+    }
+
+    #[test]
+    fn id_double_dash() {
+        let double_dash = OrgId::from_string("radicle--registry".into());
+        assert!(double_dash.is_err());
+    }
+
+    #[test]
+    fn id_valid() {
+        let valid = OrgId::from_string("radicle-registry001".into());
+        assert!(valid.is_ok());
+    }
+
+    #[test]
+    fn encode_then_decode() {
+        let id = OrgId::from_string("monadic".into()).unwrap();
+        let encoded = id.encode();
+        let decoded = OrgId::decode(&mut &encoded[..]).unwrap();
+
+        assert_eq!(id, decoded)
+    }
+
+    #[test]
+    fn encoded_then_decode_invalid() {
+        let invalid = Encode::encode("-Invalid-");
+        let decoded = OrgId::decode(&mut &invalid[..]);
+
+        assert!(decoded.is_err());
+    }
+}

runtime/src/registry.rs

@@ -341,14 +341,16 @@ pub fn decode_blake_two128_concat_key<K: parity_scale_codec::Decode>(
 
 #[cfg(test)]
 mod test {
-    use super::*;
+    use core::convert::TryFrom;
     use frame_support::storage::generator::StorageMap;
 
+    use super::*;
+
     #[test]
     /// Test that store::Orgs::decode_key after store::Orgs::storage_map_final_key
     /// is identify as to the original input id.
     fn orgs_decode_key_identity() {
-        let org_id = OrgId::from_string("Monadic".into()).unwrap();
+        let org_id = OrgId::try_from("monadic").unwrap();
         let hashed_key = store::Orgs::storage_map_final_key(org_id.clone());
         let decoded_key = store::Orgs::decode_key(&hashed_key).unwrap();
         assert_eq!(decoded_key, org_id);
@@ -358,9 +360,9 @@ mod test {
     /// Test that store::Projects::decode_key after store::Projects::storage_map_final_key
     /// is identify as to the original input id.
     fn projects_decode_key_identity() {
-        let org_id = OrgId::from_string("Monadic".into()).unwrap();
+        let org_id = OrgId::try_from("monadic").unwrap();
         let project_name = ProjectName::from_string("Radicle".into()).unwrap();
-        let project_id: ProjectId = (org_id, project_name);
+        let project_id: ProjectId = (project_name, org_id);
         let hashed_key = store::Projects::storage_map_final_key(project_id.clone());
         let decoded_key = store::Projects::decode_key(&hashed_key).unwrap();
         assert_eq!(decoded_key, project_id);

runtime/tests/checkpoing_setting.rs

@@ -12,7 +12,7 @@ async fn set_checkpoint() {
     let client = Client::new_emulator();
     let charles = key_pair_from_string("Charles");
 
-    let org_id = random_string32();
+    let org_id = random_org_id();
     let project = create_project_with_checkpoint(org_id.clone(), &client, &charles).await;
     let project_name = project.clone().name;
 
@@ -53,7 +53,7 @@ async fn set_checkpoint_without_permission() {
     let client = Client::new_emulator();
     let eve = key_pair_from_string("Eve");
 
-    let org_id = random_string32();
+    let org_id = random_org_id();
     let project = create_project_with_checkpoint(org_id.clone(), &client, &eve).await;
     let project_name = project.name.clone();
 
@@ -100,7 +100,7 @@ async fn fail_to_set_nonexistent_checkpoint() {
     let client = Client::new_emulator();
     let david = key_pair_from_string("David");
 
-    let org_id = random_string32();
+    let org_id = random_org_id();
     let project = create_project_with_checkpoint(org_id.clone(), &client, &david).await;
     let project_name = project.name.clone();
     let garbage = CheckpointId::random();
@@ -134,7 +134,7 @@ async fn set_fork_checkpoint() {
     let client = Client::new_emulator();
     let grace = key_pair_from_string("Grace");
 
-    let org_id = random_string32();
+    let org_id = random_org_id();
     let project = create_project_with_checkpoint(org_id.clone(), &client, &grace).await;
     let project_name = project.name.clone();
     let mut current_cp = project.current_cp;

runtime/tests/org_registration.rs

@@ -124,7 +124,7 @@ async fn unregister_org_with_projects() {
     let client = Client::new_emulator();
     let alice = key_pair_from_string("Alice");
 
-    let org_id = random_string32();
+    let org_id = random_org_id();
     let random_project = create_project_with_checkpoint(org_id.clone(), &client, &alice).await;
 
     assert!(

runtime/tests/project_registration.rs

@@ -91,7 +91,7 @@ async fn register_project_with_inexistent_org() {
     .result
     .unwrap();
 
-    let inexistent_org_id = random_string32();
+    let inexistent_org_id = random_org_id();
     let message = random_register_project_message(inexistent_org_id, checkpoint_id);
     let tx_applied = submit_ok(&client, &alice, message.clone()).await;
 
@@ -115,7 +115,7 @@ async fn register_project_with_duplicate_id() {
     .result
     .unwrap();
 
-    let org_id = random_string32();
+    let org_id = random_org_id();
     let register_org = message::RegisterOrg {
         org_id: org_id.clone(),
     };
@@ -165,7 +165,7 @@ async fn register_project_with_bad_checkpoint() {
 
     let checkpoint_id = H256::random();
 
-    let org_id = random_string32();
+    let org_id = random_org_id();
     let register_project = random_register_project_message(org_id.clone(), checkpoint_id);
     let register_org = message::RegisterOrg { org_id };
     submit_ok(&client, &alice, register_org.clone()).await;
@@ -189,7 +189,7 @@ async fn register_project_with_bad_actor() {
     let god_actor = key_pair_from_string("Alice");
     let bad_actor = key_pair_from_string("BadActor");
 
-    let org_id = random_string32();
+    let org_id = random_org_id();
     let register_project = random_register_project_message(org_id.clone(), H256::random());
     let register_org = message::RegisterOrg { org_id };
     submit_ok(&client, &god_actor, register_org.clone()).await;