From 4c6cfeee9e8148dbc5664852f86d7d1a9874cff9 Mon Sep 17 00:00:00 2001 From: Simon Broeng Jensen Date: Mon, 3 Feb 2025 21:41:30 +0100 Subject: [PATCH] server: remove deprecated fields from CreateUserRequest The fields first_name, last_name, and avatar have all been moved to regular attributes in the database, and are available through the GraphQL API as such as well. This commit removes the legacy fields for each on the internal CreateUserRequest type, leaving these to only be updateable through attributes. The fields are still available in the GraphQL CreateUserInput type, preserving backwards compatiblity, and if set, they will be used for the corresponding attribute values. If both fields and attributes are set, the values given through attributes will superceed the fields, and be used. This change also fixes a bug, where creation of a user would fail if either of these attributes were set as both attribute and field, as it would attempt to insert the attribute twice, violating a unique constraint in the database. --- crates/domain/src/requests.rs | 8 +- schema.graphql | 32 ++- server/src/domain/sql_backend_handler.rs | 15 +- server/src/domain/sql_user_backend_handler.rs | 105 ++++----- server/src/infra/graphql/mutation.rs | 223 +++++++++++++++--- server/src/infra/ldap_handler.rs | 49 +++- 6 files changed, 303 insertions(+), 129 deletions(-) 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