From 4dbb5eb3b9dc990ff7fe032cd045e3d4bf734211 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 29 Jul 2024 12:10:55 +0200 Subject: [PATCH 1/3] admin: set password API --- crates/handlers/src/admin/mod.rs | 2 + crates/handlers/src/admin/v1/mod.rs | 6 + crates/handlers/src/admin/v1/users/mod.rs | 2 + .../src/admin/v1/users/set_password.rs | 190 ++++++++++++++++++ crates/handlers/src/bin/api-schema.rs | 3 +- docs/api/spec.json | 76 +++++++ 6 files changed, 278 insertions(+), 1 deletion(-) create mode 100644 crates/handlers/src/admin/v1/users/set_password.rs diff --git a/crates/handlers/src/admin/mod.rs b/crates/handlers/src/admin/mod.rs index 127be426b..cc070c5b3 100644 --- a/crates/handlers/src/admin/mod.rs +++ b/crates/handlers/src/admin/mod.rs @@ -43,11 +43,13 @@ mod schema; mod v1; use self::call_context::CallContext; +use crate::passwords::PasswordManager; pub fn router() -> (OpenApi, Router) where S: Clone + Send + Sync + 'static, BoxHomeserverConnection: FromRef, + PasswordManager: FromRef, BoxRng: FromRequestParts, CallContext: FromRequestParts, Templates: FromRef, diff --git a/crates/handlers/src/admin/v1/mod.rs b/crates/handlers/src/admin/v1/mod.rs index d37994d1e..6f36456fc 100644 --- a/crates/handlers/src/admin/v1/mod.rs +++ b/crates/handlers/src/admin/v1/mod.rs @@ -21,6 +21,7 @@ use mas_matrix::BoxHomeserverConnection; use mas_storage::BoxRng; use super::call_context::CallContext; +use crate::passwords::PasswordManager; mod users; @@ -28,6 +29,7 @@ pub fn router() -> ApiRouter where S: Clone + Send + Sync + 'static, BoxHomeserverConnection: FromRef, + PasswordManager: FromRef, BoxRng: FromRequestParts, CallContext: FromRequestParts, { @@ -41,6 +43,10 @@ where "/users/:id", get_with(self::users::get, self::users::get_doc), ) + .api_route( + "/users/:id/set-password", + post_with(self::users::set_password, self::users::set_password_doc), + ) .api_route( "/users/by-username/:username", get_with(self::users::by_username, self::users::by_username_doc), diff --git a/crates/handlers/src/admin/v1/users/mod.rs b/crates/handlers/src/admin/v1/users/mod.rs index 8e3df57d5..859c63a6f 100644 --- a/crates/handlers/src/admin/v1/users/mod.rs +++ b/crates/handlers/src/admin/v1/users/mod.rs @@ -18,6 +18,7 @@ mod deactivate; mod get; mod list; mod lock; +mod set_password; mod unlock; pub use self::{ @@ -27,5 +28,6 @@ pub use self::{ get::{doc as get_doc, handler as get}, list::{doc as list_doc, handler as list}, lock::{doc as lock_doc, handler as lock}, + set_password::{doc as set_password_doc, handler as set_password}, unlock::{doc as unlock_doc, handler as unlock}, }; diff --git a/crates/handlers/src/admin/v1/users/set_password.rs b/crates/handlers/src/admin/v1/users/set_password.rs new file mode 100644 index 000000000..13c069488 --- /dev/null +++ b/crates/handlers/src/admin/v1/users/set_password.rs @@ -0,0 +1,190 @@ +// Copyright 2024 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use aide::{transform::TransformOperation, NoApi, OperationIo}; +use axum::{extract::State, response::IntoResponse, Json}; +use hyper::StatusCode; +use mas_storage::BoxRng; +use schemars::JsonSchema; +use serde::Deserialize; +use ulid::Ulid; +use zeroize::Zeroizing; + +use crate::{ + admin::{call_context::CallContext, params::UlidPathParam, response::ErrorResponse}, + impl_from_error_for_route, + passwords::PasswordManager, +}; + +#[derive(Debug, thiserror::Error, OperationIo)] +#[aide(output_with = "Json")] +pub enum RouteError { + #[error(transparent)] + Internal(Box), + + #[error("Password hashing failed")] + Password(#[source] anyhow::Error), + + #[error("User ID {0} not found")] + NotFound(Ulid), +} + +impl_from_error_for_route!(mas_storage::RepositoryError); + +impl IntoResponse for RouteError { + fn into_response(self) -> axum::response::Response { + let error = ErrorResponse::from_error(&self); + let status = match self { + Self::Internal(_) | Self::Password(_) => StatusCode::INTERNAL_SERVER_ERROR, + Self::NotFound(_) => StatusCode::NOT_FOUND, + }; + (status, Json(error)).into_response() + } +} + +fn password_example() -> String { + "hunter2".to_owned() +} + +/// # JSON payload for the `POST /api/admin/v1/users/:id/set-password` endpoint +#[derive(Deserialize, JsonSchema)] +#[schemars(rename = "SetUserPasswordRequest")] +pub struct Request { + /// The password to set for the user + #[schemars(example = "password_example")] + password: String, +} + +pub fn doc(operation: TransformOperation) -> TransformOperation { + operation + .id("setUserPassword") + .summary("Set the password for a user") + .tag("user") + .response_with::<200, StatusCode, _>(|t| t.description("Password was set")) + .response_with::<404, RouteError, _>(|t| { + let response = ErrorResponse::from_error(&RouteError::NotFound(Ulid::nil())); + t.description("User was not found").example(response) + }) +} + +#[tracing::instrument(name = "handler.admin.v1.users.set_password", skip_all, err)] +pub async fn handler( + CallContext { + mut repo, clock, .. + }: CallContext, + NoApi(mut rng): NoApi, + State(password_manager): State, + id: UlidPathParam, + Json(params): Json, +) -> Result { + let user = repo + .user() + .lookup(*id) + .await? + .ok_or(RouteError::NotFound(*id))?; + + let password = Zeroizing::new(params.password.into_bytes()); + let (version, hashed_password) = password_manager + .hash(&mut rng, password) + .await + .map_err(RouteError::Password)?; + + repo.user_password() + .add(&mut rng, &clock, &user, version, hashed_password, None) + .await?; + + repo.save().await?; + + Ok(StatusCode::NO_CONTENT) +} + +#[cfg(test)] +mod tests { + use hyper::{Request, StatusCode}; + use mas_storage::{user::UserPasswordRepository, RepositoryAccess}; + use sqlx::PgPool; + use zeroize::Zeroizing; + + use crate::test_utils::{setup, RequestBuilderExt, ResponseExt, TestState}; + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_set_password(pool: PgPool) { + setup(); + let mut state = TestState::from_pool(pool).await.unwrap(); + let token = state.token_with_scope("urn:mas:admin").await; + + // Create a user + let mut repo = state.repository().await.unwrap(); + let user = repo + .user() + .add(&mut state.rng(), &state.clock, "alice".to_owned()) + .await + .unwrap(); + + // Double-check that the user doesn't have a password + let user_password = repo.user_password().active(&user).await.unwrap(); + assert!(user_password.is_none()); + + repo.save().await.unwrap(); + + let user_id = user.id; + + // Set the password through the API + let request = Request::post(format!("/api/admin/v1/users/{user_id}/set-password")) + .bearer(&token) + .json(serde_json::json!({ + "password": "hunter2", + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::NO_CONTENT); + + // Check that the user now has a password + let mut repo = state.repository().await.unwrap(); + let user_password = repo.user_password().active(&user).await.unwrap().unwrap(); + let password = Zeroizing::new(b"hunter2".to_vec()); + state + .password_manager + .verify( + user_password.version, + password, + user_password.hashed_password, + ) + .await + .unwrap(); + } + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_unknown_user(pool: PgPool) { + setup(); + let mut state = TestState::from_pool(pool).await.unwrap(); + let token = state.token_with_scope("urn:mas:admin").await; + + // Set the password through the API + let request = Request::post("/api/admin/v1/users/01040G2081040G2081040G2081/set-password") + .bearer(&token) + .json(serde_json::json!({ + "password": "hunter2", + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::NOT_FOUND); + + let body: serde_json::Value = response.json(); + assert_eq!( + body["errors"][0]["title"], + "User ID 01040G2081040G2081040G2081 not found" + ); + } +} diff --git a/crates/handlers/src/bin/api-schema.rs b/crates/handlers/src/bin/api-schema.rs index 058d004da..bfe60831a 100644 --- a/crates/handlers/src/bin/api-schema.rs +++ b/crates/handlers/src/bin/api-schema.rs @@ -1,4 +1,4 @@ -// Copyright 2022 The Matrix.org Foundation C.I.C. +// Copyright 2024 The Matrix.org Foundation C.I.C. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -66,6 +66,7 @@ impl_from_ref!(mas_router::UrlBuilder); impl_from_ref!(mas_templates::Templates); impl_from_ref!(mas_matrix::BoxHomeserverConnection); impl_from_ref!(mas_keystore::Keystore); +impl_from_ref!(mas_handlers::passwords::PasswordManager); fn main() -> Result<(), Box> { let (mut api, _) = mas_handlers::admin_api_router::(); diff --git a/docs/api/spec.json b/docs/api/spec.json index 1d38279b8..33c8a69dc 100644 --- a/docs/api/spec.json +++ b/docs/api/spec.json @@ -310,6 +310,66 @@ } } }, + "/api/admin/v1/users/{id}/set-password": { + "post": { + "tags": [ + "user" + ], + "summary": "Set the password for a user", + "operationId": "setUserPassword", + "parameters": [ + { + "in": "path", + "name": "id", + "required": true, + "schema": { + "title": "The ID of the resource", + "$ref": "#/components/schemas/ULID" + }, + "style": "simple" + } + ], + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SetUserPasswordRequest" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + } + } + } + }, + "404": { + "description": "User was not found", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + }, + "example": { + "errors": [ + { + "title": "User ID 00000000000000000000000000 not found" + } + ] + } + } + } + } + } + } + }, "/api/admin/v1/users/by-username/{username}": { "get": { "tags": [ @@ -890,6 +950,22 @@ } } }, + "SetUserPasswordRequest": { + "title": "JSON payload for the `POST /api/admin/v1/users/:id/set-password` endpoint", + "type": "object", + "required": [ + "password" + ], + "properties": { + "password": { + "description": "The password to set for the user", + "examples": [ + "hunter2" + ], + "type": "string" + } + } + }, "UsernamePathParam": { "type": "object", "required": [ From 9551f55ba0e76b78bbfe17abeb4b8d1043e5a760 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 6 Aug 2024 17:08:51 +0200 Subject: [PATCH 2/3] admin: check password complexity in password set API --- .../src/admin/v1/users/set_password.rs | 86 ++++++++++++++++++- docs/api/spec.json | 22 +++++ 2 files changed, 105 insertions(+), 3 deletions(-) diff --git a/crates/handlers/src/admin/v1/users/set_password.rs b/crates/handlers/src/admin/v1/users/set_password.rs index 13c069488..0562cf951 100644 --- a/crates/handlers/src/admin/v1/users/set_password.rs +++ b/crates/handlers/src/admin/v1/users/set_password.rs @@ -33,6 +33,9 @@ pub enum RouteError { #[error(transparent)] Internal(Box), + #[error("Password is too weak")] + PasswordTooWeak, + #[error("Password hashing failed")] Password(#[source] anyhow::Error), @@ -47,6 +50,7 @@ impl IntoResponse for RouteError { let error = ErrorResponse::from_error(&self); let status = match self { Self::Internal(_) | Self::Password(_) => StatusCode::INTERNAL_SERVER_ERROR, + Self::PasswordTooWeak => StatusCode::BAD_REQUEST, Self::NotFound(_) => StatusCode::NOT_FOUND, }; (status, Json(error)).into_response() @@ -64,6 +68,9 @@ pub struct Request { /// The password to set for the user #[schemars(example = "password_example")] password: String, + + /// Skip the password complexity check + skip_password_check: Option, } pub fn doc(operation: TransformOperation) -> TransformOperation { @@ -72,6 +79,10 @@ pub fn doc(operation: TransformOperation) -> TransformOperation { .summary("Set the password for a user") .tag("user") .response_with::<200, StatusCode, _>(|t| t.description("Password was set")) + .response_with::<400, RouteError, _>(|t| { + let response = ErrorResponse::from_error(&RouteError::PasswordTooWeak); + t.description("Password is too weak").example(response) + }) .response_with::<404, RouteError, _>(|t| { let response = ErrorResponse::from_error(&RouteError::NotFound(Ulid::nil())); t.description("User was not found").example(response) @@ -94,6 +105,16 @@ pub async fn handler( .await? .ok_or(RouteError::NotFound(*id))?; + let skip_password_check = params.skip_password_check.unwrap_or(false); + tracing::info!(skip_password_check, "skip_password_check"); + if !skip_password_check + && !password_manager + .is_password_complex_enough(¶ms.password) + .unwrap_or(false) + { + return Err(RouteError::PasswordTooWeak); + } + let password = Zeroizing::new(params.password.into_bytes()); let (version, hashed_password) = password_manager .hash(&mut rng, password) @@ -144,7 +165,66 @@ mod tests { let request = Request::post(format!("/api/admin/v1/users/{user_id}/set-password")) .bearer(&token) .json(serde_json::json!({ - "password": "hunter2", + "password": "this is a good enough password", + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::NO_CONTENT); + + // Check that the user now has a password + let mut repo = state.repository().await.unwrap(); + let user_password = repo.user_password().active(&user).await.unwrap().unwrap(); + let password = Zeroizing::new(b"this is a good enough password".to_vec()); + state + .password_manager + .verify( + user_password.version, + password, + user_password.hashed_password, + ) + .await + .unwrap(); + } + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_weak_password(pool: PgPool) { + setup(); + let mut state = TestState::from_pool(pool).await.unwrap(); + let token = state.token_with_scope("urn:mas:admin").await; + + // Create a user + let mut repo = state.repository().await.unwrap(); + let user = repo + .user() + .add(&mut state.rng(), &state.clock, "alice".to_owned()) + .await + .unwrap(); + repo.save().await.unwrap(); + + let user_id = user.id; + + // Set a weak password through the API + let request = Request::post(format!("/api/admin/v1/users/{user_id}/set-password")) + .bearer(&token) + .json(serde_json::json!({ + "password": "password", + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::BAD_REQUEST); + + // Check that the user still has a password + let mut repo = state.repository().await.unwrap(); + let user_password = repo.user_password().active(&user).await.unwrap(); + assert!(user_password.is_none()); + repo.save().await.unwrap(); + + // Now try with the skip_password_check flag + let request = Request::post(format!("/api/admin/v1/users/{user_id}/set-password")) + .bearer(&token) + .json(serde_json::json!({ + "password": "password", + "skip_password_check": true, })); let response = state.request(request).await; @@ -153,7 +233,7 @@ mod tests { // Check that the user now has a password let mut repo = state.repository().await.unwrap(); let user_password = repo.user_password().active(&user).await.unwrap().unwrap(); - let password = Zeroizing::new(b"hunter2".to_vec()); + let password = Zeroizing::new(b"password".to_vec()); state .password_manager .verify( @@ -175,7 +255,7 @@ mod tests { let request = Request::post("/api/admin/v1/users/01040G2081040G2081040G2081/set-password") .bearer(&token) .json(serde_json::json!({ - "password": "hunter2", + "password": "this is a good enough password", })); let response = state.request(request).await; diff --git a/docs/api/spec.json b/docs/api/spec.json index 33c8a69dc..0a68de062 100644 --- a/docs/api/spec.json +++ b/docs/api/spec.json @@ -350,6 +350,23 @@ } } }, + "400": { + "description": "Password is too weak", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + }, + "example": { + "errors": [ + { + "title": "Password is too weak" + } + ] + } + } + } + }, "404": { "description": "User was not found", "content": { @@ -963,6 +980,11 @@ "hunter2" ], "type": "string" + }, + "skip_password_check": { + "description": "Skip the password complexity check", + "type": "boolean", + "nullable": true } } }, From 2ade8c23460d49e9674ff4f2fba268fef95f3061 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 7 Aug 2024 11:52:34 +0200 Subject: [PATCH 3/3] admin: better error when password auth is disabled --- .../src/admin/v1/users/set_password.rs | 38 ++++++++++++++++++- docs/api/spec.json | 17 +++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/crates/handlers/src/admin/v1/users/set_password.rs b/crates/handlers/src/admin/v1/users/set_password.rs index 0562cf951..73e0c5df8 100644 --- a/crates/handlers/src/admin/v1/users/set_password.rs +++ b/crates/handlers/src/admin/v1/users/set_password.rs @@ -36,6 +36,9 @@ pub enum RouteError { #[error("Password is too weak")] PasswordTooWeak, + #[error("Password auth is disabled")] + PasswordAuthDisabled, + #[error("Password hashing failed")] Password(#[source] anyhow::Error), @@ -50,6 +53,7 @@ impl IntoResponse for RouteError { let error = ErrorResponse::from_error(&self); let status = match self { Self::Internal(_) | Self::Password(_) => StatusCode::INTERNAL_SERVER_ERROR, + Self::PasswordAuthDisabled => StatusCode::FORBIDDEN, Self::PasswordTooWeak => StatusCode::BAD_REQUEST, Self::NotFound(_) => StatusCode::NOT_FOUND, }; @@ -83,6 +87,11 @@ pub fn doc(operation: TransformOperation) -> TransformOperation { let response = ErrorResponse::from_error(&RouteError::PasswordTooWeak); t.description("Password is too weak").example(response) }) + .response_with::<403, RouteError, _>(|t| { + let response = ErrorResponse::from_error(&RouteError::PasswordAuthDisabled); + t.description("Password auth is disabled in the server configuration") + .example(response) + }) .response_with::<404, RouteError, _>(|t| { let response = ErrorResponse::from_error(&RouteError::NotFound(Ulid::nil())); t.description("User was not found").example(response) @@ -99,6 +108,10 @@ pub async fn handler( id: UlidPathParam, Json(params): Json, ) -> Result { + if !password_manager.is_enabled() { + return Err(RouteError::PasswordAuthDisabled); + } + let user = repo .user() .lookup(*id) @@ -137,7 +150,10 @@ mod tests { use sqlx::PgPool; use zeroize::Zeroizing; - use crate::test_utils::{setup, RequestBuilderExt, ResponseExt, TestState}; + use crate::{ + passwords::PasswordManager, + test_utils::{setup, RequestBuilderExt, ResponseExt, TestState}, + }; #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] async fn test_set_password(pool: PgPool) { @@ -267,4 +283,24 @@ mod tests { "User ID 01040G2081040G2081040G2081 not found" ); } + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_disabled(pool: PgPool) { + setup(); + let mut state = TestState::from_pool(pool).await.unwrap(); + state.password_manager = PasswordManager::disabled(); + let token = state.token_with_scope("urn:mas:admin").await; + + let request = Request::post("/api/admin/v1/users/01040G2081040G2081040G2081/set-password") + .bearer(&token) + .json(serde_json::json!({ + "password": "hunter2", + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::FORBIDDEN); + + let body: serde_json::Value = response.json(); + assert_eq!(body["errors"][0]["title"], "Password auth is disabled"); + } } diff --git a/docs/api/spec.json b/docs/api/spec.json index 0a68de062..96cebf407 100644 --- a/docs/api/spec.json +++ b/docs/api/spec.json @@ -367,6 +367,23 @@ } } }, + "403": { + "description": "Password auth is disabled in the server configuration", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + }, + "example": { + "errors": [ + { + "title": "Password auth is disabled" + } + ] + } + } + } + }, "404": { "description": "User was not found", "content": {