diff --git a/crates/domain/src/requests.rs b/crates/domain/src/requests.rs index eefd6e7..28a658e 100644 --- a/crates/domain/src/requests.rs +++ b/crates/domain/src/requests.rs @@ -1,7 +1,7 @@ use serde::{Deserialize, Serialize}; use crate::types::{ - AttributeName, AttributeType, AttributeValue, Email, GroupId, GroupName, JpegPhoto, UserId, + AttributeName, AttributeType, AttributeValue, Email, GroupId, GroupName, UserId, }; #[derive(PartialEq, Eq, Debug, Serialize, Deserialize, Clone, Default)] @@ -10,9 +10,6 @@ pub struct CreateUserRequest { pub user_id: UserId, pub email: Email, pub display_name: Option, - pub first_name: Option, - pub last_name: Option, - pub avatar: Option, pub attributes: Vec, } @@ -22,9 +19,6 @@ pub struct UpdateUserRequest { pub user_id: UserId, pub email: Option, pub display_name: Option, - pub first_name: Option, - pub last_name: Option, - pub avatar: Option, pub delete_attributes: Vec, pub insert_attributes: Vec, } diff --git a/schema.graphql b/schema.graphql index f744e56..caf6a9c 100644 --- a/schema.graphql +++ b/schema.graphql @@ -65,10 +65,19 @@ input CreateUserInput { id: String! email: String displayName: String - firstName: String - lastName: String - "Base64 encoded JpegPhoto." avatar: String - "User-defined attributes." attributes: [AttributeValueInput!] + """ + First name of user. Deprecated: use attribute instead. + If both field and corresponding attribute is supplied, the attribute will take precedence. + """ firstName: String + """ + Last name of user. Deprecated: use attribute instead. + If both field and corresponding attribute is supplied, the attribute will take precedence. + """ lastName: String + """ + Base64 encoded JpegPhoto. Deprecated: use attribute instead. + If both field and corresponding attribute is supplied, the attribute will take precedence. + """ avatar: String + "Attributes." attributes: [AttributeValueInput!] } type AttributeSchema { @@ -86,9 +95,18 @@ input UpdateUserInput { id: String! email: String displayName: String - firstName: String - lastName: String - "Base64 encoded JpegPhoto." avatar: String + """ + First name of user. Deprecated: use attribute instead. + If both field and corresponding attribute is supplied, the attribute will take precedence. + """ firstName: String + """ + Last name of user. Deprecated: use attribute instead. + If both field and corresponding attribute is supplied, the attribute will take precedence. + """ lastName: String + """ + Base64 encoded JpegPhoto. Deprecated: use attribute instead. + If both field and corresponding attribute is supplied, the attribute will take precedence. + """ avatar: String """ Attribute names to remove. They are processed before insertions. diff --git a/server/src/domain/sql_backend_handler.rs b/server/src/domain/sql_backend_handler.rs index 2702771..750c817 100644 --- a/server/src/domain/sql_backend_handler.rs +++ b/server/src/domain/sql_backend_handler.rs @@ -33,7 +33,7 @@ pub mod tests { use lldap_auth::{opaque, registration}; use lldap_domain::{ requests::{CreateGroupRequest, CreateUserRequest}, - types::{GroupId, UserId}, + types::{AttributeValue as DomainAttributeValue, GroupId, Serialized, UserId}, }; use pretty_assertions::assert_eq; use sea_orm::Database; @@ -92,9 +92,16 @@ pub mod tests { user_id: UserId::new(name), email: format!("{}@bob.bob", name).into(), display_name: Some("display ".to_string() + name), - first_name: Some("first ".to_string() + name), - last_name: Some("last ".to_string() + name), - ..Default::default() + attributes: vec![ + DomainAttributeValue { + name: "first_name".into(), + value: Serialized::from(("first ".to_string() + name).as_str()), + }, + DomainAttributeValue { + name: "last_name".into(), + value: Serialized::from(("last ".to_string() + name).as_str()), + }, + ], }) .await .unwrap(); diff --git a/server/src/domain/sql_user_backend_handler.rs b/server/src/domain/sql_user_backend_handler.rs index fbbdf46..606e09e 100644 --- a/server/src/domain/sql_user_backend_handler.rs +++ b/server/src/domain/sql_user_backend_handler.rs @@ -16,8 +16,8 @@ use sea_orm::{ sea_query::{ query::OnConflict, Alias, Cond, Expr, Func, IntoColumnRef, IntoCondition, SimpleExpr, }, - ActiveModelTrait, ActiveValue, ColumnTrait, DatabaseTransaction, EntityTrait, IntoActiveValue, - ModelTrait, QueryFilter, QueryOrder, QuerySelect, QueryTrait, Set, TransactionTrait, + ActiveModelTrait, ActiveValue, ColumnTrait, DatabaseTransaction, EntityTrait, ModelTrait, + QueryFilter, QueryOrder, QuerySelect, QueryTrait, Set, TransactionTrait, }; use std::collections::HashSet; use tracing::instrument; @@ -185,11 +185,6 @@ impl SqlBackendHandler { display_name: to_value(&request.display_name), ..Default::default() }; - let to_serialized_value = |s: &Option| match s.as_ref().map(|s| s.as_str()) { - None => None, - Some("") => Some(ActiveValue::NotSet), - Some(s) => Some(ActiveValue::Set(Serialized::from(s))), - }; let mut update_user_attributes = Vec::new(); let mut remove_user_attributes = Vec::new(); let mut process_serialized = @@ -206,15 +201,6 @@ impl SqlBackendHandler { } _ => unreachable!(), }; - if let Some(value) = to_serialized_value(&request.first_name) { - process_serialized(value, "first_name".into()); - } - if let Some(value) = to_serialized_value(&request.last_name) { - process_serialized(value, "last_name".into()); - } - if let Some(avatar) = request.avatar { - process_serialized(avatar.into_active_value(), "avatar".into()); - } let schema = Self::get_schema_with_transaction(transaction).await?; for attribute in request.insert_attributes { if schema @@ -318,27 +304,6 @@ impl UserBackendHandler for SqlBackendHandler { ..Default::default() }; let mut new_user_attributes = Vec::new(); - if let Some(first_name) = request.first_name { - new_user_attributes.push(model::user_attributes::ActiveModel { - user_id: Set(request.user_id.clone()), - attribute_name: Set("first_name".into()), - value: Set(Serialized::from(&first_name)), - }); - } - if let Some(last_name) = request.last_name { - new_user_attributes.push(model::user_attributes::ActiveModel { - user_id: Set(request.user_id.clone()), - attribute_name: Set("last_name".into()), - value: Set(Serialized::from(&last_name)), - }); - } - if let Some(avatar) = request.avatar { - new_user_attributes.push(model::user_attributes::ActiveModel { - user_id: Set(request.user_id.clone()), - attribute_name: Set("avatar".into()), - value: Set(Serialized::from(&avatar)), - }); - } self.sql_pool .transaction::<_, (), DomainError>(|transaction| { Box::pin(async move { @@ -845,11 +810,21 @@ mod tests { user_id: UserId::new("bob"), email: Some("email".into()), display_name: Some("display_name".to_string()), - first_name: Some("first_name".to_string()), - last_name: Some("last_name".to_string()), - avatar: Some(JpegPhoto::for_tests()), delete_attributes: Vec::new(), - insert_attributes: Vec::new(), + insert_attributes: vec![ + AttributeValue { + name: "first_name".into(), + value: Serialized::from("first_name"), + }, + AttributeValue { + name: "last_name".into(), + value: Serialized::from("last_name"), + }, + AttributeValue { + name: "avatar".into(), + value: Serialized::from(&JpegPhoto::for_tests()), + }, + ], }) .await .unwrap(); @@ -888,9 +863,11 @@ mod tests { .handler .update_user(UpdateUserRequest { user_id: UserId::new("bob"), - first_name: None, - last_name: Some(String::new()), - avatar: Some(JpegPhoto::for_tests()), + delete_attributes: vec!["last_name".into()], + insert_attributes: vec![AttributeValue { + name: "avatar".into(), + value: Serialized::from(&JpegPhoto::for_tests()), + }], ..Default::default() }) .await @@ -925,9 +902,6 @@ mod tests { .handler .update_user(UpdateUserRequest { user_id: UserId::new("bob"), - first_name: None, - last_name: None, - avatar: None, insert_attributes: vec![AttributeValue { name: "first_name".into(), value: Serialized::from("new first"), @@ -965,9 +939,6 @@ mod tests { .handler .update_user(UpdateUserRequest { user_id: UserId::new("bob"), - first_name: None, - last_name: None, - avatar: None, delete_attributes: vec!["first_name".into()], ..Default::default() }) @@ -996,9 +967,6 @@ mod tests { .handler .update_user(UpdateUserRequest { user_id: UserId::new("bob"), - first_name: None, - last_name: None, - avatar: None, delete_attributes: vec!["first_name".into()], insert_attributes: vec![AttributeValue { name: "first_name".into(), @@ -1037,7 +1005,10 @@ mod tests { .handler .update_user(UpdateUserRequest { user_id: UserId::new("bob"), - avatar: Some(JpegPhoto::for_tests()), + insert_attributes: vec![AttributeValue { + name: "avatar".into(), + value: Serialized::from(&JpegPhoto::for_tests()), + }], ..Default::default() }) .await @@ -1057,7 +1028,10 @@ mod tests { .handler .update_user(UpdateUserRequest { user_id: UserId::new("bob"), - avatar: Some(JpegPhoto::null()), + insert_attributes: vec![AttributeValue { + name: "avatar".into(), + value: Serialized::from(&JpegPhoto::null()), + }], ..Default::default() }) .await @@ -1081,13 +1055,20 @@ mod tests { user_id: UserId::new("james"), email: "email".into(), display_name: Some("display_name".to_string()), - first_name: None, - last_name: Some("last_name".to_string()), - avatar: Some(JpegPhoto::for_tests()), - attributes: vec![AttributeValue { - name: "first_name".into(), - value: Serialized::from("First Name"), - }], + attributes: vec![ + AttributeValue { + name: "first_name".into(), + value: Serialized::from("First Name"), + }, + AttributeValue { + name: "last_name".into(), + value: Serialized::from("last_name"), + }, + AttributeValue { + name: "avatar".into(), + value: Serialized::from(&JpegPhoto::for_tests()), + }, + ], }) .await .unwrap(); diff --git a/server/src/infra/graphql/mutation.rs b/server/src/infra/graphql/mutation.rs index ad51bdb..47abca3 100644 --- a/server/src/infra/graphql/mutation.rs +++ b/server/src/infra/graphql/mutation.rs @@ -1,4 +1,4 @@ -use std::sync::Arc; +use std::{collections::BTreeMap, sync::Arc}; use crate::{ domain::{ @@ -13,7 +13,6 @@ use crate::{ }, }; use anyhow::{anyhow, Context as AnyhowContext}; -use base64::Engine; use juniper::{graphql_object, FieldError, FieldResult, GraphQLInputObject, GraphQLObject}; use lldap_domain::{ requests::{ @@ -23,7 +22,7 @@ use lldap_domain::{ schema::AttributeList, types::{ AttributeName, AttributeType, AttributeValue as DomainAttributeValue, Email, GroupId, - JpegPhoto, LdapObjectClass, UserId, + LdapObjectClass, UserId, }, }; use lldap_validation::attributes::{validate_attribute_name, ALLOWED_CHARACTERS_DESCRIPTION}; @@ -65,11 +64,16 @@ pub struct CreateUserInput { // The email can be specified as an attribute, but one of the two is required. email: Option, display_name: Option, + /// First name of user. Deprecated: use attribute instead. + /// If both field and corresponding attribute is supplied, the attribute will take precedence. first_name: Option, + /// Last name of user. Deprecated: use attribute instead. + /// If both field and corresponding attribute is supplied, the attribute will take precedence. last_name: Option, - /// Base64 encoded JpegPhoto. + /// Base64 encoded JpegPhoto. Deprecated: use attribute instead. + /// If both field and corresponding attribute is supplied, the attribute will take precedence. avatar: Option, - /// User-defined attributes. + /// Attributes. attributes: Option>, } @@ -87,9 +91,14 @@ pub struct UpdateUserInput { id: String, email: Option, display_name: Option, + /// First name of user. Deprecated: use attribute instead. + /// If both field and corresponding attribute is supplied, the attribute will take precedence. first_name: Option, + /// Last name of user. Deprecated: use attribute instead. + /// If both field and corresponding attribute is supplied, the attribute will take precedence. last_name: Option, - /// Base64 encoded JpegPhoto. + /// Base64 encoded JpegPhoto. Deprecated: use attribute instead. + /// If both field and corresponding attribute is supplied, the attribute will take precedence. avatar: Option, /// Attribute names to remove. /// They are processed before insertions. @@ -163,6 +172,52 @@ fn unpack_attributes( }) } +/// Consolidates caller supplied user fields and attributes into a list of attributes. +/// +/// A number of user fields are internally represented as attributes, but are still also +/// available as fields on user objects. This function consolidates these fields and the +/// given attributes into a resulting attribute list. If a value is supplied for both a +/// field and the corresponding attribute, the attribute will take precedence. +fn consolidate_attributes( + attributes: Vec, + first_name: Option, + last_name: Option, + avatar: Option, +) -> Vec { + // Prepare map of the client provided attributes + let mut provided_attributes: BTreeMap = attributes + .into_iter() + .map(|x| { + ( + x.name.clone().into(), + AttributeValue { + name: x.name.to_ascii_lowercase(), + value: x.value, + }, + ) + }) + .collect::>(); + // Prepare list of fallback attribute values + let field_attrs = [ + ("first_name", first_name), + ("last_name", last_name), + ("avatar", avatar), + ]; + for (name, value) in field_attrs.into_iter() { + if let Some(val) = value { + let attr_name: AttributeName = name.into(); + provided_attributes + .entry(attr_name) + .or_insert_with(|| AttributeValue { + name: name.to_string(), + value: vec![val], + }); + } + } + // Return the values of the resulting map + provided_attributes.into_values().collect() +} + #[graphql_object(context = Context)] impl Mutation { async fn create_user( @@ -177,20 +232,18 @@ impl Mutation { .get_admin_handler() .ok_or_else(field_error_callback(&span, "Unauthorized user creation"))?; let user_id = UserId::new(&user.id); - let avatar = user - .avatar - .map(|bytes| base64::engine::general_purpose::STANDARD.decode(bytes)) - .transpose() - .context("Invalid base64 image")? - .map(JpegPhoto::try_from) - .transpose() - .context("Provided image is not a valid JPEG")?; let schema = handler.get_schema().await?; + let consolidated_attributes = consolidate_attributes( + user.attributes.unwrap_or_default(), + user.first_name, + user.last_name, + user.avatar, + ); let UnpackedAttributes { email, display_name, attributes, - } = unpack_attributes(user.attributes.unwrap_or_default(), &schema, true)?; + } = unpack_attributes(consolidated_attributes, &schema, true)?; handler .create_user(CreateUserRequest { user_id: user_id.clone(), @@ -200,9 +253,6 @@ impl Mutation { .or(email) .ok_or_else(|| anyhow!("Email is required when creating a new user"))?, display_name: user.display_name.or(display_name), - first_name: user.first_name, - last_name: user.last_name, - avatar, attributes, }) .instrument(span.clone()) @@ -253,26 +303,32 @@ impl Mutation { .get_writeable_handler(&user_id) .ok_or_else(field_error_callback(&span, "Unauthorized user update"))?; let is_admin = context.validation_result.is_admin(); - let avatar = user - .avatar - .map(|bytes| base64::engine::general_purpose::STANDARD.decode(bytes)) - .transpose() - .context("Invalid base64 image")? - .map(JpegPhoto::try_from) - .transpose() - .context("Provided image is not a valid JPEG")?; let schema = handler.get_schema().await?; - let user_insert_attributes = user.insert_attributes.unwrap_or_default(); + // Consolidate attributes and fields into a combined attribute list + let consolidated_attributes = consolidate_attributes( + user.insert_attributes.unwrap_or_default(), + user.first_name, + user.last_name, + user.avatar, + ); + // Extract any empty attributes into a list of attributes for deletion + let (delete_attrs, insert_attrs): (Vec<_>, Vec<_>) = consolidated_attributes + .into_iter() + .partition(|a| a.value == vec!["".to_string()]); + // Combine lists of attributes for removal + let mut delete_attributes: Vec = + delete_attrs.iter().map(|a| a.name.to_owned()).collect(); + delete_attributes.extend(user.remove_attributes.unwrap_or_default()); + // Unpack attributes for update let UnpackedAttributes { email, display_name, attributes: insert_attributes, - } = unpack_attributes(user_insert_attributes, &schema, is_admin)?; + } = unpack_attributes(insert_attrs, &schema, is_admin)?; let display_name = display_name.or_else(|| { // If the display name is not inserted, but removed, reset it. - user.remove_attributes + delete_attributes .iter() - .flatten() .find(|attr| *attr == "display_name") .map(|_| String::new()) }); @@ -281,12 +337,7 @@ impl Mutation { user_id, email: user.email.map(Into::into).or(email), display_name: user.display_name.or(display_name), - first_name: user.first_name, - last_name: user.last_name, - avatar, - delete_attributes: user - .remove_attributes - .unwrap_or_default() + delete_attributes: delete_attributes .into_iter() .filter(|attr| attr != "mail" && attr != "display_name") .map(Into::into) @@ -962,4 +1013,104 @@ mod tests { } } } + + #[tokio::test] + async fn test_attribute_consolidation_attr_precedence() { + let attributes = vec![ + AttributeValue { + name: "first_name".to_string(), + value: vec!["expected-first".to_string()], + }, + AttributeValue { + name: "last_name".to_string(), + value: vec!["expected-last".to_string()], + }, + AttributeValue { + name: "avatar".to_string(), + value: vec!["expected-avatar".to_string()], + }, + ]; + let res = consolidate_attributes( + attributes.clone(), + Some("overridden-first".to_string()), + Some("overridden-last".to_string()), + Some("overriden-avatar".to_string()), + ); + assert_eq!( + res, + vec![ + AttributeValue { + name: "avatar".to_string(), + value: vec!["expected-avatar".to_string()], + }, + AttributeValue { + name: "first_name".to_string(), + value: vec!["expected-first".to_string()], + }, + AttributeValue { + name: "last_name".to_string(), + value: vec!["expected-last".to_string()], + }, + ] + ); + } + + #[tokio::test] + async fn test_attribute_consolidation_field_fallback() { + let attributes = Vec::new(); + let res = consolidate_attributes( + attributes.clone(), + Some("expected-first".to_string()), + Some("expected-last".to_string()), + Some("expected-avatar".to_string()), + ); + assert_eq!( + res, + vec![ + AttributeValue { + name: "avatar".to_string(), + value: vec!["expected-avatar".to_string()], + }, + AttributeValue { + name: "first_name".to_string(), + value: vec!["expected-first".to_string()], + }, + AttributeValue { + name: "last_name".to_string(), + value: vec!["expected-last".to_string()], + }, + ] + ); + } + + #[tokio::test] + async fn test_attribute_consolidation_field_fallback_2() { + let attributes = vec![AttributeValue { + name: "First_Name".to_string(), + value: vec!["expected-first".to_string()], + }]; + let res = consolidate_attributes( + attributes.clone(), + Some("overriden-first".to_string()), + Some("expected-last".to_string()), + Some("expected-avatar".to_string()), + ); + assert_eq!( + res, + vec![ + AttributeValue { + name: "avatar".to_string(), + value: vec!["expected-avatar".to_string()], + }, + AttributeValue { + name: "first_name".to_string(), + value: vec!["expected-first".to_string()], + }, + AttributeValue { + name: "last_name".to_string(), + value: vec!["expected-last".to_string()], + }, + ] + ); + } } diff --git a/server/src/infra/ldap_handler.rs b/server/src/infra/ldap_handler.rs index 35e8b4f..264485a 100644 --- a/server/src/infra/ldap_handler.rs +++ b/server/src/infra/ldap_handler.rs @@ -1,5 +1,6 @@ use crate::{ domain::{ + deserialize, handler::{BackendHandler, BindRequest, LoginHandler, ReadSchemaBackendHandler}, ldap::{ error::{LdapError, LdapResult}, @@ -27,7 +28,7 @@ use ldap3_proto::proto::{ }; use lldap_domain::{ requests::CreateUserRequest, - types::{AttributeName, Email, Group, JpegPhoto, UserAndGroups, UserId}, + types::{AttributeName, AttributeType, AttributeValue, Email, Group, UserAndGroups, UserId}, }; use std::collections::HashMap; use tracing::{debug, instrument, warn}; @@ -769,6 +770,39 @@ impl LdapHandler = Vec::new(); + if let Some(first_name) = get_attribute("givenname").transpose()? { + new_user_attributes.push(make_encoded_attribute( + "first_name", + AttributeType::String, + first_name, + )?); + } + if let Some(last_name) = get_attribute("sn").transpose()? { + new_user_attributes.push(make_encoded_attribute( + "last_name", + AttributeType::String, + last_name, + )?); + } + if let Some(avatar) = get_attribute("avatar").transpose()? { + new_user_attributes.push(make_encoded_attribute( + "avatar", + AttributeType::JpegPhoto, + avatar, + )?); + } backend_handler .create_user(CreateUserRequest { user_id, @@ -779,18 +813,7 @@ impl LdapHandler