From acf05c5ebb088d72770d3a16ebb84b9dbf3752e8 Mon Sep 17 00:00:00 2001 From: Michael Vlach Date: Mon, 11 Dec 2023 12:56:13 +0100 Subject: [PATCH] [server] Add remove user to admin endpoint #824 (#851) * fix changing roles * add tests * add remove db user --- agdb_server/openapi/schema.json | 55 ++++++ agdb_server/src/api.rs | 2 + agdb_server/src/app.rs | 4 +- agdb_server/src/db.rs | 95 ++++++++-- agdb_server/src/routes/admin/db.rs | 2 +- agdb_server/src/routes/db.rs | 14 +- agdb_server/src/routes/db/user.rs | 42 ++++- agdb_server/tests/integration.rs | 1 + agdb_server/tests/routes/db_user_add_test.rs | 24 +++ .../tests/routes/db_user_remove_test.rs | 166 ++++++++++++++++++ agdb_server/tests/routes/mod.rs | 1 + 11 files changed, 377 insertions(+), 29 deletions(-) create mode 100644 agdb_server/tests/routes/db_user_remove_test.rs diff --git a/agdb_server/openapi/schema.json b/agdb_server/openapi/schema.json index 046b675d8..abac3f4ff 100644 --- a/agdb_server/openapi/schema.json +++ b/agdb_server/openapi/schema.json @@ -351,6 +351,46 @@ ] } }, + "/api/v1/db/user/remove": { + "post": { + "tags": [ + "crate::routes::db::user" + ], + "operationId": "remove", + "requestBody": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/RemoveDbUser" + } + } + }, + "required": true + }, + "responses": { + "204": { + "description": "user removed" + }, + "401": { + "description": "unauthorized" + }, + "403": { + "description": "user must be a db admin / cannot remove last admin user" + }, + "464": { + "description": "user not found" + }, + "466": { + "description": "db not found" + } + }, + "security": [ + { + "Token": [] + } + ] + } + }, "/api/v1/status": { "get": { "tags": [ @@ -494,6 +534,21 @@ "read" ] }, + "RemoveDbUser": { + "type": "object", + "required": [ + "database", + "user" + ], + "properties": { + "database": { + "type": "string" + }, + "user": { + "type": "string" + } + } + }, "ServerDatabase": { "type": "object", "required": [ diff --git a/agdb_server/src/api.rs b/agdb_server/src/api.rs index 12b7ae523..eb0f9afad 100644 --- a/agdb_server/src/api.rs +++ b/agdb_server/src/api.rs @@ -20,6 +20,7 @@ use utoipa::OpenApi; crate::routes::db::list, crate::routes::db::remove, crate::routes::db::user::add, + crate::routes::db::user::remove, ), components(schemas( crate::routes::admin::user::UserStatus, @@ -27,6 +28,7 @@ use utoipa::OpenApi; crate::routes::db::ServerDatabase, crate::routes::db::ServerDatabaseName, crate::routes::db::user::DbUser, + crate::routes::db::user::RemoveDbUser, crate::routes::db::user::DbUserRole, crate::routes::user::ChangePassword, crate::routes::user::UserCredentials, diff --git a/agdb_server/src/app.rs b/agdb_server/src/app.rs index 6c7aa9ed7..03d43f9a1 100644 --- a/agdb_server/src/app.rs +++ b/agdb_server/src/app.rs @@ -40,7 +40,9 @@ pub(crate) fn app(config: Config, shutdown_sender: Sender<()>, db_pool: DbPool) ) .route("/login", routing::post(routes::user::login)); - let db_user_router_v1 = Router::new().route("/add", routing::post(routes::db::user::add)); + let db_user_router_v1 = Router::new() + .route("/add", routing::post(routes::db::user::add)) + .route("/remove", routing::post(routes::db::user::remove)); let db_router_v1 = Router::new() .route("/add", routing::post(routes::db::add)) diff --git a/agdb_server/src/db.rs b/agdb_server/src/db.rs index 8352418e9..e44c967f1 100644 --- a/agdb_server/src/db.rs +++ b/agdb_server/src/db.rs @@ -94,7 +94,7 @@ impl DbPool { Ok(db_pool) } - pub(crate) fn add_database(&self, user: DbId, database: Database) -> ServerResult { + pub(crate) fn add_db(&self, user: DbId, database: Database) -> ServerResult { let db = ServerDb::new(&format!("{}:{}", database.db_type, database.name)).map_err( |mut e| { e.status = ErrorCode::DbInvalid.into(); @@ -119,16 +119,38 @@ impl DbPool { Ok(()) } - pub(crate) fn add_database_user(&self, database: DbId, user: DbId, role: &str) -> ServerResult { - self.db_mut()?.exec_mut( - &QueryBuilder::insert() - .edges() - .from(user) - .to(database) - .values_uniform(vec![("role", role).into()]) - .query(), - )?; - Ok(()) + pub(crate) fn add_db_user(&self, db: DbId, user: DbId, role: &str) -> ServerResult { + self.db_mut()?.transaction_mut(|t| { + let existing_role = t.exec( + &QueryBuilder::search() + .from(user) + .to(db) + .limit(1) + .where_() + .keys(vec!["role".into()]) + .query(), + )?; + + if existing_role.result == 1 { + t.exec_mut( + &QueryBuilder::insert() + .values(vec![vec![("role", role).into()]]) + .ids(existing_role) + .query(), + )?; + } else { + t.exec_mut( + &QueryBuilder::insert() + .edges() + .from(user) + .to(db) + .values_uniform(vec![("role", role).into()]) + .query(), + )?; + } + + Ok(()) + }) } pub(crate) fn create_user(&self, user: DbUser) -> ServerResult { @@ -146,8 +168,8 @@ impl DbPool { Ok(()) } - pub(crate) fn delete_database(&self, db: Database) -> ServerResult { - let filename = self.remove_database(db)?.get()?.filename().to_string(); + pub(crate) fn delete_db(&self, db: Database) -> ServerResult { + let filename = self.remove_db(db)?.get()?.filename().to_string(); let path = Path::new(&filename); if path.exists() { @@ -162,7 +184,7 @@ impl DbPool { Ok(()) } - pub(crate) fn find_databases(&self) -> ServerResult> { + pub(crate) fn find_dbs(&self) -> ServerResult> { Ok(self .db()? .exec( @@ -179,7 +201,7 @@ impl DbPool { .try_into()?) } - pub(crate) fn find_database_id(&self, name: &str) -> ServerResult { + pub(crate) fn find_db_id(&self, name: &str) -> ServerResult { Ok(self .db()? .exec( @@ -225,7 +247,7 @@ impl DbPool { .collect()) } - pub(crate) fn find_user_databases(&self, user: DbId) -> ServerResult> { + pub(crate) fn find_user_dbs(&self, user: DbId) -> ServerResult> { Ok(self .db()? .exec( @@ -242,7 +264,7 @@ impl DbPool { .try_into()?) } - pub(crate) fn find_user_database(&self, user: DbId, db: &str) -> ServerResult { + pub(crate) fn find_user_db(&self, user: DbId, db: &str) -> ServerResult { Ok(self .0 .server_db @@ -341,6 +363,26 @@ impl DbPool { .id) } + pub(crate) fn db_admins(&self, db: DbId) -> ServerResult> { + Ok(self + .db()? + .exec( + &QueryBuilder::search() + .to(db) + .where_() + .distance(CountComparison::Equal(2)) + .and() + .beyond() + .where_() + .node() + .or() + .key("role") + .value(Comparison::Equal("admin".into())) + .query(), + )? + .ids()) + } + pub(crate) fn is_db_admin(&self, user: DbId, db: DbId) -> ServerResult { Ok(self .db()? @@ -360,7 +402,24 @@ impl DbPool { == 1) } - pub(crate) fn remove_database(&self, db: Database) -> ServerResult { + pub(crate) fn remove_db_user(&self, db: DbId, user: DbId) -> ServerResult { + self.db_mut()?.exec_mut( + &QueryBuilder::remove() + .ids( + QueryBuilder::search() + .from(user) + .to(db) + .limit(1) + .where_() + .edge() + .query(), + ) + .query(), + )?; + Ok(()) + } + + pub(crate) fn remove_db(&self, db: Database) -> ServerResult { self.db_mut()? .exec_mut(&QueryBuilder::remove().ids(db.db_id.unwrap()).query())?; diff --git a/agdb_server/src/routes/admin/db.rs b/agdb_server/src/routes/admin/db.rs index 5a1f7194d..4e357577a 100644 --- a/agdb_server/src/routes/admin/db.rs +++ b/agdb_server/src/routes/admin/db.rs @@ -19,7 +19,7 @@ pub(crate) async fn list( State(db_pool): State, ) -> ServerResponse<(StatusCode, Json>)> { let dbs = db_pool - .find_databases()? + .find_dbs()? .into_iter() .map(|db| db.into()) .collect(); diff --git a/agdb_server/src/routes/db.rs b/agdb_server/src/routes/db.rs index a28183aeb..5a45d2eb4 100644 --- a/agdb_server/src/routes/db.rs +++ b/agdb_server/src/routes/db.rs @@ -71,7 +71,7 @@ pub(crate) async fn add( State(db_pool): State, Json(request): Json, ) -> ServerResponse { - if db_pool.find_database_id(&request.name).is_ok() { + if db_pool.find_db_id(&request.name).is_ok() { return Err(ErrorCode::DbExists.into()); } @@ -81,7 +81,7 @@ pub(crate) async fn add( db_type: request.db_type.to_string(), }; - db_pool.add_database(user.0, db)?; + db_pool.add_db(user.0, db)?; Ok(StatusCode::CREATED) } @@ -102,13 +102,13 @@ pub(crate) async fn delete( State(db_pool): State, Json(request): Json, ) -> ServerResponse { - let db = db_pool.find_user_database(user.0, &request.name)?; + let db = db_pool.find_user_db(user.0, &request.name)?; if !db_pool.is_db_admin(user.0, db.db_id.unwrap())? { return Ok(StatusCode::FORBIDDEN); } - db_pool.delete_database(db)?; + db_pool.delete_db(db)?; Ok(StatusCode::NO_CONTENT) } @@ -126,7 +126,7 @@ pub(crate) async fn list( State(db_pool): State, ) -> ServerResponse<(StatusCode, Json>)> { let dbs = db_pool - .find_user_databases(user.0)? + .find_user_dbs(user.0)? .into_iter() .map(|db| db.into()) .collect(); @@ -149,13 +149,13 @@ pub(crate) async fn remove( State(db_pool): State, Json(request): Json, ) -> ServerResponse { - let db = db_pool.find_user_database(user.0, &request.name)?; + let db = db_pool.find_user_db(user.0, &request.name)?; if !db_pool.is_db_admin(user.0, db.db_id.unwrap())? { return Ok(StatusCode::FORBIDDEN); } - db_pool.remove_database(db)?; + db_pool.remove_db(db)?; Ok(StatusCode::NO_CONTENT) } diff --git a/agdb_server/src/routes/db/user.rs b/agdb_server/src/routes/db/user.rs index a96d21127..278cf6624 100644 --- a/agdb_server/src/routes/db/user.rs +++ b/agdb_server/src/routes/db/user.rs @@ -23,6 +23,12 @@ pub(crate) struct DbUser { pub(crate) role: DbUserRole, } +#[derive(Deserialize, ToSchema)] +pub(crate) struct RemoveDbUser { + pub(crate) database: String, + pub(crate) user: String, +} + impl Display for DbUserRole { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -50,14 +56,46 @@ pub(crate) async fn add( State(db_pool): State, Json(request): Json, ) -> ServerResponse { - let db = db_pool.find_database_id(&request.database)?; + let db = db_pool.find_db_id(&request.database)?; let db_user = db_pool.find_user_id(&request.user)?; if !db_pool.is_db_admin(user.0, db)? || db_user == user.0 { return Ok(StatusCode::FORBIDDEN); } - db_pool.add_database_user(db, db_user, &request.role.to_string())?; + db_pool.add_db_user(db, db_user, &request.role.to_string())?; Ok(StatusCode::CREATED) } + +#[utoipa::path(post, + path = "/api/v1/db/user/remove", + request_body = RemoveDbUser, + security(("Token" = [])), + responses( + (status = 204, description = "user removed"), + (status = 401, description = "unauthorized"), + (status = 403, description = "user must be a db admin / cannot remove last admin user"), + (status = 464, description = "user not found"), + (status = 466, description = "db not found"), + ) +)] +pub(crate) async fn remove( + user: UserId, + State(db_pool): State, + Json(request): Json, +) -> ServerResponse { + let db = db_pool.find_db_id(&request.database)?; + let db_user = db_pool.find_user_id(&request.user)?; + let admins = db_pool.db_admins(db)?; + + println!("{:?} == {:?}", admins, vec![db_user]); + + if (!admins.contains(&user.0) && user.0 != db_user) || admins == vec![db_user] { + return Ok(StatusCode::FORBIDDEN); + } + + db_pool.remove_db_user(db, db_user)?; + + Ok(StatusCode::NO_CONTENT) +} diff --git a/agdb_server/tests/integration.rs b/agdb_server/tests/integration.rs index ad780d847..4016db867 100644 --- a/agdb_server/tests/integration.rs +++ b/agdb_server/tests/integration.rs @@ -21,6 +21,7 @@ pub const USER_CHANGE_PASSWORD_URI: &str = "/user/change_password"; pub const ADMIN_USER_CREATE_URI: &str = "/admin/user/create"; pub const DB_ADD_URI: &str = "/db/add"; pub const DB_USER_ADD_URI: &str = "/db/user/add"; +pub const DB_USER_REMOVE_URI: &str = "/db/user/remove"; pub const DB_DELETE_URI: &str = "/db/delete"; pub const DB_LIST_URI: &str = "/db/list"; pub const ADMIN_DB_LIST_URI: &str = "/admin/db/list"; diff --git a/agdb_server/tests/routes/db_user_add_test.rs b/agdb_server/tests/routes/db_user_add_test.rs index 5d17e1455..2f17fc182 100644 --- a/agdb_server/tests/routes/db_user_add_test.rs +++ b/agdb_server/tests/routes/db_user_add_test.rs @@ -111,6 +111,30 @@ async fn add_admin_as_non_admin() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn change_user_role() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let (name, token) = server.init_user().await?; + let (other_name, other) = server.init_user().await?; + let db = server.init_db("memory", &token).await?; + let mut role = AddUser { + database: &db, + user: &other_name, + role: "write", + }; + assert_eq!(server.post(DB_USER_ADD_URI, &role, &token).await?.0, 201); + assert_eq!(server.post(DB_USER_ADD_URI, &role, &other).await?.0, 403); + role.role = "admin"; + assert_eq!(server.post(DB_USER_ADD_URI, &role, &token).await?.0, 201); + role.role = "write"; + role.user = &name; + assert_eq!(server.post(DB_USER_ADD_URI, &role, &other).await?.0, 201); + role.user = &other_name; + assert_eq!(server.post(DB_USER_ADD_URI, &role, &token).await?.0, 403); + + Ok(()) +} + #[tokio::test] async fn db_not_found() -> anyhow::Result<()> { let server = TestServer::new().await?; diff --git a/agdb_server/tests/routes/db_user_remove_test.rs b/agdb_server/tests/routes/db_user_remove_test.rs new file mode 100644 index 000000000..8e9780aec --- /dev/null +++ b/agdb_server/tests/routes/db_user_remove_test.rs @@ -0,0 +1,166 @@ +use crate::AddUser; +use crate::Db; +use crate::TestServer; +use crate::DB_LIST_URI; +use crate::DB_USER_ADD_URI; +use crate::DB_USER_REMOVE_URI; +use crate::NO_TOKEN; +use serde::Serialize; + +#[derive(Serialize)] +struct RemoveUser<'a> { + database: &'a str, + user: &'a str, +} + +#[tokio::test] +async fn remove() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let (_, token) = server.init_user().await?; + let (reader_name, reader_token) = server.init_user().await?; + let db = server.init_db("memory", &token).await?; + let role = AddUser { + database: &db, + user: &reader_name, + role: "read", + }; + let (_, list) = server.get::>(DB_LIST_URI, &reader_token).await?; + assert_eq!(list?, vec![]); + assert_eq!(server.post(DB_USER_ADD_URI, &role, &token).await?.0, 201); + let (_, list) = server.get::>(DB_LIST_URI, &reader_token).await?; + let expected = vec![Db { + name: db.clone(), + db_type: "memory".to_string(), + }]; + assert_eq!(list?, expected); + let rem = RemoveUser { + database: &db, + user: &reader_name, + }; + assert_eq!(server.post(DB_USER_REMOVE_URI, &rem, &token).await?.0, 204); + let (_, list) = server.get::>(DB_LIST_URI, &reader_token).await?; + assert_eq!(list?, vec![]); + Ok(()) +} + +#[tokio::test] +async fn remove_user_non_admin() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let (name, token) = server.init_user().await?; + let (other_name, other) = server.init_user().await?; + let db = server.init_db("memory", &token).await?; + let role = AddUser { + database: &db, + user: &other_name, + role: "read", + }; + assert_eq!(server.post(DB_USER_ADD_URI, &role, &token).await?.0, 201); + let rem = RemoveUser { + database: &db, + user: &name, + }; + assert_eq!(server.post(DB_USER_REMOVE_URI, &rem, &other).await?.0, 403); + Ok(()) +} + +#[tokio::test] +async fn remove_self() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let (_, token) = server.init_user().await?; + let (other_name, other) = server.init_user().await?; + let db = server.init_db("memory", &token).await?; + let role = AddUser { + database: &db, + user: &other_name, + role: "read", + }; + assert_eq!(server.post(DB_USER_ADD_URI, &role, &token).await?.0, 201); + let rem = RemoveUser { + database: &db, + user: &other_name, + }; + assert_eq!(server.post(DB_USER_REMOVE_URI, &rem, &other).await?.0, 204); + let (_, list) = server.get::>(DB_LIST_URI, &other).await?; + assert_eq!(list?, vec![]); + Ok(()) +} + +#[tokio::test] +async fn remove_self_admin() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let (_, token) = server.init_user().await?; + let (other_name, other) = server.init_user().await?; + let db = server.init_db("memory", &token).await?; + let role = AddUser { + database: &db, + user: &other_name, + role: "admin", + }; + assert_eq!(server.post(DB_USER_ADD_URI, &role, &token).await?.0, 201); + let rem = RemoveUser { + database: &db, + user: &other_name, + }; + assert_eq!(server.post(DB_USER_REMOVE_URI, &rem, &other).await?.0, 204); + let (_, list) = server.get::>(DB_LIST_URI, &other).await?; + assert_eq!(list?, vec![]); + Ok(()) +} + +#[tokio::test] +async fn remove_self_admin_last() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let (name, token) = server.init_user().await?; + let db = server.init_db("memory", &token).await?; + let rem = RemoveUser { + database: &db, + user: &name, + }; + assert_eq!(server.post(DB_USER_REMOVE_URI, &rem, &token).await?.0, 403); + let (_, list) = server.get::>(DB_LIST_URI, &token).await?; + let expected = vec![Db { + name: db, + db_type: "memory".to_string(), + }]; + assert_eq!(list?, expected); + Ok(()) +} + +#[tokio::test] +async fn db_not_found() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let (_, token) = server.init_user().await?; + let rem = RemoveUser { + database: "db_not_found", + user: "some_user", + }; + assert_eq!(server.post(DB_USER_REMOVE_URI, &rem, &token).await?.0, 466); + Ok(()) +} + +#[tokio::test] +async fn user_not_found() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let (_, token) = server.init_user().await?; + let db = server.init_db("memory", &token).await?; + let rem = RemoveUser { + database: &db, + user: "user_not_found", + }; + assert_eq!(server.post(DB_USER_REMOVE_URI, &rem, &token).await?.0, 464); + Ok(()) +} + +#[tokio::test] +async fn no_token() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let rem = RemoveUser { + database: "my_db", + user: "bob", + }; + assert_eq!( + server.post(DB_USER_REMOVE_URI, &rem, NO_TOKEN).await?.0, + 401 + ); + Ok(()) +} diff --git a/agdb_server/tests/routes/mod.rs b/agdb_server/tests/routes/mod.rs index 039868c5b..5eb0f89d3 100644 --- a/agdb_server/tests/routes/mod.rs +++ b/agdb_server/tests/routes/mod.rs @@ -7,6 +7,7 @@ mod db_delete_test; mod db_list_test; mod db_remove_test; mod db_user_add_test; +mod db_user_remove_test; mod server_test; mod user_change_password_test; mod user_login_test;