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.
This commit is contained in:
Simon Broeng Jensen
2025-02-03 21:41:30 +01:00
committed by nitnelave
parent 37a683dcb2
commit 4c6cfeee9e
6 changed files with 303 additions and 129 deletions
+1 -7
View File
@@ -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<String>,
pub first_name: Option<String>,
pub last_name: Option<String>,
pub avatar: Option<JpegPhoto>,
pub attributes: Vec<AttributeValue>,
}
@@ -22,9 +19,6 @@ pub struct UpdateUserRequest {
pub user_id: UserId,
pub email: Option<Email>,
pub display_name: Option<String>,
pub first_name: Option<String>,
pub last_name: Option<String>,
pub avatar: Option<JpegPhoto>,
pub delete_attributes: Vec<AttributeName>,
pub insert_attributes: Vec<AttributeValue>,
}
+25 -7
View File
@@ -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.
+11 -4
View File
@@ -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();
+41 -60
View File
@@ -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<String>| 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 {
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();
+187 -36
View File
@@ -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<String>,
display_name: Option<String>,
/// First name of user. Deprecated: use attribute instead.
/// If both field and corresponding attribute is supplied, the attribute will take precedence.
first_name: Option<String>,
/// Last name of user. Deprecated: use attribute instead.
/// If both field and corresponding attribute is supplied, the attribute will take precedence.
last_name: Option<String>,
/// 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<String>,
/// User-defined attributes.
/// Attributes.
attributes: Option<Vec<AttributeValue>>,
}
@@ -87,9 +91,14 @@ pub struct UpdateUserInput {
id: String,
email: Option<String>,
display_name: Option<String>,
/// First name of user. Deprecated: use attribute instead.
/// If both field and corresponding attribute is supplied, the attribute will take precedence.
first_name: Option<String>,
/// Last name of user. Deprecated: use attribute instead.
/// If both field and corresponding attribute is supplied, the attribute will take precedence.
last_name: Option<String>,
/// 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<String>,
/// 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<AttributeValue>,
first_name: Option<String>,
last_name: Option<String>,
avatar: Option<String>,
) -> Vec<AttributeValue> {
// Prepare map of the client provided attributes
let mut provided_attributes: BTreeMap<AttributeName, AttributeValue> = attributes
.into_iter()
.map(|x| {
(
x.name.clone().into(),
AttributeValue {
name: x.name.to_ascii_lowercase(),
value: x.value,
},
)
})
.collect::<BTreeMap<_, _>>();
// 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<Handler>)]
impl<Handler: BackendHandler> Mutation<Handler> {
async fn create_user(
@@ -177,20 +232,18 @@ impl<Handler: BackendHandler> Mutation<Handler> {
.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<Handler: BackendHandler> Mutation<Handler> {
.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<Handler: BackendHandler> Mutation<Handler> {
.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<String> =
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<Handler: BackendHandler> Mutation<Handler> {
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()],
},
]
);
}
}
+36 -13
View File
@@ -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<Backend: BackendHandler + LoginHandler + OpaqueHandler> LdapHandler<Backend
.map(Vec::as_slice)
.map(decode_attribute_value)
};
let make_encoded_attribute = |name: &str, typ: AttributeType, value: String| {
Ok(AttributeValue {
name: AttributeName::from(name),
value: deserialize::deserialize_attribute_value(&[value], typ, false).map_err(
|e| LdapError {
code: LdapResultCode::ConstraintViolation,
message: format!("Invalid attribute value: {}", e),
},
)?,
})
};
let mut new_user_attributes: Vec<AttributeValue> = 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<Backend: BackendHandler + LoginHandler + OpaqueHandler> LdapHandler<Backend
.unwrap_or_default(),
),
display_name: get_attribute("cn").transpose()?,
first_name: get_attribute("givenname").transpose()?,
last_name: get_attribute("sn").transpose()?,
avatar: attributes
.get("avatar")
.map(Vec::as_slice)
.map(JpegPhoto::try_from)
.transpose()
.map_err(|e| LdapError {
code: LdapResultCode::ConstraintViolation,
message: format!("Invalid JPEG photo: {:#?}", e),
})?,
..Default::default()
attributes: new_user_attributes,
})
.await
.map_err(|e| LdapError {