Keep a copy of external principal data to support roles and OAuth

7 files changed, 457 insertions, 366 deletions

diff --git a/crates/directory/src/backend/internal/manage.rs b/crates/directory/src/backend/internal/manage.rs
index de717dce..39f9ba54 100644
--- a/crates/directory/src/backend/internal/manage.rs
+++ b/crates/directory/src/backend/internal/manage.rs
@@ -33,12 +33,19 @@ pub struct PrincipalList {
     pub total: u64,
 }
 
+pub struct UpdatePrincipal<'x> {
+    query: QueryBy<'x>,
+    changes: Vec<PrincipalUpdate>,
+    tenant_id: Option<u32>,
+    validate: bool,
+}
+
 #[allow(async_fn_in_trait)]
 pub trait ManageDirectory: Sized {
     async fn get_principal_id(&self, name: &str) -> trc::Result<Option<u32>>;
     async fn get_principal_info(&self, name: &str) -> trc::Result<Option<PrincipalInfo>>;
     async fn get_or_create_principal_id(&self, name: &str, typ: Type) -> trc::Result<u32>;
-    async fn get_principal_name(&self, principal_id: u32) -> trc::Result<Option<String>>;
+    async fn get_principal(&self, principal_id: u32) -> trc::Result<Option<Principal>>;
     async fn get_member_of(&self, principal_id: u32) -> trc::Result<Vec<MemberOf>>;
     async fn get_members(&self, principal_id: u32) -> trc::Result<Vec<u32>>;
     async fn create_principal(
@@ -46,12 +53,7 @@ pub trait ManageDirectory: Sized {
         principal: Principal,
         tenant_id: Option<u32>,
     ) -> trc::Result<u32>;
-    async fn update_principal(
-        &self,
-        by: QueryBy<'_>,
-        changes: Vec<PrincipalUpdate>,
-        tenant_id: Option<u32>,
-    ) -> trc::Result<()>;
+    async fn update_principal(&self, params: UpdatePrincipal<'_>) -> trc::Result<()>;
     async fn delete_principal(&self, by: QueryBy<'_>) -> trc::Result<()>;
     async fn list_principals(
         &self,
@@ -76,12 +78,11 @@ pub trait ManageDirectory: Sized {
 }
 
 impl ManageDirectory for Store {
-    async fn get_principal_name(&self, principal_id: u32) -> trc::Result<Option<String>> {
+    async fn get_principal(&self, principal_id: u32) -> trc::Result<Option<Principal>> {
         self.get_value::<Principal>(ValueKey::from(ValueClass::Directory(
             DirectoryClass::Principal(principal_id),
         )))
         .await
-        .map(|v| v.and_then(|mut v| v.take_str(PrincipalField::Name)))
         .caused_by(trc::location!())
     }
 
@@ -130,6 +131,25 @@ impl ManageDirectory for Store {
                     .with_field(PrincipalField::Name, name.to_string()),
                 );
 
+            // Add default user role
+            if typ == Type::Individual {
+                batch
+                    .set(
+                        ValueClass::Directory(DirectoryClass::MemberOf {
+                            principal_id: MaybeDynamicId::Dynamic(0),
+                            member_of: MaybeDynamicId::Static(ROLE_USER),
+                        }),
+                        vec![Type::Role as u8],
+                    )
+                    .set(
+                        ValueClass::Directory(DirectoryClass::Members {
+                            principal_id: MaybeDynamicId::Static(ROLE_USER),
+                            has_member: MaybeDynamicId::Dynamic(0),
+                        }),
+                        vec![],
+                    );
+            }
+
             match self
                 .write(batch.build())
                 .await
@@ -589,13 +609,8 @@ impl ManageDirectory for Store {
         Ok(())
     }
 
-    async fn update_principal(
-        &self,
-        by: QueryBy<'_>,
-        changes: Vec<PrincipalUpdate>,
-        tenant_id: Option<u32>,
-    ) -> trc::Result<()> {
-        let principal_id = match by {
+    async fn update_principal(&self, params: UpdatePrincipal<'_>) -> trc::Result<()> {
+        let principal_id = match params.query {
             QueryBy::Name(name) => self
                 .get_principal_id(name)
                 .await
@@ -604,6 +619,9 @@ impl ManageDirectory for Store {
             QueryBy::Id(principal_id) => principal_id,
             QueryBy::Credentials(_) => unreachable!(),
         };
+        let changes = params.changes;
+        let tenant_id = params.tenant_id;
+        let validate = params.validate;
 
         // Fetch principal
         let mut principal = self
@@ -911,16 +929,21 @@ impl ManageDirectory for Store {
                         .collect::<Vec<_>>();
                     for email in &emails {
                         if !principal.inner.has_str_value(PrincipalField::Emails, email) {
-                            if self.rcpt(email).await.caused_by(trc::location!())? {
-                                return Err(err_exists(PrincipalField::Emails, email.to_string()));
-                            }
-                            if let Some(domain) = email.split('@').nth(1) {
-                                if !self
-                                    .is_local_domain(domain)
-                                    .await
-                                    .caused_by(trc::location!())?
-                                {
-                                    return Err(not_found(domain.to_string()));
+                            if validate {
+                                if self.rcpt(email).await.caused_by(trc::location!())? {
+                                    return Err(err_exists(
+                                        PrincipalField::Emails,
+                                        email.to_string(),
+                                    ));
+                                }
+                                if let Some(domain) = email.split('@').nth(1) {
+                                    if !self
+                                        .is_local_domain(domain)
+                                        .await
+                                        .caused_by(trc::location!())?
+                                    {
+                                        return Err(not_found(domain.to_string()));
+                                    }
                                 }
                             }
                             batch.set(
@@ -952,16 +975,18 @@ impl ManageDirectory for Store {
                         .inner
                         .has_str_value(PrincipalField::Emails, &email)
                     {
-                        if self.rcpt(&email).await.caused_by(trc::location!())? {
-                            return Err(err_exists(PrincipalField::Emails, email));
-                        }
-                        if let Some(domain) = email.split('@').nth(1) {
-                            if !self
-                                .is_local_domain(domain)
-                                .await
-                                .caused_by(trc::location!())?
-                            {
-                                return Err(not_found(domain.to_string()));
+                        if validate {
+                            if self.rcpt(&email).await.caused_by(trc::location!())? {
+                                return Err(err_exists(PrincipalField::Emails, email));
+                            }
+                            if let Some(domain) = email.split('@').nth(1) {
+                                if !self
+                                    .is_local_domain(domain)
+                                    .await
+                                    .caused_by(trc::location!())?
+                                {
+                                    return Err(not_found(domain.to_string()));
+                                }
                             }
                         }
                         batch.set(
@@ -1564,9 +1589,10 @@ impl ManageDirectory for Store {
                         }
                         principal_id => {
                             if let Some(name) = self
-                                .get_principal_name(principal_id)
+                                .get_principal(principal_id)
                                 .await
                                 .caused_by(trc::location!())?
+                                .and_then(|mut p| p.take_str(PrincipalField::Name))
                             {
                                 principal.append_str(field, name);
                             }
@@ -1651,9 +1677,10 @@ impl ManageDirectory for Store {
         if let Some(tenant_id) = principal.take_int(PrincipalField::Tenant) {
             if fields.is_empty() || fields.contains(&PrincipalField::Tenant) {
                 if let Some(name) = self
-                    .get_principal_name(tenant_id as u32)
+                    .get_principal(tenant_id as u32)
                     .await
                     .caused_by(trc::location!())?
+                    .and_then(|mut p| p.take_str(PrincipalField::Name))
                 {
                     principal.set(PrincipalField::Tenant, name);
                 }
@@ -1723,6 +1750,41 @@ impl From<Principal> for MaybeDynamicValue {
     }
 }
 
+impl<'x> UpdatePrincipal<'x> {
+    pub fn by_id(id: u32) -> Self {
+        Self {
+            query: QueryBy::Id(id),
+            changes: Vec::new(),
+            validate: true,
+            tenant_id: None,
+        }
+    }
+
+    pub fn by_name(name: &'x str) -> Self {
+        Self {
+            query: QueryBy::Name(name),
+            changes: Vec::new(),
+            validate: true,
+            tenant_id: None,
+        }
+    }
+
+    pub fn with_tenant(mut self, tenant_id: Option<u32>) -> Self {
+        self.tenant_id = tenant_id;
+        self
+    }
+
+    pub fn with_updates(mut self, changes: Vec<PrincipalUpdate>) -> Self {
+        self.changes = changes;
+        self
+    }
+
+    pub fn no_validate(mut self) -> Self {
+        self.validate = false;
+        self
+    }
+}
+
 fn validate_member_of(
     field: PrincipalField,
     typ: Type,
diff --git a/crates/directory/src/backend/ldap/lookup.rs b/crates/directory/src/backend/ldap/lookup.rs
index 5cc2a3d6..f238c7b2 100644
--- a/crates/directory/src/backend/ldap/lookup.rs
+++ b/crates/directory/src/backend/ldap/lookup.rs
@@ -9,7 +9,11 @@ use mail_send::Credentials;
 use trc::AddContext;
 
 use crate::{
-    backend::internal::{manage::ManageDirectory, PrincipalField},
+    backend::internal::{
+        lookup::DirectoryStore,
+        manage::{self, ManageDirectory, UpdatePrincipal},
+        PrincipalField,
+    },
     IntoError, Principal, QueryBy, Type, ROLE_ADMIN, ROLE_USER,
 };
 
@@ -22,35 +26,38 @@ impl LdapDirectory {
         return_member_of: bool,
     ) -> trc::Result<Option<Principal>> {
         let mut conn = self.pool.get().await.map_err(|err| err.into_error())?;
-        let mut account_id = None;
-        let account_name;
 
-        let principal = match by {
+        let (mut external_principal, stored_principal) = match by {
             QueryBy::Name(username) => {
-                account_name = username.to_string();
-
                 if let Some(principal) = self
                     .find_principal(&mut conn, &self.mappings.filter_name.build(username))
                     .await?
                 {
-                    principal
+                    (
+                        principal.with_field(PrincipalField::Name, username.to_string()),
+                        None,
+                    )
                 } else {
                     return Ok(None);
                 }
             }
             QueryBy::Id(uid) => {
-                if let Some(username) = self.data_store.get_principal_name(uid).await? {
-                    account_name = username;
-                } else {
-                    return Ok(None);
-                }
-                account_id = Some(uid);
-
-                if let Some(principal) = self
-                    .find_principal(&mut conn, &self.mappings.filter_name.build(&account_name))
+                if let Some(stored_principal_) = self
+                    .data_store
+                    .query(QueryBy::Id(uid), return_member_of)
                     .await?
                 {
-                    principal
+                    if let Some(principal) = self
+                        .find_principal(
+                            &mut conn,
+                            &self.mappings.filter_name.build(stored_principal_.name()),
+                        )
+                        .await?
+                    {
+                        (principal, Some(stored_principal_))
+                    } else {
+                        return Ok(None);
+                    }
                 } else {
                     return Ok(None);
                 }
@@ -61,7 +68,6 @@ impl LdapDirectory {
                     Credentials::OAuthBearer { token } => (token, token),
                     Credentials::XOauth2 { username, secret } => (username, secret),
                 };
-                account_name = username.to_string();
 
                 if let Some(auth_bind) = &self.auth_bind {
                     let (conn, mut ldap) = LdapConnAsync::with_settings(
@@ -91,7 +97,10 @@ impl LdapDirectory {
                         .find_principal(&mut ldap, &self.mappings.filter_name.build(username))
                         .await
                     {
-                        Ok(Some(principal)) => principal,
+                        Ok(Some(principal)) => (
+                            principal.with_field(PrincipalField::Name, username.to_string()),
+                            None,
+                        ),
                         Err(err)
                             if err.matches(trc::EventType::Store(trc::StoreEvent::LdapError))
                                 && err
@@ -109,7 +118,10 @@ impl LdapDirectory {
                     .await?
                 {
                     if principal.verify_secret(secret).await? {
-                        principal
+                        (
+                            principal.with_field(PrincipalField::Name, username.to_string()),
+                            None,
+                        )
                     } else {
                         return Ok(None);
                     }
@@ -118,27 +130,12 @@ impl LdapDirectory {
                 }
             }
         };
-        let mut principal = principal;
 
-        // Obtain account ID if not available
-        if let Some(account_id) = account_id {
-            principal.id = account_id;
-        } else {
-            principal.id = self
-                .data_store
-                .get_or_create_principal_id(&account_name, Type::Individual)
-                .await?;
-        }
-        principal.append_str(PrincipalField::Name, account_name);
-
-        if return_member_of {
-            // Obtain groups
-            if principal.has_field(PrincipalField::MemberOf) {
-                let mut member_of = Vec::new();
-                for mut name in principal
-                    .take_str_array(PrincipalField::MemberOf)
-                    .unwrap_or_default()
-                {
+        // Query groups
+        match external_principal.take_str_array(PrincipalField::MemberOf) {
+            Some(names) if return_member_of => {
+                let mut member_of = Vec::with_capacity(names.len());
+                for mut name in names {
                     if name.contains('=') {
                         let (rs, _res) = conn
                             .search(
@@ -174,35 +171,39 @@ impl LdapDirectory {
                 }
 
                 // Map ids
-                principal.set(PrincipalField::MemberOf, member_of);
+                external_principal.set(PrincipalField::MemberOf, member_of);
             }
+            _ => (),
+        }
 
-            // Obtain roles
-            let mut did_role_cleanup = false;
-            for member in self
+        // Obtain account ID if not available
+        let mut principal = if let Some(stored_principal) = stored_principal {
+            stored_principal
+        } else {
+            let id = self
                 .data_store
-                .get_member_of(principal.id)
+                .get_or_create_principal_id(external_principal.name(), Type::Individual)
+                .await
+                .caused_by(trc::location!())?;
+
+            self.data_store
+                .query(QueryBy::Id(id), return_member_of)
                 .await
                 .caused_by(trc::location!())?
-            {
-                match member.typ {
-                    Type::List => {
-                        principal.append_int(PrincipalField::Lists, member.principal_id);
-                    }
-                    Type::Role => {
-                        if !did_role_cleanup {
-                            principal.remove(PrincipalField::Roles);
-                            did_role_cleanup = true;
-                        }
-                        principal.append_int(PrincipalField::Roles, member.principal_id);
-                    }
-                    _ => {
-                        principal.append_int(PrincipalField::MemberOf, member.principal_id);
-                    }
-                }
-            }
-        } else if principal.has_field(PrincipalField::MemberOf) {
-            principal.remove(PrincipalField::MemberOf);
+                .ok_or_else(|| manage::not_found(id).caused_by(trc::location!()))?
+        };
+
+        // Keep the internal store up to date with the LDAP server
+        let changes = principal.update_external(external_principal);
+        if !changes.is_empty() {
+            self.data_store
+                .update_principal(
+                    UpdatePrincipal::by_id(principal.id)
+                        .with_updates(changes)
+                        .no_validate(),
+                )
+                .await
+                .caused_by(trc::location!())?;
         }
 
         Ok(Some(principal))
diff --git a/crates/directory/src/backend/sql/lookup.rs b/crates/directory/src/backend/sql/lookup.rs
index 7e2607cb..79ab78b8 100644
--- a/crates/directory/src/backend/sql/lookup.rs
+++ b/crates/directory/src/backend/sql/lookup.rs
@@ -9,7 +9,11 @@ use store::{NamedRows, Rows, Value};
 use trc::AddContext;
 
 use crate::{
-    backend::internal::{manage::ManageDirectory, PrincipalField, PrincipalValue},
+    backend::internal::{
+        lookup::DirectoryStore,
+        manage::{self, ManageDirectory, UpdatePrincipal},
+        PrincipalField, PrincipalValue,
+    },
     Principal, QueryBy, Type, ROLE_ADMIN, ROLE_USER,
 };
 
@@ -21,144 +25,117 @@ impl SqlDirectory {
         by: QueryBy<'_>,
         return_member_of: bool,
     ) -> trc::Result<Option<Principal>> {
-        let mut account_id = None;
-        let account_name;
-        let mut secret = None;
-
-        let result = match by {
-            QueryBy::Name(username) => {
-                account_name = username.to_string();
-
-                self.store
-                    .query::<NamedRows>(&self.mappings.query_name, vec![username.into()])
-                    .await
+        let (external_principal, stored_principal) = match by {
+            QueryBy::Name(username) => (
+                self.mappings
+                    .row_to_principal(
+                        self.store
+                            .query::<NamedRows>(&self.mappings.query_name, vec![username.into()])
+                            .await
+                            .caused_by(trc::location!())?,
+                    )
                     .caused_by(trc::location!())?
-            }
+                    .map(|p| p.with_field(PrincipalField::Name, username.to_string())),
+                None,
+            ),
             QueryBy::Id(uid) => {
-                if let Some(username) = self
+                if let Some(principal) = self
                     .data_store
-                    .get_principal_name(uid)
+                    .query(QueryBy::Id(uid), return_member_of)
                     .await
                     .caused_by(trc::location!())?
                 {
-                    account_name = username;
+                    (
+                        self.mappings
+                            .row_to_principal(
+                                self.store
+                                    .query::<NamedRows>(
+                                        &self.mappings.query_name,
+                                        vec![principal.name().into()],
+                                    )
+                                    .await
+                                    .caused_by(trc::location!())?,
+                            )
+                            .caused_by(trc::location!())?,
+                        Some(principal),
+                    )
                 } else {
                     return Ok(None);
                 }
-                account_id = Some(uid);
-
-                self.store
-                    .query::<NamedRows>(
-                        &self.mappings.query_name,
-                        vec![account_name.clone().into()],
-                    )
-                    .await
-                    .caused_by(trc::location!())?
             }
             QueryBy::Credentials(credentials) => {
-                let (username, secret_) = match credentials {
+                let (username, secret) = match credentials {
                     Credentials::Plain { username, secret } => (username, secret),
                     Credentials::OAuthBearer { token } => (token, token),
                     Credentials::XOauth2 { username, secret } => (username, secret),
                 };
-                account_name = username.to_string();
-                secret = secret_.into();
 
-                self.store
-                    .query::<NamedRows>(&self.mappings.query_name, vec![username.into()])
-                    .await
+                match self
+                    .mappings
+                    .row_to_principal(
+                        self.store
+                            .query::<NamedRows>(&self.mappings.query_name, vec![username.into()])
+                            .await
+                            .caused_by(trc::location!())?,
+                    )
                     .caused_by(trc::location!())?
+                {
+                    Some(principal)
+                        if principal
+                            .verify_secret(secret)
+                            .await
+                            .caused_by(trc::location!())? =>
+                    {
+                        (
+                            Some(principal.with_field(PrincipalField::Name, username.to_string())),
+                            None,
+                        )
+                    }
+                    _ => (None, None),
+                }
             }
         };
 
-        if result.rows.is_empty() {
-            return Ok(None);
-        }
-
-        // Map row to principal
-        let mut principal = self
-            .mappings
-            .row_to_principal(result)
-            .caused_by(trc::location!())?;
-
-        // Validate password
-        if let Some(secret) = secret {
-            if !principal
-                .verify_secret(secret)
-                .await
-                .caused_by(trc::location!())?
-            {
-                return Ok(None);
-            }
-        }
-
-        // Obtain account ID if not available
-        if let Some(account_id) = account_id {
-            principal.id = account_id;
+        let mut external_principal = if let Some(external_principal) = external_principal {
+            external_principal
         } else {
-            principal.id = self
-                .data_store
-                .get_or_create_principal_id(&account_name, Type::Individual)
-                .await
-                .caused_by(trc::location!())?;
-        }
-        principal.set(PrincipalField::Name, account_name);
+            return Ok(None);
+        };
 
         // Obtain members
-        if return_member_of {
-            if !self.mappings.query_members.is_empty() {
-                for row in self
-                    .store
-                    .query::<Rows>(&self.mappings.query_members, vec![principal.name().into()])
-                    .await
-                    .caused_by(trc::location!())?
-                    .rows
-                {
-                    if let Some(Value::Text(account_id)) = row.values.first() {
-                        principal.append_int(
-                            PrincipalField::MemberOf,
-                            self.data_store
-                                .get_or_create_principal_id(account_id, Type::Group)
-                                .await
-                                .caused_by(trc::location!())?,
-                        );
-                    }
-                }
-            }
-
-            // Obtain roles
-            let mut did_role_cleanup = false;
-            for member in self
-                .data_store
-                .get_member_of(principal.id)
+        if return_member_of && !self.mappings.query_members.is_empty() {
+            for row in self
+                .store
+                .query::<Rows>(
+                    &self.mappings.query_members,
+                    vec![external_principal.name().into()],
+                )
                 .await
                 .caused_by(trc::location!())?
+                .rows
             {
-                match member.typ {
-                    Type::List => {
-                        principal.append_int(PrincipalField::Lists, member.principal_id);
-                    }
-                    Type::Role => {
-                        if !did_role_cleanup {
-                            principal.remove(PrincipalField::Roles);
-                            did_role_cleanup = true;
-                        }
-                        principal.append_int(PrincipalField::Roles, member.principal_id);
-                    }
-                    _ => {
-                        principal.append_int(PrincipalField::MemberOf, member.principal_id);
-                    }
+                if let Some(Value::Text(account_id)) = row.values.first() {
+                    external_principal.append_int(
+                        PrincipalField::MemberOf,
+                        self.data_store
+                            .get_or_create_principal_id(account_id, Type::Group)
+                            .await
+                            .caused_by(trc::location!())?,
+                    );
                 }
             }
         }
 
         // Obtain emails
         if !self.mappings.query_emails.is_empty() {
-            principal.set(
+            external_principal.set(
                 PrincipalField::Emails,
                 PrincipalValue::StringList(
                     self.store
-                        .query::<Rows>(&self.mappings.query_emails, vec![principal.name().into()])
+                        .query::<Rows>(
+                            &self.mappings.query_emails,
+                            vec![external_principal.name().into()],
+                        )
                         .await
                         .caused_by(trc::location!())?
                         .into(),
@@ -166,6 +143,36 @@ impl SqlDirectory {
             );
         }
 
+        // Obtain account ID if not available
+        let mut principal = if let Some(stored_principal) = stored_principal {
+            stored_principal
+        } else {
+            let id = self
+                .data_store
+                .get_or_create_principal_id(external_principal.name(), Type::Individual)
+                .await
+                .caused_by(trc::location!())?;
+
+            self.data_store
+                .query(QueryBy::Id(id), return_member_of)
+                .await
+                .caused_by(trc::location!())?
+                .ok_or_else(|| manage::not_found(id).caused_by(trc::location!()))?
+        };
+
+        // Keep the internal store up to date with the SQL server
+        let changes = principal.update_external(external_principal);
+        if !changes.is_empty() {
+            self.data_store
+                .update_principal(
+                    UpdatePrincipal::by_id(principal.id)
+                        .with_updates(changes)
+                        .no_validate(),
+                )
+                .await
+                .caused_by(trc::location!())?;
+        }
+
         Ok(Some(principal))
     }
 
@@ -233,7 +240,11 @@ impl SqlDirectory {
 }
 
 impl SqlMappings {
-    pub fn row_to_principal(&self, rows: NamedRows) -> trc::Result<Principal> {
+    pub fn row_to_principal(&self, rows: NamedRows) -> trc::Result<Option<Principal>> {
+        if rows.rows.is_empty() {
+            return Ok(None);
+        }
+
         let mut principal = Principal::default();
         let mut role = ROLE_USER;
 
@@ -271,6 +282,6 @@ impl SqlMappings {
             }
         }
 
-        Ok(principal.with_field(PrincipalField::Roles, role))
+        Ok(Some(principal.with_field(PrincipalField::Roles, role)))
     }
 }
diff --git a/crates/directory/src/core/principal.rs b/crates/directory/src/core/principal.rs
index 89d67fce..ef126329 100644
--- a/crates/directory/src/core/principal.rs
+++ b/crates/directory/src/core/principal.rs
@@ -14,7 +14,7 @@ use serde::{
 use store::U64_LEN;
 
 use crate::{
-    backend::internal::{PrincipalField, PrincipalValue},
+    backend::internal::{PrincipalField, PrincipalUpdate, PrincipalValue},
     Permission, Principal, Type, ROLE_ADMIN,
 };
 
@@ -367,6 +367,65 @@ impl Principal {
         }
     }
 
+    pub fn update_external(&mut self, mut external: Principal) -> Vec<PrincipalUpdate> {
+        let mut updates = Vec::new();
+        if let Some(name) = external.take_str(PrincipalField::Description) {
+            if self.get_str(PrincipalField::Description) != Some(name.as_str()) {
+                updates.push(PrincipalUpdate::set(
+                    PrincipalField::Description,
+                    PrincipalValue::String(name.clone()),
+                ));
+                self.set(PrincipalField::Description, name);
+            }
+        }
+
+        for field in [PrincipalField::Secrets, PrincipalField::Emails] {
+            if let Some(secrets) = external.take_str_array(field).filter(|s| !s.is_empty()) {
+                if self.get_str_array(field) != Some(secrets.as_ref()) {
+                    updates.push(PrincipalUpdate::set(
+                        field,
+                        PrincipalValue::StringList(secrets.clone()),
+                    ));
+                    self.set(field, secrets);
+                }
+            }
+        }
+
+        if let Some(quota) = external.take_int(PrincipalField::Quota) {
+            if self.get_int(PrincipalField::Quota) != Some(quota) {
+                updates.push(PrincipalUpdate::set(
+                    PrincipalField::Quota,
+                    PrincipalValue::Integer(quota),
+                ));
+                self.set(PrincipalField::Quota, quota);
+            }
+        }
+
+        // Add external members
+        if let Some(member_of) = external
+            .take_int_array(PrincipalField::MemberOf)
+            .filter(|s| !s.is_empty())
+        {
+            self.set(PrincipalField::MemberOf, member_of);
+        }
+
+        // If the principal has no roles, take the ones from the external principal
+        if let Some(member_of) = external
+            .take_int_array(PrincipalField::Roles)
+            .filter(|s| !s.is_empty())
+        {
+            if self
+                .get_int_array(PrincipalField::Roles)
+                .filter(|s| !s.is_empty())
+                .is_none()
+            {
+                self.set(PrincipalField::Roles, member_of);
+            }
+        }
+
+        updates
+    }
+
     pub fn fallback_admin(fallback_pass: impl Into<String>) -> Self {
         Principal {
             id: u32::MAX,
diff --git a/crates/jmap/src/api/management/principal.rs b/crates/jmap/src/api/management/principal.rs
index add50bfc..51994756 100644
--- a/crates/jmap/src/api/management/principal.rs
+++ b/crates/jmap/src/api/management/principal.rs
@@ -10,7 +10,7 @@ use common::auth::AccessToken;
 use directory::{
     backend::internal::{
         lookup::DirectoryStore,
-        manage::{self, not_found, ManageDirectory},
+        manage::{self, not_found, ManageDirectory, UpdatePrincipal},
         PrincipalAction, PrincipalField, PrincipalUpdate, PrincipalValue, SpecialSecrets,
     },
     DirectoryInner, Permission, Principal, QueryBy, Type,
@@ -344,14 +344,13 @@ impl JMAP {
 
                         for change in &changes {
                             match change.field {
-                                PrincipalField::Name | PrincipalField::Emails => {
-                                    needs_assert = true;
-                                }
                                 PrincipalField::Secrets => {
                                     expire_session = true;
                                     needs_assert = true;
                                 }
-                                PrincipalField::Quota
+                                PrincipalField::Name
+                                | PrincipalField::Emails
+                                | PrincipalField::Quota
                                 | PrincipalField::UsedQuota
                                 | PrincipalField::Description
                                 | PrincipalField::Type
@@ -392,9 +391,9 @@ impl JMAP {
                             .storage
                             .data
                             .update_principal(
-                                QueryBy::Id(account_id),
-                                changes,
-                                access_token.tenant.map(|t| t.id),
+                                UpdatePrincipal::by_id(account_id)
+                                    .with_updates(changes)
+                                    .with_tenant(access_token.tenant.map(|t| t.id)),
                             )
                             .await?;
 
@@ -571,9 +570,9 @@ impl JMAP {
             .storage
             .data
             .update_principal(
-                QueryBy::Id(access_token.primary_id()),
-                actions,
-                access_token.tenant.map(|t| t.id),
+                UpdatePrincipal::by_id(access_token.primary_id())
+                    .with_updates(actions)
+                    .with_tenant(access_token.tenant.map(|t| t.id)),
             )
             .await?;
 
diff --git a/tests/src/directory/internal.rs b/tests/src/directory/internal.rs
index 3cde6c15..fed8a2cb 100644
--- a/tests/src/directory/internal.rs
+++ b/tests/src/directory/internal.rs
@@ -8,7 +8,7 @@ use ahash::AHashSet;
 use directory::{
     backend::internal::{
         lookup::DirectoryStore,
-        manage::{self, ManageDirectory},
+        manage::{self, ManageDirectory, UpdatePrincipal},
         PrincipalField, PrincipalUpdate, PrincipalValue,
     },
     Principal, QueryBy, Type,
@@ -102,14 +102,12 @@ async fn internal_directory() {
         // Add an email address
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("john"),
-                    vec![PrincipalUpdate::add_item(
+                .update_principal(UpdatePrincipal::by_name("john").with_updates(vec![
+                    PrincipalUpdate::add_item(
                         PrincipalField::Emails,
                         PrincipalValue::String("john@example.org".to_string()),
-                    )],
-                    None
-                )
+                    )
+                ]))
                 .await,
             Ok(())
         );
@@ -122,14 +120,12 @@ async fn internal_directory() {
         // Using non-existent domain should fail
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("john"),
-                    vec![PrincipalUpdate::add_item(
+                .update_principal(UpdatePrincipal::by_name("john").with_updates(vec![
+                    PrincipalUpdate::add_item(
                         PrincipalField::Emails,
                         PrincipalValue::String("john@otherdomain.org".to_string()),
-                    )],
-                    None
-                )
+                    )
+                ]))
                 .await,
             Err(manage::not_found("otherdomain.org".to_string()))
         );
@@ -230,14 +226,12 @@ async fn internal_directory() {
             .unwrap();
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("list"),
-                    vec![PrincipalUpdate::set(
+                .update_principal(UpdatePrincipal::by_name("list").with_updates(vec![
+                    PrincipalUpdate::set(
                         PrincipalField::Members,
                         PrincipalValue::StringList(vec!["john".to_string(), "jane".to_string()]),
-                    )],
-                    None
-                )
+                    )
+                ]))
                 .await,
             Ok(())
         );
@@ -310,20 +304,16 @@ async fn internal_directory() {
         // Add John to the Sales and Support groups
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("john"),
-                    vec![
-                        PrincipalUpdate::add_item(
-                            PrincipalField::MemberOf,
-                            PrincipalValue::String("sales".to_string()),
-                        ),
-                        PrincipalUpdate::add_item(
-                            PrincipalField::MemberOf,
-                            PrincipalValue::String("support".to_string()),
-                        )
-                    ],
-                    None
-                )
+                .update_principal(UpdatePrincipal::by_name("john").with_updates(vec![
+                    PrincipalUpdate::add_item(
+                        PrincipalField::MemberOf,
+                        PrincipalValue::String("sales".to_string()),
+                    ),
+                    PrincipalUpdate::add_item(
+                        PrincipalField::MemberOf,
+                        PrincipalValue::String("support".to_string()),
+                    )
+                ]))
                 .await,
             Ok(())
         );
@@ -353,14 +343,12 @@ async fn internal_directory() {
         // Adding a non-existent user should fail
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("john"),
-                    vec![PrincipalUpdate::add_item(
+                .update_principal(UpdatePrincipal::by_name("john").with_updates(vec![
+                    PrincipalUpdate::add_item(
                         PrincipalField::MemberOf,
                         PrincipalValue::String("accounting".to_string()),
-                    )],
-                    None
-                )
+                    )
+                ]))
                 .await,
             Err(manage::not_found("accounting".to_string()))
         );
@@ -368,14 +356,12 @@ async fn internal_directory() {
         // Remove a member from a group
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("john"),
-                    vec![PrincipalUpdate::remove_item(
+                .update_principal(UpdatePrincipal::by_name("john").with_updates(vec![
+                    PrincipalUpdate::remove_item(
                         PrincipalField::MemberOf,
                         PrincipalValue::String("support".to_string()),
-                    )],
-                    None
-                )
+                    )
+                ]))
                 .await,
             Ok(())
         );
@@ -401,33 +387,29 @@ async fn internal_directory() {
         // Update multiple fields
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("john"),
-                    vec![
-                        PrincipalUpdate::set(
-                            PrincipalField::Name,
-                            PrincipalValue::String("john.doe".to_string())
-                        ),
-                        PrincipalUpdate::set(
-                            PrincipalField::Description,
-                            PrincipalValue::String("Johnny Doe".to_string())
-                        ),
-                        PrincipalUpdate::set(
-                            PrincipalField::Secrets,
-                            PrincipalValue::StringList(vec!["12345".to_string()])
-                        ),
-                        PrincipalUpdate::set(PrincipalField::Quota, PrincipalValue::Integer(1024)),
-                        PrincipalUpdate::remove_item(
-                            PrincipalField::Emails,
-                            PrincipalValue::String("john@example.org".to_string()),
-                        ),
-                        PrincipalUpdate::add_item(
-                            PrincipalField::Emails,
-                            PrincipalValue::String("john.doe@example.org".to_string()),
-                        )
-                    ],
-                    None
-                )
+                .update_principal(UpdatePrincipal::by_name("john").with_updates(vec![
+                    PrincipalUpdate::set(
+                        PrincipalField::Name,
+                        PrincipalValue::String("john.doe".to_string())
+                    ),
+                    PrincipalUpdate::set(
+                        PrincipalField::Description,
+                        PrincipalValue::String("Johnny Doe".to_string())
+                    ),
+                    PrincipalUpdate::set(
+                        PrincipalField::Secrets,
+                        PrincipalValue::StringList(vec!["12345".to_string()])
+                    ),
+                    PrincipalUpdate::set(PrincipalField::Quota, PrincipalValue::Integer(1024)),
+                    PrincipalUpdate::remove_item(
+                        PrincipalField::Emails,
+                        PrincipalValue::String("john@example.org".to_string()),
+                    ),
+                    PrincipalUpdate::add_item(
+                        PrincipalField::Emails,
+                        PrincipalValue::String("john.doe@example.org".to_string()),
+                    )
+                ]))
                 .await,
             Ok(())
         );
@@ -459,14 +441,12 @@ async fn internal_directory() {
         // Remove a member from a mailing list and then add it back
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("list"),
-                    vec![PrincipalUpdate::remove_item(
+                .update_principal(UpdatePrincipal::by_name("list").with_updates(vec![
+                    PrincipalUpdate::remove_item(
                         PrincipalField::Members,
                         PrincipalValue::String("john.doe".to_string()),
-                    )],
-                    None
-                )
+                    )
+                ]))
                 .await,
             Ok(())
         );
@@ -476,14 +456,12 @@ async fn internal_directory() {
         );
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("list"),
-                    vec![PrincipalUpdate::add_item(
+                .update_principal(UpdatePrincipal::by_name("list").with_updates(vec![
+                    PrincipalUpdate::add_item(
                         PrincipalField::Members,
                         PrincipalValue::String("john.doe".to_string()),
-                    )],
-                    None
-                )
+                    )
+                ]))
                 .await,
             Ok(())
         );
@@ -500,27 +478,23 @@ async fn internal_directory() {
         // Field validation
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("john.doe"),
-                    vec![PrincipalUpdate::set(
+                .update_principal(UpdatePrincipal::by_name("john.doe").with_updates(vec![
+                    PrincipalUpdate::set(
                         PrincipalField::Name,
                         PrincipalValue::String("jane".to_string())
-                    ),],
-                    None
-                )
+                    ),
+                ]))
                 .await,
             Err(manage::err_exists(PrincipalField::Name, "jane".to_string()))
         );
         assert_eq!(
             store
-                .update_principal(
-                    QueryBy::Name("john.doe"),
-                    vec![PrincipalUpdate::add_item(
+                .update_principal(UpdatePrincipal::by_name("john.doe").with_updates(vec![
+                    PrincipalUpdate::add_item(
                         PrincipalField::Emails,
                         PrincipalValue::String("jane@example.org".to_string())
-                    ),],
-                    None
-                )
+                    ),
+                ]))
                 .await,
             Err(manage::err_exists(
                 PrincipalField::Emails,
@@ -743,28 +717,24 @@ impl TestInternalDirectory for Store {
         let role = if login == "admin" { "admin" } else { "user" };
         self.create_test_domains(emails).await;
         if let Some(principal) = self.query(QueryBy::Name(login), false).await.unwrap() {
-            self.update_principal(
-                QueryBy::Id(principal.id()),
-                vec![
-                    PrincipalUpdate::set(
-                        PrincipalField::Secrets,
-                        PrincipalValue::StringList(vec![secret.to_string()]),
-                    ),
-                    PrincipalUpdate::set(
-                        PrincipalField::Description,
-                        PrincipalValue::String(name.to_string()),
-                    ),
-                    PrincipalUpdate::set(
-                        PrincipalField::Emails,
-                        PrincipalValue::StringList(emails.iter().map(|s| s.to_string()).collect()),
-                    ),
-                    PrincipalUpdate::add_item(
-                        PrincipalField::Roles,
-                        PrincipalValue::String(role.to_string()),
-                    ),
-                ],
-                None,
-            )
+            self.update_principal(UpdatePrincipal::by_id(principal.id()).with_updates(vec![
+                PrincipalUpdate::set(
+                    PrincipalField::Secrets,
+                    PrincipalValue::StringList(vec![secret.to_string()]),
+                ),
+                PrincipalUpdate::set(
+                    PrincipalField::Description,
+                    PrincipalValue::String(name.to_string()),
+                ),
+                PrincipalUpdate::set(
+                    PrincipalField::Emails,
+                    PrincipalValue::StringList(emails.iter().map(|s| s.to_string()).collect()),
+                ),
+                PrincipalUpdate::add_item(
+                    PrincipalField::Roles,
+                    PrincipalValue::String(role.to_string()),
+                ),
+            ]))
             .await
             .unwrap();
             principal.id()
@@ -841,53 +811,42 @@ impl TestInternalDirectory for Store {
     }
 
     async fn set_test_quota(&self, login: &str, quota: u32) {
-        self.update_principal(
-            QueryBy::Name(login),
-            vec![PrincipalUpdate::set(
-                PrincipalField::Quota,
-                PrincipalValue::Integer(quota as u64),
-            )],
-            None,
-        )
+        self.update_principal(UpdatePrincipal::by_name(login).with_updates(vec![
+            PrincipalUpdate::set(PrincipalField::Quota, PrincipalValue::Integer(quota as u64)),
+        ]))
         .await
         .unwrap();
     }
 
     async fn add_to_group(&self, login: &str, group: &str) {
-        self.update_principal(
-            QueryBy::Name(login),
-            vec![PrincipalUpdate::add_item(
+        self.update_principal(UpdatePrincipal::by_name(login).with_updates(vec![
+            PrincipalUpdate::add_item(
                 PrincipalField::MemberOf,
                 PrincipalValue::String(group.to_string()),
-            )],
-            None,
-        )
+            ),
+        ]))
         .await
         .unwrap();
     }
 
     async fn remove_from_group(&self, login: &str, group: &str) {
-        self.update_principal(
-            QueryBy::Name(login),
-            vec![PrincipalUpdate::remove_item(
+        self.update_principal(UpdatePrincipal::by_name(login).with_updates(vec![
+            PrincipalUpdate::remove_item(
                 PrincipalField::MemberOf,
                 PrincipalValue::String(group.to_string()),
-            )],
-            None,
-        )
+            ),
+        ]))
         .await
         .unwrap();
     }
 
     async fn remove_test_alias(&self, login: &str, alias: &str) {
-        self.update_principal(
-            QueryBy::Name(login),
-            vec![PrincipalUpdate::remove_item(
+        self.update_principal(UpdatePrincipal::by_name(login).with_updates(vec![
+            PrincipalUpdate::remove_item(
                 PrincipalField::Emails,
                 PrincipalValue::String(alias.to_string()),
-            )],
-            None,
-        )
+            ),
+        ]))
         .await
         .unwrap();
     }
diff --git a/tests/src/directory/sql.rs b/tests/src/directory/sql.rs
index 27a86047..20f2c6eb 100644
--- a/tests/src/directory/sql.rs
+++ b/tests/src/directory/sql.rs
@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-SEL
  */
 
-use directory::{backend::internal::manage::ManageDirectory, QueryBy, Type, ROLE_ADMIN, ROLE_USER};
+use directory::{backend::internal::manage::ManageDirectory, QueryBy, Type, ROLE_USER};
 use mail_send::Credentials;
 use store::{LookupStore, Store};
 
@@ -181,7 +181,7 @@ async fn sql_directory() {
                 description: "Administrator".to_string().into(),
                 secrets: vec!["very_secret".to_string()],
                 typ: Type::Individual,
-                roles: vec![ROLE_ADMIN.to_string()],
+                roles: vec![ROLE_USER.to_string()],
                 ..Default::default()
             }
         );