server, app: Add validation for attribute names (#1075)

This commit adds support for basic validation of attribute
names at creation, and also in the schema overview. Both
user and group attributes are validated with the same rules.

For now, attribute names will be considered valid, if they
only contain alphanumeric characters and dashes.

Validation has been added the following places:

- In graphql API, for creation of both user and group attributes.
  Request will be rejected, if attribute name is invalid.

- In frontend, before submitting a request to create a new user
  or group attribute. Rejection here will show an error message
  including a list of the invalid characters used.

As this change adds stricter validation to attributes, and, since
the rationale for this is partly compatibility with other LDAP
systems, this change also adds a warning in the schema overviews
to any attribute using invalid characters.

Cargo.lock

@@ -2536,6 +2536,7 @@ dependencies = [
  "ldap3_proto",
  "lettre",
  "lldap_auth",
+ "lldap_validation",
  "log",
  "mockall",
  "nix",
@@ -2589,6 +2590,7 @@ dependencies = [
  "indexmap 1.6.2",
  "jwt 0.13.0",
  "lldap_auth",
+ "lldap_validation",
  "rand 0.8.5",
  "serde",
  "serde_json",
@@ -2653,6 +2655,10 @@ dependencies = [
  "serde_json",
 ]
 
+[[package]]
+name = "lldap_validation"
+version = "0.6.0"
+
 [[package]]
 name = "local-channel"
 version = "0.1.5"

app/Cargo.toml

@@ -60,6 +60,9 @@ features = [
 path = "../auth"
 features = [ "opaque_client" ]
 
+[dependencies.lldap_validation]
+path = "../validation"
+
 [dependencies.image]
 features = ["jpeg"]
 default-features = false

app/src/components/create_group_attribute.rs

@@ -12,6 +12,7 @@ use crate::{
 use anyhow::{bail, Result};
 use gloo_console::log;
 use graphql_client::GraphQLQuery;
+use lldap_validation::attributes::validate_attribute_name;
 use validator_derive::Validate;
 use yew::prelude::*;
 use yew_form_derive::Model;
@@ -62,6 +63,13 @@ impl CommonComponent<CreateGroupAttributeForm> for CreateGroupAttributeForm {
                     bail!("Check the form for errors");
                 }
                 let model = self.form.model();
+                validate_attribute_name(&model.attribute_name).or_else(|invalid_chars| {
+                    let invalid = String::from_iter(invalid_chars);
+                    bail!(
+                        "Attribute name contains one or more invalid characters: {}",
+                        invalid
+                    );
+                })?;
                 let attribute_type = model.attribute_type.parse::<AttributeType>().unwrap();
                 let req = create_group_attribute::Variables {
                     name: model.attribute_name,

app/src/components/create_user_attribute.rs

@@ -12,6 +12,7 @@ use crate::{
 use anyhow::{bail, Result};
 use gloo_console::log;
 use graphql_client::GraphQLQuery;
+use lldap_validation::attributes::validate_attribute_name;
 use validator_derive::Validate;
 use yew::prelude::*;
 use yew_form_derive::Model;
@@ -66,6 +67,13 @@ impl CommonComponent<CreateUserAttributeForm> for CreateUserAttributeForm {
                 if model.is_editable && !model.is_visible {
                     bail!("Editable attributes must also be visible");
                 }
+                validate_attribute_name(&model.attribute_name).or_else(|invalid_chars| {
+                    let invalid = String::from_iter(invalid_chars);
+                    bail!(
+                        "Attribute name contains one or more invalid characters: {}",
+                        invalid
+                    );
+                })?;
                 let attribute_type = model.attribute_type.parse::<AttributeType>().unwrap();
                 let req = create_user_attribute::Variables {
                     name: model.attribute_name,

app/src/components/fragments/attribute_schema.rs

@@ -1,36 +1,52 @@
+use crate::infra::attributes::AttributeDescription;
+use lldap_validation::attributes::{validate_attribute_name, ALLOWED_CHARACTERS_DESCRIPTION};
 use yew::{html, Html};
 
-use crate::infra::attributes::AttributeDescription;
-
-pub fn render_attribute_name(
-    hardcoded: bool,
-    attribute_identifier: &String,
-    attribute_description: &AttributeDescription,
-) -> Html {
-    if hardcoded {
-        html! {
-          <>
-            {&attribute_description.attribute_name}
-            {
-              if attribute_description.aliases.is_empty() {
-                html!{}
-              } else {
-                html!{
-                  <>
-                    <br/>
-                    <small class="text-muted">
-                      {"Aliases: "}
-                      {attribute_description.aliases.join(", ")}
-                    </small>
-                  </>
-                }
-              }
-            }
-          </>
-        }
+fn render_attribute_aliases(attribute_description: &AttributeDescription) -> Html {
+    if attribute_description.aliases.is_empty() {
+        html! {}
     } else {
         html! {
-          {&attribute_identifier}
+          <>
+            <br/>
+            <small class="text-muted">
+              {"Aliases: "}
+              {attribute_description.aliases.join(", ")}
+            </small>
+          </>
         }
     }
 }
+
+fn render_attribute_validation_warnings(attribute_name: &str) -> Html {
+    match validate_attribute_name(attribute_name) {
+        Ok(()) => {
+            html! {}
+        }
+        Err(_invalid_chars) => {
+            html! {
+              <>
+                <br/>
+                <small class="text-warning">
+                  {"Warning: This attribute uses one or more invalid characters "}
+                  {"("}{ALLOWED_CHARACTERS_DESCRIPTION}{"). "}
+                  {"Some clients may not support it."}
+                </small>
+              </>
+            }
+        }
+    }
+}
+
+pub fn render_attribute_name(
+    hardcoded: bool,
+    attribute_description: &AttributeDescription,
+) -> Html {
+    html! {
+      <>
+        {&attribute_description.attribute_name}
+        {if hardcoded {render_attribute_aliases(attribute_description)} else {html!{}}}
+        {render_attribute_validation_warnings(attribute_description.attribute_name)}
+      </>
+    }
+}

app/src/components/group_schema_table.rs

@@ -157,7 +157,7 @@ impl GroupSchemaTable {
         let desc = group::resolve_group_attribute_description_or_default(&attribute.name);
         html! {
             <tr key={attribute.name.clone()}>
-                <td>{render_attribute_name(hardcoded, &attribute.name, &desc)}</td>
+                <td>{render_attribute_name(hardcoded, &desc)}</td>
                 <td>{if attribute.is_list { format!("List<{attribute_type}>")} else {attribute_type.to_string()}}</td>
                 <td>{if attribute.is_visible {checkmark.clone()} else {html!{}}}</td>
                 {

app/src/components/user_schema_table.rs

@@ -156,7 +156,7 @@ impl UserSchemaTable {
         let desc = user::resolve_user_attribute_description_or_default(&attribute.name);
         html! {
             <tr key={attribute.name.clone()}>
-                <td>{render_attribute_name(hardcoded, &attribute.name, &desc)}</td>
+                <td>{render_attribute_name(hardcoded, &desc)}</td>
                 <td>{if attribute.is_list { format!("List<{attribute_type}>")} else {attribute_type.to_string()}}</td>
                 <td>{if attribute.is_editable {checkmark.clone()} else {html!{}}}</td>
                 <td>{if attribute.is_visible {checkmark.clone()} else {html!{}}}</td>

server/Cargo.toml

@@ -85,6 +85,9 @@ version = "0.10.1"
 path = "../auth"
 features = ["opaque_server", "opaque_client", "sea_orm"]
 
+[dependencies.lldap_validation]
+path = "../validation"
+
 [dependencies.opaque-ke]
 version = "0.7"
 

server/src/infra/graphql/mutation.rs

@@ -23,7 +23,8 @@ use crate::{
 };
 use anyhow::{anyhow, Context as AnyhowContext};
 use base64::Engine;
-use juniper::{graphql_object, FieldResult, GraphQLInputObject, GraphQLObject};
+use juniper::{graphql_object, FieldError, FieldResult, GraphQLInputObject, GraphQLObject};
+use lldap_validation::attributes::{validate_attribute_name, ALLOWED_CHARACTERS_DESCRIPTION};
 use tracing::{debug, debug_span, Instrument, Span};
 
 #[derive(PartialEq, Eq, Debug)]
@@ -440,6 +441,22 @@ impl<Handler: BackendHandler> Mutation<Handler> {
         span.in_scope(|| {
             debug!(?name, ?attribute_type, is_list, is_visible, is_editable);
         });
+        validate_attribute_name(&name).map_err(|invalid_chars: Vec<char>| -> FieldError {
+            let chars = String::from_iter(invalid_chars);
+            span.in_scope(|| {
+                debug!(
+                    "Cannot create attribute with invalid name. Valid characters: {}. Invalid chars found: {}",
+                    ALLOWED_CHARACTERS_DESCRIPTION,
+                    chars
+                )
+            });
+            anyhow!(
+                "Cannot create attribute with invalid name. Valid characters: {}. Invalid chars found: {}",
+                ALLOWED_CHARACTERS_DESCRIPTION,
+                chars
+            )
+            .into()
+        })?;
         let handler = context
             .get_admin_handler()
             .ok_or_else(field_error_callback(
@@ -471,6 +488,20 @@ impl<Handler: BackendHandler> Mutation<Handler> {
         span.in_scope(|| {
             debug!(?name, ?attribute_type, is_list, is_visible, is_editable);
         });
+        validate_attribute_name(&name).map_err(|invalid_chars: Vec<char>| -> FieldError {
+            let chars = String::from_iter(invalid_chars);
+            span.in_scope(|| {
+                debug!(
+                    "Cannot create attribute with invalid name. Invalid chars found: {}",
+                    chars
+                )
+            });
+            anyhow!(
+                "Cannot create attribute with invalid name. Valid characters: {}",
+                ALLOWED_CHARACTERS_DESCRIPTION
+            )
+            .into()
+        })?;
         let handler = context
             .get_admin_handler()
             .ok_or_else(field_error_callback(
@@ -695,3 +726,240 @@ fn deserialize_attribute(
         value: deserialized_values,
     })
 }
+
+#[cfg(test)]
+mod tests {
+
+    use super::*;
+    use crate::{
+        domain::types::{AttributeName, AttributeType},
+        infra::{
+            access_control::{Permission, ValidationResults},
+            graphql::query::Query,
+            test_utils::MockTestBackendHandler,
+        },
+    };
+    use juniper::{
+        execute, graphql_value, DefaultScalarValue, EmptySubscription, GraphQLType, InputValue,
+        RootNode, Variables,
+    };
+    use mockall::predicate::eq;
+    use pretty_assertions::assert_eq;
+
+    fn mutation_schema<'q, C, Q, M>(
+        query_root: Q,
+        mutation_root: M,
+    ) -> RootNode<'q, Q, M, EmptySubscription<C>>
+    where
+        Q: GraphQLType<DefaultScalarValue, Context = C, TypeInfo = ()> + 'q,
+        M: GraphQLType<DefaultScalarValue, Context = C, TypeInfo = ()> + 'q,
+    {
+        RootNode::new(query_root, mutation_root, EmptySubscription::<C>::new())
+    }
+
+    #[tokio::test]
+    async fn test_create_user_attribute_valid() {
+        const QUERY: &str = r#"
+            mutation CreateUserAttribute($name: String!, $attributeType: AttributeType!, $isList: Boolean!, $isVisible: Boolean!, $isEditable: Boolean!) {
+                addUserAttribute(name: $name, attributeType: $attributeType, isList: $isList, isVisible: $isVisible, isEditable: $isEditable) {
+                    ok
+                }
+            }
+        "#;
+        let mut mock = MockTestBackendHandler::new();
+        mock.expect_add_user_attribute()
+            .with(eq(CreateAttributeRequest {
+                name: AttributeName::new("AttrName0"),
+                attribute_type: AttributeType::String,
+                is_list: false,
+                is_visible: false,
+                is_editable: false,
+            }))
+            .return_once(|_| Ok(()));
+        let context = Context::<MockTestBackendHandler>::new_for_tests(
+            mock,
+            ValidationResults {
+                user: UserId::new("bob"),
+                permission: Permission::Admin,
+            },
+        );
+        let vars = Variables::from([
+            ("name".to_string(), InputValue::scalar("AttrName0")),
+            (
+                "attributeType".to_string(),
+                InputValue::enum_value("STRING"),
+            ),
+            ("isList".to_string(), InputValue::scalar(false)),
+            ("isVisible".to_string(), InputValue::scalar(false)),
+            ("isEditable".to_string(), InputValue::scalar(false)),
+        ]);
+        let schema = mutation_schema(
+            Query::<MockTestBackendHandler>::new(),
+            Mutation::<MockTestBackendHandler>::new(),
+        );
+        assert_eq!(
+            execute(QUERY, None, &schema, &vars, &context).await,
+            Ok((
+                graphql_value!(
+                {
+                    "addUserAttribute": {
+                        "ok": true
+                    }
+                } ),
+                vec![]
+            ))
+        );
+    }
+
+    #[tokio::test]
+    async fn test_create_user_attribute_invalid() {
+        const QUERY: &str = r#"
+            mutation CreateUserAttribute($name: String!, $attributeType: AttributeType!, $isList: Boolean!, $isVisible: Boolean!, $isEditable: Boolean!) {
+                addUserAttribute(name: $name, attributeType: $attributeType, isList: $isList, isVisible: $isVisible, isEditable: $isEditable) {
+                    ok
+                }
+            }
+        "#;
+        let mock = MockTestBackendHandler::new();
+        let context = Context::<MockTestBackendHandler>::new_for_tests(
+            mock,
+            ValidationResults {
+                user: UserId::new("bob"),
+                permission: Permission::Admin,
+            },
+        );
+        let vars = Variables::from([
+            ("name".to_string(), InputValue::scalar("AttrName_0")),
+            (
+                "attributeType".to_string(),
+                InputValue::enum_value("STRING"),
+            ),
+            ("isList".to_string(), InputValue::scalar(false)),
+            ("isVisible".to_string(), InputValue::scalar(false)),
+            ("isEditable".to_string(), InputValue::scalar(false)),
+        ]);
+        let schema = mutation_schema(
+            Query::<MockTestBackendHandler>::new(),
+            Mutation::<MockTestBackendHandler>::new(),
+        );
+        let result = execute(QUERY, None, &schema, &vars, &context).await;
+        match result {
+            Ok(res) => {
+                let (response, errors) = res;
+                assert!(response.is_null());
+                let expected_error_msg =
+                    "Cannot create attribute with invalid name. Valid characters: a-z, A-Z, 0-9, and dash (-). Invalid chars found: _"
+                        .to_string();
+                assert!(errors
+                    .iter()
+                    .all(|e| e.error().message() == expected_error_msg));
+            }
+            Err(_) => {
+                panic!();
+            }
+        }
+    }
+
+    #[tokio::test]
+    async fn test_create_group_attribute_valid() {
+        const QUERY: &str = r#"
+            mutation CreateGroupAttribute($name: String!, $attributeType: AttributeType!, $isList: Boolean!, $isVisible: Boolean!) {
+                addGroupAttribute(name: $name, attributeType: $attributeType, isList: $isList, isVisible: $isVisible, isEditable: false) {
+                    ok
+                }
+            }
+        "#;
+        let mut mock = MockTestBackendHandler::new();
+        mock.expect_add_group_attribute()
+            .with(eq(CreateAttributeRequest {
+                name: AttributeName::new("AttrName0"),
+                attribute_type: AttributeType::String,
+                is_list: false,
+                is_visible: false,
+                is_editable: false,
+            }))
+            .return_once(|_| Ok(()));
+        let context = Context::<MockTestBackendHandler>::new_for_tests(
+            mock,
+            ValidationResults {
+                user: UserId::new("bob"),
+                permission: Permission::Admin,
+            },
+        );
+        let vars = Variables::from([
+            ("name".to_string(), InputValue::scalar("AttrName0")),
+            (
+                "attributeType".to_string(),
+                InputValue::enum_value("STRING"),
+            ),
+            ("isList".to_string(), InputValue::scalar(false)),
+            ("isVisible".to_string(), InputValue::scalar(false)),
+            ("isEditable".to_string(), InputValue::scalar(false)),
+        ]);
+        let schema = mutation_schema(
+            Query::<MockTestBackendHandler>::new(),
+            Mutation::<MockTestBackendHandler>::new(),
+        );
+        assert_eq!(
+            execute(QUERY, None, &schema, &vars, &context).await,
+            Ok((
+                graphql_value!(
+                {
+                    "addGroupAttribute": {
+                        "ok": true
+                    }
+                } ),
+                vec![]
+            ))
+        );
+    }
+
+    #[tokio::test]
+    async fn test_create_group_attribute_invalid() {
+        const QUERY: &str = r#"
+            mutation CreateUserAttribute($name: String!, $attributeType: AttributeType!, $isList: Boolean!, $isVisible: Boolean!, $isEditable: Boolean!) {
+                addUserAttribute(name: $name, attributeType: $attributeType, isList: $isList, isVisible: $isVisible, isEditable: $isEditable) {
+                    ok
+                }
+            }
+        "#;
+        let mock = MockTestBackendHandler::new();
+        let context = Context::<MockTestBackendHandler>::new_for_tests(
+            mock,
+            ValidationResults {
+                user: UserId::new("bob"),
+                permission: Permission::Admin,
+            },
+        );
+        let vars = Variables::from([
+            ("name".to_string(), InputValue::scalar("AttrName_0")),
+            (
+                "attributeType".to_string(),
+                InputValue::enum_value("STRING"),
+            ),
+            ("isList".to_string(), InputValue::scalar(false)),
+            ("isVisible".to_string(), InputValue::scalar(false)),
+            ("isEditable".to_string(), InputValue::scalar(false)),
+        ]);
+        let schema = mutation_schema(
+            Query::<MockTestBackendHandler>::new(),
+            Mutation::<MockTestBackendHandler>::new(),
+        );
+        let result = execute(QUERY, None, &schema, &vars, &context).await;
+        match result {
+            Ok(res) => {
+                let (response, errors) = res;
+                assert!(response.is_null());
+                let expected_error_msg =
+                    "Cannot create attribute with invalid name. Valid characters: a-z, A-Z, 0-9, and dash (-). Invalid chars found: _"
+                        .to_string();
+                assert!(errors
+                    .iter()
+                    .all(|e| e.error().message() == expected_error_msg));
+            }
+            Err(_) => {
+                panic!();
+            }
+        }
+    }
+}

validation/Cargo.toml

@@ -0,0 +1,9 @@
+[package]
+authors = ["Simon Broeng Jensen <sbj@cwconsult.dk>"]
+description = "Validation logic for LLDAP"
+edition = "2021"
+homepage = "https://github.com/lldap/lldap"
+license = "GPL-3.0-only"
+name = "lldap_validation"
+repository = "https://github.com/lldap/lldap"
+version = "0.6.0"

validation/src/attributes.rs

@@ -0,0 +1,45 @@
+// Description of allowed characters. Intended for error messages.
+pub const ALLOWED_CHARACTERS_DESCRIPTION: &str = "a-z, A-Z, 0-9, and dash (-)";
+
+pub fn validate_attribute_name(attribute_name: &str) -> Result<(), Vec<char>> {
+    let invalid_chars: Vec<char> = attribute_name
+        .chars()
+        .filter(|c| !(c.is_alphanumeric() || *c == '-'))
+        .collect();
+    if invalid_chars.is_empty() {
+        Ok(())
+    } else {
+        Err(invalid_chars)
+    }
+}
+
+mod tests {
+
+    #[test]
+    fn test_valid_attribute_name() {
+        let valid1: String = "AttrName-01".to_string();
+        let result = super::validate_attribute_name(&valid1);
+        assert!(result == Ok(()));
+    }
+
+    #[test]
+    fn test_invalid_attribute_name_chars() {
+        fn test_invalid_char(c: char) {
+            let prefix: String = "AttrName".to_string();
+            let suffix: String = "01".to_string();
+            let name: String = format!("{prefix}{c}{suffix}");
+            let result = super::validate_attribute_name(&name);
+            match result {
+                Ok(()) => {
+                    panic!()
+                }
+                Err(invalid) => {
+                    assert!(invalid == vec![c.to_owned()]);
+                }
+            }
+        }
+        test_invalid_char(' ');
+        test_invalid_char('_');
+        test_invalid_char('#');
+    }
+}

validation/src/lib.rs

@@ -0,0 +1,3 @@
+#![forbid(non_ascii_idents)]
+
+pub mod attributes;