From e68ede7ed4c33fb58878cd191dc373a16a098b8b Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 26 Jul 2024 18:25:17 +0200 Subject: [PATCH 1/5] admin: add API to create users --- crates/handlers/src/admin/mod.rs | 4 + crates/handlers/src/admin/v1/mod.rs | 12 +- crates/handlers/src/admin/v1/users/add.rs | 179 ++++++++++++++++++ crates/handlers/src/admin/v1/users/mod.rs | 2 + crates/handlers/src/bin/api-schema.rs | 3 + crates/handlers/src/graphql/mutations/user.rs | 2 +- docs/api/spec.json | 107 ++++++++++- 7 files changed, 301 insertions(+), 8 deletions(-) create mode 100644 crates/handlers/src/admin/v1/users/add.rs diff --git a/crates/handlers/src/admin/mod.rs b/crates/handlers/src/admin/mod.rs index da086fe4b..127be426b 100644 --- a/crates/handlers/src/admin/mod.rs +++ b/crates/handlers/src/admin/mod.rs @@ -26,10 +26,12 @@ use hyper::header::{ACCEPT, AUTHORIZATION, CONTENT_TYPE}; use indexmap::IndexMap; use mas_axum_utils::FancyError; use mas_http::CorsLayerExt; +use mas_matrix::BoxHomeserverConnection; use mas_router::{ ApiDoc, ApiDocCallback, OAuth2AuthorizationEndpoint, OAuth2TokenEndpoint, Route, SimpleRoute, UrlBuilder, }; +use mas_storage::BoxRng; use mas_templates::{ApiDocContext, Templates}; use tower_http::cors::{Any, CorsLayer}; @@ -45,6 +47,8 @@ use self::call_context::CallContext; pub fn router() -> (OpenApi, Router) where S: Clone + Send + Sync + 'static, + BoxHomeserverConnection: FromRef, + BoxRng: FromRequestParts, CallContext: FromRequestParts, Templates: FromRef, UrlBuilder: FromRef, diff --git a/crates/handlers/src/admin/v1/mod.rs b/crates/handlers/src/admin/v1/mod.rs index 0dabc3ec2..463af20bd 100644 --- a/crates/handlers/src/admin/v1/mod.rs +++ b/crates/handlers/src/admin/v1/mod.rs @@ -13,7 +13,9 @@ // limitations under the License. use aide::axum::{routing::get_with, ApiRouter}; -use axum::extract::FromRequestParts; +use axum::extract::{FromRef, FromRequestParts}; +use mas_matrix::BoxHomeserverConnection; +use mas_storage::BoxRng; use super::call_context::CallContext; @@ -22,10 +24,16 @@ mod users; pub fn router() -> ApiRouter where S: Clone + Send + Sync + 'static, + BoxHomeserverConnection: FromRef, + BoxRng: FromRequestParts, CallContext: FromRequestParts, { ApiRouter::::new() - .api_route("/users", get_with(self::users::list, self::users::list_doc)) + .api_route( + "/users", + get_with(self::users::list, self::users::list_doc) + .post_with(self::users::add, self::users::add_doc), + ) .api_route( "/users/:id", get_with(self::users::get, self::users::get_doc), diff --git a/crates/handlers/src/admin/v1/users/add.rs b/crates/handlers/src/admin/v1/users/add.rs new file mode 100644 index 000000000..647a8d184 --- /dev/null +++ b/crates/handlers/src/admin/v1/users/add.rs @@ -0,0 +1,179 @@ +// 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_matrix::BoxHomeserverConnection; +use mas_storage::{ + job::{JobRepositoryExt, ProvisionUserJob}, + BoxRng, +}; +use schemars::JsonSchema; +use serde::Deserialize; +use tracing::warn; + +use crate::{ + admin::{ + call_context::CallContext, + model::User, + response::{ErrorResponse, SingleResponse}, + }, + impl_from_error_for_route, +}; + +fn valid_username_character(c: char) -> bool { + c.is_ascii_lowercase() + || c.is_ascii_digit() + || c == '=' + || c == '_' + || c == '-' + || c == '.' + || c == '/' + || c == '+' +} + +// XXX: this should be shared with the graphql handler +fn username_valid(username: &str) -> bool { + if username.is_empty() || username.len() > 255 { + return false; + } + + // Should not start with an underscore + if username.starts_with('_') { + return false; + } + + // Should only contain valid characters + if !username.chars().all(valid_username_character) { + return false; + } + + true +} + +#[derive(Debug, thiserror::Error, OperationIo)] +#[aide(output_with = "Json")] +pub enum RouteError { + #[error(transparent)] + Internal(Box), + + #[error(transparent)] + Homeserver(anyhow::Error), + + #[error("Username is not valid")] + UsernameNotValid, + + #[error("User already exists")] + UserAlreadyExists, + + #[error("Username is reserved by the homeserver")] + UsernameReserved, +} + +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::Homeserver(_) => StatusCode::INTERNAL_SERVER_ERROR, + Self::UsernameNotValid => StatusCode::BAD_REQUEST, + Self::UserAlreadyExists | Self::UsernameReserved => StatusCode::CONFLICT, + }; + (status, Json(error)).into_response() + } +} + +#[derive(Deserialize, JsonSchema)] +pub struct AddUserParams { + /// The username of the user to add. + username: String, + + /// Skip checking with the homeserver whether the username is available. + /// + /// Use this with caution! The main reason to use this, is when a user used + /// by an application service needs to exist in MAS to craft special + /// tokens (like with admin access) for them + #[serde(default)] + skip_homeserver_check: bool, +} + +pub fn doc(operation: TransformOperation) -> TransformOperation { + operation + .summary("Create a new user") + .tag("user") + .response_with::<200, Json>, _>(|t| { + let [sample, ..] = User::samples(); + let response = SingleResponse::new_canonical(sample); + t.description("User was created").example(response) + }) + .response_with::<400, RouteError, _>(|t| { + let response = ErrorResponse::from_error(&RouteError::UsernameNotValid); + t.description("Username is not valid").example(response) + }) + .response_with::<409, RouteError, _>(|t| { + let response = ErrorResponse::from_error(&RouteError::UserAlreadyExists); + t.description("User already exists").example(response) + }) + .response_with::<409, RouteError, _>(|t| { + let response = ErrorResponse::from_error(&RouteError::UsernameReserved); + t.description("Username is reserved by the homeserver") + .example(response) + }) +} + +#[tracing::instrument(name = "handler.admin.v1.users.add", skip_all, err)] +pub async fn handler( + CallContext { + mut repo, clock, .. + }: CallContext, + NoApi(mut rng): NoApi, + State(homeserver): State, + Json(params): Json, +) -> Result>, RouteError> { + if repo.user().exists(¶ms.username).await? { + return Err(RouteError::UserAlreadyExists); + } + + // Do some basic check on the username + if !username_valid(¶ms.username) { + return Err(RouteError::UsernameNotValid); + } + + // Ask the homeserver if the username is available + let homeserver_available = homeserver + .is_localpart_available(¶ms.username) + .await + .map_err(RouteError::Homeserver)?; + + if !homeserver_available { + if !params.skip_homeserver_check { + return Err(RouteError::UsernameReserved); + } + + // If we skipped the check, we still want to shout about it + warn!("Skipped homeserver check for username {}", params.username); + } + + let user = repo.user().add(&mut rng, &clock, params.username).await?; + + repo.job() + .schedule_job(ProvisionUserJob::new(&user)) + .await?; + + repo.save().await?; + + Ok(Json(SingleResponse::new_canonical(User::from(user)))) +} diff --git a/crates/handlers/src/admin/v1/users/mod.rs b/crates/handlers/src/admin/v1/users/mod.rs index 9ea1c5254..05615a40b 100644 --- a/crates/handlers/src/admin/v1/users/mod.rs +++ b/crates/handlers/src/admin/v1/users/mod.rs @@ -12,11 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. +mod add; mod by_username; mod get; mod list; pub use self::{ + add::{doc as add_doc, handler as add}, by_username::{doc as by_username_doc, handler as by_username}, get::{doc as get_doc, handler as get}, list::{doc as list_doc, handler as list}, diff --git a/crates/handlers/src/bin/api-schema.rs b/crates/handlers/src/bin/api-schema.rs index e9a260123..058d004da 100644 --- a/crates/handlers/src/bin/api-schema.rs +++ b/crates/handlers/src/bin/api-schema.rs @@ -60,9 +60,12 @@ macro_rules! impl_from_ref { impl_from_request_parts!(mas_storage::BoxRepository); impl_from_request_parts!(mas_storage::BoxClock); +impl_from_request_parts!(mas_storage::BoxRng); impl_from_request_parts!(mas_handlers::BoundActivityTracker); impl_from_ref!(mas_router::UrlBuilder); impl_from_ref!(mas_templates::Templates); +impl_from_ref!(mas_matrix::BoxHomeserverConnection); +impl_from_ref!(mas_keystore::Keystore); fn main() -> Result<(), Box> { let (mut api, _) = mas_handlers::admin_api_router::(); diff --git a/crates/handlers/src/graphql/mutations/user.rs b/crates/handlers/src/graphql/mutations/user.rs index d8b21d6b3..7affdb20e 100644 --- a/crates/handlers/src/graphql/mutations/user.rs +++ b/crates/handlers/src/graphql/mutations/user.rs @@ -349,7 +349,7 @@ fn username_valid(username: &str) -> bool { } // Should not start with an underscore - if username.get(0..1) == Some("_") { + if username.starts_with('_') { return false; } diff --git a/docs/api/spec.json b/docs/api/spec.json index b56844359..bf640b01c 100644 --- a/docs/api/spec.json +++ b/docs/api/spec.json @@ -159,6 +159,86 @@ } } } + }, + "post": { + "tags": [ + "user" + ], + "summary": "Create a new user", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/AddUserParams" + } + } + }, + "required": true + }, + "responses": { + "200": { + "description": "User was created", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SingleResponse_for_User" + }, + "example": { + "data": { + "type": "user", + "id": "01040G2081040G2081040G2081", + "attributes": { + "username": "alice", + "created_at": "1970-01-01T00:00:00Z", + "locked_at": null, + "can_request_admin": false + }, + "links": { + "self": "/api/admin/v1/users/01040G2081040G2081040G2081" + } + }, + "links": { + "self": "/api/admin/v1/users/01040G2081040G2081040G2081" + } + } + } + } + }, + "400": { + "description": "Username is not valid", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + }, + "example": { + "errors": [ + { + "title": "Username is not valid" + } + ] + } + } + } + }, + "409": { + "description": "Username is reserved by the homeserver", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ErrorResponse" + }, + "example": { + "errors": [ + { + "title": "Username is reserved by the homeserver" + } + ] + } + } + } + } + } } }, "/api/admin/v1/users/{id}": { @@ -557,15 +637,20 @@ } } }, - "UlidInPath": { + "AddUserParams": { "type": "object", "required": [ - "id" + "username" ], "properties": { - "id": { - "title": "The ID of the resource", - "$ref": "#/components/schemas/ULID" + "username": { + "description": "The username of the user to add.", + "type": "string" + }, + "skip_homeserver_check": { + "description": "Skip checking with the homeserver whether the username is available.\n\nUse this with caution! The main reason to use this, is when a user used by an application service needs to exist in MAS to craft special tokens (like with admin access) for them", + "default": false, + "type": "boolean" } } }, @@ -585,6 +670,18 @@ } } }, + "UlidInPath": { + "type": "object", + "required": [ + "id" + ], + "properties": { + "id": { + "title": "The ID of the resource", + "$ref": "#/components/schemas/ULID" + } + } + }, "UsernamePathParam": { "type": "object", "required": [ From 1468ea9e2c49900a27a6cdef27e0bb390c35d779 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 29 Jul 2024 10:40:25 +0200 Subject: [PATCH 2/5] Rename the payload struct to appease clippy --- crates/handlers/src/admin/v1/users/add.rs | 6 ++++-- docs/api/spec.json | 5 +++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/handlers/src/admin/v1/users/add.rs b/crates/handlers/src/admin/v1/users/add.rs index 647a8d184..06bd4ae51 100644 --- a/crates/handlers/src/admin/v1/users/add.rs +++ b/crates/handlers/src/admin/v1/users/add.rs @@ -96,8 +96,10 @@ impl IntoResponse for RouteError { } } +/// # JSON payload for the `POST /api/admin/v1/users` endpoint #[derive(Deserialize, JsonSchema)] -pub struct AddUserParams { +#[serde(rename = "AddUserRequest")] +pub struct Request { /// The username of the user to add. username: String, @@ -141,7 +143,7 @@ pub async fn handler( }: CallContext, NoApi(mut rng): NoApi, State(homeserver): State, - Json(params): Json, + Json(params): Json, ) -> Result>, RouteError> { if repo.user().exists(¶ms.username).await? { return Err(RouteError::UserAlreadyExists); diff --git a/docs/api/spec.json b/docs/api/spec.json index bf640b01c..4058fa9b8 100644 --- a/docs/api/spec.json +++ b/docs/api/spec.json @@ -169,7 +169,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/AddUserParams" + "$ref": "#/components/schemas/AddUserRequest" } } }, @@ -637,7 +637,8 @@ } } }, - "AddUserParams": { + "AddUserRequest": { + "title": "JSON payload for the `POST /api/admin/v1/users` endpoint", "type": "object", "required": [ "username" From 9b7a9a3f169fa79bf8b3d07dc90ac782fc62d5f8 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 29 Jul 2024 10:40:58 +0200 Subject: [PATCH 3/5] handlers: test utility to help request the admin API --- crates/handlers/src/test_utils.rs | 45 +++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/crates/handlers/src/test_utils.rs b/crates/handlers/src/test_utils.rs index 49e58e29d..497e11170 100644 --- a/crates/handlers/src/test_utils.rs +++ b/crates/handlers/src/test_utils.rs @@ -46,6 +46,7 @@ use mas_router::{SimpleRoute, UrlBuilder}; use mas_storage::{clock::MockClock, BoxClock, BoxRepository, BoxRng}; use mas_storage_pg::{DatabaseError, PgRepository}; use mas_templates::{SiteConfigExt, Templates}; +use oauth2_types::{registration::ClientRegistrationResponse, requests::AccessTokenResponse}; use rand::SeedableRng; use rand_chacha::ChaChaRng; use serde::{de::DeserializeOwned, Serialize}; @@ -249,6 +250,7 @@ impl TestState { .merge(crate::compat_router()) .merge(crate::human_router(self.templates.clone())) .merge(crate::graphql_router(false)) + .merge(crate::admin_api_router().1) .with_state(self.clone()) .into_service(); @@ -274,6 +276,49 @@ impl TestState { Response::from_parts(parts, body) } + /// Get a token with the given scope + pub async fn token_with_scope(&mut self, scope: &str) -> String { + // Provision a client + let request = + Request::post(mas_router::OAuth2RegistrationEndpoint::PATH).json(serde_json::json!({ + "client_uri": "https://example.com/", + "contacts": ["contact@example.com"], + "token_endpoint_auth_method": "client_secret_post", + "grant_types": ["client_credentials"], + })); + let response = self.request(request).await; + response.assert_status(StatusCode::CREATED); + let response: ClientRegistrationResponse = response.json(); + let client_id = response.client_id; + let client_secret = response.client_secret.expect("to have a client secret"); + + // Make the client admin + let state = { + let mut state = self.clone(); + state.policy_factory = policy_factory(serde_json::json!({ + "admin_clients": [client_id], + })) + .await + .unwrap(); + state + }; + + // Ask for a token with the admin scope + let request = + Request::post(mas_router::OAuth2TokenEndpoint::PATH).form(serde_json::json!({ + "grant_type": "client_credentials", + "client_id": client_id, + "client_secret": client_secret, + "scope": scope, + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::OK); + let AccessTokenResponse { access_token, .. } = response.json(); + + access_token + } + pub async fn repository(&self) -> Result { let repo = PgRepository::from_pool(&self.pool).await?; Ok(repo.boxed()) From 6326d9d8ad2197f12fa6caca906750342c15da81 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 29 Jul 2024 10:41:32 +0200 Subject: [PATCH 4/5] handlers: tests for the add user admin API --- crates/handlers/src/admin/v1/users/add.rs | 142 ++++++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/crates/handlers/src/admin/v1/users/add.rs b/crates/handlers/src/admin/v1/users/add.rs index 06bd4ae51..6611dffe3 100644 --- a/crates/handlers/src/admin/v1/users/add.rs +++ b/crates/handlers/src/admin/v1/users/add.rs @@ -179,3 +179,145 @@ pub async fn handler( Ok(Json(SingleResponse::new_canonical(User::from(user)))) } + +#[cfg(test)] +mod tests { + use hyper::{Request, StatusCode}; + use mas_storage::{user::UserRepository, RepositoryAccess}; + use sqlx::PgPool; + + use crate::test_utils::{setup, RequestBuilderExt, ResponseExt, TestState}; + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_add_user(pool: PgPool) { + setup(); + let mut state = TestState::from_pool(pool).await.unwrap(); + let token = state.token_with_scope("urn:mas:admin").await; + + let request = Request::post("/api/admin/v1/users") + .bearer(&token) + .json(serde_json::json!({ + "username": "alice", + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::OK); + + let body: serde_json::Value = response.json(); + assert_eq!(body["data"]["type"], "user"); + let id = body["data"]["id"].as_str().unwrap(); + assert_eq!(body["data"]["attributes"]["username"], "alice"); + + // Check that the user was created in the database + let mut repo = state.repository().await.unwrap(); + let user = repo + .user() + .lookup(id.parse().unwrap()) + .await + .unwrap() + .unwrap(); + + assert_eq!(user.username, "alice"); + } + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_add_user_invalid_username(pool: PgPool) { + setup(); + let mut state = TestState::from_pool(pool).await.unwrap(); + let token = state.token_with_scope("urn:mas:admin").await; + + let request = Request::post("/api/admin/v1/users") + .bearer(&token) + .json(serde_json::json!({ + "username": "this is invalid", + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::BAD_REQUEST); + + let body: serde_json::Value = response.json(); + assert_eq!(body["errors"][0]["title"], "Username is not valid"); + } + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_add_user_exists(pool: PgPool) { + setup(); + let mut state = TestState::from_pool(pool).await.unwrap(); + let token = state.token_with_scope("urn:mas:admin").await; + + let request = Request::post("/api/admin/v1/users") + .bearer(&token) + .json(serde_json::json!({ + "username": "alice", + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::OK); + + let body: serde_json::Value = response.json(); + assert_eq!(body["data"]["type"], "user"); + assert_eq!(body["data"]["attributes"]["username"], "alice"); + + let request = Request::post("/api/admin/v1/users") + .bearer(&token) + .json(serde_json::json!({ + "username": "alice", + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::CONFLICT); + + let body: serde_json::Value = response.json(); + assert_eq!(body["errors"][0]["title"], "User already exists"); + } + + #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] + async fn test_add_user_reserved(pool: PgPool) { + setup(); + let mut state = TestState::from_pool(pool).await.unwrap(); + let token = state.token_with_scope("urn:mas:admin").await; + + // Reserve a username on the homeserver and try to add it + state.homeserver_connection.reserve_localpart("bob").await; + + let request = Request::post("/api/admin/v1/users") + .bearer(&token) + .json(serde_json::json!({ + "username": "bob", + })); + + let response = state.request(request).await; + + let body: serde_json::Value = response.json(); + assert_eq!( + body["errors"][0]["title"], + "Username is reserved by the homeserver" + ); + + // But we can force it with the skip_homeserver_check flag + let request = Request::post("/api/admin/v1/users") + .bearer(&token) + .json(serde_json::json!({ + "username": "bob", + "skip_homeserver_check": true, + })); + + let response = state.request(request).await; + response.assert_status(StatusCode::OK); + + let body: serde_json::Value = response.json(); + let id = body["data"]["id"].as_str().unwrap(); + assert_eq!(body["data"]["attributes"]["username"], "bob"); + + // Check that the user was created in the database + let mut repo = state.repository().await.unwrap(); + let user = repo + .user() + .lookup(id.parse().unwrap()) + .await + .unwrap() + .unwrap(); + + assert_eq!(user.username, "bob"); + } +} From 089ba5acb67372d4d23a949fb1dc239b94efb1d4 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Tue, 30 Jul 2024 17:34:25 +0200 Subject: [PATCH 5/5] admin: add operation ID on user add operation --- crates/handlers/src/admin/v1/users/add.rs | 1 + docs/api/spec.json | 1 + 2 files changed, 2 insertions(+) diff --git a/crates/handlers/src/admin/v1/users/add.rs b/crates/handlers/src/admin/v1/users/add.rs index 6611dffe3..04d1347da 100644 --- a/crates/handlers/src/admin/v1/users/add.rs +++ b/crates/handlers/src/admin/v1/users/add.rs @@ -114,6 +114,7 @@ pub struct Request { pub fn doc(operation: TransformOperation) -> TransformOperation { operation + .id("createUser") .summary("Create a new user") .tag("user") .response_with::<200, Json>, _>(|t| { diff --git a/docs/api/spec.json b/docs/api/spec.json index 4058fa9b8..c1551a023 100644 --- a/docs/api/spec.json +++ b/docs/api/spec.json @@ -165,6 +165,7 @@ "user" ], "summary": "Create a new user", + "operationId": "createUser", "requestBody": { "content": { "application/json": {