From 843314d4509fe30d8bcff637e45833e5b15c85d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Gori=C4=8Dar?= Date: Mon, 23 Sep 2024 12:50:42 +0200 Subject: [PATCH] refactor: rework how require_authentication! and similar macros work, begin refactoring user endpoints --- kolomoni/src/api/errors.rs | 3 + kolomoni/src/api/macros.rs | 87 ++++++- kolomoni/src/api/openapi.rs | 5 +- kolomoni/src/api/v1/dictionary/categories.rs | 16 +- .../src/api/v1/dictionary/english/meaning.rs | 8 +- .../src/api/v1/dictionary/english/word.rs | 16 +- .../src/api/v1/dictionary/slovene/meaning.rs | 8 +- .../src/api/v1/dictionary/slovene/word.rs | 16 +- .../src/api/v1/dictionary/translations.rs | 12 +- kolomoni/src/api/v1/users/all.rs | 17 +- kolomoni/src/api/v1/users/current.rs | 90 ++++--- kolomoni/src/api/v1/users/mod.rs | 3 +- kolomoni/src/api/v1/users/registration.rs | 7 +- kolomoni/src/api/v1/users/specific.rs | 243 ++++++++++-------- kolomoni/src/cli.rs | 2 +- kolomoni/src/state.rs | 2 +- kolomoni_auth/src/permissions.rs | 7 + kolomoni_auth/src/roles.rs | 2 +- kolomoni_core/src/api_models/users.rs | 4 +- 19 files changed, 329 insertions(+), 219 deletions(-) diff --git a/kolomoni/src/api/errors.rs b/kolomoni/src/api/errors.rs index 61f69ce..1494404 100644 --- a/kolomoni/src/api/errors.rs +++ b/kolomoni/src/api/errors.rs @@ -454,6 +454,9 @@ impl From for APIError { UserQueryError::HasherError { error } => Self::InternalGenericError { error: Box::new(error), }, + UserQueryError::DatabaseConsistencyError { reason } => { + Self::InternalErrorWithReason { reason } + } } } } diff --git a/kolomoni/src/api/macros.rs b/kolomoni/src/api/macros.rs index ecfcfd0..e3bd506 100644 --- a/kolomoni/src/api/macros.rs +++ b/kolomoni/src/api/macros.rs @@ -423,7 +423,7 @@ macro_rules! json_error_response_with_reason { /// } /// ``` #[macro_export] -macro_rules! require_authentication { +macro_rules! require_user_authentication { ($user_auth_extractor:expr) => { $user_auth_extractor .authenticated_user() @@ -556,7 +556,8 @@ macro_rules! require_permission_with_optional_authentication { /// ``` /// #[macro_export] -macro_rules! require_permission { +#[deprecated] +macro_rules! require_permission_OLD { ($permission_set:expr, $required_permission:expr) => { if !$permission_set.has_permission($required_permission) { return Err( @@ -576,3 +577,85 @@ macro_rules! require_permission { } }; } + + +#[macro_export] +macro_rules! require_permission_in_set { + ($permission_set:expr, $required_permission:expr) => { + if !$permission_set.has_permission($required_permission) { + return Err( + $crate::api::errors::APIError::missing_specific_permission($required_permission), + ); + } + }; +} + + +#[macro_export] +macro_rules! require_permission_on_user { + ($database_connection:expr, $authenticated_user:expr, $required_permission:expr) => {{ + if !$authenticated_user + .transitively_has_permission($database_connection, $required_permission) + .await? + { + return Err( + $crate::api::errors::APIError::missing_specific_permission($required_permission), + ); + } + + $authenticated_user + }}; +} + +#[macro_export] +macro_rules! require_user_authentication_and_permission { + ($database_connection:expr, $authentication_extractor:expr, $required_permission:expr) => {{ + let __authenticated_user = $crate::require_user_authentication!($authentication_extractor); + + $crate::require_permission_on_user!( + $database_connection, + __authenticated_user, + $required_permission + ) + }}; +} + +#[deprecated] +#[macro_export] +macro_rules! require_permissionOLD2 { + ($database_connection:expr, on authentication extractor $authentication_extractor:expr, $required_permission:expr) => { + if let Some(authenticated_user) = $authentication_extractor.authenticated_user() { + require_permission!( + $database_connection, + authenticated_user, + $required_permission + ) + } else { + if !$authentication_extractor.is_permission_granted_to_all($required_permission) { + return Err( + $crate::api::errors::APIError::missing_specific_permission($required_permission), + ); + } + } + + if !$authentication_extractor + .transitively_has_permission($database_connection, $required_permission) + .await? + { + return Err( + $crate::api::errors::APIError::missing_specific_permission($required_permission), + ); + } + }; + + ($database_connection:expr, on user $authenticated_user:expr, $required_permission:expr) => { + if !$authenticated_user + .transitively_has_permission($database_connection, $required_permission) + .await? + { + return Err( + $crate::api::errors::APIError::missing_specific_permission($required_permission), + ); + } + }; +} diff --git a/kolomoni/src/api/openapi.rs b/kolomoni/src/api/openapi.rs index 55e2ce0..31e2de2 100644 --- a/kolomoni/src/api/openapi.rs +++ b/kolomoni/src/api/openapi.rs @@ -44,6 +44,7 @@ pub trait RequiredPermission { macro_rules! generate_standalone_requirement_struct { ($permission_variant:ident) => { ::paste::paste! { + // FIXME this first doc isn't emitted for some reason? not a problem, but it's a bit annoying #[doc = concat!( "Corresponds to the [`Permission::", stringify!($permission_variant), @@ -176,7 +177,7 @@ impl utoipa::IntoResponses for FailedAuthenticationRespon ) .build(); - let missing_user_permission_decription = format!("Missing the `{}` permission.", P::name()); + let missing_user_permission_description = format!("Missing the `{}` permission.", P::name()); let mut missing_user_permission_example = serde_json::Map::with_capacity(1); missing_user_permission_example.insert( @@ -185,7 +186,7 @@ impl utoipa::IntoResponses for FailedAuthenticationRespon ); let missing_user_permission_response = ResponseBuilder::new() - .description(missing_user_permission_decription) + .description(missing_user_permission_description) .content( mime::APPLICATION_JSON.to_string(), ContentBuilder::new() diff --git a/kolomoni/src/api/v1/dictionary/categories.rs b/kolomoni/src/api/v1/dictionary/categories.rs index 68bbf94..91d7a3d 100644 --- a/kolomoni/src/api/v1/dictionary/categories.rs +++ b/kolomoni/src/api/v1/dictionary/categories.rs @@ -21,9 +21,9 @@ use crate::{ impl_json_response_builder, json_error_response_with_reason, obtain_database_connection, - require_authentication, - require_permission, + require_permission_OLD, require_permission_with_optional_authentication, + require_user_authentication, state::ApplicationState, }; @@ -121,8 +121,8 @@ pub async fn create_category( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut transaction, authenticated_user, Permission::CategoryCreate @@ -406,8 +406,8 @@ pub async fn update_specific_category( let mut transaction = database_connection.begin().await?; - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut transaction, authenticated_user, Permission::CategoryUpdate @@ -565,8 +565,8 @@ pub async fn delete_specific_category( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut transaction, authenticated_user, Permission::CategoryDelete diff --git a/kolomoni/src/api/v1/dictionary/english/meaning.rs b/kolomoni/src/api/v1/dictionary/english/meaning.rs index 6786a5e..9305cf7 100644 --- a/kolomoni/src/api/v1/dictionary/english/meaning.rs +++ b/kolomoni/src/api/v1/dictionary/english/meaning.rs @@ -17,7 +17,7 @@ use crate::{ authentication::UserAuthenticationExtractor, impl_json_response_builder, obtain_database_connection, - require_permission, + require_permission_OLD, require_permission_with_optional_authentication, state::ApplicationState, }; @@ -165,7 +165,7 @@ pub async fn create_english_word_meaning( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - require_permission!( + require_permission_OLD!( &mut transaction, authentication, Permission::WordUpdate @@ -228,7 +228,7 @@ pub async fn update_english_word_meaning( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - require_permission!( + require_permission_OLD!( &mut transaction, authentication, Permission::WordUpdate @@ -306,7 +306,7 @@ pub async fn delete_english_word_meaning( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - require_permission!( + require_permission_OLD!( &mut transaction, authentication, Permission::WordUpdate diff --git a/kolomoni/src/api/v1/dictionary/english/word.rs b/kolomoni/src/api/v1/dictionary/english/word.rs index 0693a9b..ccd539d 100644 --- a/kolomoni/src/api/v1/dictionary/english/word.rs +++ b/kolomoni/src/api/v1/dictionary/english/word.rs @@ -32,8 +32,8 @@ use crate::{ impl_json_response_builder, json_error_response_with_reason, obtain_database_connection, - require_authentication, - require_permission, + require_user_authentication, + require_permission_OLD, require_permission_with_optional_authentication, state::ApplicationState, }; @@ -323,8 +323,8 @@ pub async fn create_english_word( ) -> EndpointResult { let mut database_connection = obtain_database_connection!(state); - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut database_connection, authenticated_user, Permission::WordCreate @@ -600,8 +600,8 @@ pub async fn update_specific_english_word( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut transaction, authenticated_user, Permission::WordUpdate @@ -720,8 +720,8 @@ pub async fn delete_english_word( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut transaction, authenticated_user, Permission::WordDelete diff --git a/kolomoni/src/api/v1/dictionary/slovene/meaning.rs b/kolomoni/src/api/v1/dictionary/slovene/meaning.rs index ceab1b4..1215565 100644 --- a/kolomoni/src/api/v1/dictionary/slovene/meaning.rs +++ b/kolomoni/src/api/v1/dictionary/slovene/meaning.rs @@ -22,7 +22,7 @@ use crate::{ authentication::UserAuthenticationExtractor, impl_json_response_builder, obtain_database_connection, - require_permission, + require_permission_OLD, require_permission_with_optional_authentication, state::ApplicationState, }; @@ -172,7 +172,7 @@ pub async fn create_slovene_word_meaning( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - require_permission!( + require_permission_OLD!( &mut transaction, authentication, Permission::WordUpdate @@ -238,7 +238,7 @@ pub async fn update_slovene_word_meaning( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - require_permission!( + require_permission_OLD!( &mut transaction, authentication, Permission::WordUpdate @@ -317,7 +317,7 @@ pub async fn delete_slovene_word_meaning( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - require_permission!( + require_permission_OLD!( &mut transaction, authentication, Permission::WordUpdate diff --git a/kolomoni/src/api/v1/dictionary/slovene/word.rs b/kolomoni/src/api/v1/dictionary/slovene/word.rs index 7b1b87e..73b3aa6 100644 --- a/kolomoni/src/api/v1/dictionary/slovene/word.rs +++ b/kolomoni/src/api/v1/dictionary/slovene/word.rs @@ -28,8 +28,8 @@ use crate::{ impl_json_response_builder, json_error_response_with_reason, obtain_database_connection, - require_authentication, - require_permission, + require_user_authentication, + require_permission_OLD, require_permission_with_optional_authentication, state::ApplicationState, }; @@ -393,8 +393,8 @@ pub async fn create_slovene_word( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut transaction, authenticated_user, Permission::WordCreate @@ -671,8 +671,8 @@ pub async fn update_specific_slovene_word( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut transaction, authenticated_user, Permission::WordUpdate @@ -791,8 +791,8 @@ pub async fn delete_specific_slovene_word( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut transaction, authenticated_user, Permission::WordDelete diff --git a/kolomoni/src/api/v1/dictionary/translations.rs b/kolomoni/src/api/v1/dictionary/translations.rs index 14a440b..78c3db0 100644 --- a/kolomoni/src/api/v1/dictionary/translations.rs +++ b/kolomoni/src/api/v1/dictionary/translations.rs @@ -16,8 +16,8 @@ use crate::{ authentication::UserAuthenticationExtractor, json_error_response_with_reason, obtain_database_connection, - require_authentication, - require_permission, + require_permission_OLD, + require_user_authentication, state::ApplicationState, }; @@ -78,8 +78,8 @@ pub async fn create_translation( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut transaction, authenticated_user, Permission::TranslationCreate @@ -216,8 +216,8 @@ pub async fn delete_translation( let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - let authenticated_user = require_authentication!(authentication); - require_permission!( + let authenticated_user = require_user_authentication!(authentication); + require_permission_OLD!( &mut transaction, authenticated_user, Permission::TranslationDelete diff --git a/kolomoni/src/api/v1/users/all.rs b/kolomoni/src/api/v1/users/all.rs index 8164ae9..385aa45 100644 --- a/kolomoni/src/api/v1/users/all.rs +++ b/kolomoni/src/api/v1/users/all.rs @@ -9,8 +9,7 @@ use crate::{ authentication::UserAuthenticationExtractor, impl_json_response_builder, obtain_database_connection, - require_authentication, - require_permission, + require_user_authentication_and_permission, state::ApplicationState, }; @@ -54,25 +53,19 @@ pub async fn get_all_registered_users( // To access this endpoint, the user: // - MUST provide their authentication token, and // - MUST have the `user.any:read` permission. - let authenticated_user = require_authentication!(authentication); - require_permission!( + require_user_authentication_and_permission!( &mut database_connection, - authenticated_user, + authentication, Permission::UserAnyRead ); - - // Load all users from the database and parse them info `UserInformation` instances. + // Load all users from the database and parse each into [`UserInfo`]. let mut all_users_stream = entities::UserQuery::get_all_users(&mut database_connection); - let mut parsed_users = Vec::new(); - while let Some(next_user_result) = all_users_stream.next().await { - let next_user_as_api_model = next_user_result?.into_api_model(); - - parsed_users.push(next_user_as_api_model); + parsed_users.push(next_user_result?.into_api_model()); } diff --git a/kolomoni/src/api/v1/users/current.rs b/kolomoni/src/api/v1/users/current.rs index c55543a..a7f7a57 100644 --- a/kolomoni/src/api/v1/users/current.rs +++ b/kolomoni/src/api/v1/users/current.rs @@ -26,8 +26,9 @@ use crate::{ authentication::UserAuthenticationExtractor, json_error_response_with_reason, obtain_database_connection, - require_authentication, - require_permission, + require_permission_in_set, + require_user_authentication, + require_user_authentication_and_permission, state::ApplicationState, }; @@ -86,14 +87,12 @@ pub async fn get_current_user_info( // To access this endpoint, the user: // - MUST provide an authentication token, and // - MUST have the `user.self:read` permission. - let authenticated_user = require_authentication!(authentication_extractor); - require_permission!( + let authenticated_user = require_user_authentication_and_permission!( &mut database_connection, - authenticated_user, + authentication_extractor, Permission::UserSelfRead ); - let authenticated_user_id = authenticated_user.user_id(); @@ -152,35 +151,34 @@ pub async fn get_current_user_info( #[get("/me/roles")] pub async fn get_current_user_roles( state: ApplicationState, - authentication: UserAuthenticationExtractor, + authentication_extractor: UserAuthenticationExtractor, ) -> EndpointResult { let mut database_connection = obtain_database_connection!(state); - let authenticated_user = require_authentication!(authentication); - require_permission!( + // To access this endpoint, the user: + // - MUST provide an authentication token, and + // - MUST have the `user.self:read` permission. + let authenticated_user = require_user_authentication_and_permission!( &mut database_connection, - authenticated_user, + authentication_extractor, Permission::UserSelfRead ); + let authenticated_user_id = authenticated_user.user_id(); - let user_exists = entities::UserQuery::exists_by_id( - &mut database_connection, - authenticated_user.user_id(), - ) - .await?; + + let user_exists = + entities::UserQuery::exists_by_id(&mut database_connection, authenticated_user_id).await?; if !user_exists { return Err(APIError::not_found()); } - let user_roles = entities::UserRoleQuery::roles_for_user( - &mut database_connection, - authenticated_user.user_id(), - ) - .await?; + let user_roles = + entities::UserRoleQuery::roles_for_user(&mut database_connection, authenticated_user_id) + .await?; let user_role_names = user_roles.role_names(); @@ -231,24 +229,21 @@ async fn get_current_user_effective_permissions( ) -> EndpointResult { let mut database_connection = obtain_database_connection!(state); - - // User must be authenticated and - // have the `user.self:read` permission to access this endpoint. - let authenticated_user = require_authentication!(authentication_extractor); + // To access this endpoint, the user: + // - MUST provide an authentication token, and + // - MUST have the `user.self:read` permission. + let authenticated_user = require_user_authentication!(authentication_extractor); let user_permissions = authenticated_user .fetch_transitive_permissions(&mut database_connection) .await?; - require_permission!(user_permissions, Permission::UserSelfRead); + require_permission_in_set!(user_permissions, Permission::UserSelfRead); - let permission_names = user_permissions - .permission_names() - .into_iter() - .map(|name_static_str| name_static_str.to_string()) - .collect(); - - Ok(UserPermissionsResponse::from_permission_names(permission_names).into_response()) + Ok( + UserPermissionsResponse::from_permission_names(user_permissions.permission_names()) + .into_response(), + ) } @@ -305,33 +300,34 @@ async fn get_current_user_effective_permissions( async fn update_current_user_display_name( state: ApplicationState, authentication_extractor: UserAuthenticationExtractor, - json_data: web::Json, + request_data: web::Json, ) -> EndpointResult { let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - // User must be authenticated and have - // the `user.self:write` permission to access this endpoint. - let authenticated_user = require_authentication!(authentication_extractor); - require_permission!( + // To access this endpoint, the user: + // - MUST provide an authentication token, and + // - MUST have the `user.self:write` permission. + let authenticated_user = require_user_authentication_and_permission!( &mut transaction, - authenticated_user, + authentication_extractor, Permission::UserSelfWrite ); - let authenticated_user_id = authenticated_user.user_id(); - let json_data = json_data.into_inner(); + let request_data = request_data.into_inner(); // Ensure the display name is unique. - let display_name_already_exists = - entities::UserQuery::exists_by_display_name(&mut *transaction, &json_data.new_display_name) - .await?; + let new_display_name_already_exists = entities::UserQuery::exists_by_display_name( + &mut transaction, + &request_data.new_display_name, + ) + .await?; - if display_name_already_exists { + if new_display_name_already_exists { return Ok(json_error_response_with_reason!( StatusCode::CONFLICT, "User with given display name already exists." @@ -341,9 +337,9 @@ async fn update_current_user_display_name( // Update user in the database. let updated_user = entities::UserMutation::change_display_name_by_user_id( - &mut *transaction, + &mut transaction, authenticated_user_id, - &json_data.new_display_name, + &request_data.new_display_name, ) .await?; @@ -353,7 +349,7 @@ async fn update_current_user_display_name( info!( user_id = %authenticated_user_id, - new_display_name = json_data.new_display_name, + new_display_name = request_data.new_display_name, "User has updated their display name." ); diff --git a/kolomoni/src/api/v1/users/mod.rs b/kolomoni/src/api/v1/users/mod.rs index 0e173a0..054bd76 100644 --- a/kolomoni/src/api/v1/users/mod.rs +++ b/kolomoni/src/api/v1/users/mod.rs @@ -6,6 +6,7 @@ use kolomoni_core::api_models::{ UserPermissionsResponse, UserRolesResponse, }; +use kolomoni_database::entities; use registration::register_user; use self::all::get_all_registered_users; @@ -32,7 +33,7 @@ pub mod registration; pub mod specific; -impl IntoApiModel for kolomoni_database::entities::UserModel { +impl IntoApiModel for entities::UserModel { type ApiModel = UserInfo; fn into_api_model(self) -> Self::ApiModel { diff --git a/kolomoni/src/api/v1/users/registration.rs b/kolomoni/src/api/v1/users/registration.rs index 9f59334..652597b 100644 --- a/kolomoni/src/api/v1/users/registration.rs +++ b/kolomoni/src/api/v1/users/registration.rs @@ -61,13 +61,13 @@ impl_json_response_builder!(UserRegistrationResponse); #[post("")] pub async fn register_user( state: ApplicationState, - json_data: web::Json, + request_data: web::Json, ) -> EndpointResult { let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - let registration_request_data = json_data.into_inner(); + let registration_request_data = request_data.into_inner(); // Ensure the provided username is unique. @@ -113,6 +113,9 @@ pub async fn register_user( .await?; + transaction.commit().await?; + + Ok(UserRegistrationResponse { user: newly_created_user.into_api_model(), } diff --git a/kolomoni/src/api/v1/users/specific.rs b/kolomoni/src/api/v1/users/specific.rs index e2b2ecf..f2d252b 100644 --- a/kolomoni/src/api/v1/users/specific.rs +++ b/kolomoni/src/api/v1/users/specific.rs @@ -23,14 +23,16 @@ use crate::{ authentication::UserAuthenticationExtractor, json_error_response_with_reason, obtain_database_connection, - require_authentication, - require_permission, + require_permission_in_set, require_permission_with_optional_authentication, + require_user_authentication, + require_user_authentication_and_permission, state::ApplicationState, }; + /// Get a user's information /// /// This is an expanded version of the `GET /users/me` endpoint, @@ -47,7 +49,7 @@ use crate::{ ( "user_id" = Uuid, Path, - description = "ID of the user to get information about." + description = "ID (UUIDv7) of the user to get information about." ) ), responses( @@ -57,7 +59,7 @@ use crate::{ body = UserInfoResponse, example = json!({ "user": { - "id": 1, + "id": "01921e6d-a0cb-724c-b8aa-31f5b4c78267", "username": "janeznovak", "display_name": "Janez Novak", "joined_at": "2023-06-27T20:33:53.078789Z", @@ -96,20 +98,17 @@ async fn get_specific_user_info( // Return information about the requested user. - let requested_user_id = path_info.into_inner().0; - + let requested_user_id = UserId::new(path_info.into_inner().0); - let user_info_if_they_exist = entities::UserQuery::get_user_by_id( - &mut database_connection, - UserId::new(requested_user_id), - ) - .await?; + let user_info_if_they_exist = + entities::UserQuery::get_user_by_id(&mut database_connection, requested_user_id).await?; let Some(user_info) = user_info_if_they_exist else { return Ok(HttpResponse::NotFound().finish()); }; + Ok(UserInfoResponse { user: user_info.into_api_model(), } @@ -118,6 +117,7 @@ async fn get_specific_user_info( + /// Get a user's roles /// /// # Authentication @@ -131,7 +131,7 @@ async fn get_specific_user_info( ( "user_id" = Uuid, Path, - description = "ID of the user to query roles for." + description = "ID (UUIDv7) of the user to query roles for." ) ), responses( @@ -169,11 +169,11 @@ pub async fn get_specific_user_roles( ); - let target_user_id = UserId::new(path_info.into_inner().0); + let requested_user_id = UserId::new(path_info.into_inner().0); let target_user_exists = - entities::UserQuery::exists_by_id(&mut database_connection, target_user_id).await?; + entities::UserQuery::exists_by_id(&mut database_connection, requested_user_id).await?; if !target_user_exists { return Err(APIError::not_found()); @@ -181,19 +181,18 @@ pub async fn get_specific_user_roles( let target_user_role_set = - entities::UserRoleQuery::roles_for_user(&mut database_connection, target_user_id).await?; - + entities::UserRoleQuery::roles_for_user(&mut database_connection, requested_user_id).await?; - let target_user_role_names = target_user_role_set.role_names(); Ok(UserRolesResponse { - role_names: target_user_role_names, + role_names: target_user_role_set.role_names(), } .into_response()) } + /// Get a user's effective permissions /// /// Returns a list of effective permissions. @@ -212,7 +211,7 @@ pub async fn get_specific_user_roles( ( "user_id" = Uuid, Path, - description = "ID of the user to get effective permissions for." + description = "ID (UUIDv7) of the user to get effective permissions for." ) ), responses( @@ -241,47 +240,42 @@ async fn get_specific_user_effective_permissions( let mut database_connection = obtain_database_connection!(state); - // Only authenticated users with the `user.any:read` permission can access this endpoint. - let authenticated_user = require_authentication!(authentication_extractor); - require_permission!( + // To access this endpoint, the user: + // - MUST provide an authentication token, and + // - MUST have the `user.self:read` permission. + require_user_authentication_and_permission!( &mut database_connection, - authenticated_user, + authentication_extractor, Permission::UserAnyRead ); // Get requested user's permissions. - let target_user_id = UserId::new(path_info.into_inner().0); + let requested_user_id = UserId::new(path_info.into_inner().0); - let target_user_exists = - entities::UserQuery::exists_by_id(&mut database_connection, target_user_id).await?; + let requested_user_exists = + entities::UserQuery::exists_by_id(&mut database_connection, requested_user_id).await?; - if !target_user_exists { + if !requested_user_exists { return Ok(HttpResponse::NotFound().finish()); } - let target_user_permission_set = entities::UserRoleQuery::transitive_permissions_for_user( + let requested_user_permission_set = entities::UserRoleQuery::transitive_permissions_for_user( &mut database_connection, - target_user_id, + requested_user_id, ) .await?; - let permission_names = target_user_permission_set - .permission_names() - .into_iter() - .map(|name| name.to_string()) - .collect(); - Ok(UserPermissionsResponse { - permissions: permission_names, + permissions: requested_user_permission_set.permission_names(), } .into_response()) } -// TODO Continue from here. + /// Update a user's display name @@ -302,7 +296,7 @@ async fn get_specific_user_effective_permissions( ( "user_id" = Uuid, Path, - description = "User ID." + description = "User ID (UUIDv7)." ) ), request_body( @@ -315,7 +309,7 @@ async fn get_specific_user_effective_permissions( body = UserDisplayNameChangeResponse, example = json!({ "user": { - "id": 1, + "id": "01921e73-08f4-7d7c-9e24-769ebe361edc", "username": "janeznovak", "display_name": "Janez Novak Veliki", "joined_at": "2023-06-27T20:33:53.078789Z", @@ -349,28 +343,31 @@ async fn update_specific_user_display_name( state: ApplicationState, authentication_extractor: UserAuthenticationExtractor, path_info: web::Path<(Uuid,)>, - json_data: web::Json, + request_data: web::Json, ) -> EndpointResult { let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - // Only authenticated users with the `user.any:write` permission can modify - // others' display names. Intended for moderation tooling. - let authenticated_user = require_authentication!(authentication_extractor); - require_permission!( + // To access this endpoint, the user: + // - MUST provide an authentication token, and + // - MUST have the `user.self:write` permission. + // + // Intended for moderation tooling. + let authenticated_user = require_user_authentication_and_permission!( &mut transaction, - authenticated_user, + authentication_extractor, Permission::UserAnyWrite ); - let authenticated_user_id = authenticated_user.user_id(); - let target_user_id = UserId::new(path_info.into_inner().0); + + let requested_user_id = UserId::new(path_info.into_inner().0); + let request_data = request_data.into_inner(); // Disallow modifying your own account on these `/{user_id}/*` endpoints. - if authenticated_user_id == target_user_id { + if authenticated_user_id == requested_user_id { return Ok(json_error_response_with_reason!( StatusCode::FORBIDDEN, "Can't modify your own account on this endpoint." @@ -378,25 +375,23 @@ async fn update_specific_user_display_name( } - let target_user_exists = - entities::UserQuery::exists_by_id(&mut transaction, target_user_id).await?; + let requested_user_exists = + entities::UserQuery::exists_by_id(&mut transaction, requested_user_id).await?; - if !target_user_exists { + if !requested_user_exists { return Err(APIError::not_found_with_reason("no such user")); } - let change_request_data = json_data.into_inner(); - // Modify requested user's display name. - let display_name_already_exists = entities::UserQuery::exists_by_display_name( + let new_display_name_already_exists = entities::UserQuery::exists_by_display_name( &mut transaction, - &change_request_data.new_display_name, + &request_data.new_display_name, ) .await?; - if display_name_already_exists { + if new_display_name_already_exists { return Ok(json_error_response_with_reason!( StatusCode::CONFLICT, "User with given display name already exists." @@ -407,8 +402,8 @@ async fn update_specific_user_display_name( // Update requested user's display name. let updated_user = entities::UserMutation::change_display_name_by_user_id( &mut transaction, - target_user_id, - &change_request_data.new_display_name, + requested_user_id, + &request_data.new_display_name, ) .await?; @@ -417,8 +412,8 @@ async fn update_specific_user_display_name( info!( operator_id = %authenticated_user_id, - target_user_id = %target_user_id, - new_display_name = %change_request_data.new_display_name, + target_user_id = %requested_user_id, + new_display_name = %request_data.new_display_name, "User has updated another user's display name." ); @@ -463,7 +458,7 @@ pub struct UserRoleAddRequest { ( "user_id" = Uuid, Path, - description = "ID of the user to add roles to." + description = "ID (UUIDv7) of the user to add roles to." ) ), request_body( @@ -513,7 +508,7 @@ pub struct UserRoleAddRequest { #[post("/{user_id}/roles")] pub async fn add_roles_to_specific_user( state: ApplicationState, - authentication: UserAuthenticationExtractor, + authentication_extractor: UserAuthenticationExtractor, path_info: web::Path<(Uuid,)>, json_data: web::Json, ) -> EndpointResult { @@ -521,27 +516,29 @@ pub async fn add_roles_to_specific_user( let mut transaction = database_connection.begin().await?; - // Only authenticated users with the `user.any:write` permission can add roles - // to other users, but only if they also have that role. + // To access this endpoint, the user: + // - MUST provide an authentication token, and + // - MUST have the `user.self:write` permission. + // // Intended for moderation tooling. - let authenticated_user = require_authentication!(authentication); + let authenticated_user = require_user_authentication!(authentication_extractor); let authenticated_user_roles = authenticated_user.fetch_roles(&mut transaction).await?; - let authenticated_user_permissions = authenticated_user_roles.granted_permission_set(); + let authenticated_user_effective_permissions = authenticated_user_roles.granted_permission_set(); - require_permission!( - authenticated_user_permissions, + require_permission_in_set!( + authenticated_user_effective_permissions, Permission::UserAnyWrite ); let authenticated_user_id = authenticated_user.user_id(); - let target_user_id = UserId::new(path_info.into_inner().0); + let requested_user_id = UserId::new(path_info.into_inner().0); let request_data = json_data.into_inner(); // Disallow modifying your own user account on this endpoint. - if authenticated_user_id == target_user_id { + if authenticated_user_id == requested_user_id { return Ok(json_error_response_with_reason!( StatusCode::FORBIDDEN, "Can't modify your own account on this endpoint." @@ -549,20 +546,22 @@ pub async fn add_roles_to_specific_user( } - let mut roles_to_add_to_user = HashSet::with_capacity(request_data.roles_to_add.len()); + let roles_to_add_to_user = { + let mut roles_to_add_to_user = HashSet::with_capacity(request_data.roles_to_add.len()); - for raw_role_name in request_data.roles_to_add { - let Some(role) = Role::from_name(&raw_role_name) else { - return Ok(json_error_response_with_reason!( - StatusCode::BAD_REQUEST, - format!("{} is an invalid role name", raw_role_name) - )); - }; + for raw_role_name in request_data.roles_to_add { + let Some(role) = Role::from_name(&raw_role_name) else { + return Ok(json_error_response_with_reason!( + StatusCode::BAD_REQUEST, + format!("{} is an invalid role name", raw_role_name) + )); + }; - roles_to_add_to_user.insert(role); - } + roles_to_add_to_user.insert(role); + } - let roles_to_add_to_user = RoleSet::from_role_hash_set(roles_to_add_to_user); + RoleSet::from_role_hash_set(roles_to_add_to_user) + }; // Validate that the authenticated user has all of the roles @@ -582,7 +581,7 @@ pub async fn add_roles_to_specific_user( - let user_exists = entities::UserQuery::exists_by_id(&mut transaction, target_user_id).await?; + let user_exists = entities::UserQuery::exists_by_id(&mut transaction, requested_user_id).await?; if !user_exists { return Err(APIError::not_found_with_reason( @@ -593,14 +592,22 @@ pub async fn add_roles_to_specific_user( let full_updated_user_role_set = entities::UserRoleMutation::add_roles_to_user( &mut transaction, - target_user_id, - roles_to_add_to_user, + requested_user_id, + roles_to_add_to_user.clone(), ) .await?; transaction.commit().await?; + info!( + operator_id = %authenticated_user_id, + updated_user_id = %requested_user_id, + roles_added = ?roles_to_add_to_user, + new_updated_role_set = ?full_updated_user_role_set, + "Added roles to user." + ); + Ok(UserRolesResponse { role_names: full_updated_user_role_set.role_names(), @@ -642,7 +649,7 @@ pub struct UserRoleRemoveRequest { ( "user_id" = Uuid, Path, - description = "ID of the user to remove roles from." + description = "ID (UUIDv7) of the user to remove roles from." ) ), request_body( @@ -692,35 +699,37 @@ pub struct UserRoleRemoveRequest { #[delete("/{user_id}/roles")] pub async fn remove_roles_from_specific_user( state: ApplicationState, - authentication: UserAuthenticationExtractor, + authentication_extractor: UserAuthenticationExtractor, path_info: web::Path<(Uuid,)>, - json_data: web::Json, + request_data: web::Json, ) -> EndpointResult { let mut database_connection = obtain_database_connection!(state); let mut transaction = database_connection.begin().await?; - // Only authenticated users with the `user.any:write` permission can remove roles - // from other users, but only if they also have that role. + // To access this endpoint, the user: + // - MUST provide an authentication token, and + // - MUST have the `user.self:write` permission. + // // Intended for moderation tooling. - let authenticated_user = require_authentication!(authentication); + let authenticated_user = require_user_authentication!(authentication_extractor); let authenticated_user_roles = authenticated_user.fetch_roles(&mut transaction).await?; - let authenticated_user_permissions = authenticated_user_roles.granted_permission_set(); + let authenticated_user_effective_permissions = authenticated_user_roles.granted_permission_set(); - require_permission!( - authenticated_user_permissions, + require_permission_in_set!( + authenticated_user_effective_permissions, Permission::UserAnyWrite ); let authenticated_user_id = authenticated_user.user_id(); - let target_user_id = UserId::new(path_info.into_inner().0); - let request_data = json_data.into_inner(); + let requested_user_id = UserId::new(path_info.into_inner().0); + let request_data = request_data.into_inner(); // Disallow modifying your own user account on this endpoint. - if authenticated_user_id == target_user_id { + if authenticated_user_id == requested_user_id { return Ok(json_error_response_with_reason!( StatusCode::FORBIDDEN, "Can't modify your own account on this endpoint." @@ -728,20 +737,23 @@ pub async fn remove_roles_from_specific_user( } - let mut roles_to_remove_from_user = HashSet::with_capacity(request_data.roles_to_remove.len()); + let roles_to_remove_from_user = { + let mut roles_to_remove_from_user = + HashSet::with_capacity(request_data.roles_to_remove.len()); - for raw_role_name in request_data.roles_to_remove { - let Some(role) = Role::from_name(&raw_role_name) else { - return Ok(json_error_response_with_reason!( - StatusCode::BAD_REQUEST, - format!("{} is an invalid role name", raw_role_name) - )); - }; + for raw_role_name in request_data.roles_to_remove { + let Some(role) = Role::from_name(&raw_role_name) else { + return Ok(json_error_response_with_reason!( + StatusCode::BAD_REQUEST, + format!("{} is an invalid role name", raw_role_name) + )); + }; - roles_to_remove_from_user.insert(role); - } + roles_to_remove_from_user.insert(role); + } - let roles_to_remove_from_user = RoleSet::from_role_hash_set(roles_to_remove_from_user); + RoleSet::from_role_hash_set(roles_to_remove_from_user) + }; // Validate that the authenticated user (caller) has all of the roles @@ -760,7 +772,7 @@ pub async fn remove_roles_from_specific_user( } - let user_exists = entities::UserQuery::exists_by_id(&mut transaction, target_user_id).await?; + let user_exists = entities::UserQuery::exists_by_id(&mut transaction, requested_user_id).await?; if !user_exists { return Err(APIError::not_found_with_reason( @@ -771,12 +783,23 @@ pub async fn remove_roles_from_specific_user( let full_updated_user_role_set = entities::UserRoleMutation::remove_roles_from_user( &mut transaction, - target_user_id, - roles_to_remove_from_user, + requested_user_id, + roles_to_remove_from_user.clone(), ) .await?; + transaction.commit().await?; + + info!( + operator_id = %authenticated_user_id, + updated_user_id = %requested_user_id, + roles_removed = ?roles_to_remove_from_user, + new_updated_role_set = ?full_updated_user_role_set, + "Removed roles from user." + ); + + Ok(UserRolesResponse { role_names: full_updated_user_role_set.role_names(), } diff --git a/kolomoni/src/cli.rs b/kolomoni/src/cli.rs index 8e92e32..5e172de 100644 --- a/kolomoni/src/cli.rs +++ b/kolomoni/src/cli.rs @@ -2,7 +2,7 @@ use std::path::PathBuf; -use clap::Parser; +use clap::{ArgAction, Parser}; /// Server command-line arguments. diff --git a/kolomoni/src/state.rs b/kolomoni/src/state.rs index d92923d..829e688 100644 --- a/kolomoni/src/state.rs +++ b/kolomoni/src/state.rs @@ -13,7 +13,7 @@ use tokio::sync::mpsc; use crate::connect_and_set_up_database; -// TODO needs to be reworked to be more general, then connect search into it, or maybe even setup this whole thing to be decoupled by using db triggers or something +// TODO needs to be reworked to be more general (a cache layer), then connect search into it, or maybe even setup this whole thing to be decoupled by using db triggers or something /// A dictionary search engine. /// /// Handles searching, seeding and incrementally updating the internal index and cache. diff --git a/kolomoni_auth/src/permissions.rs b/kolomoni_auth/src/permissions.rs index a4aa68a..afca7c0 100644 --- a/kolomoni_auth/src/permissions.rs +++ b/kolomoni_auth/src/permissions.rs @@ -279,6 +279,13 @@ impl PermissionSet { .map(|permission| permission.name()) .collect() } + + pub fn permission_names_owned(&self) -> Vec { + self.permissions + .iter() + .map(|permission| permission.name().to_string()) + .collect() + } } diff --git a/kolomoni_auth/src/roles.rs b/kolomoni_auth/src/roles.rs index e11cf53..ff78da0 100644 --- a/kolomoni_auth/src/roles.rs +++ b/kolomoni_auth/src/roles.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::{collections::HashSet, fmt::Display}; use serde::{Deserialize, Serialize}; diff --git a/kolomoni_core/src/api_models/users.rs b/kolomoni_core/src/api_models/users.rs index d1708bf..0508ff3 100644 --- a/kolomoni_core/src/api_models/users.rs +++ b/kolomoni_core/src/api_models/users.rs @@ -216,11 +216,11 @@ pub struct UserRolesResponse { }) )] pub struct UserPermissionsResponse { - pub permissions: Vec, + pub permissions: Vec<&'static str>, } impl UserPermissionsResponse { - pub fn from_permission_names(permission_names: Vec) -> Self { + pub fn from_permission_names(permission_names: Vec<&'static str>) -> Self { Self { permissions: permission_names, }