diff --git a/Cargo.lock b/Cargo.lock index c99e5393..0b879323 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4915,6 +4915,7 @@ version = "0.5.14" dependencies = [ "async-bb8-diesel", "async-trait", + "base64 0.23.1", "bb8", "chrono", "diesel", diff --git a/README.md b/README.md index 4771fd0e..3c471706 100644 --- a/README.md +++ b/README.md @@ -85,8 +85,6 @@ The following endpoints are injected by the `inject_endpoints` macro: | GET | /.well-known/jwks.json | Retrieve JWKS for verifying access tokens | | GET | /self | View information about the calling user | | GET | /api-user/{identifier} | View information about a specific user | -| POST | /api-user | Create a new user | -| POST | /api-user/{identifier} | Update information on an existing user | | GET | /api-user/{identifier}/token | View all API tokens for an existing user | | POST | /api-user/{identifier}/token | Create a new API token for an existing user | | GET | /api-user/{identifier}/token/{token_identifier} | View an existing API token | @@ -97,7 +95,8 @@ The following endpoints are injected by the `inject_endpoints` macro: | POST | /api-user-provider/{identifier}/link-token | TBD | | GET | /group | View all groups | | POST | /group | Create a new group | -| PUT | /group/{group_id} | Update an existing group | +| POST | /group/{group_id}/permission | Add a single permission to an existing group | +| DELETE | /group/{group_id}/permission | Remove a single permission from an existing group | | DELETE | /group/{group_id} | Delete an existing group | | GET | /mapper | View all mappers | | POST | /mapper | Create a new mapper | diff --git a/v-api-permission-derive/src/lib.rs b/v-api-permission-derive/src/lib.rs index ae614825..f482e06f 100644 --- a/v-api-permission-derive/src/lib.rs +++ b/v-api-permission-derive/src/lib.rs @@ -953,6 +953,8 @@ fn permission_storage_trait_tokens( ) -> proc_macro2::TokenStream { let contract_tokens = permission_storage_contract_tokens(permission_type, contract_settings); let expand_tokens = permission_storage_expand_tokens(permission_type, expand_settings.clone()); + let expand_value_tokens = + permission_storage_expand_value_tokens(permission_type, &expand_settings); let implies_tokens = permission_storage_implies_tokens( permission_type, implies_settings, @@ -964,11 +966,49 @@ fn permission_storage_trait_tokens( impl v_model::permissions::PermissionStorage for #permission_type { #contract_tokens #expand_tokens + #expand_value_tokens #implies_tokens } } } +/// Generates `expand_value`, which splits a permission that carries a set of values into one +/// permission per value. +/// +/// Only iter expansions without an external source qualify. These are permissions who hold all of +/// the direct values required for expansion. Expansions that depend on an external source are left +/// alone. +fn permission_storage_expand_value_tokens( + permission_type: &Ident, + expand_settings: &[(Variant, ExpandSettings)], +) -> proc_macro2::TokenStream { + let branches = expand_settings + .iter() + .filter(|(_, setting)| matches!(setting.kind, ExpandKind::Iter) && setting.source.is_none()) + .map(|(variant, setting)| { + let set_ident = &variant.ident; + let value_ident = &setting.variant; + quote! { + #permission_type::#set_ident(values) => Some( + values + .iter() + .map(|value| #permission_type::#value_ident(value.clone())) + .collect() + ), + } + }) + .collect::>(); + + quote! { + fn expand_value(&self) -> Option> { + match self { + #(#branches)* + _ => None, + } + } + } +} + fn permission_storage_contract_tokens( permission_type: &Ident, contract_settings: Vec<(Variant, ContractSettings)>, diff --git a/v-api-permission-derive/tests/derive.rs b/v-api-permission-derive/tests/derive.rs index 9fc47cdf..2d590ad8 100644 --- a/v-api-permission-derive/tests/derive.rs +++ b/v-api-permission-derive/tests/derive.rs @@ -603,6 +603,7 @@ fn joined_actor( created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, } } diff --git a/v-api/src/context/group.rs b/v-api/src/context/group.rs index 1b27a559..445579af 100644 --- a/v-api/src/context/group.rs +++ b/v-api/src/context/group.rs @@ -6,7 +6,7 @@ use newtype_uuid::TypedUuid; use std::sync::Arc; use v_model::{ AccessGroup, AccessGroupId, NewAccessGroup, - permissions::Caller, + permissions::{Caller, PermissionStorage, Permissions}, storage::{AccessGroupFilter, AccessGroupStore, ListPagination, StoreError}, }; @@ -16,6 +16,13 @@ use crate::{ response::{OptionalResource, ResourceResult, resource_conflict, resource_restricted}, }; +/// A change to a stored group. Each kind of change carries what is needed to check the caller's +/// authority to make it. +enum GroupChange<'a, T> { + AddPermission(T), + RemovePermission(&'a T), +} + #[derive(Clone)] pub struct GroupContext { storage: Arc>, @@ -126,24 +133,94 @@ where } } - pub async fn update_group( + /// Adds a permission to a group. + pub async fn add_permission_to_group( &self, caller: &Caller, - group: NewAccessGroup, + group_id: &TypedUuid, + permission: T, ) -> ResourceResult, StoreError> { - if self.is_preset(&group.id) { - return resource_conflict(); - } + self.modify_group(caller, group_id, GroupChange::AddPermission(permission)) + .await + } - if caller.can(&VPermission::ManageGroup(group.id).into()) - && caller.can_grant_all(&group.permissions) - { - Ok(AccessGroupStore::upsert(&*self.storage, &group).await?) + /// Removes a permission from a group. + pub async fn remove_permission_from_group( + &self, + caller: &Caller, + group_id: &TypedUuid, + permission: &T, + ) -> ResourceResult, StoreError> { + self.modify_group(caller, group_id, GroupChange::RemovePermission(permission)) + .await + } + + /// Applies a change to a stored group. + async fn modify_group( + &self, + caller: &Caller, + group_id: &TypedUuid, + change: GroupChange<'_, T>, + ) -> ResourceResult, StoreError> { + if self.is_preset(group_id) { + resource_conflict() + } else if Self::can_make_change(caller, group_id, &change) { + let current = self.get_group(caller, group_id).await?; + + let mut update = NewAccessGroup { + id: current.id, + name: current.name, + permissions: current.permissions.expand_values(), + }; + Self::apply_change(&mut update, change); + update.permissions = ::contract(&update.permissions); + + match AccessGroupStore::update(&*self.storage, &update, current.generation).await? { + Some(updated) => Ok(updated), + None => { + tracing::info!( + ?group_id, + generation = current.generation, + "Group was written by someone else since it was read" + ); + resource_conflict() + } + } } else { resource_restricted() } } + /// Returns true if the caller has the authority to make the change to the group + fn can_make_change( + caller: &Caller, + group_id: &TypedUuid, + change: &GroupChange, + ) -> bool { + let can_manage = caller.can(&VPermission::ManageGroup(*group_id).into()); + + match change { + GroupChange::AddPermission(permission) => can_manage && caller.can_grant(permission), + GroupChange::RemovePermission(permission) => can_manage && caller.can_grant(permission), + } + } + + fn apply_change(update: &mut NewAccessGroup, change: GroupChange) { + match change { + GroupChange::AddPermission(permission) => { + for value in Permissions::from_iter([permission]) + .expand_values() + .into_iter() + { + update.permissions.insert(value); + } + } + GroupChange::RemovePermission(permission) => { + update.permissions = update.permissions.without(permission); + } + } + } + pub async fn delete_group( &self, caller: &Caller, @@ -167,7 +244,10 @@ where mod tests { use chrono::Utc; use newtype_uuid::{GenericUuid, TypedUuid}; - use std::{collections::BTreeSet, sync::Arc}; + use std::{ + collections::BTreeSet, + sync::{Arc, Mutex}, + }; use uuid::Uuid; use v_model::{ AccessGroup, AccessGroupSource, NewAccessGroup, @@ -190,6 +270,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, } } @@ -202,6 +283,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, } } @@ -290,17 +372,15 @@ mod tests { let ctx = ctx_with_preset(vec![]); let preset = preset_group("admins"); - let update = ctx - .update_group( - &admin_caller(), - NewAccessGroup { - id: preset.id, - name: "renamed".to_string(), - permissions: Permissions::new(), - }, - ) + let add = ctx + .add_permission_to_group(&admin_caller(), &preset.id, VPermission::CreateGroup) + .await; + assert!(matches!(add, Err(ResourceError::Conflict))); + + let remove = ctx + .remove_permission_from_group(&admin_caller(), &preset.id, &VPermission::CreateGroup) .await; - assert!(matches!(update, Err(ResourceError::Conflict))); + assert!(matches!(remove, Err(ResourceError::Conflict))); let delete = ctx.delete_group(&admin_caller(), &preset.id).await; assert!(matches!(delete, Err(ResourceError::Conflict))); @@ -324,4 +404,136 @@ mod tests { assert!(matches!(create, Err(ResourceError::Conflict))); } + + /// The permissions written by each successful group change + type GroupWrites = Arc>>>; + + /// A context holding one stored group with the supplied permissions. Every write succeeds + /// and the written permissions are recorded. + fn ctx_with_group( + permissions: Permissions, + ) -> ( + GroupContext, + AccessGroup, + GroupWrites, + ) { + let group = AccessGroup { + permissions, + ..dynamic_group("engineering") + }; + let writes: GroupWrites = Arc::new(Mutex::new(vec![])); + + let mut group_store = MockAccessGroupStore::new(); + let stored = group.clone(); + group_store + .expect_get() + .returning(move |_, _| Ok(Some(stored.clone()))); + let recorder = writes.clone(); + let template = group.clone(); + group_store.expect_update().returning(move |update, _| { + recorder.lock().unwrap().push(update.permissions.clone()); + Ok(Some(AccessGroup { + name: update.name.clone(), + permissions: update.permissions.clone(), + ..template.clone() + })) + }); + + let mut storage = MockStorage::new(); + storage.access_group_store = Some(Arc::new(group_store)); + + (GroupContext::new(Arc::new(storage)), group, writes) + } + + fn caller_with(permissions: Vec) -> Caller { + Caller { + id: TypedUuid::new_v4(), + permissions: permissions.into(), + extensions: Default::default(), + } + } + + /// Every member receives a group's permissions, so adding one requires managing the group + /// and being able to grant the permission + #[tokio::test] + async fn adding_a_group_permission_requires_grant() { + let (ctx, group, writes) = ctx_with_group(Permissions::new()); + + let cannot_grant = caller_with(vec![VPermission::ManageGroup(group.id)]); + let refused = ctx + .add_permission_to_group(&cannot_grant, &group.id, VPermission::CreateMapper) + .await; + assert!(matches!(refused, Err(ResourceError::Restricted))); + assert!(writes.lock().unwrap().is_empty()); + + let can_grant = caller_with(vec![ + VPermission::GetGroup(group.id), + VPermission::ManageGroup(group.id), + VPermission::CreateMapper, + ]); + let updated = ctx + .add_permission_to_group(&can_grant, &group.id, VPermission::CreateMapper) + .await + .unwrap(); + assert!(updated.permissions.can(&VPermission::CreateMapper)); + } + + /// Removing a group permission requires being able to grant it, so the group's members never + /// lose permissions outside of the caller's authority + #[tokio::test] + async fn removing_a_group_permission_requires_grant() { + let (ctx, group, writes) = + ctx_with_group(vec![VPermission::CreateMapper, VPermission::CreateGroup].into()); + + let cannot_grant = caller_with(vec![VPermission::ManageGroup(group.id)]); + let refused = ctx + .remove_permission_from_group(&cannot_grant, &group.id, &VPermission::CreateMapper) + .await; + assert!(matches!(refused, Err(ResourceError::Restricted))); + assert!(writes.lock().unwrap().is_empty()); + + let can_grant = caller_with(vec![ + VPermission::GetGroup(group.id), + VPermission::ManageGroup(group.id), + VPermission::CreateMapper, + ]); + let updated = ctx + .remove_permission_from_group(&can_grant, &group.id, &VPermission::CreateMapper) + .await + .unwrap(); + assert!(!updated.permissions.can(&VPermission::CreateMapper)); + assert!(updated.permissions.can(&VPermission::CreateGroup)); + } + + /// A write against a generation that has moved on is refused as a conflict after a single + /// attempt, and is never retried + #[tokio::test] + async fn a_stale_group_write_is_a_conflict() { + let group = dynamic_group("engineering"); + let stored = group.clone(); + + let mut group_store = MockAccessGroupStore::new(); + group_store + .expect_get() + .times(1) + .returning(move |_, _| Ok(Some(stored.clone()))); + group_store + .expect_update() + .times(1) + .returning(|_, _| Ok(None)); + + let mut storage = MockStorage::new(); + storage.access_group_store = Some(Arc::new(group_store)); + let ctx = GroupContext::new(Arc::new(storage)); + + let manager = caller_with(vec![ + VPermission::GetGroup(group.id), + VPermission::ManageGroup(group.id), + VPermission::CreateMapper, + ]); + let conflict = ctx + .add_permission_to_group(&manager, &group.id, VPermission::CreateMapper) + .await; + assert!(matches!(conflict, Err(ResourceError::Conflict))); + } } diff --git a/v-api/src/context/mapping.rs b/v-api/src/context/mapping.rs index c8141ace..131af7d8 100644 --- a/v-api/src/context/mapping.rs +++ b/v-api/src/context/mapping.rs @@ -6,8 +6,8 @@ use newtype_uuid::TypedUuid; use serde_json::Value; use std::{collections::BTreeSet, sync::Arc}; use v_model::{ - AccessGroupId, Mapper, MapperEvent, MapperId, MapperSource, NewMapper, NewMapperEvent, - Permissions, UserId, + AccessGroup, AccessGroupId, Mapper, MapperEvent, MapperId, MapperSource, NewMapper, + NewMapperEvent, UserId, permissions::Caller, storage::{ListPagination, MapperEventStore, MapperFilter, MapperStore, StoreError}, }; @@ -15,13 +15,13 @@ use v_model::{ use crate::{ VApiStorage, endpoints::login::UserInfo, - mapper::MappingEngine, + mapper::{MappingEngine, resolve_mapped_groups}, permissions::{VAppPermission, VPermission}, response::{OptionalResource, ResourceResult, resource_restricted}, }; pub struct MappingContext { - engine: Option>>, + engine: Option>, storage: Arc>, preset_mappers: Vec, } @@ -44,8 +44,8 @@ where pub fn set_engine( &mut self, - engine: Option>>, - ) -> Option>> { + engine: Option>, + ) -> Option> { let previous = self.engine.take(); self.engine = engine; previous @@ -92,18 +92,73 @@ where } } + /// Stores a new mapper. pub async fn add_mapper( &self, caller: &Caller, + visible_groups: &[AccessGroup], new_mapper: &NewMapper, ) -> ResourceResult { - if caller.can(&VPermission::CreateMapper.into()) { + if caller.can(&VPermission::CreateMapper.into()) + && self.can_grant_mapping(caller, visible_groups, &new_mapper.rule) + { Ok(MapperStore::upsert(&*self.storage, new_mapper).await?) } else { resource_restricted() } } + // TODO: Really don't like that we are passing in the visible_groups here. Needs some more + // thought before figuring out how this is changed. + + /// Returns true if the caller could grant everything the rule grants. + fn can_grant_mapping( + &self, + caller: &Caller, + visible_groups: &[AccessGroup], + rule: &Value, + ) -> bool { + match self + .engine + .as_ref() + .map(|engine| engine.create_mapping(rule)) + { + Some(Ok(mapping)) => { + let grants = mapping.grants(); + let named = grants + .groups + .iter() + .map(|name| visible_groups.iter().find(|group| &group.name == name)) + .collect::>>(); + + match named { + Some(named) => named.iter().all(|group| { + caller.can(&VPermission::ManageGroupMembership(group.id).into()) + && caller.can_grant_all(&group.permissions) + }), + None => { + tracing::warn!( + groups = ?grants.groups, + "Mapping rule names a group that does not exist or is not visible" + ); + false + } + } + } + Some(Err(err)) => { + tracing::warn!( + ?err, + "Failed to build a mapping rule to check what it grants" + ); + false + } + None => { + tracing::warn!("No mapping engine is installed to determine what a rule grants"); + false + } + } + } + pub async fn remove_mapper( &self, caller: &Caller, @@ -116,13 +171,14 @@ where } } + /// The permissions and groups that every matching mapper hands to the user. pub async fn get_mapped_fields( &self, caller: &Caller, + visible_groups: &[AccessGroup], info: &UserInfo, user_id: TypedUuid, - ) -> ResourceResult<(Permissions, BTreeSet>), StoreError> { - let mut mapped_permissions = Permissions::new(); + ) -> ResourceResult>, StoreError> { let mut mapped_groups = BTreeSet::new(); // We only need to run mapping logic if there is mapping engine available to transform @@ -136,24 +192,26 @@ where tracing::trace!(?mapper.name, is_preset, "Attempt to run mapper"); // Try to transform this mapper into a mapping - let mapping = engine.create_mapping(mapper.clone()); + let mapping = engine.create_mapping(&mapper.rule); - let (mut permissions, mut groups) = match mapping { + let mut groups = match mapping { Ok(mapping) => { - tracing::trace!(?mapper.name, "Applying mapping"); - ( - mapping.permissions_for(info).await?, - mapping.groups_for(info).await?, - ) + if mapping.matches(info).await { + tracing::trace!(?mapper.name, "Applying mapping"); + let grants = mapping.grants(); + resolve_mapped_groups(&grants.groups, visible_groups) + } else { + BTreeSet::default() + } } Err(err) => { // Errors here can be expected. They are reported, but not acted upon tracing::info!(?err, "No mapping was found for mapper"); - (Permissions::new(), BTreeSet::default()) + BTreeSet::default() } }; - let apply = if !permissions.is_empty() || !groups.is_empty() { + let apply = if !groups.is_empty() { if is_preset { // Preset mappers always apply - no activation gating true @@ -190,13 +248,12 @@ where ); } - mapped_permissions.append(&mut permissions); mapped_groups.append(&mut groups); } } }; - Ok((mapped_permissions, mapped_groups)) + Ok(mapped_groups) } // TODO: Create a permission for this that only the registration user has @@ -231,3 +288,201 @@ where MapperEventStore::record(&*self.storage, &event).await } } + +#[cfg(test)] +mod tests { + use chrono::Utc; + use newtype_uuid::TypedUuid; + use serde_json::json; + use std::{collections::BTreeSet, sync::Arc}; + use v_model::{ + AccessGroup, AccessGroupSource, Mapper, MapperEvent, MapperSource, NewMapper, + permissions::{Caller, Permissions}, + storage::{MockMapperEventStore, MockMapperStore}, + }; + + use super::MappingContext; + use crate::{ + context::test_mocks::MockStorage, + endpoints::login::{ExternalUserId, UserInfo}, + mapper::DefaultMappingEngine, + permissions::VPermission, + response::ResourceError, + }; + + fn group(name: &str) -> AccessGroup { + AccessGroup { + id: TypedUuid::new_v4(), + name: name.to_string(), + permissions: vec![VPermission::CreateGroup].into(), + source: AccessGroupSource::Dynamic, + created_at: Utc::now(), + updated_at: Utc::now(), + deleted_at: None, + generation: 0, + } + } + + /// A mapping context using the default engine, whose store accepts new mappers + fn mapping_context() -> MappingContext { + let mut mapper_store = MockMapperStore::new(); + mapper_store.expect_upsert().returning(|mapper| { + Ok(Mapper { + id: mapper.id, + name: mapper.name.clone(), + rule: mapper.rule.clone(), + activations: mapper.activations, + max_activations: mapper.max_activations, + source: MapperSource::Dynamic, + depleted_at: None, + created_at: Utc::now(), + updated_at: Utc::now(), + deleted_at: None, + }) + }); + + let mut storage = MockStorage::new(); + storage.mapper_store = Some(Arc::new(mapper_store)); + + let mut mapping = MappingContext::new(Arc::new(storage)); + mapping.set_engine(Some(Arc::new(DefaultMappingEngine))); + mapping + } + + fn mapper(groups: Vec<&str>) -> NewMapper { + NewMapper { + id: TypedUuid::new_v4(), + name: "rule".to_string(), + rule: json!({ "rule": "default", "groups": groups }), + activations: None, + max_activations: None, + } + } + + fn caller_with(permissions: Vec) -> Caller { + Caller::from(Permissions::::from(permissions)) + } + + /// Mapping a user into a group is the same as adding them to it, so the creator needs the + /// group's membership permission and must be able to grant the group's permissions + #[tokio::test] + async fn creating_a_mapper_requires_adding_to_its_groups() { + let engineering = group("engineering"); + let visible = vec![engineering.clone()]; + let mapping = mapping_context(); + let rule = mapper(vec!["engineering"]); + + let no_membership = caller_with(vec![VPermission::CreateMapper, VPermission::CreateGroup]); + let refused = mapping.add_mapper(&no_membership, &visible, &rule).await; + assert!(matches!(refused, Err(ResourceError::Restricted))); + + let cannot_grant_group = caller_with(vec![ + VPermission::CreateMapper, + VPermission::ManageGroupMembership(engineering.id), + ]); + let refused = mapping + .add_mapper(&cannot_grant_group, &visible, &rule) + .await; + assert!(matches!(refused, Err(ResourceError::Restricted))); + + let can_add = caller_with(vec![ + VPermission::CreateMapper, + VPermission::ManageGroupMembership(engineering.id), + VPermission::CreateGroup, + ]); + assert!(mapping.add_mapper(&can_add, &visible, &rule).await.is_ok()); + } + + /// Every group a rule names must be one the caller can see. A name that resolves to nothing + /// could later resolve to a group someone else creates, and a group the caller can not see + /// is not theirs to hand out, so both are refused. + #[tokio::test] + async fn creating_a_mapper_refuses_groups_the_caller_can_not_see() { + let mapping = mapping_context(); + let visible = vec![group("engineering")]; + let rule = mapper(vec!["engineering", "not-visible"]); + + let caller = caller_with(vec![ + VPermission::CreateMapper, + VPermission::ManageGroupMembershipsAll, + VPermission::CreateGroup, + ]); + let refused = mapping.add_mapper(&caller, &visible, &rule).await; + assert!(matches!(refused, Err(ResourceError::Restricted))); + } + + fn preset_mapper(name: &str, rule: serde_json::Value) -> Mapper { + Mapper { + id: TypedUuid::new_v4(), + name: name.to_string(), + rule, + activations: None, + max_activations: None, + source: MapperSource::Preset, + depleted_at: None, + created_at: Utc::now(), + updated_at: Utc::now(), + deleted_at: None, + } + } + + /// A login applies exactly the groups of each rule it matches, resolved against the groups + /// the caller can see. A rule that does not match contributes nothing. + #[tokio::test] + async fn logins_apply_exactly_the_grants_of_matching_rules() { + let engineering = group("engineering"); + let admins = group("admins"); + let visible = vec![engineering.clone(), admins]; + + let mut mapper_store = MockMapperStore::new(); + mapper_store.expect_list().returning(|_, _| Ok(vec![])); + let mut event_store = MockMapperEventStore::new(); + event_store.expect_record().returning(|event| { + Ok(MapperEvent { + id: event.id, + mapper_id: event.mapper_id, + mapper_name: event.mapper_name.clone(), + user_id: event.user_id, + rule: event.rule.clone(), + source: event.source, + created_at: Utc::now(), + }) + }); + + let mut storage = MockStorage::new(); + storage.mapper_store = Some(Arc::new(mapper_store)); + storage.mapper_event_store = Some(Arc::new(event_store)); + + let mut mapping = MappingContext::new(Arc::new(storage)); + mapping.set_engine(Some(Arc::new(DefaultMappingEngine))); + mapping.set_preset_mappers(vec![ + preset_mapper( + "matching", + json!({ "rule": "default", "groups": ["engineering"] }), + ), + preset_mapper( + "not-matching", + json!({ + "rule": "email_address", + "email": "someone-else@example.com", + "groups": ["admins"], + }), + ), + ]); + + let registration = caller_with(vec![VPermission::GetMappersAll]); + let user = UserInfo { + external_id: ExternalUserId::MagicLink("user@example.com".to_string()), + verified_emails: vec!["user@example.com".to_string()], + display_name: None, + idp_token: None, + }; + + let groups = mapping + .get_mapped_fields(®istration, &visible, &user, TypedUuid::new_v4()) + .await + .unwrap(); + + assert_eq!(BTreeSet::from([engineering.id]), groups); + } +} diff --git a/v-api/src/context/mod.rs b/v-api/src/context/mod.rs index 6481e94e..f10a581f 100644 --- a/v-api/src/context/mod.rs +++ b/v-api/src/context/mod.rs @@ -24,8 +24,7 @@ use v_model::saga::{ }; use v_model::{ AccessGroup, AccessGroupId, AccessGroupSource, ApiUserInfo, ApiUserProvider, LinkRequest, - Mapper, MapperSource, NewApiUser, NewApiUserProvider, NewLinkRequest, Permissions, UserId, - UserProviderId, + Mapper, MapperSource, NewApiUserProvider, NewLinkRequest, Permissions, UserId, UserProviderId, permissions::{Caller, Permission}, storage::{ AccessGroupFilter, AccessGroupStore, AccessTokenStore, ApiKeyStore, @@ -53,7 +52,7 @@ use crate::{ error::ApiError, mapper::DefaultMappingEngine, permissions::{VAppPermission, VPermission}, - response::{OptionalResource, ResourceErrorInner}, + response::ResourceErrorInner, util::response::{ ResourceResult, client_error, internal_error, resource_error, resource_restricted, }, @@ -513,18 +512,21 @@ where } }; - let (mapped_permissions, mapped_groups) = self + let visible_groups = self + .group + .list_groups(caller, AccessGroupFilter::default()) + .await + .inner_err_into()?; + let mapped_groups = self .mapping - .get_mapped_fields(caller, &info, user_id) + .get_mapped_fields(caller, &visible_groups, &info, user_id) .await .inner_err_into()?; - tracing::debug!(?mapped_permissions, "Computed mapping permissions"); tracing::debug!(?mapped_groups, "Computed mapped groups"); if is_new_user { tracing::info!( - ?mapped_permissions, ?mapped_groups, "Did not find any existing users. Registering a new user." ); @@ -545,7 +547,7 @@ where let user = self .user - .create_api_user(caller, user_id, mapped_permissions, groups) + .create_api_user(caller, user_id, Permissions::default(), groups) .await .inner_err_into()?; @@ -588,12 +590,6 @@ where tracing::info!(?provider.id, ?provider.user_id, "Updating found user permissions and groups"); - // Add mapped permissions to the existing user - self.user - .add_permissions_to_user(caller, &provider.user_id, mapped_permissions) - .await - .inner_err_into()?; - // Add mapped groups to the existing user for group_id in &mapped_groups { self.add_api_user_to_group(caller, &provider.user_id, group_id) @@ -659,40 +655,9 @@ where group_id: &TypedUuid, ) -> ResourceResult, StoreError> { let group = self.group.get_group(caller, group_id).await?; - let can_manage_membership = - caller.can(&VPermission::ManageGroupMembership(*group_id).into()); - let can_grant_permissions = caller.can_grant_all(&group.permissions); - - if !can_manage_membership || !can_grant_permissions { - // This check runs on every login for mapper matched groups. Denials fail the - // entire login, so record exactly which check failed and for which group. - tracing::warn!( - caller = ?caller.id, - ?group_id, - group_name = ?group.name, - can_manage_membership, - can_grant_permissions, - "Caller is not permitted to add user to group" - ); - } - - if can_manage_membership && can_grant_permissions { - // TODO: This needs to be wrapped in a transaction. That requires reworking the way the - // store traits are handled. Ideally we could have an API that still abstracts away the - // underlying connection management while allowing for transactions. Possibly something - // that takes a closure and passes in a connection that implements all of the expected - // data store traits - let info = ApiUserStore::get(&*self.storage, api_user_id, false) - .await - .optional()?; - - let mut update: NewApiUser = info.user.into(); - update.groups.insert(*group_id); - - Ok(ApiUserStore::upsert(&*self.storage, update).await?) - } else { - resource_restricted() - } + self.user + .add_user_to_group(caller, api_user_id, &group) + .await } pub async fn remove_api_user_from_group( @@ -701,23 +666,10 @@ where api_user_id: &TypedUuid, group_id: &TypedUuid, ) -> ResourceResult, StoreError> { - if caller.can(&VPermission::ManageGroupMembership(*group_id).into()) { - // TODO: This needs to be wrapped in a transaction. That requires reworking the way the - // store traits are handled. Ideally we could have an API that still abstracts away the - // underlying connection management while allowing for transactions. Possibly something - // that takes a closure and passes in a connection that implements all of the expected - // data store traits - let info = ApiUserStore::get(&*self.storage, api_user_id, false) - .await - .optional()?; - - let mut update: NewApiUser = info.user.into(); - update.groups.retain(|id| id != group_id); - - Ok(ApiUserStore::upsert(&*self.storage, update).await?) - } else { - resource_restricted() - } + let group = self.group.get_group(caller, group_id).await?; + self.user + .remove_user_from_group(caller, api_user_id, &group) + .await } pub async fn complete_link_request( @@ -1061,6 +1013,7 @@ where created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, } }) .collect(); @@ -1133,10 +1086,7 @@ where } let mut mapping_ctx = MappingContext::new(storage.clone()); - mapping_ctx.set_engine(Some(Arc::new(DefaultMappingEngine::new( - auth_ctx.builtin_registration_user(), - group_ctx.clone(), - )))); + mapping_ctx.set_engine(Some(Arc::new(DefaultMappingEngine))); // Convert preset mapper configs into Mapper structs with deterministic IDs let preset_mappers: Vec = self @@ -1270,6 +1220,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }; let provider = ApiUserProvider { @@ -1314,6 +1265,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }; let pagination = ListPagination::unlimited(); let mut group_store = MockAccessGroupStore::new(); @@ -1340,6 +1292,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: None, providers: vec![], @@ -1466,6 +1419,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: None, providers: vec![], @@ -1539,6 +1493,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: None, providers: vec![], @@ -1549,8 +1504,8 @@ mod tests { .expect_get() .with(eq(user_id), eq(false)) .returning(move |_, _| Ok(Some(user.clone()))); - user_store.expect_upsert().returning(|update| { - Ok(ApiUserInfo { + user_store.expect_update().returning(|update, _| { + Ok(Some(ApiUserInfo { user: ApiUser { id: update.id, permissions: update.permissions, @@ -1558,10 +1513,11 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: None, providers: vec![], - }) + })) }); let mut group_store = MockAccessGroupStore::new(); @@ -1584,6 +1540,7 @@ mod tests { let caller = Caller { id: user_id, permissions: vec![ + VPermission::GetApiUsersAll, VPermission::GetGroupsAll, VPermission::ManageGroupMembershipsAll, // Required to grant the permissions held by the group @@ -1600,6 +1557,27 @@ mod tests { .unwrap(); assert_eq!(BTreeSet::from([group_id]), added.user.groups); + // Leaving the group takes away CreateGroup, which this caller can not grant, so it may + // not remove the user even though it can manage the group's membership + let cannot_grant = Caller { + id: user_id, + permissions: vec![ + VPermission::GetApiUsersAll, + VPermission::GetGroupsAll, + VPermission::ManageGroupMembershipsAll, + ] + .into(), + extensions: Default::default(), + }; + let ctx = ctx_with_memberships(BTreeSet::from([group_id])).await; + let refused = ctx + .remove_api_user_from_group(&cannot_grant, &user_id, &group_id) + .await; + assert!(matches!( + refused, + Err(crate::response::ResourceError::Restricted) + )); + let ctx = ctx_with_memberships(BTreeSet::from([group_id])).await; let removed = ctx .remove_api_user_from_group(&caller, &user_id, &group_id) @@ -1649,6 +1627,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }]) }); @@ -1832,10 +1811,7 @@ pub(crate) mod test_mocks { ) -> VContext { let mut ctx = build_ctx_with_groups(storage, groups).await.unwrap(); - let mapping_engine = Arc::new(DefaultMappingEngine::new( - ctx.builtin_registration_user(), - ctx.group.clone(), - )); + let mapping_engine = Arc::new(DefaultMappingEngine); ctx.mapping.set_engine(Some(mapping_engine)); ctx.auth.insert_oauth_provider( @@ -1965,6 +1941,19 @@ pub(crate) mod test_mocks { self.api_user_store.as_ref().unwrap().upsert(api_user).await } + async fn update( + &self, + api_user: NewApiUser, + expected_generation: i64, + ) -> Result>, v_model::storage::StoreError> + { + self.api_user_store + .as_ref() + .unwrap() + .update(api_user, expected_generation) + .await + } + async fn delete( &self, id: &TypedUuid, @@ -2255,6 +2244,18 @@ pub(crate) mod test_mocks { ) -> Result, v_model::storage::StoreError> { self.oauth_client_store.as_ref().unwrap().delete(id).await } + + async fn assign_api_user( + &self, + id: &TypedUuid, + api_user_id: &TypedUuid, + ) -> Result, v_model::storage::StoreError> { + self.oauth_client_store + .as_ref() + .unwrap() + .assign_api_user(id, api_user_id) + .await + } } #[async_trait] @@ -2345,6 +2346,19 @@ pub(crate) mod test_mocks { .await } + async fn update( + &self, + group: &NewAccessGroup, + expected_generation: i64, + ) -> Result>, v_model::storage::StoreError> + { + self.access_group_store + .as_ref() + .unwrap() + .update(group, expected_generation) + .await + } + async fn delete( &self, id: &TypedUuid, diff --git a/v-api/src/context/oauth.rs b/v-api/src/context/oauth.rs index 9b30dfae..86537def 100644 --- a/v-api/src/context/oauth.rs +++ b/v-api/src/context/oauth.rs @@ -81,6 +81,7 @@ where &*self.storage, OAuthClientFilter { id: None, + api_user_id: None, deleted: false, }, &ListPagination::default(), @@ -229,6 +230,8 @@ mod tests { client_store.expect_get().returning(move |id, _| { Ok(Some(OAuthClient { id: *id, + api_user_id: None, + keys: vec![], secrets: vec![], redirect_uris: vec![], created_at: Utc::now(), @@ -284,6 +287,8 @@ mod tests { client_store.expect_get().returning(move |id, _| { Ok(Some(OAuthClient { id: *id, + api_user_id: None, + keys: vec![], secrets: vec![], redirect_uris: vec![], created_at: Utc::now(), diff --git a/v-api/src/context/user.rs b/v-api/src/context/user.rs index 13ce81a9..9073cbbb 100644 --- a/v-api/src/context/user.rs +++ b/v-api/src/context/user.rs @@ -29,7 +29,8 @@ use crate::{ }, permissions::{VAppPermission, VPermission}, response::{ - OptionalResource, ResourceError, ResourceResult, resource_not_found, resource_restricted, + OptionalResource, ResourceError, ResourceResult, resource_conflict, resource_not_found, + resource_restricted, }, }; @@ -88,6 +89,15 @@ where } } +/// A change to the permissions or groups held directly by a user. Each kind of change carries +/// what is needed to check the caller's authority to make it. +enum UserChange<'a, T> { + AddPermissions { permissions: Permissions }, + RemovePermission { permission: &'a T }, + JoinGroup(&'a AccessGroup), + LeaveGroup(&'a AccessGroup), +} + #[derive(Clone)] pub struct UserContext { caller_extension_handlers: Vec>>, @@ -346,21 +356,25 @@ where caller: &Caller, id: &TypedUuid, ) -> ResourceResult, StoreError> { - if caller.can(&VPermission::GetApiUser(*id).into()) { - let mut info = ApiUserStore::get(&*self.storage, id, false) - .await - .optional()?; + let mut info = self.get_stored_api_user(caller, id).await?; + let extensions = self.get_extensions(&info.user).await; - let extensions = self.get_extensions(&info.user).await; + info.user.permissions = + ::expand(&info.user.permissions, &info.user, None, &extensions); - info.user.permissions = ::expand( - &info.user.permissions, - &info.user, - None, - &extensions, - ); + Ok(info) + } - Ok(info) + /// Reads a user as it is (notably without expanding permissions). + async fn get_stored_api_user( + &self, + caller: &Caller, + id: &TypedUuid, + ) -> ResourceResult, StoreError> { + if caller.can(&VPermission::GetApiUser(*id).into()) { + ApiUserStore::get(&*self.storage, id, false) + .await + .optional() } else { resource_restricted() } @@ -408,27 +422,8 @@ where } } - /// Replaces the direct permissions on a user. Group membership is not affected. - /// The caller must hold every permission that is being newly added. - #[instrument(skip(self, caller, user_id, permissions), fields(caller = ?caller.id))] - pub async fn set_api_user_permissions( - &self, - caller: &Caller, - user_id: &TypedUuid, - permissions: Permissions, - ) -> ResourceResult, StoreError> { - if caller.can(&VPermission::ManageApiUser(*user_id).into()) - && caller.can_grant_all(&permissions) - { - let info = self.get_api_user(caller, user_id).await?; - let mut update: NewApiUser = info.user.into(); - update.permissions = permissions; - self.save_api_user(update).await - } else { - resource_restricted() - } - } - + /// Adds permissions directly to a user. The caller must be able to manage the user's + /// permissions and grant every new permission for any permissions to be added. #[instrument(skip(self, caller, user_id, new_permissions), fields(caller = ?caller.id))] pub async fn add_permissions_to_user( &self, @@ -436,24 +431,18 @@ where user_id: &TypedUuid, new_permissions: Permissions, ) -> ResourceResult, StoreError> { - if caller.can(&VPermission::ManageApiUser(*user_id).into()) - && caller.can_grant_all(&new_permissions) - { - let info = self.get_api_user(caller, user_id).await?; - - let mut user_update: NewApiUser = info.user.into(); - for permission in new_permissions.into_iter() { - tracing::info!(id = ?user_id, ?permission, "Adding permission to user"); - user_update.permissions.insert(permission); - } - - self.save_api_user(user_update).await - } else { - resource_restricted() - } + self.modify_api_user( + caller, + user_id, + UserChange::AddPermissions { + permissions: new_permissions, + }, + ) + .await } - /// Adds a single permission to a user. The caller must hold the permission being added. + /// Adds a permission directly to a user. The caller must be able to manage the user's + /// permissions and grant the permission. #[instrument(skip(self, caller, user_id, permission), fields(caller = ?caller.id))] pub async fn add_permission_to_user( &self, @@ -461,22 +450,18 @@ where user_id: &TypedUuid, permission: T, ) -> ResourceResult, StoreError> { - if caller.can(&VPermission::ManageApiUser(*user_id).into()) && caller.can_grant(&permission) - { - let info = self.get_api_user(caller, user_id).await?; - - let mut user_update: NewApiUser = info.user.into(); - tracing::info!(id = ?user_id, ?permission, "Adding permission to user"); - user_update.permissions.insert(permission); - - self.save_api_user(user_update).await - } else { - resource_restricted() - } + self.modify_api_user( + caller, + user_id, + UserChange::AddPermissions { + permissions: Permissions::from_iter([permission]), + }, + ) + .await } - /// Removes a single permission from a user. The caller does not need to hold the - /// permission being removed, only the ability to manage the target user. + /// Removes a permission held directly by a user. This does not affect permissions + /// attributed through group memberships. #[instrument(skip(self, caller, user_id, permission), fields(caller = ?caller.id))] pub async fn remove_permission_from_user( &self, @@ -484,21 +469,134 @@ where user_id: &TypedUuid, permission: &T, ) -> ResourceResult, StoreError> { - if caller.can(&VPermission::ManageApiUser(*user_id).into()) { - let info = self.get_api_user(caller, user_id).await?; + self.modify_api_user(caller, user_id, UserChange::RemovePermission { permission }) + .await + } - let mut user_update: NewApiUser = info.user.into(); - tracing::info!(id = ?user_id, ?permission, "Removing permission from user"); - user_update.permissions.remove(permission); + /// Adds a user to a group. + pub async fn add_user_to_group( + &self, + caller: &Caller, + user_id: &TypedUuid, + group: &AccessGroup, + ) -> ResourceResult, StoreError> { + self.modify_api_user(caller, user_id, UserChange::JoinGroup(group)) + .await + } - self.save_api_user(user_update).await + /// Removes a user from a group. + pub async fn remove_user_from_group( + &self, + caller: &Caller, + user_id: &TypedUuid, + group: &AccessGroup, + ) -> ResourceResult, StoreError> { + self.modify_api_user(caller, user_id, UserChange::LeaveGroup(group)) + .await + } + + /// Applies a change to a user's permissions and groups. + async fn modify_api_user( + &self, + caller: &Caller, + user_id: &TypedUuid, + change: UserChange<'_, T>, + ) -> ResourceResult, StoreError> { + if Self::can_make_change(caller, user_id, &change) { + let current = self.get_stored_api_user(caller, user_id).await?; + let read_generation = current.user.generation; + + let mut update: NewApiUser = current.user.into(); + update.permissions = update.permissions.expand_values(); + Self::apply_change(&mut update, change); + update.permissions = ::contract(&update.permissions); + + match ApiUserStore::update(&*self.storage, update, read_generation).await? { + Some(updated) => Ok(updated), + None => { + tracing::info!( + ?user_id, + generation = read_generation, + "User was written by someone else since it was read" + ); + resource_conflict() + } + } } else { resource_restricted() } } - /// Contracts permissions and persists the user. Callers are responsible for performing - /// any authorization and escalation checks before calling this method. + fn apply_change(update: &mut NewApiUser, change: UserChange) { + match change { + UserChange::AddPermissions { permissions, .. } => { + for permission in permissions.expand_values().into_iter() { + tracing::info!(id = ?update.id, ?permission, "Adding permission to user"); + update.permissions.insert(permission); + } + } + UserChange::RemovePermission { permission, .. } => { + tracing::info!(id = ?update.id, ?permission, "Removing permission from user"); + update.permissions = update.permissions.without(permission); + } + UserChange::JoinGroup(group) => { + update.groups.insert(group.id); + } + UserChange::LeaveGroup(group) => { + update.groups.retain(|id| id != &group.id); + } + } + } + + /// Returns true if the caller can make this change to the user + fn can_make_change( + caller: &Caller, + user_id: &TypedUuid, + change: &UserChange, + ) -> bool { + match change { + UserChange::AddPermissions { permissions } => { + Self::can_manage_permissions(caller, user_id) && caller.can_grant_all(permissions) + } + UserChange::RemovePermission { permission } => { + Self::can_manage_permissions(caller, user_id) && caller.can_grant(permission) + } + UserChange::JoinGroup(group) => Self::can_change_membership(caller, group, "add"), + UserChange::LeaveGroup(group) => Self::can_change_membership(caller, group, "remove"), + } + } + + /// Returns true if the caller can change the permissions directly assigned to the user. + fn can_manage_permissions(caller: &Caller, user_id: &TypedUuid) -> bool { + caller.can(&VPermission::ManageApiUser(*user_id).into()) + } + + // TODO: This is evidence of a problem in our data model. Storing groups on the user model means + // that we have to modify a user to assign a group. But what we actually care about are group + // permissions. This is for a future larger refactoring of the data model. + + /// Returns true if the caller may add users to or remove users from the group. + fn can_change_membership(caller: &Caller, group: &AccessGroup, action: &str) -> bool { + let can_manage_membership = + caller.can(&VPermission::ManageGroupMembership(group.id).into()); + let can_grant_permissions = caller.can_grant_all(&group.permissions); + + if !can_manage_membership || !can_grant_permissions { + tracing::warn!( + caller = ?caller.id, + group_id = ?group.id, + group_name = ?group.name, + action, + can_manage_membership, + can_grant_permissions, + "Caller is not permitted to change membership of group" + ); + } + + can_manage_membership && can_grant_permissions + } + + /// Contracts permissions and stores a new user. async fn save_api_user( &self, mut api_user: NewApiUser, diff --git a/v-api/src/endpoints/api_user.rs b/v-api/src/endpoints/api_user.rs index 0f16ca99..67258f21 100644 --- a/v-api/src/endpoints/api_user.rs +++ b/v-api/src/endpoints/api_user.rs @@ -2,12 +2,12 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -use std::collections::{BTreeSet, HashMap, hash_map::Entry}; +use std::collections::{HashMap, hash_map::Entry}; use chrono::{DateTime, Utc}; use dropshot::{ HttpError, HttpResponseCreated, HttpResponseOk, HttpResponseUpdatedNoContent, Path, - RequestContext, TypedBody, + RequestContext, }; use newtype_uuid::{GenericUuid, TypedUuid}; use partial_struct::partial; @@ -19,7 +19,7 @@ use uuid::Uuid; use v_model::{ AccessGroupId, ApiKeyId, ApiUser, ApiUserContactEmail, ApiUserProvider, NewApiKey, UserId, permissions::{Caller, Permission, PermissionStorage, Permissions}, - storage::{AccessGroupFilter, ApiUserFilter, ApiUserProviderFilter, ListPagination}, + storage::{ApiUserFilter, ApiUserProviderFilter, ListPagination}, }; use crate::{ @@ -48,6 +48,7 @@ where created_at: user.created_at, updated_at: user.updated_at, deleted_at: user.deleted_at, + generation: user.generation, } } @@ -189,124 +190,11 @@ where Ok(HttpResponseOk(responses)) } -#[derive(Debug, Clone, PartialEq, Deserialize, JsonSchema)] -pub struct ApiUserUpdateParams { - permissions: Permissions, - #[serde(default)] - group_ids: BTreeSet>, -} - -/// Creates a new API user with the specified permissions and group memberships. -/// -/// Returns the newly created user's information along with any associated providers. -#[instrument(skip(rqctx), err(Debug))] -pub async fn create_api_user_op( - rqctx: &RequestContext>, - body: TypedBody>, -) -> Result>, HttpError> -where - T: VAppPermission + JsonSchema + PermissionStorage, - U: VAppPermissionResponse + From + JsonSchema, -{ - let (ctx, caller) = rqctx.as_ctx().await?; - let body = body.into_inner(); - - create_api_user_inner(ctx, caller, body).await -} - -#[instrument(skip(ctx, body))] -pub async fn create_api_user_inner( - ctx: &VContext, - caller: Caller, - body: ApiUserUpdateParams, -) -> Result>, HttpError> -where - T: VAppPermission + JsonSchema + PermissionStorage, - U: VAppPermissionResponse + From + JsonSchema, -{ - // Resolve the full groups so that create_api_user can verify the caller - // is allowed to grant the permissions they carry. - let groups = ctx - .group - .list_groups( - &caller, - AccessGroupFilter { - id: Some(body.group_ids.into_iter().collect()), - ..Default::default() - }, - ) - .await?; - - let info = ctx - .user - .create_api_user(&caller, TypedUuid::new_v4(), body.permissions, groups) - .await?; - - let filter = ApiUserProviderFilter { - api_user_id: Some(vec![info.user.id]), - ..Default::default() - }; - let providers = ctx - .user - .list_api_user_provider(&caller, filter, &ListPagination::default().limit(10)) - .await?; - - Ok(HttpResponseCreated(GetUserResponse::new( - info.user, providers, - ))) -} - #[derive(Clone, Debug, Deserialize, JsonSchema)] pub struct ApiUserPath { user_id: TypedUuid, } -/// Updates an existing API user's direct permissions. Group membership is managed -/// separately via the group membership endpoints. -/// -/// Returns the updated user's information along with their associated providers. -#[instrument(skip(rqctx), err(Debug))] -pub async fn update_api_user_op( - rqctx: &RequestContext>, - path: ApiUserPath, - body: ApiUserUpdateParams, -) -> Result>, HttpError> -where - T: VAppPermission + PermissionStorage, - U: VAppPermissionResponse + From + JsonSchema, -{ - let (ctx, caller) = rqctx.as_ctx().await?; - update_api_user_inner(ctx, caller, path, body).await -} - -#[instrument(skip(ctx, body))] -pub async fn update_api_user_inner( - ctx: &VContext, - caller: Caller, - path: ApiUserPath, - body: ApiUserUpdateParams, -) -> Result>, HttpError> -where - T: VAppPermission + PermissionStorage, - U: VAppPermissionResponse + From + JsonSchema, -{ - let info = ctx - .user - .set_api_user_permissions(&caller, &path.user_id, body.permissions) - .await?; - - let filter = ApiUserProviderFilter { - api_user_id: Some(vec![info.user.id]), - ..Default::default() - }; - let providers = ctx - .user - .list_api_user_provider(&caller, filter, &ListPagination::default().limit(10)) - .await?; - - Ok(HttpResponseOk(GetUserResponse::new(info.user, providers))) -} - /// Lists all API tokens for a specific user. /// /// Returns a collection of API tokens associated with the specified user, @@ -868,7 +756,7 @@ where mod tests { use std::{ collections::{BTreeSet, HashMap}, - sync::Arc, + sync::{Arc, Mutex}, }; use chrono::{TimeDelta, Utc}; @@ -876,28 +764,26 @@ mod tests { use mockall::predicate::eq; use newtype_uuid::TypedUuid; use v_model::{ - ApiKey, ApiUser, ApiUserContactEmail, ApiUserInfo, ApiUserProvider, NewApiUser, - permissions::{Caller, Permissions}, + ApiKey, ApiUser, ApiUserContactEmail, ApiUserInfo, ApiUserProvider, OAuthClient, UserId, + permissions::{Caller, PermissionStorage, Permissions}, storage::{ - ApiKeyFilter, ListPagination, MockAccessGroupStore, MockApiKeyStore, - MockApiUserContactEmailStore, MockApiUserProviderStore, MockApiUserStore, StoreError, + ApiKeyFilter, ListPagination, MockApiKeyStore, MockApiUserContactEmailStore, + MockApiUserProviderStore, MockApiUserStore, MockOAuthClientStore, StoreError, }, }; use crate::{ context::test_mocks::{MockStorage, mock_context}, endpoints::api_user::{ - ApiKeyCreateParams, ApiUserEmailUpdateParams, ApiUserPath, ApiUserTokenPath, - create_api_user_inner, create_api_user_token_inner, delete_api_user_token_inner, - get_api_user_token_inner, list_api_user_tokens_inner, set_api_user_contact_email_inner, - update_api_user_inner, + ApiKeyCreateParams, ApiUserEmailUpdateParams, ApiUserPath, ApiUserPermissionParams, + ApiUserTokenPath, add_api_user_permission_inner, create_api_user_token_inner, + delete_api_user_token_inner, get_api_user_token_inner, list_api_user_tokens_inner, + remove_api_user_permission_inner, set_api_user_contact_email_inner, }, permissions::{VPermission, VPermissionResponse}, util::tests::get_status, }; - use super::ApiUserUpdateParams; - fn mock_user() -> ApiUser { ApiUser { id: TypedUuid::new_v4(), @@ -906,260 +792,10 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, } } - #[tokio::test] - async fn test_create_api_user_permissions() { - let successful_update = ApiUserUpdateParams { - permissions: vec![VPermission::CreateApiUser].into(), - group_ids: BTreeSet::new(), - }; - - let failure_update = ApiUserUpdateParams { - permissions: vec![VPermission::GetApiUsersAll].into(), - group_ids: BTreeSet::new(), - }; - - let mut store = MockApiUserStore::new(); - store - .expect_upsert() - .withf(|x: &NewApiUser| x.permissions.can(&VPermission::CreateApiUser)) - .returning(|user| { - Ok(ApiUserInfo { - user: ApiUser { - id: user.id, - permissions: user.permissions, - groups: BTreeSet::new(), - created_at: Utc::now(), - updated_at: Utc::now(), - deleted_at: None, - }, - email: None, - providers: vec![], - }) - }); - store - .expect_upsert() - .withf(|x: &NewApiUser| x.permissions.can(&VPermission::GetApiUsersAll)) - .returning(|_| Err(StoreError::Unknown)); - let mut api_user_provider_store = MockApiUserProviderStore::new(); - api_user_provider_store - .expect_list() - .returning(|_, _| Ok(vec![])); - - let mut access_group_store = MockAccessGroupStore::new(); - access_group_store - .expect_list() - .returning(|_, _| Ok(vec![])); - - let mut storage = MockStorage::new(); - storage.api_user_store = Some(Arc::new(store)); - storage.api_user_provider_store = Some(Arc::new(api_user_provider_store)); - storage.access_group_store = Some(Arc::new(access_group_store)); - - let ctx = mock_context(Arc::new(storage)).await; - - let user1 = mock_user(); - - // 1. Fail to create due to lack of permissions - let no_permissions = Caller { - id: user1.id, - permissions: Permissions::new(), - extensions: HashMap::default(), - }; - - let resp = create_api_user_inner::( - &ctx, - no_permissions, - successful_update.clone(), - ) - .await; - - assert!(resp.is_err()); - assert_eq!(get_status(&resp), StatusCode::FORBIDDEN); - - let user2 = mock_user(); - - // 2. Succeed in creating new api user - let with_permissions = Caller { - id: user2.id, - permissions: vec![VPermission::CreateApiUser].into(), - extensions: HashMap::default(), - }; - - let resp = create_api_user_inner::( - &ctx, - with_permissions.clone(), - successful_update.clone(), - ) - .await; - - assert!(resp.is_ok()); - assert_eq!(get_status(&resp), StatusCode::CREATED); - - // 3. Handle storage failure and return error - let resp = create_api_user_inner::( - &ctx, - with_permissions, - failure_update.clone(), - ) - .await; - - assert!(resp.is_err()); - assert_eq!(get_status(&resp), StatusCode::FORBIDDEN); - } - - #[tokio::test] - async fn test_update_api_user_permissions() { - let success_id = TypedUuid::new_v4(); - let successful_update = ApiUserUpdateParams { - permissions: Permissions::new(), - group_ids: BTreeSet::new(), - }; - - let failure_id = TypedUuid::new_v4(); - let failure_update = ApiUserUpdateParams { - permissions: Permissions::new(), - group_ids: BTreeSet::new(), - }; - - let mut store = MockApiUserStore::new(); - store.expect_get().returning(|id, _| { - Ok(Some(ApiUserInfo { - user: ApiUser { - id: *id, - permissions: Permissions::new(), - groups: BTreeSet::new(), - created_at: Utc::now(), - updated_at: Utc::now(), - deleted_at: None, - }, - email: None, - providers: vec![], - })) - }); - store - .expect_upsert() - .withf(move |x: &NewApiUser| x.id == success_id) - .returning(|user| { - Ok(ApiUserInfo { - user: ApiUser { - id: user.id, - permissions: user.permissions, - groups: BTreeSet::new(), - created_at: Utc::now(), - updated_at: Utc::now(), - deleted_at: None, - }, - email: None, - providers: vec![], - }) - }); - store - .expect_upsert() - .withf(move |x: &NewApiUser| x.id == failure_id) - .returning(|_| Err(StoreError::Unknown)); - let mut api_user_provider_store = MockApiUserProviderStore::new(); - api_user_provider_store - .expect_list() - .returning(|_, _| Ok(vec![])); - let mut access_group_store = MockAccessGroupStore::new(); - access_group_store - .expect_list() - .returning(|_, _| Ok(vec![])); - - let mut storage = MockStorage::new(); - storage.api_user_store = Some(Arc::new(store)); - storage.api_user_provider_store = Some(Arc::new(api_user_provider_store)); - storage.access_group_store = Some(Arc::new(access_group_store)); - - let ctx = mock_context(Arc::new(storage)).await; - - let success_path = ApiUserPath { - user_id: success_id, - }; - let failure_path = ApiUserPath { - user_id: failure_id, - }; - - let user1 = mock_user(); - - // 1. Fail to create due to lack of permissions - let no_permissions = Caller { - id: user1.id, - permissions: Permissions::new(), - extensions: HashMap::default(), - }; - - let resp = update_api_user_inner::( - &ctx, - no_permissions, - success_path.clone(), - successful_update.clone(), - ) - .await; - - assert!(resp.is_err()); - assert_eq!(get_status(&resp), StatusCode::FORBIDDEN); - - let user2 = mock_user(); - - // 2. Succeed in updating api user with direct permission - let with_specific_permissions = Caller { - id: user2.id, - permissions: vec![ - VPermission::GetApiUser(success_path.user_id), - VPermission::ManageApiUser(success_path.user_id), - ] - .into(), - extensions: HashMap::default(), - }; - - let resp = update_api_user_inner::( - &ctx, - with_specific_permissions, - success_path.clone(), - successful_update.clone(), - ) - .await; - - assert!(resp.is_ok()); - assert_eq!(get_status(&resp), StatusCode::OK); - - let user3 = mock_user(); - - // 3. Succeed in updating api user with general permission - let with_general_permissions = Caller { - id: user3.id, - permissions: vec![VPermission::GetApiUsersAll, VPermission::ManageApiUsersAll].into(), - extensions: HashMap::default(), - }; - - let resp = update_api_user_inner::( - &ctx, - with_general_permissions.clone(), - success_path, - successful_update.clone(), - ) - .await; - - assert!(resp.is_ok()); - assert_eq!(get_status(&resp), StatusCode::OK); - - // 4. Handle storage failure and return error - let resp = update_api_user_inner::( - &ctx, - with_general_permissions, - failure_path, - failure_update.clone(), - ) - .await; - - assert!(resp.is_err()); - assert_eq!(get_status(&resp), StatusCode::INTERNAL_SERVER_ERROR); - } - #[tokio::test] async fn test_list_api_user_token_permissions() { let success_id = TypedUuid::new_v4(); @@ -1287,6 +923,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }; let api_user_path = ApiUserPath { @@ -1345,6 +982,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: None, providers: vec![], @@ -1814,6 +1452,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: None, providers: vec![ApiUserProvider { @@ -1931,4 +1570,307 @@ mod tests { assert_eq!(get_status(&resp), StatusCode::OK); assert_eq!("user@company".to_string(), resp.unwrap().0.email); } + + fn stored_user( + id: TypedUuid, + permissions: Permissions, + ) -> ApiUserInfo { + ApiUserInfo { + user: ApiUser { + id, + permissions, + groups: BTreeSet::new(), + created_at: Utc::now(), + updated_at: Utc::now(), + deleted_at: None, + generation: 0, + }, + email: None, + providers: vec![], + } + } + + fn caller_with(permissions: Vec) -> Caller { + Caller { + id: TypedUuid::new_v4(), + permissions: permissions.into(), + extensions: HashMap::default(), + } + } + + /// The permissions written by each successful permission change + type Writes = Arc>>>; + + /// A context holding a single user. When `client` is set, that OAuth client is backed by the + /// user. Every write succeeds and is recorded. + async fn permission_ctx( + user: ApiUserInfo, + client: Option, + ) -> (crate::VContext, Writes) { + let writes: Writes = Arc::new(Mutex::new(vec![])); + + let mut user_store = MockApiUserStore::new(); + let current = user.clone(); + user_store + .expect_get() + .returning(move |_, _| Ok(Some(current.clone()))); + let recorder = writes.clone(); + user_store.expect_update().returning(move |update, _| { + recorder.lock().unwrap().push(update.permissions.clone()); + Ok(Some(stored_user(update.id, update.permissions))) + }); + + let mut client_store = MockOAuthClientStore::new(); + client_store.expect_list().returning(move |filter, _| { + Ok(client + .iter() + .filter(|client| { + filter.api_user_id.as_ref() == client.api_user_id.map(|id| vec![id]).as_ref() + }) + .cloned() + .collect()) + }); + + let mut provider_store = MockApiUserProviderStore::new(); + provider_store.expect_list().returning(|_, _| Ok(vec![])); + + let mut storage = MockStorage::new(); + storage.api_user_store = Some(Arc::new(user_store)); + storage.oauth_client_store = Some(Arc::new(client_store)); + storage.api_user_provider_store = Some(Arc::new(provider_store)); + + (mock_context(Arc::new(storage)).await, writes) + } + + /// Adding a permission requires managing the user and being able to grant the permission + #[tokio::test] + async fn test_add_api_user_permission_requires_grant() { + let user_id = TypedUuid::new_v4(); + let (ctx, writes) = permission_ctx(stored_user(user_id, Permissions::new()), None).await; + let body = || ApiUserPermissionParams { + permission: VPermission::CreateGroup, + }; + + let cannot_grant = caller_with(vec![VPermission::ManageApiUser(user_id)]); + let resp = add_api_user_permission_inner::( + &ctx, + cannot_grant, + ApiUserPath { user_id }, + body(), + ) + .await; + assert_eq!(get_status(&resp), StatusCode::FORBIDDEN); + + let cannot_manage = caller_with(vec![VPermission::CreateGroup]); + let resp = add_api_user_permission_inner::( + &ctx, + cannot_manage, + ApiUserPath { user_id }, + body(), + ) + .await; + assert_eq!(get_status(&resp), StatusCode::FORBIDDEN); + assert!(writes.lock().unwrap().is_empty()); + + let can_grant = caller_with(vec![ + VPermission::GetApiUser(user_id), + VPermission::ManageApiUser(user_id), + VPermission::CreateGroup, + ]); + let resp = add_api_user_permission_inner::( + &ctx, + can_grant, + ApiUserPath { user_id }, + body(), + ) + .await; + assert_eq!(get_status(&resp), StatusCode::OK); + assert!(writes.lock().unwrap()[0].can(&VPermission::CreateGroup)); + } + + /// Removing a permission requires being able to grant it, so a caller can never take away + /// permissions outside of their own authority. Removing one value of a stored set keeps the + /// rest of that set. + #[tokio::test] + async fn test_remove_api_user_permission_requires_grant() { + let user_id = TypedUuid::new_v4(); + let (a, b) = (TypedUuid::new_v4(), TypedUuid::new_v4()); + let user = stored_user( + user_id, + vec![VPermission::ManageGroups(BTreeSet::from([a, b]))].into(), + ); + let (ctx, writes) = permission_ctx(user, None).await; + let body = || ApiUserPermissionParams { + permission: VPermission::ManageGroup(a), + }; + + let cannot_grant = caller_with(vec![VPermission::ManageApiUser(user_id)]); + let resp = remove_api_user_permission_inner::( + &ctx, + cannot_grant, + ApiUserPath { user_id }, + body(), + ) + .await; + assert_eq!(get_status(&resp), StatusCode::FORBIDDEN); + assert!(writes.lock().unwrap().is_empty()); + + // Holding only `a` is enough to remove `a`, and leaves `b` with the user + let can_grant = caller_with(vec![ + VPermission::GetApiUser(user_id), + VPermission::ManageApiUser(user_id), + VPermission::ManageGroup(a), + ]); + let resp = remove_api_user_permission_inner::( + &ctx, + can_grant, + ApiUserPath { user_id }, + body(), + ) + .await; + assert_eq!(get_status(&resp), StatusCode::OK); + + let written = writes.lock().unwrap()[0].clone(); + assert!(!written.can(&VPermission::ManageGroup(a))); + assert!(written.can(&VPermission::ManageGroup(b))); + assert_eq!( + written, + VPermission::contract(&written), + "write was not contracted" + ); + } + + /// The service user behind an OAuth client can be managed by anyone who manages the client, + /// still only within what they can grant + #[tokio::test] + async fn test_service_user_permissions_follow_client_management() { + let user_id = TypedUuid::new_v4(); + let client = OAuthClient { + id: TypedUuid::new_v4(), + api_user_id: Some(user_id), + secrets: vec![], + redirect_uris: vec![], + keys: vec![], + created_at: Utc::now(), + deleted_at: None, + }; + let client_id = client.id; + let (ctx, writes) = + permission_ctx(stored_user(user_id, Permissions::new()), Some(client)).await; + let body = || ApiUserPermissionParams { + permission: VPermission::CreateGroup, + }; + + let other_client = caller_with(vec![ + VPermission::ManageOAuthClient(TypedUuid::new_v4()), + VPermission::CreateGroup, + ]); + let resp = add_api_user_permission_inner::( + &ctx, + other_client, + ApiUserPath { user_id }, + body(), + ) + .await; + assert_eq!(get_status(&resp), StatusCode::FORBIDDEN); + + let manages_client_cannot_grant = + caller_with(vec![VPermission::ManageOAuthClient(client_id)]); + let resp = add_api_user_permission_inner::( + &ctx, + manages_client_cannot_grant, + ApiUserPath { user_id }, + body(), + ) + .await; + assert_eq!(get_status(&resp), StatusCode::FORBIDDEN); + assert!(writes.lock().unwrap().is_empty()); + + // The client is only taken into account when the caller can see it + let cannot_see_client = caller_with(vec![ + VPermission::GetApiUser(user_id), + VPermission::ManageOAuthClient(client_id), + VPermission::CreateGroup, + ]); + let resp = add_api_user_permission_inner::( + &ctx, + cannot_see_client, + ApiUserPath { user_id }, + body(), + ) + .await; + assert_eq!(get_status(&resp), StatusCode::FORBIDDEN); + + // Changes read the user through the authorized lookup, so managing the client is not + // enough without also being able to read its service user + let cannot_read_user = caller_with(vec![ + VPermission::GetOAuthClient(client_id), + VPermission::ManageOAuthClient(client_id), + VPermission::CreateGroup, + ]); + let resp = add_api_user_permission_inner::( + &ctx, + cannot_read_user, + ApiUserPath { user_id }, + body(), + ) + .await; + assert_eq!(get_status(&resp), StatusCode::FORBIDDEN); + assert!(writes.lock().unwrap().is_empty()); + + let manages_client = caller_with(vec![ + VPermission::GetApiUser(user_id), + VPermission::GetOAuthClient(client_id), + VPermission::ManageOAuthClient(client_id), + VPermission::CreateGroup, + ]); + let resp = add_api_user_permission_inner::( + &ctx, + manages_client, + ApiUserPath { user_id }, + body(), + ) + .await; + assert_eq!(get_status(&resp), StatusCode::OK); + } + + /// A write against a generation that has moved on is refused as a conflict after a single + /// attempt, and is never retried + #[tokio::test] + async fn test_permission_change_is_a_conflict_when_stale() { + let user_id = TypedUuid::new_v4(); + let mut user_store = MockApiUserStore::new(); + user_store + .expect_get() + .times(1) + .returning(move |_, _| Ok(Some(stored_user(user_id, Permissions::new())))); + user_store + .expect_update() + .times(1) + .returning(|_, _| Ok(None)); + + let mut client_store = MockOAuthClientStore::new(); + client_store.expect_list().returning(|_, _| Ok(vec![])); + + let mut storage = MockStorage::new(); + storage.api_user_store = Some(Arc::new(user_store)); + storage.oauth_client_store = Some(Arc::new(client_store)); + let ctx = mock_context(Arc::new(storage)).await; + + let caller = caller_with(vec![ + VPermission::GetApiUsersAll, + VPermission::ManageApiUsersAll, + VPermission::CreateGroup, + ]); + let resp = add_api_user_permission_inner::( + &ctx, + caller, + ApiUserPath { user_id }, + ApiUserPermissionParams { + permission: VPermission::CreateGroup, + }, + ) + .await; + assert_eq!(get_status(&resp), StatusCode::CONFLICT); + } } diff --git a/v-api/src/endpoints/group.rs b/v-api/src/endpoints/group.rs index 5c45f5c7..f9c0a5b8 100644 --- a/v-api/src/endpoints/group.rs +++ b/v-api/src/endpoints/group.rs @@ -39,6 +39,7 @@ where created_at: group.created_at, updated_at: group.updated_at, deleted_at: group.deleted_at, + generation: group.generation, } } @@ -97,11 +98,24 @@ pub struct AccessGroupPath { group_id: TypedUuid, } +#[derive(Debug, Clone, PartialEq, Deserialize, JsonSchema)] +pub struct AccessGroupPermissionParams { + permission: T, +} + +fn preset_group_error() -> HttpError { + client_error( + ClientErrorStatusCode::CONFLICT, + "Preset groups are managed via service configuration and cannot be modified at runtime", + ) +} + +/// Adds a single permission to a group. The caller must be able to grant it. #[instrument(skip(rqctx), err(Debug))] -pub async fn update_group_op( +pub async fn add_group_permission_op( rqctx: &RequestContext>, path: AccessGroupPath, - body: AccessGroupUpdateParams, + body: AccessGroupPermissionParams, ) -> Result>, HttpError> where T: VAppPermission + PermissionStorage, @@ -110,25 +124,40 @@ where let (ctx, caller) = rqctx.as_ctx().await?; if ctx.group.is_preset(&path.group_id) { - return Err(client_error( - ClientErrorStatusCode::CONFLICT, - "Preset groups are managed via service configuration and cannot be updated at runtime", - )); + Err(preset_group_error()) + } else { + Ok(HttpResponseOk( + ctx.group + .add_permission_to_group(&caller, &path.group_id, body.permission) + .await + .map(into_group_response)?, + )) } +} - Ok(HttpResponseOk( - ctx.group - .update_group( - &caller, - NewAccessGroup { - id: path.group_id, - name: body.name, - permissions: body.permissions, - }, - ) - .await - .map(into_group_response)?, - )) +/// Removes a single permission from a group. The caller must be able to grant it. +#[instrument(skip(rqctx), err(Debug))] +pub async fn remove_group_permission_op( + rqctx: &RequestContext>, + path: AccessGroupPath, + body: AccessGroupPermissionParams, +) -> Result>, HttpError> +where + T: VAppPermission + PermissionStorage, + U: From + Permission + JsonSchema, +{ + let (ctx, caller) = rqctx.as_ctx().await?; + + if ctx.group.is_preset(&path.group_id) { + Err(preset_group_error()) + } else { + Ok(HttpResponseOk( + ctx.group + .remove_permission_from_group(&caller, &path.group_id, &body.permission) + .await + .map(into_group_response)?, + )) + } } #[instrument(skip(rqctx), err(Debug))] diff --git a/v-api/src/endpoints/handlers.rs b/v-api/src/endpoints/handlers.rs index b0bf73d2..e12c03f3 100644 --- a/v-api/src/endpoints/handlers.rs +++ b/v-api/src/endpoints/handlers.rs @@ -17,21 +17,22 @@ mod macros { use v_api::endpoints::{ api_user::{ - add_api_user_permission_op, add_api_user_to_group_op, create_api_user_op, create_api_user_token_op, + add_api_user_permission_op, add_api_user_to_group_op, create_api_user_token_op, delete_api_user_token_op, get_api_user_op, list_api_user_op, get_api_user_token_op, get_self_op, link_provider_op, list_api_user_tokens_op, remove_api_user_from_group_op, remove_api_user_permission_op, set_api_user_contact_email_op, list_api_users_for_group_op, - update_api_user_op, AddGroupBody, ApiKeyCreateParams, ApiKeyResponse, ApiUserPath, + AddGroupBody, ApiKeyCreateParams, ApiKeyResponse, ApiUserPath, ApiUserPermissionParams, ApiUserProviderLinkPayload, ApiUserRemoveGroupPath, ApiUserTokenPath, - ApiUserUpdateParams, GetUserResponse, InitialApiKeyResponse, ApiUserEmailUpdateParams, ApiUserGroupPath + GetUserResponse, InitialApiKeyResponse, ApiUserEmailUpdateParams, ApiUserGroupPath }, api_user_provider::{ create_link_token_op, ApiUserLinkRequestPayload, ApiUserLinkRequestResponse, ApiUserProviderPath, }, group::{ - create_group_op, delete_group_op, get_groups_op, update_group_op, AccessGroupPath, - AccessGroupUpdateParams, + add_group_permission_op, create_group_op, delete_group_op, get_groups_op, + remove_group_permission_op, AccessGroupPath, + AccessGroupPermissionParams, AccessGroupUpdateParams, }, login::{ magic_link:: { @@ -464,32 +465,6 @@ mod macros { get_api_user_op(&rqctx, path).await } - /// Create a new user - #[endpoint { - method = POST, - path = "/api-user", - }] - pub async fn create_api_user( - rqctx: RequestContext<$context_type>, - body: TypedBody>, - ) -> Result>, HttpError> { - create_api_user_op(&rqctx, body).await - } - - /// Update the permissions assigned to a given user. These replace any existing - /// permissions. - #[endpoint { - method = POST, - path = "/api-user/{user_id}", - }] - pub async fn update_api_user( - rqctx: RequestContext<$context_type>, - path: Path, - body: TypedBody>, - ) -> Result>, HttpError> { - update_api_user_op(&rqctx, path.into_inner(), body.into_inner()).await - } - /// Add a single permission to a user #[endpoint { method = POST, @@ -641,17 +616,30 @@ mod macros { create_group_op(&rqctx, body.into_inner()).await } - /// Update a group + /// Add a permission to a group #[endpoint { - method = PUT, - path = "/group/{group_id}", + method = POST, + path = "/group/{group_id}/permission", }] - pub async fn update_group( + pub async fn add_group_permission( rqctx: RequestContext<$context_type>, path: Path, - body: TypedBody>, + body: TypedBody>, ) -> Result>, HttpError> { - update_group_op(&rqctx, path.into_inner(), body.into_inner()).await + add_group_permission_op(&rqctx, path.into_inner(), body.into_inner()).await + } + + /// Remove a permission from a group + #[endpoint { + method = DELETE, + path = "/group/{group_id}/permission", + }] + pub async fn remove_group_permission( + rqctx: RequestContext<$context_type>, + path: Path, + body: TypedBody>, + ) -> Result>, HttpError> { + remove_group_permission_op(&rqctx, path.into_inner(), body.into_inner()).await } /// Delete a group @@ -750,10 +738,6 @@ mod macros { .expect("Failed to register endpoint"); $api.register(get_api_user) .expect("Failed to register endpoint"); - $api.register(create_api_user) - .expect("Failed to register endpoint"); - $api.register(update_api_user) - .expect("Failed to register endpoint"); $api.register(add_api_user_permission) .expect("Failed to register endpoint"); $api.register(remove_api_user_permission) @@ -784,7 +768,9 @@ mod macros { .expect("Failed to register endpoint"); $api.register(create_group) .expect("Failed to register endpoint"); - $api.register(update_group) + $api.register(add_group_permission) + .expect("Failed to register endpoint"); + $api.register(remove_group_permission) .expect("Failed to register endpoint"); $api.register(delete_group) .expect("Failed to register endpoint"); diff --git a/v-api/src/endpoints/login/magic_link/client.rs b/v-api/src/endpoints/login/magic_link/client.rs index c4a4864f..cadd8489 100644 --- a/v-api/src/endpoints/login/magic_link/client.rs +++ b/v-api/src/endpoints/login/magic_link/client.rs @@ -273,6 +273,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, } } @@ -296,8 +297,8 @@ mod tests { providers: vec![], })) }); - user_store.expect_upsert().returning(|user| { - Ok(ApiUserInfo { + user_store.expect_update().returning(|user, _| { + Ok(Some(ApiUserInfo { user: ApiUser { id: user.id, permissions: user.permissions, @@ -305,10 +306,11 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: None, providers: vec![], - }) + })) }); let mut store = MockMagicLinkStore::new(); diff --git a/v-api/src/endpoints/login/oauth/client.rs b/v-api/src/endpoints/login/oauth/client.rs index 1357b914..a8553fbb 100644 --- a/v-api/src/endpoints/login/oauth/client.rs +++ b/v-api/src/endpoints/login/oauth/client.rs @@ -272,6 +272,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, } } @@ -295,8 +296,8 @@ mod tests { providers: vec![], })) }); - user_store.expect_upsert().returning(|user| { - Ok(ApiUserInfo { + user_store.expect_update().returning(|user, _| { + Ok(Some(ApiUserInfo { user: ApiUser { id: user.id, permissions: user.permissions, @@ -304,16 +305,19 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: None, providers: vec![], - }) + })) }); let mut store = MockOAuthClientStore::new(); store.expect_upsert().returning(|client| { Ok(OAuthClient { id: client.id, + api_user_id: None, + keys: vec![], secrets: vec![], redirect_uris: vec![], created_at: Utc::now(), diff --git a/v-api/src/endpoints/login/oauth/flow/code.rs b/v-api/src/endpoints/login/oauth/flow/code.rs index 3852eae3..4c3e5d9e 100644 --- a/v-api/src/endpoints/login/oauth/flow/code.rs +++ b/v-api/src/endpoints/login/oauth/flow/code.rs @@ -1133,6 +1133,8 @@ mod tests { .returning(move |_, _| { Ok(Some(OAuthClient { id: client_id, + api_user_id: None, + keys: vec![], secrets: vec![], redirect_uris: vec![OAuthClientRedirectUri { id: TypedUuid::new_v4(), @@ -1163,6 +1165,8 @@ mod tests { ctx, OAuthClient { id: client_id, + api_user_id: None, + keys: vec![], secrets: vec![OAuthClientSecret { id: TypedUuid::new_v4(), oauth_client_id: client_id, @@ -1189,6 +1193,8 @@ mod tests { let client_id = TypedUuid::new_v4(); let client = OAuthClient { id: client_id, + api_user_id: None, + keys: vec![], secrets: vec![], redirect_uris: vec![OAuthClientRedirectUri { id: TypedUuid::new_v4(), @@ -2361,6 +2367,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: None, providers: vec![], @@ -2438,6 +2445,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: None, providers: vec![], @@ -2876,6 +2884,8 @@ mod tests { .returning(move |_, _| { Ok(Some(OAuthClient { id: client_id, + api_user_id: None, + keys: vec![], secrets: vec![], redirect_uris: vec![], // No registered URIs created_at: Utc::now(), diff --git a/v-api/src/endpoints/mappers.rs b/v-api/src/endpoints/mappers.rs index ae92b09c..47f675c0 100644 --- a/v-api/src/endpoints/mappers.rs +++ b/v-api/src/endpoints/mappers.rs @@ -13,6 +13,7 @@ use tracing::instrument; use v_model::{ Mapper, MapperId, NewMapper, permissions::{AsScope, Permission, PermissionStorage}, + storage::AccessGroupFilter, }; use crate::{ @@ -64,10 +65,15 @@ where { let (ctx, caller) = rqctx.as_ctx().await?; if ctx.mapping.validate(&body.rule) { + let visible_groups = ctx + .group + .list_groups(&caller, AccessGroupFilter::default()) + .await?; let res = ctx .mapping .add_mapper( &caller, + &visible_groups, &NewMapper { id: TypedUuid::new_v4(), name: body.name, diff --git a/v-api/src/mapper/default.rs b/v-api/src/mapper/default.rs index bfc5a957..6143e6ad 100644 --- a/v-api/src/mapper/default.rs +++ b/v-api/src/mapper/default.rs @@ -3,72 +3,40 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use async_trait::async_trait; -use newtype_uuid::TypedUuid; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use std::collections::BTreeSet; -use tracing::instrument; -use v_model::{ - AccessGroupId, - permissions::{Caller, Permissions}, - storage::{AccessGroupFilter, StoreError}, -}; -use crate::{ - context::group::GroupContext, endpoints::login::UserInfo, permissions::VAppPermission, - util::response::ResourceResult, -}; +use crate::endpoints::login::UserInfo; -use super::MapperRule; +use super::{MapperRule, MappingGrants}; -pub struct DefaultMapper { - caller: Caller, - group: GroupContext, - data: DefaultMapperData, +/// Adds all users to a set of groups. +pub struct DefaultMapper { + grants: MappingGrants, } #[derive(Debug, Deserialize, Serialize, JsonSchema)] -pub struct DefaultMapperData { - permissions: Option>, - #[serde(default)] +pub struct DefaultMapperData { groups: Vec, } -impl DefaultMapper { - pub fn new(caller: Caller, group: GroupContext, data: DefaultMapperData) -> Self { +impl DefaultMapper { + pub fn new(data: DefaultMapperData) -> Self { Self { - caller, - group, - data, + grants: MappingGrants { + groups: data.groups, + }, } } } #[async_trait] -impl MapperRule for DefaultMapper -where - T: VAppPermission, -{ - #[instrument(skip(self, _user), field(data = ?self.data))] - async fn permissions_for(&self, _user: &UserInfo) -> Result, StoreError> { - tracing::trace!("Running default mapper"); - Ok(self.data.permissions.clone().unwrap_or_default()) +impl MapperRule for DefaultMapper { + fn grants(&self) -> &MappingGrants { + &self.grants } - #[instrument(skip(self, _user), field(data = ?self.data))] - async fn groups_for( - &self, - _user: &UserInfo, - ) -> ResourceResult>, StoreError> { - tracing::trace!("Running default mapper"); - let known_groups = self - .group - .list_groups(&self.caller, AccessGroupFilter::default()) - .await?; - - Ok(super::resolve_mapped_groups( - &self.data.groups, - &known_groups, - )) + async fn matches(&self, _user: &UserInfo) -> bool { + true } } diff --git a/v-api/src/mapper/email_address.rs b/v-api/src/mapper/email_address.rs index 6fc2cde4..9fe90bfc 100644 --- a/v-api/src/mapper/email_address.rs +++ b/v-api/src/mapper/email_address.rs @@ -3,92 +3,45 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use async_trait::async_trait; -use newtype_uuid::TypedUuid; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use std::collections::BTreeSet; -use tracing::instrument; -use v_model::{ - AccessGroupId, - permissions::{Caller, Permissions}, - storage::{AccessGroupFilter, StoreError}, -}; -use crate::{ - context::group::GroupContext, endpoints::login::UserInfo, permissions::VAppPermission, - util::response::ResourceResult, -}; +use crate::endpoints::login::UserInfo; -use super::MapperRule; +use super::{MapperRule, MappingGrants}; -pub struct EmailAddressMapper { - caller: Caller, - group: GroupContext, - data: EmailAddressMapperData, +/// Adds a user with a specific verified email address to a set of groups. +pub struct EmailAddressMapper { + email: String, + grants: MappingGrants, } #[derive(Debug, Deserialize, Serialize, JsonSchema)] -pub struct EmailAddressMapperData { +pub struct EmailAddressMapperData { email: String, - permissions: Option>, - #[serde(default)] groups: Vec, } -impl EmailAddressMapper { - pub fn new(caller: Caller, group: GroupContext, data: EmailAddressMapperData) -> Self { +impl EmailAddressMapper { + pub fn new(data: EmailAddressMapperData) -> Self { Self { - caller, - group, - data, + email: data.email, + grants: MappingGrants { + groups: data.groups, + }, } } } #[async_trait] -impl MapperRule for EmailAddressMapper -where - T: VAppPermission, -{ - #[instrument(skip(self, user), field(data = ?self.data))] - async fn permissions_for(&self, user: &UserInfo) -> Result, StoreError> { - tracing::trace!("Running email address mapper"); - - if user - .verified_emails - .iter() - .any(|email| email == &self.data.email) - { - Ok(self.data.permissions.clone().unwrap_or_default()) - } else { - Ok(Permissions::new()) - } +impl MapperRule for EmailAddressMapper { + fn grants(&self) -> &MappingGrants { + &self.grants } - #[instrument(skip(self, user), field(data = ?self.data))] - async fn groups_for( - &self, - user: &UserInfo, - ) -> ResourceResult>, StoreError> { - tracing::trace!("Running email address mapper"); - - let found_email = user - .verified_emails + async fn matches(&self, user: &UserInfo) -> bool { + user.verified_emails .iter() - .any(|email| email == &self.data.email); - - if found_email { - let known_groups = self - .group - .list_groups(&self.caller, AccessGroupFilter::default()) - .await?; - - Ok(super::resolve_mapped_groups( - &self.data.groups, - &known_groups, - )) - } else { - Ok(BTreeSet::new()) - } + .any(|email| email == &self.email) } } diff --git a/v-api/src/mapper/email_domain.rs b/v-api/src/mapper/email_domain.rs index 224e1a24..910f1b21 100644 --- a/v-api/src/mapper/email_domain.rs +++ b/v-api/src/mapper/email_domain.rs @@ -3,151 +3,72 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use async_trait::async_trait; -use newtype_uuid::TypedUuid; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use std::collections::BTreeSet; -use tracing::instrument; -use v_model::{ - AccessGroupId, - permissions::{Caller, Permissions}, - storage::{AccessGroupFilter, StoreError}, -}; - -use crate::{ - context::group::GroupContext, endpoints::login::UserInfo, permissions::VAppPermission, - util::response::ResourceResult, -}; - -use super::MapperRule; - -pub struct EmailDomainMapper { - caller: Caller, - group: GroupContext, - data: EmailDomainMapperData, + +use crate::endpoints::login::UserInfo; + +use super::{MapperRule, MappingGrants}; + +/// Adds a user with a verified email address in a specific domain to a set of groups. +pub struct EmailDomainMapper { + domain: String, + grants: MappingGrants, } #[derive(Debug, Deserialize, Serialize, JsonSchema)] -pub struct EmailDomainMapperData { +pub struct EmailDomainMapperData { domain: String, - permissions: Option>, - #[serde(default)] groups: Vec, } -impl EmailDomainMapper { - pub fn new(caller: Caller, group: GroupContext, data: EmailDomainMapperData) -> Self { +impl EmailDomainMapper { + pub fn new(data: EmailDomainMapperData) -> Self { Self { - caller, - group, - data, + domain: data.domain, + grants: MappingGrants { + groups: data.groups, + }, } } } #[async_trait] -impl MapperRule for EmailDomainMapper -where - T: VAppPermission, -{ - #[instrument(skip(self, _user), field(data = ?self.data))] - async fn permissions_for(&self, _user: &UserInfo) -> Result, StoreError> { - Ok(Permissions::new()) +impl MapperRule for EmailDomainMapper { + fn grants(&self) -> &MappingGrants { + &self.grants } - #[instrument(skip(self, user), field(data = ?self.data))] - async fn groups_for( - &self, - user: &UserInfo, - ) -> ResourceResult>, StoreError> { - tracing::trace!("Running email domain mapper"); - let has_email_in_domain = user.verified_emails.iter().any(|email| { + async fn matches(&self, user: &UserInfo) -> bool { + user.verified_emails.iter().any(|email| { let mut parts = email.split('@'); parts .nth(1) - .map(|domain| domain == self.data.domain) + .map(|domain| domain == self.domain) .unwrap_or(false) - }); - - if has_email_in_domain { - let known_groups = self - .group - .list_groups(&self.caller, AccessGroupFilter::default()) - .await?; - - Ok(super::resolve_mapped_groups( - &self.data.groups, - &known_groups, - )) - } else { - Ok(BTreeSet::new()) - } + }) } } #[cfg(test)] mod tests { - use std::{collections::BTreeSet, sync::Arc}; - - use chrono::Utc; - use newtype_uuid::TypedUuid; - use std::collections::HashMap; - use v_model::{ - AccessGroup, AccessGroupSource, - permissions::{Caller, Permissions}, - storage::MockAccessGroupStore, - }; + use serde_json::json; use crate::{ - context::{group::GroupContext, test_mocks::MockStorage}, endpoints::login::{ExternalUserId, UserInfo}, mapper::MapperRule, - permissions::VPermission, }; use super::{EmailDomainMapper, EmailDomainMapperData}; - /// Build a `GroupContext` whose backing store returns a single group with - /// the provided name/id, mimicking a deployment that has an "employees" - /// group reserved for members of a configured email domain. - fn group_context_with_group( - group_id: TypedUuid, - group_name: &str, - ) -> GroupContext { - let group = AccessGroup { - id: group_id, - name: group_name.to_string(), - permissions: Permissions::::new(), - source: AccessGroupSource::Dynamic, - created_at: Utc::now(), - updated_at: Utc::now(), - deleted_at: None, - }; - - let mut group_store = MockAccessGroupStore::new(); - group_store - .expect_list() - .returning(move |_, _| Ok(vec![group.clone()])); - - let mut storage = MockStorage::new(); - storage.access_group_store = Some(Arc::new(group_store)); - - GroupContext::new(Arc::new(storage)) - } - - /// Mirror the builtin registration caller, which the mapping engine uses - /// when running mappers during `register_api_user`. It holds broad group - /// permissions, so every matched group is unconditionally granted. - fn registration_caller() -> Caller { - Caller { - id: "00000000-0000-4000-8000-000000000001".parse().unwrap(), - permissions: vec![ - VPermission::GetGroupsAll, - VPermission::ManageGroupMembershipsAll, - ] - .into(), - extensions: HashMap::default(), - } + fn employees_mapper() -> EmailDomainMapper { + EmailDomainMapper::new( + serde_json::from_value::(json!({ + "domain": "example.com", + "groups": ["employees"], + })) + .unwrap(), + ) } fn user_with_email(email: &str) -> UserInfo { @@ -159,83 +80,39 @@ mod tests { } } - /// Sanity check: a genuine member of the configured domain is mapped into - /// the reserved group. This is the intended behavior. + /// As a sanity check, a genuine member of the configured domain matches and is handed the + /// reserved group. This is the intended behavior. #[tokio::test] - async fn maps_legitimate_domain_member_into_group() { - let group_id = TypedUuid::new_v4(); - let mapper = EmailDomainMapper::new( - registration_caller(), - group_context_with_group(group_id, "employees"), - EmailDomainMapperData:: { - domain: "example.com".to_string(), - permissions: None, - groups: vec!["employees".to_string()], - }, - ); - - let groups = mapper - .groups_for(&user_with_email("alice@example.com")) - .await - .unwrap(); + async fn matches_legitimate_domain_member() { + let mapper = employees_mapper(); - assert_eq!(groups, BTreeSet::from([group_id])); + assert!(mapper.matches(&user_with_email("alice@example.com")).await); + assert_eq!(vec!["employees".to_string()], mapper.grants().groups); } - /// Exploit: an attacker who owns a lookalike domain whose name merely - /// *ends with* the configured domain (`evilexample.com` vs `example.com`) - /// is granted the reserved group. The `ends_with` check on - /// `email.ends_with(&self.data.domain)` has no `@`/`.` boundary, so - /// `a@evilexample.com`.ends_with(`example.com`) is true. + /// An attacker who owns a lookalike domain whose name merely *ends with* the + /// configured domain (`evilexample.com` vs `example.com`) must not match. An `ends_with` + /// check without an `@` boundary would treat `a@evilexample.com` as a member. #[tokio::test] - async fn lookalike_domain_must_not_be_granted_reserved_group() { - let group_id = TypedUuid::new_v4(); - let mapper = EmailDomainMapper::new( - registration_caller(), - group_context_with_group(group_id, "employees"), - EmailDomainMapperData:: { - domain: "example.com".to_string(), - permissions: None, - groups: vec!["employees".to_string()], - }, - ); - - // Attacker-controlled address at a domain the attacker registered. + async fn lookalike_domain_must_not_match() { let attacker = user_with_email("a@evilexample.com"); - let groups = mapper.groups_for(&attacker).await.unwrap(); assert!( - groups.is_empty(), - "authorization bypass: an attacker owning the lookalike domain \ - 'evilexample.com' was granted the reserved groups {:?} intended \ - only for 'example.com' members", - groups, + !employees_mapper().matches(&attacker).await, + "authorization bypass: an attacker owning the lookalike domain 'evilexample.com' \ + matched a rule intended only for 'example.com' members", ); } - /// A related boundary case: a bare-suffix match without any `@` separator + /// In a related boundary case, a bare suffix match without any `@` separator /// (`notexample.com`) must also be rejected. #[tokio::test] - async fn suffix_only_domain_must_not_be_granted_reserved_group() { - let group_id = TypedUuid::new_v4(); - let mapper = EmailDomainMapper::new( - registration_caller(), - group_context_with_group(group_id, "employees"), - EmailDomainMapperData:: { - domain: "example.com".to_string(), - permissions: None, - groups: vec!["employees".to_string()], - }, - ); - + async fn suffix_only_domain_must_not_match() { let attacker = user_with_email("user@notexample.com"); - let groups = mapper.groups_for(&attacker).await.unwrap(); assert!( - groups.is_empty(), - "authorization bypass: 'notexample.com' was treated as a member of \ - 'example.com' and granted {:?}", - groups, + !employees_mapper().matches(&attacker).await, + "authorization bypass: 'notexample.com' was treated as a member of 'example.com'", ); } } diff --git a/v-api/src/mapper/github_username.rs b/v-api/src/mapper/github_username.rs index 8f9308e7..66cb29fe 100644 --- a/v-api/src/mapper/github_username.rs +++ b/v-api/src/mapper/github_username.rs @@ -3,273 +3,119 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use async_trait::async_trait; -use newtype_uuid::TypedUuid; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use std::collections::BTreeSet; -use v_model::{ - AccessGroupId, - permissions::{Caller, Permissions}, - storage::{AccessGroupFilter, StoreError}, -}; -use crate::{ - context::group::GroupContext, - endpoints::login::{ExternalUserId, UserInfo}, - permissions::VAppPermission, - util::response::ResourceResult, -}; +use crate::endpoints::login::{ExternalUserId, UserInfo}; -use super::MapperRule; +use super::{MapperRule, MappingGrants}; -pub struct GitHubUsernameMapper { - caller: Caller, - group: GroupContext, - data: GitHubUsernameMapperData, +/// Adds a user with a specific GitHub username to a set of groups. +pub struct GitHubUsernameMapper { + github_username: String, + grants: MappingGrants, } #[derive(Debug, Deserialize, Serialize, JsonSchema)] -pub struct GitHubUsernameMapperData { +pub struct GitHubUsernameMapperData { github_username: String, - permissions: Option>, - #[serde(default)] groups: Vec, } -impl GitHubUsernameMapper { - pub fn new( - caller: Caller, - group: GroupContext, - data: GitHubUsernameMapperData, - ) -> Self { +impl GitHubUsernameMapper { + pub fn new(data: GitHubUsernameMapperData) -> Self { Self { - caller, - group, - data, + github_username: data.github_username, + grants: MappingGrants { + groups: data.groups, + }, } } } #[async_trait] -impl MapperRule for GitHubUsernameMapper -where - T: VAppPermission, -{ - async fn permissions_for(&self, user: &UserInfo) -> Result, StoreError> { - if matches!(user.external_id, ExternalUserId::GitHub(_)) { - if user - .display_name - .as_ref() - .map(|u| u == &self.data.github_username) - .unwrap_or(false) - { - Ok(self.data.permissions.clone().unwrap_or_default()) - } else { - Ok(Permissions::new()) - } - } else { - Ok(Permissions::new()) - } +impl MapperRule for GitHubUsernameMapper { + fn grants(&self) -> &MappingGrants { + &self.grants } - async fn groups_for( - &self, - user: &UserInfo, - ) -> ResourceResult>, StoreError> { - if matches!(user.external_id, ExternalUserId::GitHub(_)) { - if user + /// Only a GitHub login can match. Display names from other providers are freely editable by + /// their owners, so they say nothing about who holds a GitHub username. + async fn matches(&self, user: &UserInfo) -> bool { + matches!(user.external_id, ExternalUserId::GitHub(_)) + && user .display_name .as_ref() - .map(|u| u == &self.data.github_username) + .map(|name| name == &self.github_username) .unwrap_or(false) - { - let known_groups = self - .group - .list_groups(&self.caller, AccessGroupFilter::default()) - .await?; - - Ok(super::resolve_mapped_groups( - &self.data.groups, - &known_groups, - )) - } else { - Ok(BTreeSet::new()) - } - } else { - Ok(BTreeSet::new()) - } } } #[cfg(test)] mod tests { - use std::{collections::BTreeSet, collections::HashMap, sync::Arc}; - - use chrono::Utc; - use newtype_uuid::TypedUuid; - use v_model::{ - AccessGroup, AccessGroupSource, - permissions::{Caller, Permissions}, - storage::MockAccessGroupStore, - }; - use crate::{ - context::{group::GroupContext, test_mocks::MockStorage}, endpoints::login::{ExternalUserId, UserInfo}, mapper::MapperRule, - permissions::VPermission, }; use super::{GitHubUsernameMapper, GitHubUsernameMapperData}; - /// Build a `GroupContext` whose backing store returns a single group with - /// the provided name/id, mimicking a deployment that has a "contributors" - /// group reserved for a trusted GitHub contributor. - fn group_context_with_group( - group_id: TypedUuid, - group_name: &str, - ) -> GroupContext { - let group = AccessGroup { - id: group_id, - name: group_name.to_string(), - permissions: Permissions::::new(), - source: AccessGroupSource::Dynamic, - created_at: Utc::now(), - updated_at: Utc::now(), - deleted_at: None, - }; - - let mut group_store = MockAccessGroupStore::new(); - group_store - .expect_list() - .returning(move |_, _| Ok(vec![group.clone()])); - - let mut storage = MockStorage::new(); - storage.access_group_store = Some(Arc::new(group_store)); - - GroupContext::new(Arc::new(storage)) + /// A mapper configured to trust the GitHub username "trusted-dev", granting the + /// `GetMappersAll` permission and membership in the "contributors" group. + fn trusted_dev_mapper() -> GitHubUsernameMapper { + GitHubUsernameMapper::new(GitHubUsernameMapperData { + github_username: "trusted-dev".to_string(), + groups: vec!["contributors".to_string()], + }) } - /// Mirror the builtin registration caller, which the mapping engine uses - /// when running mappers during `register_api_user`. It can grant anything, - /// so every matched group is unconditionally granted. - fn registration_caller() -> Caller { - Caller { - id: "00000000-0000-4000-8000-000000000001".parse().unwrap(), - permissions: vec![ - VPermission::GetGroupsAll, - VPermission::ManageGroupMembershipsAll, - ] - .into(), - extensions: HashMap::default(), + fn user(external_id: ExternalUserId) -> UserInfo { + UserInfo { + external_id, + verified_emails: vec!["trusted-dev@example.com".to_string()], + display_name: Some("trusted-dev".to_string()), + idp_token: None, } } - /// A mapper configured to trust the GitHub username "trusted-dev", granting - /// the `GetMappersAll` permission and membership in the "contributors" - /// group. - fn trusted_dev_mapper( - group_id: TypedUuid, - ) -> GitHubUsernameMapper { - GitHubUsernameMapper::new( - registration_caller(), - group_context_with_group(group_id, "contributors"), - GitHubUsernameMapperData:: { - github_username: "trusted-dev".to_string(), - permissions: Some(vec![VPermission::GetMappersAll].into()), - groups: vec!["contributors".to_string()], - }, - ) - } - - /// Sanity check: a genuine GitHub login whose login/display name is - /// "trusted-dev" receives the mapped permissions and groups. This is the - /// intended behavior. + /// As a sanity check, a genuine GitHub login whose login or display name is "trusted-dev" + /// matches and is handed the mapped permissions and groups. This is the intended behavior. #[tokio::test] - async fn maps_genuine_github_user() { - let group_id = TypedUuid::new_v4(); - let mapper = trusted_dev_mapper(group_id); - - let github_user = UserInfo { - external_id: ExternalUserId::GitHub("trusted-dev".to_string()), - verified_emails: vec!["trusted-dev@example.com".to_string()], - display_name: Some("trusted-dev".to_string()), - idp_token: None, - }; + async fn matches_genuine_github_user() { + let mapper = trusted_dev_mapper(); - assert_eq!( - mapper.permissions_for(&github_user).await.unwrap(), - Permissions::::from(vec![VPermission::GetMappersAll]), - ); - assert_eq!( - mapper.groups_for(&github_user).await.unwrap(), - BTreeSet::from([group_id]), + assert!( + mapper + .matches(&user(ExternalUserId::GitHub("trusted-dev".to_string()))) + .await ); + assert_eq!(vec!["contributors".to_string()], mapper.grants().groups); } - /// Exploit: an attacker with a free Google account sets the account's - /// freely-editable profile display name to "trusted-dev" and logs in via - /// the Google OAuth flow. Because the mapper only compares - /// `user.display_name` and never checks that `user.external_id` is the - /// GitHub variant, the attacker impersonates the trusted GitHub - /// contributor and is granted their permissions and groups. + /// An attacker with a free Google account sets the account's freely editable + /// profile display name to "trusted-dev" and logs in via the Google OAuth flow. Comparing + /// only the display name without checking the provider would let them impersonate the + /// trusted GitHub contributor. #[tokio::test] async fn google_login_must_not_impersonate_github_user() { - let group_id = TypedUuid::new_v4(); - let mapper = trusted_dev_mapper(group_id); - - // Attacker's throwaway Google account with a spoofed display name. - let attacker = UserInfo { - external_id: ExternalUserId::Google("attacker-google-id".to_string()), - verified_emails: vec!["attacker@gmail.com".to_string()], - display_name: Some("trusted-dev".to_string()), - idp_token: None, - }; + let attacker = user(ExternalUserId::Google("attacker-google-id".to_string())); - let permissions = mapper.permissions_for(&attacker).await.unwrap(); assert!( - permissions.is_empty(), - "authorization bypass: a Google login with a spoofed display name \ - 'trusted-dev' was granted the GitHub-mapped permissions {:?}", - permissions, - ); - - let groups = mapper.groups_for(&attacker).await.unwrap(); - assert!( - groups.is_empty(), - "authorization bypass: a Google login with a spoofed display name \ - 'trusted-dev' was granted the GitHub-mapped groups {:?}", - groups, + !trusted_dev_mapper().matches(&attacker).await, + "authorization bypass: a Google login with a spoofed display name 'trusted-dev' \ + matched the GitHub username rule", ); } - /// The same bypass applies to Zendesk, whose `user.name` is self-editable. + /// The same bypass applies to Zendesk, whose `user.name` is self editable. #[tokio::test] async fn zendesk_login_must_not_impersonate_github_user() { - let group_id = TypedUuid::new_v4(); - let mapper = trusted_dev_mapper(group_id); - - let attacker = UserInfo { - external_id: ExternalUserId::Zendesk("attacker-zendesk-id".to_string()), - verified_emails: vec!["attacker@zendesk.example".to_string()], - display_name: Some("trusted-dev".to_string()), - idp_token: None, - }; - - let permissions = mapper.permissions_for(&attacker).await.unwrap(); - assert!( - permissions.is_empty(), - "authorization bypass: a Zendesk login with a spoofed display name \ - 'trusted-dev' was granted the GitHub-mapped permissions {:?}", - permissions, - ); + let attacker = user(ExternalUserId::Zendesk("attacker-zendesk-id".to_string())); - let groups = mapper.groups_for(&attacker).await.unwrap(); assert!( - groups.is_empty(), - "authorization bypass: a Zendesk login with a spoofed display name \ - 'trusted-dev' was granted the GitHub-mapped groups {:?}", - groups, + !trusted_dev_mapper().matches(&attacker).await, + "authorization bypass: a Zendesk login with a spoofed display name 'trusted-dev' \ + matched the GitHub username rule", ); } } diff --git a/v-api/src/mapper/mod.rs b/v-api/src/mapper/mod.rs index 05b5b88b..5e9073a2 100644 --- a/v-api/src/mapper/mod.rs +++ b/v-api/src/mapper/mod.rs @@ -14,16 +14,9 @@ use serde_json::Value; use std::{collections::BTreeSet, error::Error as StdError, fmt::Debug}; use tap::TapFallible; use thiserror::Error; -use v_model::{ - AccessGroup, AccessGroupId, Mapper, - permissions::{Caller, Permissions}, - storage::StoreError, -}; +use v_model::{AccessGroup, AccessGroupId}; -use crate::{ - context::group::GroupContext, endpoints::login::UserInfo, permissions::VAppPermission, - util::response::ResourceResult, -}; +use crate::endpoints::login::UserInfo; use self::{ default::DefaultMapper, email_address::EmailAddressMapper, email_domain::EmailDomainMapper, @@ -36,18 +29,11 @@ pub mod email_domain; pub mod github_username; #[async_trait] -/// Mapping rules that determine permissions and groups for users -pub trait MapperRule: Send + Sync -where - T: VAppPermission, -{ - /// Determines the permissions for a given user. - async fn permissions_for(&self, user: &UserInfo) -> Result, StoreError>; - /// Determines the access groups for a given user. - async fn groups_for( - &self, - user: &UserInfo, - ) -> ResourceResult>, StoreError>; +pub trait MapperRule: Send + Sync + 'static { + /// Everything the rule hands to a user it matches. + fn grants(&self) -> &MappingGrants; + /// Whether the rule applies to the given user. + async fn matches(&self, user: &UserInfo) -> bool; } #[derive(Debug, Error)] @@ -58,11 +44,20 @@ pub enum MappingEngineError { Other(Box), } +/// The groups a mapping rule assigns to a user. +#[derive(Debug, Clone, PartialEq)] +pub struct MappingGrants { + /// Names of the groups a matched user is added to. + pub groups: Vec, +} + /// Interface for generating mapping rules from mapper configurations -pub trait MappingEngine: Send + Sync + 'static { - /// Creates a new mapping rule from a Mapper configuration - fn create_mapping(&self, value: Mapper) -> Result>, MappingEngineError>; - /// Validates whether the provided data represents a known mapping rule +pub trait MappingEngine: Send + Sync + 'static { + // TODO: Would love to fix this so it isn't a Value... + /// Creates a mapping rule from a blob of rule data. + fn create_mapping(&self, rule: &Value) -> Result, MappingEngineError>; + // TODO: Would love to fix this so it isn't a Value... + /// Validates whether the provided data represents a known mapping rule. fn validate_mapping_data(&self, value: &Value) -> bool; } @@ -94,79 +89,42 @@ pub(crate) fn resolve_mapped_groups( } /// Default implementation of the MappingEngine trait -pub struct DefaultMappingEngine { - caller: Caller, - group: GroupContext, -} - -impl DefaultMappingEngine -where - T: VAppPermission, -{ - pub fn new(caller: Caller, group: GroupContext) -> Self { - Self { caller, group } - } -} +#[derive(Default)] +pub struct DefaultMappingEngine; /// The default mapping rule configurations that are supported by the default mapping engine #[derive(Debug, Deserialize, Serialize, JsonSchema)] #[serde(tag = "rule", rename_all = "snake_case")] -pub enum MappingRulesData { - Default(DefaultMapperData), - EmailAddress(EmailAddressMapperData), - EmailDomain(EmailDomainMapperData), +pub enum MappingRulesData { + Default(DefaultMapperData), + EmailAddress(EmailAddressMapperData), + EmailDomain(EmailDomainMapperData), #[serde(rename = "github_username")] - GitHubUsername(GitHubUsernameMapperData), + GitHubUsername(GitHubUsernameMapperData), } -impl MappingEngine for DefaultMappingEngine -where - T: VAppPermission, -{ - fn create_mapping(&self, value: Mapper) -> Result>, MappingEngineError> { - serde_json::from_value::>(value.rule) +impl MappingEngine for DefaultMappingEngine { + fn create_mapping(&self, rule: &Value) -> Result, MappingEngineError> { + serde_json::from_value::(rule.clone()) .map_err(|err| { tracing::error!(?err, "Failed to translate stored rule to mapper"); MappingEngineError::Other(Box::new(err)) }) - .map(move |rule| match rule { - MappingRulesData::Default(data) => { - let res: Box> = Box::new(DefaultMapper::new( - self.caller.clone(), - self.group.clone(), - data, - )); - res - } - MappingRulesData::EmailAddress(data) => { - let res: Box> = Box::new(EmailAddressMapper::new( - self.caller.clone(), - self.group.clone(), - data, - )); - res - } - MappingRulesData::EmailDomain(data) => { - let res: Box> = Box::new(EmailDomainMapper::new( - self.caller.clone(), - self.group.clone(), - data, - )); - res - } - MappingRulesData::GitHubUsername(data) => { - let res: Box> = Box::new(GitHubUsernameMapper::new( - self.caller.clone(), - self.group.clone(), - data, - )); - res - } + .map(|rule| { + let mapping: Box = match rule { + MappingRulesData::Default(data) => Box::new(DefaultMapper::new(data)), + MappingRulesData::EmailAddress(data) => Box::new(EmailAddressMapper::new(data)), + MappingRulesData::EmailDomain(data) => Box::new(EmailDomainMapper::new(data)), + MappingRulesData::GitHubUsername(data) => { + Box::new(GitHubUsernameMapper::new(data)) + } + }; + mapping }) } fn validate_mapping_data(&self, value: &Value) -> bool { - serde_json::from_value::>(value.clone()) + serde_json::from_value::(value.clone()) .tap_err(|err| { tracing::warn!(?value, ?err, "Failed to parse mapping rule"); }) @@ -193,6 +151,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, } } diff --git a/v-api/src/permissions.rs b/v-api/src/permissions.rs index 5e39d57b..464f170f 100644 --- a/v-api/src/permissions.rs +++ b/v-api/src/permissions.rs @@ -22,3 +22,91 @@ impl VAppPermissionResponse for T where T: Permission {} #[partial(VPermissionResponse, attributes(#[serde(tag = "kind", content = "value")]))] #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, JsonSchema)] pub enum VPermission {} + +#[cfg(test)] +mod tests { + use newtype_uuid::TypedUuid; + use std::collections::BTreeSet; + use v_model::permissions::{PermissionStorage, Permissions}; + + use super::VPermission; + + /// Only permissions whose own field carries a set of values are split. Permissions that are + /// relative to their holder stay whole. + #[test] + fn only_value_sets_are_expanded() { + let (a, b) = (TypedUuid::new_v4(), TypedUuid::new_v4()); + + let expanded = VPermission::ManageGroups(BTreeSet::from([a, b])) + .expand_value() + .unwrap(); + assert_eq!(2, expanded.len()); + assert!(expanded.contains(&VPermission::ManageGroup(a))); + assert!(expanded.contains(&VPermission::ManageGroup(b))); + + for relative in [ + VPermission::GetApiUserSelf, + VPermission::ManageApiUsersAssigned, + VPermission::GetGroupsJoined, + VPermission::ManageGroupsAll, + VPermission::ManageGroup(a), + ] { + assert_eq!(None, relative.expand_value(), "{relative:?} was expanded"); + } + } + + /// A set is granted value by value, so it can be assembled from separate holdings but not + /// from holdings that miss any value + #[test] + fn value_sets_are_granted_per_value() { + let (a, b, c) = ( + TypedUuid::new_v4(), + TypedUuid::new_v4(), + TypedUuid::new_v4(), + ); + let held: Permissions = vec![ + VPermission::ManageGroup(a), + VPermission::ManageGroups(BTreeSet::from([b])), + ] + .into(); + + assert!(held.can_grant(&VPermission::ManageGroups(BTreeSet::from([a, b])))); + assert!(!held.can_grant(&VPermission::ManageGroups(BTreeSet::from([a, c])))); + + let all: Permissions = vec![VPermission::ManageGroupsAll].into(); + assert!(all.can_grant(&VPermission::ManageGroups(BTreeSet::from([a, c])))); + } + + /// Removing one value from a stored set keeps the other values of that set + #[test] + fn removing_a_value_keeps_the_rest_of_its_set() { + let (a, b) = (TypedUuid::new_v4(), TypedUuid::new_v4()); + let stored: Permissions = vec![ + VPermission::ManageGroups(BTreeSet::from([a, b])), + VPermission::CreateGroup, + ] + .into(); + + let remaining = VPermission::contract(&stored.without(&VPermission::ManageGroup(a))); + + assert!(remaining.can(&VPermission::ManageGroup(b))); + assert!(!remaining.can(&VPermission::ManageGroup(a))); + assert!(remaining.can(&VPermission::CreateGroup)); + } + + /// Permissions relative to their holder are stored and removed as written, never turned into + /// the specific values they would resolve to + #[test] + fn relative_permissions_are_kept_as_written() { + let stored: Permissions = + vec![VPermission::GetApiUserSelf, VPermission::GetGroupsJoined].into(); + + let remaining = VPermission::contract(&stored.without(&VPermission::CreateGroup)); + assert!(remaining.iter().any(|p| p == &VPermission::GetApiUserSelf)); + assert!(remaining.iter().any(|p| p == &VPermission::GetGroupsJoined)); + + let remaining = VPermission::contract(&stored.without(&VPermission::GetApiUserSelf)); + assert!(!remaining.iter().any(|p| p == &VPermission::GetApiUserSelf)); + assert!(remaining.iter().any(|p| p == &VPermission::GetGroupsJoined)); + } +} diff --git a/v-api/tests/endpoint_macros.rs b/v-api/tests/endpoint_macros.rs index 930f67d0..2f53b09f 100644 --- a/v-api/tests/endpoint_macros.rs +++ b/v-api/tests/endpoint_macros.rs @@ -90,6 +90,7 @@ fn system_endpoints_are_registered() { "/api-user/{user_id}/token/{api_key_id}", "/group", "/group/{group_id}", + "/group/{group_id}/permission", "/login/magic/{channel}/send", "/login/oauth/{provider}/code/token", "/login/oauth/{provider}/device", diff --git a/v-model/Cargo.toml b/v-model/Cargo.toml index 72370433..08200a19 100644 --- a/v-model/Cargo.toml +++ b/v-model/Cargo.toml @@ -13,6 +13,7 @@ sagas = ["steno"] [dependencies] async-bb8-diesel = { workspace = true } async-trait = { workspace = true } +base64 = { workspace = true } bb8 = { workspace = true } chrono = { workspace = true, features = ["serde"] } diesel = { workspace = true, features = ["chrono", "uuid", "serde_json"] } diff --git a/v-model/migrations/2026-10-20-045817_oauth_client_credentials/down.sql b/v-model/migrations/2026-10-20-045817_oauth_client_credentials/down.sql new file mode 100644 index 00000000..00e511f4 --- /dev/null +++ b/v-model/migrations/2026-10-20-045817_oauth_client_credentials/down.sql @@ -0,0 +1,3 @@ +DROP TABLE oauth_client_assertion; +DROP TABLE oauth_client_key; +ALTER TABLE oauth_client DROP COLUMN api_user_id; diff --git a/v-model/migrations/2026-10-20-045817_oauth_client_credentials/up.sql b/v-model/migrations/2026-10-20-045817_oauth_client_credentials/up.sql new file mode 100644 index 00000000..4f01b05d --- /dev/null +++ b/v-model/migrations/2026-10-20-045817_oauth_client_credentials/up.sql @@ -0,0 +1,27 @@ +-- The service user that a client acts as when it authenticates with client credentials. NULL means +-- client credentials are not enabled for the client. A client has exactly one user at most. +ALTER TABLE oauth_client ADD COLUMN api_user_id UUID UNIQUE REFERENCES api_user (id); + +-- Public keys a client signs its assertions with. The thumbprint is unique across every row, +-- including deleted ones. Once registered a key can never be used by another client, even it +-- has been removed. +CREATE TABLE oauth_client_key( + id UUID PRIMARY KEY, + oauth_client_id UUID REFERENCES oauth_client (id) NOT NULL, + thumbprint VARCHAR NOT NULL UNIQUE, + jwk JSONB NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + deleted_at TIMESTAMPTZ +); + +-- Assertions that have been accepted. We keep assertions until their expiration has passed to +-- ensure they can not be replayed. +CREATE TABLE oauth_client_assertion( + oauth_client_id UUID REFERENCES oauth_client (id) NOT NULL, + jti VARCHAR NOT NULL, + expires_at TIMESTAMPTZ NOT NULL, + created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + PRIMARY KEY (oauth_client_id, jti) +); + +CREATE INDEX oauth_client_assertion_expires_at ON oauth_client_assertion (expires_at); diff --git a/v-model/migrations/2026-10-20-164342_record_generation/down.sql b/v-model/migrations/2026-10-20-164342_record_generation/down.sql new file mode 100644 index 00000000..00e62385 --- /dev/null +++ b/v-model/migrations/2026-10-20-164342_record_generation/down.sql @@ -0,0 +1,2 @@ +ALTER TABLE access_groups DROP COLUMN generation; +ALTER TABLE api_user DROP COLUMN generation; diff --git a/v-model/migrations/2026-10-20-164342_record_generation/up.sql b/v-model/migrations/2026-10-20-164342_record_generation/up.sql new file mode 100644 index 00000000..149f4ad4 --- /dev/null +++ b/v-model/migrations/2026-10-20-164342_record_generation/up.sql @@ -0,0 +1,3 @@ +-- Add counters so that read then write updates can be performed without races +ALTER TABLE api_user ADD COLUMN generation BIGINT NOT NULL DEFAULT 0; +ALTER TABLE access_groups ADD COLUMN generation BIGINT NOT NULL DEFAULT 0; diff --git a/v-model/src/db.rs b/v-model/src/db.rs index 6c7c6506..b99346a7 100644 --- a/v-model/src/db.rs +++ b/v-model/src/db.rs @@ -9,12 +9,14 @@ use serde_json::Value; use uuid::Uuid; use crate::{ + jwk::PublicJwk, permissions::Permissions, schema::{ access_groups, api_key, api_user, api_user_access_token, api_user_contact_email, api_user_provider, link_request, login_attempt, magic_link_attempt, magic_link_client, magic_link_client_redirect_uri, magic_link_client_secret, mapper, mapper_event, - oauth_client, oauth_client_redirect_uri, oauth_client_secret, + oauth_client, oauth_client_assertion, oauth_client_key, oauth_client_redirect_uri, + oauth_client_secret, }, schema_ext::MapperSource, schema_ext::{LoginAttemptState, MagicLinkAttemptState}, @@ -43,6 +45,7 @@ pub struct ApiUserModel { pub updated_at: DateTime, pub deleted_at: Option>, pub groups: Vec>, + pub generation: i64, } #[derive(Debug, Deserialize, Serialize, Queryable, Insertable)] @@ -111,6 +114,27 @@ pub struct OAuthClientModel { pub id: Uuid, pub created_at: DateTime, pub deleted_at: Option>, + pub api_user_id: Option, +} + +#[derive(Debug, Deserialize, Serialize, Queryable, Insertable)] +#[diesel(table_name = oauth_client_key)] +pub struct OAuthClientKeyModel { + pub id: Uuid, + pub oauth_client_id: Uuid, + pub thumbprint: String, + pub jwk: PublicJwk, + pub created_at: DateTime, + pub deleted_at: Option>, +} + +#[derive(Debug, Deserialize, Serialize, Queryable, Insertable)] +#[diesel(table_name = oauth_client_assertion)] +pub struct OAuthClientAssertionModel { + pub oauth_client_id: Uuid, + pub jti: String, + pub expires_at: DateTime, + pub created_at: DateTime, } #[derive(Debug, Deserialize, Serialize, Queryable, Insertable)] @@ -188,6 +212,7 @@ pub struct AccessGroupModel { pub created_at: DateTime, pub updated_at: DateTime, pub deleted_at: Option>, + pub generation: i64, } #[derive(Debug, Deserialize, Serialize, Queryable, QueryableByName, Insertable)] diff --git a/v-model/src/jwk.rs b/v-model/src/jwk.rs new file mode 100644 index 00000000..fa0d8646 --- /dev/null +++ b/v-model/src/jwk.rs @@ -0,0 +1,180 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. +use diesel::{ + AsExpression, FromSqlRow, + backend::Backend, + deserialize::{self, FromSql}, + pg::Pg, + serialize::{self, Output, ToSql}, + sql_types::Jsonb, +}; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +/// A public key in JWK form. +#[derive( + Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema, AsExpression, FromSqlRow, +)] +#[diesel(sql_type = Jsonb)] +#[serde(tag = "kty")] +#[non_exhaustive] +pub enum PublicJwk { + /// An Octet Key Pair key as defined by RFC 8037 + #[serde(rename = "OKP")] + Okp(OkpPublicKey), +} + +impl PublicJwk { + /// An Ed25519 public key + pub fn ed25519(public_key: [u8; 32]) -> Self { + Self::Okp(OkpPublicKey::Ed25519 { x: public_key }) + } +} + +/// The public half of an Octet Key Pair key, selected by its `crv` member +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)] +#[serde(tag = "crv", deny_unknown_fields)] +#[non_exhaustive] +pub enum OkpPublicKey { + #[non_exhaustive] + Ed25519 { + /// The 32 byte public key + #[serde(with = "base64url_bytes")] + #[schemars(with = "String")] + x: [u8; 32], + }, +} + +/// RFC 7515: Encodes fixed size key material as unpadded base64url. +mod base64url_bytes { + use base64::{Engine, engine::general_purpose::URL_SAFE_NO_PAD}; + use serde::{Deserialize, Deserializer, Serializer, de::Error}; + + pub fn serialize(bytes: &[u8; N], serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&URL_SAFE_NO_PAD.encode(bytes)) + } + + pub fn deserialize<'de, D, const N: usize>(deserializer: D) -> Result<[u8; N], D::Error> + where + D: Deserializer<'de>, + { + let encoded = String::deserialize(deserializer)?; + let decoded = URL_SAFE_NO_PAD + .decode(encoded.as_bytes()) + .map_err(D::Error::custom)?; + + <[u8; N]>::try_from(decoded.as_slice()).map_err(|_| { + D::Error::invalid_length( + decoded.len(), + &format!("{N} bytes of key material").as_str(), + ) + }) + } +} + +impl ToSql for PublicJwk { + fn to_sql(&self, out: &mut Output) -> serialize::Result { + let value = serde_json::to_value(self)?; + >::to_sql(&value, &mut out.reborrow()) + } +} + +impl FromSql for PublicJwk { + fn from_sql(bytes: ::RawValue<'_>) -> deserialize::Result { + let value = >::from_sql(bytes)?; + Ok(serde_json::from_value(value)?) + } +} + +#[cfg(test)] +mod tests { + use base64::{Engine, engine::general_purpose::URL_SAFE_NO_PAD}; + use serde_json::{Value, json}; + + use super::{OkpPublicKey, PublicJwk}; + + const KEY: [u8; 32] = [7; 32]; + + fn parse(value: Value) -> Result { + serde_json::from_value(value) + } + + #[test] + fn serializes_as_an_rfc_8037_jwk() { + let jwk = serde_json::to_value(PublicJwk::ed25519(KEY)).unwrap(); + + assert_eq!( + json!({ "kty": "OKP", "crv": "Ed25519", "x": URL_SAFE_NO_PAD.encode(KEY) }), + jwk + ); + assert_eq!(PublicJwk::ed25519(KEY), parse(jwk).unwrap()); + } + + /// Members may arrive in any order, as JSON objects are unordered + #[test] + fn selects_the_key_by_type_and_curve() { + let jwk = + parse(json!({ "x": URL_SAFE_NO_PAD.encode(KEY), "crv": "Ed25519", "kty": "OKP" })) + .unwrap(); + + match jwk { + PublicJwk::Okp(OkpPublicKey::Ed25519 { x }) => assert_eq!(KEY, x), + } + } + + /// Unknown members are refused, which is what keeps a private key out of storage + #[test] + fn refuses_unknown_members() { + for (member, value) in [ + ("d", json!(URL_SAFE_NO_PAD.encode(KEY))), + ("x5c", json!(["certificate"])), + ("use", json!("sig")), + ] { + let mut jwk = + json!({ "kty": "OKP", "crv": "Ed25519", "x": URL_SAFE_NO_PAD.encode(KEY) }); + jwk[member] = value; + assert!(parse(jwk).is_err(), "accepted a key with {member}"); + } + } + + #[test] + fn refuses_unsupported_key_types_and_curves() { + for (kty, crv) in [ + ("EC", "P-256"), + ("RSA", "Ed25519"), + ("OKP", "Ed448"), + ("OKP", "X25519"), + ] { + let jwk = json!({ "kty": kty, "crv": crv, "x": URL_SAFE_NO_PAD.encode(KEY) }); + assert!(parse(jwk).is_err(), "accepted kty {kty} with crv {crv}"); + } + + assert!(parse(json!({ "crv": "Ed25519", "x": URL_SAFE_NO_PAD.encode(KEY) })).is_err()); + assert!(parse(json!({ "kty": "OKP", "x": URL_SAFE_NO_PAD.encode(KEY) })).is_err()); + } + + #[test] + fn refuses_ed25519_keys_that_are_not_32_bytes() { + for length in [31, 33, 57] { + let x = URL_SAFE_NO_PAD.encode(vec![7; length]); + let jwk = json!({ "kty": "OKP", "crv": "Ed25519", "x": x }); + assert!(parse(jwk).is_err(), "accepted a {length} byte key"); + } + } + + /// JWK members are unpadded base64url, so padded or standard base64 is refused + #[test] + fn refuses_other_base64_encodings() { + let padded = format!("{}=", URL_SAFE_NO_PAD.encode(KEY)); + let standard = base64::engine::general_purpose::STANDARD.encode([0xfb; 32]); + + for x in [padded, standard] { + let jwk = json!({ "kty": "OKP", "crv": "Ed25519", "x": x }); + assert!(parse(jwk).is_err(), "accepted {x}"); + } + } +} diff --git a/v-model/src/lib.rs b/v-model/src/lib.rs index 637bdd9c..1993b5d5 100644 --- a/v-model/src/lib.rs +++ b/v-model/src/lib.rs @@ -7,8 +7,10 @@ use db::{ AccessGroupModel, ApiKeyModel, ApiUserAccessTokenModel, ApiUserContactEmailModel, ApiUserModel, ApiUserProviderModel, LinkRequestModel, LoginAttemptModel, MagicLinkAttemptModel, MagicLinkModel, MagicLinkRedirectUriModel, MagicLinkSecretModel, MapperEventModel, MapperModel, - OAuthClientModel, OAuthClientRedirectUriModel, OAuthClientSecretModel, + OAuthClientAssertionModel, OAuthClientKeyModel, OAuthClientModel, OAuthClientRedirectUriModel, + OAuthClientSecretModel, }; +use jwk::PublicJwk; use newtype_uuid::{GenericUuid, TypedUuid, TypedUuidKind, TypedUuidTag}; use partial_struct::partial; use schema_ext::MagicLinkAttemptState; @@ -20,6 +22,7 @@ use thiserror::Error; use url::Url; pub mod db; +pub mod jwk; pub mod migrations; pub mod permissions; #[cfg(feature = "sagas")] @@ -77,6 +80,9 @@ pub struct ApiUser { pub updated_at: DateTime, #[partial(NewApiUser(skip))] pub deleted_at: Option>, + #[partial(NewApiUser(skip))] + #[serde(skip)] + pub generation: i64, } impl From> for ApiUser { @@ -92,6 +98,7 @@ impl From> for ApiUser { created_at: value.created_at, updated_at: value.updated_at, deleted_at: value.deleted_at, + generation: value.generation, } } } @@ -400,10 +407,14 @@ impl TypedUuidKind for OAuthClientId { pub struct OAuthClient { pub id: TypedUuid, #[partial(NewOAuthClient(skip))] + pub api_user_id: Option>, + #[partial(NewOAuthClient(skip))] pub secrets: Vec, #[partial(NewOAuthClient(skip))] pub redirect_uris: Vec, #[partial(NewOAuthClient(skip))] + pub keys: Vec, + #[partial(NewOAuthClient(skip))] pub created_at: DateTime, #[partial(NewOAuthClient(skip))] pub deleted_at: Option>, @@ -414,11 +425,14 @@ impl OAuthClient { client: OAuthClientModel, secrets: Vec, redirect_uris: Vec, + keys: Vec, ) -> Self { OAuthClient { id: TypedUuid::from_untyped_uuid(client.id), + api_user_id: client.api_user_id.map(TypedUuid::from_untyped_uuid), secrets, redirect_uris, + keys, created_at: client.created_at, deleted_at: client.deleted_at, } @@ -427,13 +441,7 @@ impl OAuthClient { impl From for OAuthClient { fn from(value: OAuthClientModel) -> Self { - OAuthClient { - id: TypedUuid::from_untyped_uuid(value.id), - secrets: vec![], - redirect_uris: vec![], - created_at: value.created_at, - deleted_at: value.deleted_at, - } + OAuthClient::new(value, vec![], vec![], vec![]) } } @@ -503,6 +511,64 @@ impl From for OAuthClientRedirectUri { } } +#[derive(JsonSchema)] +pub enum OAuthClientKeyId {} +impl TypedUuidKind for OAuthClientKeyId { + fn tag() -> TypedUuidTag { + const TAG: TypedUuidTag = TypedUuidTag::new("oauth-client-key"); + TAG + } +} + +#[partial(NewOAuthClientKey)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct OAuthClientKey { + pub id: TypedUuid, + pub oauth_client_id: TypedUuid, + /// The RFC 7638 thumbprint of the key, which also serves as its `kid`. Unique across all + /// clients. + pub thumbprint: String, + pub jwk: PublicJwk, + #[partial(NewOAuthClientKey(skip))] + pub created_at: DateTime, + #[partial(NewOAuthClientKey(skip))] + pub deleted_at: Option>, +} + +impl From for OAuthClientKey { + fn from(value: OAuthClientKeyModel) -> Self { + OAuthClientKey { + id: TypedUuid::from_untyped_uuid(value.id), + oauth_client_id: TypedUuid::from_untyped_uuid(value.oauth_client_id), + thumbprint: value.thumbprint, + jwk: value.jwk, + created_at: value.created_at, + deleted_at: value.deleted_at, + } + } +} + +#[partial(NewOAuthClientAssertion)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] +pub struct OAuthClientAssertion { + pub oauth_client_id: TypedUuid, + pub jti: String, + pub expires_at: DateTime, + #[partial(NewOAuthClientAssertion(skip))] + pub created_at: DateTime, +} + +impl From for OAuthClientAssertion { + fn from(value: OAuthClientAssertionModel) -> Self { + OAuthClientAssertion { + oauth_client_id: TypedUuid::from_untyped_uuid(value.oauth_client_id), + jti: value.jti, + expires_at: value.expires_at, + created_at: value.created_at, + } + } +} + #[derive(JsonSchema)] pub enum MagicLinkId {} impl TypedUuidKind for MagicLinkId { @@ -690,6 +756,9 @@ pub struct AccessGroup { pub updated_at: DateTime, #[partial(NewAccessGroup(skip))] pub deleted_at: Option>, + #[partial(NewAccessGroup(skip))] + #[serde(skip)] + pub generation: i64, } impl From> for AccessGroup { @@ -703,6 +772,7 @@ impl From> for AccessGroup { created_at: value.created_at, updated_at: value.updated_at, deleted_at: value.deleted_at, + generation: value.generation, } } } @@ -848,6 +918,7 @@ mod tests { created_at: Utc::now(), updated_at: Utc::now(), deleted_at: None, + generation: 0, }, email: Some(ApiUserContactEmail { id: TypedUuid::new_v4(), diff --git a/v-model/src/permissions.rs b/v-model/src/permissions.rs index 916dbd12..6911b0db 100644 --- a/v-model/src/permissions.rs +++ b/v-model/src/permissions.rs @@ -107,13 +107,12 @@ where result } - /// Alias for [`can`](Self::can). A caller can grant any permission they hold. + /// Returns true if the caller could grant the target. See [`Permissions::can_grant`]. pub fn can_grant(&self, target: &T) -> bool { - self.can(target) + self.permissions.can_grant(target) } - /// Alias for [`all`](Self::all). A caller can grant a set of permissions if they hold - /// every one of them. + /// Returns true if the caller could grant every permission in the targets pub fn can_grant_all(&self, targets: &Permissions) -> bool { self.permissions.can_grant_all(targets) } @@ -220,22 +219,57 @@ where } /// Returns true if any held permission implies the target, either by exact match - /// or through the permission hierarchy (e.g., an "All" variant implies specific instances). + /// or through a permission definition. pub fn can(&self, permission: &T) -> bool { let res = self.iter().any(|held| T::implies(held, permission)); tracing::trace!(available = ?self.0, requested = ?permission, result = ?res, "Permissions existence check"); res } - /// Alias for [`can`](Self::can). A permission set can grant any permission it holds. + /// Returns true if this set could grant the target to someone else. pub fn can_grant(&self, target: &T) -> bool { - self.can(target) + // We need to expand the target to verify that every permission it expresses is covered by + // this permission set. We can not use equality as this permission set may contain supersets + // for value carrying permissions compared to the target permission. + match target.expand_value() { + Some(values) => values.iter().all(|value| self.can(value)), + None => self.can(target), + } } - /// Alias for [`all`](Self::all). A permission set can grant a collection of permissions - /// if it holds every one of them. + /// Returns true if this set could grant every permission in the targets pub fn can_grant_all(&self, targets: &Permissions) -> bool { - targets.iter().all(|target| self.can(target)) + targets.iter().all(|target| self.can_grant(target)) + } + + /// Splits every permission that carries a set of values into one permission per value. + pub fn expand_values(&self) -> Permissions { + let mut expanded = Permissions::new(); + for permission in self.iter() { + match permission.expand_value() { + Some(values) => values.into_iter().for_each(|value| { + expanded.insert(value); + }), + None => { + expanded.insert(permission.clone()); + } + } + } + + expanded + } + + /// Returns this set with the target removed. + pub fn without(&self, target: &T) -> Permissions { + let removed = target + .expand_value() + .unwrap_or_else(|| vec![target.clone()]); + let mut remaining = self.expand_values(); + removed.iter().for_each(|value| { + remaining.remove(value); + }); + + remaining } } @@ -359,4 +393,14 @@ pub trait PermissionStorage { fn implies(held: &Self, target: &Self) -> bool where Self: Sized; + + /// Splits a permission that carries a set of values into one permission per value. Returns + /// `None` for any permission that does not carry a set of values, which includes permissions + /// whose meaning is relative to whoever holds them. + fn expand_value(&self) -> Option> + where + Self: Sized, + { + None + } } diff --git a/v-model/src/schema.rs b/v-model/src/schema.rs index 27ddbee1..3917de31 100644 --- a/v-model/src/schema.rs +++ b/v-model/src/schema.rs @@ -20,6 +20,7 @@ diesel::table! { created_at -> Timestamptz, updated_at -> Timestamptz, deleted_at -> Nullable, + generation -> Int8, } } @@ -44,6 +45,7 @@ diesel::table! { updated_at -> Timestamptz, deleted_at -> Nullable, groups -> Array>, + generation -> Int8, } } @@ -202,6 +204,27 @@ diesel::table! { id -> Uuid, created_at -> Timestamptz, deleted_at -> Nullable, + api_user_id -> Nullable, + } +} + +diesel::table! { + oauth_client_assertion (oauth_client_id, jti) { + oauth_client_id -> Uuid, + jti -> Varchar, + expires_at -> Timestamptz, + created_at -> Timestamptz, + } +} + +diesel::table! { + oauth_client_key (id) { + id -> Uuid, + oauth_client_id -> Uuid, + thumbprint -> Varchar, + jwk -> Jsonb, + created_at -> Timestamptz, + deleted_at -> Nullable, } } @@ -257,6 +280,9 @@ diesel::joinable!(api_user_provider -> api_user (api_user_id)); diesel::joinable!(magic_link_attempt -> magic_link_client (magic_link_client_id)); diesel::joinable!(magic_link_client_redirect_uri -> magic_link_client (magic_link_client_id)); diesel::joinable!(magic_link_client_secret -> magic_link_client (magic_link_client_id)); +diesel::joinable!(oauth_client -> api_user (api_user_id)); +diesel::joinable!(oauth_client_assertion -> oauth_client (oauth_client_id)); +diesel::joinable!(oauth_client_key -> oauth_client (oauth_client_id)); diesel::joinable!(oauth_client_redirect_uri -> oauth_client (oauth_client_id)); diesel::joinable!(oauth_client_secret -> oauth_client (oauth_client_id)); @@ -279,6 +305,8 @@ diesel::allow_tables_to_appear_in_same_query!( mapper, mapper_event, oauth_client, + oauth_client_assertion, + oauth_client_key, oauth_client_redirect_uri, oauth_client_secret, ); diff --git a/v-model/src/storage/mod.rs b/v-model/src/storage/mod.rs index 0108b647..aac423f8 100644 --- a/v-model/src/storage/mod.rs +++ b/v-model/src/storage/mod.rs @@ -7,6 +7,7 @@ use std::fmt::Debug; pub use async_bb8_diesel::{ConnectionError, PoolError}; use async_trait::async_trait; use bb8::RunError; +use chrono::{DateTime, Utc}; pub use diesel::result::Error as DbError; use diesel::result::{DatabaseErrorKind, Error as DieselError}; #[cfg(feature = "mock")] @@ -23,8 +24,9 @@ use crate::{ MapperId, MapperSource, NewAccessGroup, NewAccessToken, NewApiKey, NewApiUser, NewApiUserContactEmail, NewApiUserProvider, NewLinkRequest, NewLoginAttempt, NewMagicLink, NewMagicLinkAttempt, NewMagicLinkRedirectUri, NewMagicLinkSecret, NewMapper, NewMapperEvent, - NewOAuthClient, NewOAuthClientRedirectUri, NewOAuthClientSecret, OAuthClient, OAuthClientId, - OAuthClientRedirectUri, OAuthClientSecret, OAuthRedirectUriId, OAuthSecretId, + NewOAuthClient, NewOAuthClientAssertion, NewOAuthClientKey, NewOAuthClientRedirectUri, + NewOAuthClientSecret, OAuthClient, OAuthClientAssertion, OAuthClientId, OAuthClientKey, + OAuthClientKeyId, OAuthClientRedirectUri, OAuthClientSecret, OAuthRedirectUriId, OAuthSecretId, UserContactEmailId, UserId, UserProviderId, schema_ext::{LoginAttemptState, MagicLinkAttemptState}, }; @@ -145,6 +147,11 @@ pub trait ApiUserStore { pagination: &ListPagination, ) -> Result>, StoreError>; async fn upsert(&self, api_user: NewApiUser) -> Result, StoreError>; + async fn update( + &self, + api_user: NewApiUser, + expected_generation: i64, + ) -> Result>, StoreError>; async fn delete(&self, id: &TypedUuid) -> Result>, StoreError>; } @@ -297,6 +304,7 @@ pub trait LoginAttemptStore { #[derive(Debug, Default)] pub struct OAuthClientFilter { pub id: Option>>, + pub api_user_id: Option>>, pub deleted: bool, } @@ -318,6 +326,14 @@ pub trait OAuthClientStore { &self, id: &TypedUuid, ) -> Result, StoreError>; + /// Assign the service user that the client acts as when it authenticates with client + /// credentials. This is a one time operation that can not be undone. None is returned if there + /// the client has already been assigned a user. + async fn assign_api_user( + &self, + id: &TypedUuid, + api_user_id: &TypedUuid, + ) -> Result, StoreError>; } #[cfg_attr(feature = "mock", automock)] @@ -343,6 +359,49 @@ pub trait OAuthClientRedirectUriStore { ) -> Result, StoreError>; } +#[derive(Debug, Default)] +pub struct OAuthClientKeyFilter { + pub id: Option>>, + pub oauth_client_id: Option>>, + pub thumbprint: Option>, + pub deleted: bool, +} + +#[cfg_attr(feature = "mock", automock)] +#[async_trait] +pub trait OAuthClientKeyStore { + async fn get( + &self, + id: &TypedUuid, + deleted: bool, + ) -> Result, StoreError>; + async fn list( + &self, + filter: OAuthClientKeyFilter, + pagination: &ListPagination, + ) -> Result, StoreError>; + /// Store a new key. A key whose thumbprint is already registered to any client, even as a + /// deleted key must fail with [`StoreError::Conflict`]. + async fn insert(&self, key: NewOAuthClientKey) -> Result; + async fn delete( + &self, + id: &TypedUuid, + ) -> Result, StoreError>; +} + +#[cfg_attr(feature = "mock", automock)] +#[async_trait] +pub trait OAuthClientAssertionStore { + /// Record an accepted assertion. This must fail with [`StoreError::Conflict`] when an + /// assertion with the same jti, for a given client, already exists. + async fn record( + &self, + assertion: NewOAuthClientAssertion, + ) -> Result; + /// Returning how many assertions were removed. + async fn delete_expired(&self, before: DateTime) -> Result; +} + #[derive(Debug, Default)] pub struct MagicLinkFilter { pub id: Option>>, @@ -443,7 +502,18 @@ pub trait AccessGroupStore { filter: AccessGroupFilter, pagination: &ListPagination, ) -> Result>, StoreError>; + /// Store a group. When a group with the same id already exists only its permissions are + /// replaced, and its name is kept. async fn upsert(&self, group: &NewAccessGroup) -> Result, StoreError>; + /// Write the group's permissions only if its generation is still `expected_generation`. + /// + /// A group's name is never changed after it is created. Mappers resolve groups by name, so + /// renaming a group would silently change which users mappers place into it. + async fn update( + &self, + group: &NewAccessGroup, + expected_generation: i64, + ) -> Result>, StoreError>; async fn delete( &self, id: &TypedUuid, diff --git a/v-model/src/storage/postgres.rs b/v-model/src/storage/postgres.rs index 995453fd..48fbcd77 100644 --- a/v-model/src/storage/postgres.rs +++ b/v-model/src/storage/postgres.rs @@ -5,7 +5,7 @@ use async_bb8_diesel::{AsyncRunQueryDsl, ConnectionError, ConnectionManager}; use async_trait::async_trait; use bb8::Pool; -use chrono::Utc; +use chrono::{DateTime, Utc}; use diesel::{ ExpressionMethods, OptionalExtension as OptionalExtension2, PgArrayExpressionMethods, insert_into, pg::PgConnection, query_dsl::QueryDsl, update, upsert::excluded, @@ -24,22 +24,24 @@ use crate::{ MapperEvent, MapperId, NewAccessGroup, NewAccessToken, NewApiKey, NewApiUser, NewApiUserContactEmail, NewApiUserProvider, NewLinkRequest, NewLoginAttempt, NewMagicLink, NewMagicLinkAttempt, NewMagicLinkRedirectUri, NewMagicLinkSecret, NewMapper, NewMapperEvent, - NewOAuthClient, NewOAuthClientRedirectUri, NewOAuthClientSecret, OAuthClient, OAuthClientId, - OAuthClientRedirectUri, OAuthClientSecret, OAuthRedirectUriId, OAuthSecretId, + NewOAuthClient, NewOAuthClientAssertion, NewOAuthClientKey, NewOAuthClientRedirectUri, + NewOAuthClientSecret, OAuthClient, OAuthClientAssertion, OAuthClientId, OAuthClientKey, + OAuthClientKeyId, OAuthClientRedirectUri, OAuthClientSecret, OAuthRedirectUriId, OAuthSecretId, UserContactEmailId, UserId, UserProviderId, db::{ AccessGroupModel, ApiKeyModel, ApiUserAccessTokenModel, ApiUserContactEmailModel, ApiUserModel, ApiUserProviderModel, LinkRequestModel, LoginAttemptModel, MagicLinkAttemptModel, MagicLinkModel, MagicLinkRedirectUriModel, MagicLinkSecretModel, - MapperEventModel, MapperModel, OAuthClientModel, OAuthClientRedirectUriModel, - OAuthClientSecretModel, + MapperEventModel, MapperModel, OAuthClientAssertionModel, OAuthClientKeyModel, + OAuthClientModel, OAuthClientRedirectUriModel, OAuthClientSecretModel, }, permissions::Permission, schema::{ access_groups, api_key, api_user, api_user_access_token, api_user_contact_email, api_user_provider, link_request, login_attempt, magic_link_attempt, magic_link_client, magic_link_client_redirect_uri, magic_link_client_secret, mapper, mapper_event, - oauth_client, oauth_client_redirect_uri, oauth_client_secret, + oauth_client, oauth_client_assertion, oauth_client_key, oauth_client_redirect_uri, + oauth_client_secret, }, schema_ext::{LoginAttemptState, MagicLinkAttemptState}, storage::{LinkRequestFilter, LinkRequestStore, StoreError}, @@ -51,8 +53,9 @@ use super::{ ApiUserProviderFilter, ApiUserProviderStore, ApiUserStore, ListPagination, LoginAttemptFilter, LoginAttemptStore, MagicLinkAttemptFilter, MagicLinkAttemptStore, MagicLinkFilter, MagicLinkRedirectUriStore, MagicLinkSecretStore, MagicLinkStore, MapperEventFilter, - MapperEventStore, MapperFilter, MapperStore, OAuthClientFilter, OAuthClientRedirectUriStore, - OAuthClientSecretStore, OAuthClientStore, + MapperEventStore, MapperFilter, MapperStore, OAuthClientAssertionStore, OAuthClientFilter, + OAuthClientKeyFilter, OAuthClientKeyStore, OAuthClientRedirectUriStore, OAuthClientSecretStore, + OAuthClientStore, }; pub type DbPool = Pool>; @@ -228,6 +231,7 @@ where .set(( api_user::permissions.eq(excluded(api_user::permissions)), api_user::groups.eq(excluded(api_user::groups)), + api_user::generation.eq(api_user::generation + 1), )) .execute_async(&*self.pool.get().await?) .await?; @@ -237,10 +241,40 @@ where Ok(ApiUserStore::get(self, &user.id, false).await?.unwrap()) } + async fn update( + &self, + user: NewApiUser, + expected_generation: i64, + ) -> Result>, StoreError> { + let updated = update(api_user::dsl::api_user) + .filter(api_user::id.eq(user.id.into_untyped_uuid())) + .filter(api_user::generation.eq(expected_generation)) + .filter(api_user::deleted_at.is_null()) + .set(( + api_user::permissions.eq(user.permissions.clone()), + api_user::groups.eq(user + .groups + .into_iter() + .map(|g| g.into_untyped_uuid()) + .collect::>()), + api_user::generation.eq(api_user::generation + 1), + )) + .execute_async(&*self.pool.get().await?) + .await?; + + match updated { + 0 => Ok(None), + _ => ApiUserStore::get(self, &user.id, false).await, + } + } + async fn delete(&self, id: &TypedUuid) -> Result>, StoreError> { let _ = update(api_user::dsl::api_user) .filter(api_user::id.eq(id.into_untyped_uuid())) - .set(api_user::deleted_at.eq(Utc::now())) + .set(( + api_user::deleted_at.eq(Utc::now()), + api_user::generation.eq(api_user::generation + 1), + )) .execute_async(&*self.pool.get().await?) .await?; @@ -839,6 +873,7 @@ impl OAuthClientStore for PostgresStore { OAuthClientFilter { id: Some(vec![*id]), deleted, + ..Default::default() }, &ListPagination::default().limit(1), ) @@ -855,9 +890,14 @@ impl OAuthClientStore for PostgresStore { let mut query = oauth_client::dsl::oauth_client .left_join(oauth_client_secret::table) .left_join(oauth_client_redirect_uri::table) + .left_join(oauth_client_key::table) .into_boxed(); - let OAuthClientFilter { id, deleted } = filter; + let OAuthClientFilter { + id, + api_user_id, + deleted, + } = filter; if let Some(id) = id { query = query.filter( @@ -865,6 +905,13 @@ impl OAuthClientStore for PostgresStore { ); } + if let Some(api_user_id) = api_user_id { + query = query.filter( + oauth_client::api_user_id + .eq_any(api_user_id.into_iter().map(|user| user.into_untyped_uuid())), + ); + } + if !deleted { query = query.filter(oauth_client::deleted_at.is_null()); } @@ -875,18 +922,20 @@ impl OAuthClientStore for PostgresStore { OAuthClientModel, Option, Option, + Option, )>(&*self.pool.get().await?) .await? .into_iter() .fold( BTreeMap::new(), - |mut clients, (client, secret, redirect)| { - // The joins produce a row per secret x redirect uri combination, so each - // list is keyed by id to drop the duplicate entries + |mut clients, (client, secret, redirect, key)| { + // The joins produce a row per secret x redirect uri x key combination, so + // each list is keyed by id to drop the duplicate entries let value = clients.entry(client.id).or_insert(( client, BTreeMap::::new(), BTreeMap::::new(), + BTreeMap::::new(), )); // Only include secrets that have not been deleted @@ -906,15 +955,23 @@ impl OAuthClientStore for PostgresStore { .or_insert_with(|| redirect.into()); } + // Only include keys that have not been deleted + if let Some(key) = key + && key.deleted_at.is_none() + { + value.3.entry(key.id).or_insert_with(|| key.into()); + } + clients }, ) .into_iter() - .map(|(_, (client, secrets, redirect_uris))| { + .map(|(_, (client, secrets, redirect_uris, keys))| { OAuthClient::new( client, secrets.into_values().collect(), redirect_uris.into_values().collect(), + keys.into_values().collect(), ) }) .skip(pagination.offset as usize) @@ -945,6 +1002,152 @@ impl OAuthClientStore for PostgresStore { OAuthClientStore::get(self, id, true).await } + + async fn assign_api_user( + &self, + id: &TypedUuid, + api_user_id: &TypedUuid, + ) -> Result, StoreError> { + // This assignment must be one time operation, therefore we are filtering for clients that + // do not have an api_user assigned. + let _ = update(oauth_client::dsl::oauth_client) + .filter(oauth_client::id.eq(id.into_untyped_uuid())) + .filter(oauth_client::api_user_id.is_null()) + .filter(oauth_client::deleted_at.is_null()) + .set(oauth_client::api_user_id.eq(api_user_id.into_untyped_uuid())) + .execute_async(&*self.pool.get().await?) + .await?; + + OAuthClientStore::get(self, id, false).await + } +} + +#[async_trait] +impl OAuthClientKeyStore for PostgresStore { + async fn get( + &self, + id: &TypedUuid, + deleted: bool, + ) -> Result, StoreError> { + let key = OAuthClientKeyStore::list( + self, + OAuthClientKeyFilter { + id: Some(vec![*id]), + deleted, + ..Default::default() + }, + &ListPagination::default().limit(1), + ) + .await?; + + Ok(key.into_iter().nth(0)) + } + + async fn list( + &self, + filter: OAuthClientKeyFilter, + pagination: &ListPagination, + ) -> Result, StoreError> { + let mut query = oauth_client_key::dsl::oauth_client_key.into_boxed(); + + let OAuthClientKeyFilter { + id, + oauth_client_id, + thumbprint, + deleted, + } = filter; + + if let Some(id) = id { + query = query.filter( + oauth_client_key::id.eq_any(id.into_iter().map(|key| key.into_untyped_uuid())), + ); + } + + if let Some(oauth_client_id) = oauth_client_id { + query = query.filter( + oauth_client_key::oauth_client_id.eq_any( + oauth_client_id + .into_iter() + .map(|client| client.into_untyped_uuid()), + ), + ); + } + + if let Some(thumbprint) = thumbprint { + query = query.filter(oauth_client_key::thumbprint.eq_any(thumbprint)); + } + + if !deleted { + query = query.filter(oauth_client_key::deleted_at.is_null()); + } + + let results = query + .offset(pagination.offset) + .limit(pagination.limit) + .order(oauth_client_key::created_at.desc()) + .get_results_async::(&*self.pool.get().await?) + .await?; + + Ok(results.into_iter().map(|key| key.into()).collect()) + } + + async fn insert(&self, key: NewOAuthClientKey) -> Result { + let key_m: OAuthClientKeyModel = insert_into(oauth_client_key::dsl::oauth_client_key) + .values(( + oauth_client_key::id.eq(key.id.into_untyped_uuid()), + oauth_client_key::oauth_client_id.eq(key.oauth_client_id.into_untyped_uuid()), + oauth_client_key::thumbprint.eq(key.thumbprint), + oauth_client_key::jwk.eq(key.jwk), + )) + .get_result_async(&*self.pool.get().await?) + .await?; + + Ok(key_m.into()) + } + + async fn delete( + &self, + id: &TypedUuid, + ) -> Result, StoreError> { + let _ = update(oauth_client_key::dsl::oauth_client_key) + .filter(oauth_client_key::id.eq(id.into_untyped_uuid())) + .filter(oauth_client_key::deleted_at.is_null()) + .set(oauth_client_key::deleted_at.eq(Utc::now())) + .execute_async(&*self.pool.get().await?) + .await?; + + OAuthClientKeyStore::get(self, id, true).await + } +} + +#[async_trait] +impl OAuthClientAssertionStore for PostgresStore { + async fn record( + &self, + assertion: NewOAuthClientAssertion, + ) -> Result { + let assertion_m: OAuthClientAssertionModel = + insert_into(oauth_client_assertion::dsl::oauth_client_assertion) + .values(( + oauth_client_assertion::oauth_client_id + .eq(assertion.oauth_client_id.into_untyped_uuid()), + oauth_client_assertion::jti.eq(assertion.jti), + oauth_client_assertion::expires_at.eq(assertion.expires_at), + )) + .get_result_async(&*self.pool.get().await?) + .await?; + + Ok(assertion_m.into()) + } + + async fn delete_expired(&self, before: DateTime) -> Result { + Ok( + diesel::delete(oauth_client_assertion::dsl::oauth_client_assertion) + .filter(oauth_client_assertion::expires_at.lt(before)) + .execute_async(&*self.pool.get().await?) + .await?, + ) + } } #[async_trait] @@ -1438,8 +1641,9 @@ where .on_conflict(access_groups::id) .do_update() .set(( - access_groups::name.eq(excluded(access_groups::name)), + // The name of an existing group is never changed, see AccessGroupStore::update access_groups::permissions.eq(excluded(access_groups::permissions)), + access_groups::generation.eq(access_groups::generation + 1), )) .get_result_async(&*self.pool.get().await?) .await?; @@ -1447,13 +1651,36 @@ where Ok(group_m.into()) } + async fn update( + &self, + group: &NewAccessGroup, + expected_generation: i64, + ) -> Result>, StoreError> { + let group_m: Option> = update(access_groups::dsl::access_groups) + .filter(access_groups::id.eq(group.id.into_untyped_uuid())) + .filter(access_groups::generation.eq(expected_generation)) + .filter(access_groups::deleted_at.is_null()) + .set(( + access_groups::permissions.eq(group.permissions.clone()), + access_groups::generation.eq(access_groups::generation + 1), + )) + .get_result_async(&*self.pool.get().await?) + .await + .optional()?; + + Ok(group_m.map(|group| group.into())) + } + async fn delete( &self, id: &TypedUuid, ) -> Result>, StoreError> { let _ = update(access_groups::dsl::access_groups) .filter(access_groups::id.eq(id.into_untyped_uuid())) - .set(access_groups::deleted_at.eq(Utc::now())) + .set(( + access_groups::deleted_at.eq(Utc::now()), + access_groups::generation.eq(access_groups::generation + 1), + )) .execute_async(&*self.pool.get().await?) .await?; diff --git a/v-model/tests/postgres.rs b/v-model/tests/postgres.rs index b28560a8..d86aafff 100644 --- a/v-model/tests/postgres.rs +++ b/v-model/tests/postgres.rs @@ -16,12 +16,16 @@ use serde::{Deserialize, Serialize}; use std::ops::{Add, Sub}; use uuid::Uuid; use v_model::{ - NewApiKey, NewApiUser, NewMagicLink, NewMagicLinkAttempt, NewMapper, UserId, + NewAccessGroup, NewApiKey, NewApiUser, NewMagicLink, NewMagicLinkAttempt, NewMapper, + NewOAuthClient, NewOAuthClientAssertion, NewOAuthClientKey, OAuthClientId, UserId, + jwk::PublicJwk, migrations::run_migrations_quiet, schema_ext::MagicLinkAttemptState, storage::{ - ApiKeyFilter, ApiKeyStore, ApiUserFilter, ApiUserStore, ListPagination, - MagicLinkAttemptStore, MagicLinkStore, MapperStore, postgres::PostgresStore, + AccessGroupStore, ApiKeyFilter, ApiKeyStore, ApiUserFilter, ApiUserStore, ListPagination, + MagicLinkAttemptStore, MagicLinkStore, MapperStore, OAuthClientAssertionStore, + OAuthClientKeyFilter, OAuthClientKeyStore, OAuthClientStore, StoreError, + postgres::PostgresStore, }, }; @@ -598,3 +602,333 @@ async fn test_mapper_activation_limit() { assert_eq!(final_mapper.activations, Some(1)); assert!(final_mapper.depleted_at.is_some()); } + +async fn create_user(store: &PostgresStore) -> TypedUuid { + let id = TypedUuid::new_v4(); + ApiUserStore::::upsert( + store, + NewApiUser { + id, + permissions: Default::default(), + groups: BTreeSet::new(), + }, + ) + .await + .unwrap(); + id +} + +async fn create_client(store: &PostgresStore) -> TypedUuid { + let id = TypedUuid::new_v4(); + OAuthClientStore::upsert(store, NewOAuthClient { id }) + .await + .unwrap(); + id +} + +fn new_key(client_id: TypedUuid, thumbprint: &str) -> NewOAuthClientKey { + NewOAuthClientKey { + id: TypedUuid::new_v4(), + oauth_client_id: client_id, + thumbprint: thumbprint.to_string(), + // A distinct key per thumbprint, which is all these tests need + jwk: PublicJwk::ed25519([*thumbprint.as_bytes().last().unwrap(); 32]), + } +} + +// A client's service user can be assigned once and is never rebound afterwards +#[tokio::test] +async fn test_oauth_client_service_user() { + let db = TestDb::new("test_oauth_client_service_user"); + let store = PostgresStore::new(&db.url()).await.unwrap(); + + let client_id = create_client(&store).await; + let client = OAuthClientStore::get(&store, &client_id, false) + .await + .unwrap() + .unwrap(); + assert_eq!(None, client.api_user_id); + + let first = create_user(&store).await; + let client = OAuthClientStore::assign_api_user(&store, &client_id, &first) + .await + .unwrap() + .unwrap(); + assert_eq!(Some(first), client.api_user_id); + + // A second assignment leaves the original service user in place + let second = create_user(&store).await; + let client = OAuthClientStore::assign_api_user(&store, &client_id, &second) + .await + .unwrap() + .unwrap(); + assert_eq!(Some(first), client.api_user_id); + + // A service user backs at most one client + let other_client = create_client(&store).await; + let result = OAuthClientStore::assign_api_user(&store, &other_client, &first).await; + assert!( + matches!(result, Err(StoreError::Conflict)), + "a service user was assigned to two clients, got {result:?}" + ); + + // A deleted client is never assigned a service user + let deleted_client = create_client(&store).await; + OAuthClientStore::delete(&store, &deleted_client) + .await + .unwrap(); + let assigned = OAuthClientStore::assign_api_user(&store, &deleted_client, &second) + .await + .unwrap(); + assert!(assigned.is_none()); + let deleted = OAuthClientStore::get(&store, &deleted_client, true) + .await + .unwrap() + .unwrap(); + assert_eq!(None, deleted.api_user_id); +} + +// Keys are listed on their client, deleted keys drop out of listings, and a thumbprint can +// never be registered twice +#[tokio::test] +async fn test_oauth_client_keys() { + let db = TestDb::new("test_oauth_client_keys"); + let store = PostgresStore::new(&db.url()).await.unwrap(); + + let client_id = create_client(&store).await; + let first = OAuthClientKeyStore::insert(&store, new_key(client_id, "thumbprint-a")) + .await + .unwrap(); + + // The key reads back from its JSONB column exactly as it was written + assert_eq!(new_key(client_id, "thumbprint-a").jwk, first.jwk); + let second = OAuthClientKeyStore::insert(&store, new_key(client_id, "thumbprint-b")) + .await + .unwrap(); + + // The client carries its keys + let client = OAuthClientStore::get(&store, &client_id, false) + .await + .unwrap() + .unwrap(); + let thumbprints = client + .keys + .iter() + .map(|key| key.thumbprint.as_str()) + .collect::>(); + assert_eq!( + BTreeSet::from(["thumbprint-a", "thumbprint-b"]), + thumbprints + ); + + // Keys can be found by thumbprint, which is how an assertion's kid is resolved + let found = OAuthClientKeyStore::list( + &store, + OAuthClientKeyFilter { + thumbprint: Some(vec!["thumbprint-b".to_string()]), + ..Default::default() + }, + &ListPagination::default(), + ) + .await + .unwrap(); + assert_eq!( + vec![second.id], + found.iter().map(|key| key.id).collect::>() + ); + + // A deleted key leaves listings and its client, but is still retrievable when asked for + let deleted = OAuthClientKeyStore::delete(&store, &first.id) + .await + .unwrap() + .unwrap(); + assert!(deleted.deleted_at.is_some()); + assert!( + OAuthClientKeyStore::get(&store, &first.id, false) + .await + .unwrap() + .is_none() + ); + let client = OAuthClientStore::get(&store, &client_id, false) + .await + .unwrap() + .unwrap(); + assert_eq!( + vec![second.id], + client.keys.iter().map(|key| key.id).collect::>() + ); + + // Deleting again keeps the original deletion time + let deleted_again = OAuthClientKeyStore::delete(&store, &first.id) + .await + .unwrap() + .unwrap(); + assert_eq!(deleted.deleted_at, deleted_again.deleted_at); + + // A thumbprint is unique across every client, including keys that have been deleted + let other_client = create_client(&store).await; + for thumbprint in ["thumbprint-a", "thumbprint-b"] { + let result = OAuthClientKeyStore::insert(&store, new_key(other_client, thumbprint)).await; + assert!( + matches!(result, Err(StoreError::Conflict)), + "{thumbprint} was registered twice, got {result:?}" + ); + } +} + +// An assertion is accepted once per client, and expired assertions can be pruned +#[tokio::test] +async fn test_oauth_client_assertions() { + let db = TestDb::new("test_oauth_client_assertions"); + let store = PostgresStore::new(&db.url()).await.unwrap(); + + let client_id = create_client(&store).await; + let other_client = create_client(&store).await; + let expires_at = Utc::now() + TimeDelta::minutes(5); + + let assertion = + |client: TypedUuid, jti: &str, expires_at| NewOAuthClientAssertion { + oauth_client_id: client, + jti: jti.to_string(), + expires_at, + }; + + OAuthClientAssertionStore::record(&store, assertion(client_id, "jti-1", expires_at)) + .await + .unwrap(); + + // Presenting the same jti again is a replay + let replay = + OAuthClientAssertionStore::record(&store, assertion(client_id, "jti-1", expires_at)).await; + assert!( + matches!(replay, Err(StoreError::Conflict)), + "a replayed assertion was accepted, got {replay:?}" + ); + + // The jti is chosen by the client, so another client may use the same value + OAuthClientAssertionStore::record(&store, assertion(other_client, "jti-1", expires_at)) + .await + .unwrap(); + + // Only assertions that expired before the cutoff are pruned + let expired = Utc::now() - TimeDelta::minutes(10); + OAuthClientAssertionStore::record(&store, assertion(client_id, "jti-2", expired)) + .await + .unwrap(); + let pruned = OAuthClientAssertionStore::delete_expired(&store, Utc::now()) + .await + .unwrap(); + assert_eq!(1, pruned); + + // A pruned assertion's jti no longer conflicts, while live ones still do + OAuthClientAssertionStore::record(&store, assertion(client_id, "jti-2", expires_at)) + .await + .unwrap(); + let replay = + OAuthClientAssertionStore::record(&store, assertion(client_id, "jti-1", expires_at)).await; + assert!(matches!(replay, Err(StoreError::Conflict))); +} + +// A conditional write lands only when the record is unchanged since it was read, which is what +// keeps concurrent permission changes from overwriting each other +#[tokio::test] +async fn test_conditional_updates() { + let db = TestDb::new("test_conditional_updates"); + let store = PostgresStore::new(&db.url()).await.unwrap(); + + let user_id = create_user(&store).await; + let read = ApiUserStore::::get(&store, &user_id, false) + .await + .unwrap() + .unwrap(); + + let change = |permissions: Vec| NewApiUser { + id: user_id, + permissions: permissions.into(), + groups: BTreeSet::new(), + }; + + // The first write from this read lands + let written = ApiUserStore::update( + &store, + change(vec![TestPermission::CreateApiUser]), + read.user.generation, + ) + .await + .unwrap() + .expect("a write from a current read should land"); + assert_eq!(read.user.generation + 1, written.user.generation); + + // A second write from the same, now stale, read is rejected and changes nothing + let stale = ApiUserStore::update( + &store, + change(vec![TestPermission::GetApiKey(user_id)]), + read.user.generation, + ) + .await + .unwrap(); + assert!(stale.is_none()); + let current = ApiUserStore::::get(&store, &user_id, false) + .await + .unwrap() + .unwrap(); + assert_eq!(written.user.permissions, current.user.permissions); + + // A deleted user is never written + ApiUserStore::::delete(&store, &user_id) + .await + .unwrap(); + let deleted = ApiUserStore::update(&store, change(vec![]), current.user.generation) + .await + .unwrap(); + assert!(deleted.is_none()); + + // Groups follow the same rule + let group_id = TypedUuid::new_v4(); + let group = AccessGroupStore::::upsert( + &store, + &NewAccessGroup { + id: group_id, + name: "engineering".to_string(), + permissions: Default::default(), + }, + ) + .await + .unwrap(); + let change = |name: &str, permissions: Vec| NewAccessGroup:: { + id: group_id, + name: name.to_string(), + permissions: permissions.into(), + }; + + // A write from the current read lands, and changes the permissions but never the name, even + // when a different name is supplied. Mappers resolve groups by name. + let written = AccessGroupStore::update( + &store, + &change("platform", vec![TestPermission::CreateApiUser]), + group.generation, + ) + .await + .unwrap() + .expect("a write from a current read should land"); + assert_eq!("engineering", written.name); + assert_eq!( + v_model::Permissions::from(vec![TestPermission::CreateApiUser]), + written.permissions + ); + assert_eq!(group.generation + 1, written.generation); + + // Upserting an existing group keeps its name too + let upserted = AccessGroupStore::upsert( + &store, + &change("other", vec![TestPermission::CreateApiUser]), + ) + .await + .unwrap(); + assert_eq!("engineering", upserted.name); + + let stale = AccessGroupStore::update(&store, &change("engineering", vec![]), group.generation) + .await + .unwrap(); + assert!(stale.is_none()); +}