From 027a60661bc4e7359e46edf1e9eb7fd792019d56 Mon Sep 17 00:00:00 2001 From: Michael Vlach Date: Wed, 20 Dec 2023 21:01:43 +0100 Subject: [PATCH 1/2] rework add/remove db user --- agdb_server/openapi/schema.json | 20 ++++-- agdb_server/src/db_pool.rs | 20 ------ agdb_server/src/routes/admin/db/user.rs | 32 ++++++++-- agdb_server/src/routes/db/user.rs | 42 +++++++++--- .../tests/routes/admin_db_user_add_test.rs | 22 ++++++- .../tests/routes/admin_db_user_remove_test.rs | 39 +++++++---- agdb_server/tests/routes/db_user_add_test.rs | 23 +++++-- .../tests/routes/db_user_remove_test.rs | 64 ++++++++----------- 8 files changed, 167 insertions(+), 95 deletions(-) diff --git a/agdb_server/openapi/schema.json b/agdb_server/openapi/schema.json index c52a776a7..01b9afc7b 100644 --- a/agdb_server/openapi/schema.json +++ b/agdb_server/openapi/schema.json @@ -169,13 +169,16 @@ "description": "unauthorized" }, "403": { - "description": "user must be a db admin / cannot add self" + "description": "cannot change role of db owner" }, "464": { "description": "user not found" }, "466": { "description": "db not found" + }, + "467": { + "description": "db invalid" } }, "security": [ @@ -243,13 +246,16 @@ "description": "unauthorized" }, "403": { - "description": "cannot remove last admin user" + "description": "cannot remove db owner" }, "464": { "description": "user not found" }, "466": { "description": "db not found" + }, + "467": { + "description": "db invalid" } }, "security": [ @@ -679,13 +685,16 @@ "description": "unauthorized" }, "403": { - "description": "user must be a db admin / cannot add self" + "description": "user must be a db admin / cannot change role of db owner" }, "464": { "description": "user not found" }, "466": { "description": "db not found" + }, + "467": { + "description": "db invalid" } }, "security": [ @@ -753,13 +762,16 @@ "description": "unauthorized" }, "403": { - "description": "user must be a db admin / cannot remove last admin user" + "description": "must be admin / cannot remove db owner" }, "464": { "description": "user not found" }, "466": { "description": "db not found" + }, + "467": { + "description": "db invalid" } }, "security": [ diff --git a/agdb_server/src/db_pool.rs b/agdb_server/src/db_pool.rs index d1e6aefd8..ae7edc62e 100644 --- a/agdb_server/src/db_pool.rs +++ b/agdb_server/src/db_pool.rs @@ -455,26 +455,6 @@ 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(DbUserRole::Admin.into())) - .query(), - )? - .ids()) - } - pub(crate) fn db_user_id(&self, db: DbId, name: &str) -> ServerResult { Ok(self .db()? diff --git a/agdb_server/src/routes/admin/db/user.rs b/agdb_server/src/routes/admin/db/user.rs index 6c967174d..a44613f7a 100644 --- a/agdb_server/src/routes/admin/db/user.rs +++ b/agdb_server/src/routes/admin/db/user.rs @@ -1,4 +1,5 @@ use crate::db_pool::DbPool; +use crate::error_code::ErrorCode; use crate::routes::db::user::DbUser; use crate::routes::db::user::RemoveDbUser; use crate::routes::db::ServerDatabaseName; @@ -17,9 +18,10 @@ use axum::Json; responses( (status = 201, description = "user added"), (status = 401, description = "unauthorized"), - (status = 403, description = "user must be a db admin / cannot add self"), + (status = 403, description = "cannot change role of db owner"), (status = 464, description = "user not found"), (status = 466, description = "db not found"), + (status = 467, description = "db invalid"), ) )] pub(crate) async fn add( @@ -27,6 +29,18 @@ pub(crate) async fn add( State(db_pool): State, Json(request): Json, ) -> ServerResponse { + let (db_user, _db) = request + .database + .split_once('/') + .ok_or(ErrorCode::DbInvalid)?; + + if db_user == request.user { + return Err(ServerError::new( + StatusCode::FORBIDDEN, + "cannot change role of db owner", + )); + } + let db = db_pool.find_db_id(&request.database)?; let db_user = db_pool.find_user_id(&request.user)?; db_pool.add_db_user(db, db_user, request.role)?; @@ -72,9 +86,10 @@ pub(crate) async fn list( responses( (status = 204, description = "user removed"), (status = 401, description = "unauthorized"), - (status = 403, description = "cannot remove last admin user"), + (status = 403, description = "cannot remove db owner"), (status = 464, description = "user not found"), (status = 466, description = "db not found"), + (status = 467, description = "db invalid"), ) )] pub(crate) async fn remove( @@ -82,17 +97,20 @@ pub(crate) async fn remove( State(db_pool): State, Json(request): Json, ) -> ServerResponse { - let db = db_pool.find_db_id(&request.database)?; - let db_user = db_pool.db_user_id(db, &request.user)?; - let admins = db_pool.db_admins(db)?; + let (db_user, _db) = request + .database + .split_once('/') + .ok_or(ErrorCode::DbInvalid)?; - if admins == vec![db_user] { + if db_user == request.user { return Err(ServerError::new( StatusCode::FORBIDDEN, - "removed user must not be the last db admin", + "cannot remove db owner", )); } + let db = db_pool.find_db_id(&request.database)?; + let db_user = db_pool.db_user_id(db, &request.user)?; db_pool.remove_db_user(db, db_user)?; Ok(StatusCode::NO_CONTENT) diff --git a/agdb_server/src/routes/db/user.rs b/agdb_server/src/routes/db/user.rs index b44761837..515460ad2 100644 --- a/agdb_server/src/routes/db/user.rs +++ b/agdb_server/src/routes/db/user.rs @@ -1,4 +1,5 @@ use crate::db_pool::DbPool; +use crate::error_code::ErrorCode; use crate::routes::db::ServerDatabaseName; use crate::server_error::ServerError; use crate::server_error::ServerResponse; @@ -39,9 +40,10 @@ pub(crate) struct RemoveDbUser { responses( (status = 201, description = "user added"), (status = 401, description = "unauthorized"), - (status = 403, description = "user must be a db admin / cannot add self"), + (status = 403, description = "user must be a db admin / cannot change role of db owner"), (status = 464, description = "user not found"), (status = 466, description = "db not found"), + (status = 467, description = "db invalid"), ) )] pub(crate) async fn add( @@ -49,16 +51,28 @@ pub(crate) async fn add( State(db_pool): State, Json(request): Json, ) -> ServerResponse { + let (owner, _db) = request + .database + .split_once('/') + .ok_or(ErrorCode::DbInvalid)?; + + if owner == request.user { + return Err(ServerError::new( + StatusCode::FORBIDDEN, + "cannot change role of db owner", + )); + } + 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 { + if !db_pool.is_db_admin(user.0, db)? { return Err(ServerError::new( StatusCode::FORBIDDEN, - "user must be a db admin / cannot add self", + "must be a db admin", )); } + let db_user = db_pool.find_user_id(&request.user)?; db_pool.add_db_user(db, db_user, request.role)?; Ok(StatusCode::CREATED) @@ -102,9 +116,10 @@ pub(crate) async fn list( 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 = 403, description = "must be admin / cannot remove db owner"), (status = 464, description = "user not found"), (status = 466, description = "db not found"), + (status = 467, description = "db invalid"), ) )] pub(crate) async fn remove( @@ -112,14 +127,25 @@ pub(crate) async fn remove( State(db_pool): State, Json(request): Json, ) -> ServerResponse { + let (owner, _db) = request + .database + .split_once('/') + .ok_or(ErrorCode::DbInvalid)?; + + if owner == request.user { + return Err(ServerError::new( + StatusCode::FORBIDDEN, + "cannot remove db owner", + )); + } + let db = db_pool.find_db_id(&request.database)?; let db_user = db_pool.db_user_id(db, &request.user)?; - let admins = db_pool.db_admins(db)?; - if (!admins.contains(&user.0) && user.0 != db_user) || admins == vec![db_user] { + if user.0 != db_user && !db_pool.is_db_admin(user.0, db)? { return Err(ServerError::new( StatusCode::FORBIDDEN, - "user must be a db admin / cannot remove last admin user", + "must be a db admin", )); } diff --git a/agdb_server/tests/routes/admin_db_user_add_test.rs b/agdb_server/tests/routes/admin_db_user_add_test.rs index f66677c4f..87ea03bdf 100644 --- a/agdb_server/tests/routes/admin_db_user_add_test.rs +++ b/agdb_server/tests/routes/admin_db_user_add_test.rs @@ -73,7 +73,7 @@ async fn change_user_role() -> anyhow::Result<()> { .post(ADMIN_DB_USER_ADD_URI, &role, &server.admin_token) .await? .0, - 201 + 403 ); Ok(()) @@ -83,7 +83,7 @@ async fn change_user_role() -> anyhow::Result<()> { async fn db_not_found() -> anyhow::Result<()> { let server = TestServer::new().await?; let role = AddUser { - database: "db_not_found", + database: "user/db_not_found", user: "some_user", role: "read", }; @@ -97,6 +97,24 @@ async fn db_not_found() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn db_invalid() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let role = AddUser { + database: "invalid", + user: "some_user", + role: "read", + }; + assert_eq!( + server + .post(ADMIN_DB_USER_ADD_URI, &role, &server.admin_token) + .await? + .0, + 467 + ); + Ok(()) +} + #[tokio::test] async fn user_not_found() -> anyhow::Result<()> { let server = TestServer::new().await?; diff --git a/agdb_server/tests/routes/admin_db_user_remove_test.rs b/agdb_server/tests/routes/admin_db_user_remove_test.rs index 6cbaf2d83..4f354b8ba 100644 --- a/agdb_server/tests/routes/admin_db_user_remove_test.rs +++ b/agdb_server/tests/routes/admin_db_user_remove_test.rs @@ -61,10 +61,20 @@ async fn remove() -> anyhow::Result<()> { } #[tokio::test] -async fn remove_last_admin() -> anyhow::Result<()> { +async fn remove_owner() -> anyhow::Result<()> { let server = TestServer::new().await?; let user = server.init_user().await?; + let other = server.init_user().await?; let db = server.init_db("memory", &user).await?; + let role = AddUser { + database: &db, + user: &other.name, + role: "admin", + }; + assert_eq!( + server.post(DB_USER_ADD_URI, &role, &user.token).await?.0, + 201 + ); let rem = RemoveUser { database: db.clone(), user: user.name, @@ -76,16 +86,6 @@ async fn remove_last_admin() -> anyhow::Result<()> { .0, 403 ); - let (_, list) = server - .get::>(DB_LIST_URI, &user.token) - .await?; - let expected = vec![DbWithRole { - name: db, - db_type: "memory".to_string(), - role: "admin".to_string(), - size: 2600, - }]; - assert_eq!(list?, expected); Ok(()) } @@ -107,6 +107,23 @@ async fn db_not_found() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn db_invalid() -> anyhow::Result<()> { + let server = TestServer::new().await?; + let rem = RemoveUser { + database: "invalid".to_string(), + user: String::new(), + }; + assert_eq!( + server + .post(ADMIN_DB_USER_REMOVE_URI, &rem, &server.admin_token) + .await? + .0, + 467 + ); + Ok(()) +} + #[tokio::test] async fn user_not_found() -> anyhow::Result<()> { let server = TestServer::new().await?; diff --git a/agdb_server/tests/routes/db_user_add_test.rs b/agdb_server/tests/routes/db_user_add_test.rs index 5a52b05d0..0bcca8166 100644 --- a/agdb_server/tests/routes/db_user_add_test.rs +++ b/agdb_server/tests/routes/db_user_add_test.rs @@ -173,11 +173,6 @@ async fn change_user_role() -> anyhow::Result<()> { role.user = &user.name; assert_eq!( server.post(DB_USER_ADD_URI, &role, &other.token).await?.0, - 201 - ); - role.user = &other.name; - assert_eq!( - server.post(DB_USER_ADD_URI, &role, &user.token).await?.0, 403 ); @@ -189,7 +184,7 @@ async fn db_not_found() -> anyhow::Result<()> { let server = TestServer::new().await?; let user = server.init_user().await?; let role = AddUser { - database: "db_not_found", + database: "user/db_not_found", user: "some_user", role: "read", }; @@ -200,6 +195,22 @@ async fn db_not_found() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn db_invalid() -> anyhow::Result<()> { + let server: TestServer = TestServer::new().await?; + let user = server.init_user().await?; + let role = AddUser { + database: "invalid", + user: "some_user", + role: "read", + }; + assert_eq!( + server.post(DB_USER_ADD_URI, &role, &user.token).await?.0, + 467 + ); + Ok(()) +} + #[tokio::test] async fn user_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 index 6da732c0b..46814237c 100644 --- a/agdb_server/tests/routes/db_user_remove_test.rs +++ b/agdb_server/tests/routes/db_user_remove_test.rs @@ -58,7 +58,7 @@ async fn remove() -> anyhow::Result<()> { } #[tokio::test] -async fn remove_user_non_admin() -> anyhow::Result<()> { +async fn remove_owner() -> anyhow::Result<()> { let server = TestServer::new().await?; let user = server.init_user().await?; let other = server.init_user().await?; @@ -66,7 +66,7 @@ async fn remove_user_non_admin() -> anyhow::Result<()> { let role = AddUser { database: &db, user: &other.name, - role: "read", + role: "admin", }; assert_eq!( server.post(DB_USER_ADD_URI, &role, &user.token).await?.0, @@ -84,37 +84,42 @@ async fn remove_user_non_admin() -> anyhow::Result<()> { } #[tokio::test] -async fn remove_self() -> anyhow::Result<()> { +async fn remove_user_non_admin() -> anyhow::Result<()> { let server = TestServer::new().await?; let user = server.init_user().await?; - let other = server.init_user().await?; + let other1 = server.init_user().await?; + let other2 = server.init_user().await?; let db = server.init_db("memory", &user).await?; - let role = AddUser { + let mut role = AddUser { database: &db, - user: &other.name, + user: &other1.name, role: "read", }; assert_eq!( server.post(DB_USER_ADD_URI, &role, &user.token).await?.0, 201 ); + role.user = &other2.name; + assert_eq!( + server.post(DB_USER_ADD_URI, &role, &user.token).await?.0, + 201 + ); let rem = RemoveUser { database: db, - user: other.name, + user: other1.name, }; assert_eq!( - server.post(DB_USER_REMOVE_URI, &rem, &other.token).await?.0, - 204 + server + .post(DB_USER_REMOVE_URI, &rem, &other2.token) + .await? + .0, + 403 ); - let (_, list) = server - .get::>(DB_LIST_URI, &other.token) - .await?; - assert_eq!(list?, vec![]); Ok(()) } #[tokio::test] -async fn remove_self_admin() -> anyhow::Result<()> { +async fn remove_self() -> anyhow::Result<()> { let server = TestServer::new().await?; let user = server.init_user().await?; let other = server.init_user().await?; @@ -122,7 +127,7 @@ async fn remove_self_admin() -> anyhow::Result<()> { let role = AddUser { database: &db, user: &other.name, - role: "admin", + role: "read", }; assert_eq!( server.post(DB_USER_ADD_URI, &role, &user.token).await?.0, @@ -136,50 +141,35 @@ async fn remove_self_admin() -> anyhow::Result<()> { server.post(DB_USER_REMOVE_URI, &rem, &other.token).await?.0, 204 ); - let (_, list) = server - .get::>(DB_LIST_URI, &other.token) - .await?; - assert_eq!(list?, vec![]); Ok(()) } #[tokio::test] -async fn remove_self_admin_last() -> anyhow::Result<()> { +async fn db_not_found() -> anyhow::Result<()> { let server = TestServer::new().await?; let user = server.init_user().await?; - let db = server.init_db("memory", &user).await?; let rem = RemoveUser { - database: db.clone(), - user: user.name, + database: format!("{}/db_not_found", user.name), + user: String::new(), }; assert_eq!( server.post(DB_USER_REMOVE_URI, &rem, &user.token).await?.0, - 403 + 466 ); - let (_, list) = server - .get::>(DB_LIST_URI, &user.token) - .await?; - let expected = vec![DbWithRole { - name: db, - db_type: "memory".to_string(), - role: "admin".to_string(), - size: 2600, - }]; - assert_eq!(list?, expected); Ok(()) } #[tokio::test] -async fn db_not_found() -> anyhow::Result<()> { +async fn db_invalid() -> anyhow::Result<()> { let server = TestServer::new().await?; let user = server.init_user().await?; let rem = RemoveUser { - database: format!("{}/db_not_found", user.name), + database: "invalid".to_string(), user: String::new(), }; assert_eq!( server.post(DB_USER_REMOVE_URI, &rem, &user.token).await?.0, - 466 + 467 ); Ok(()) } From cf87cc8022e801b95ae29706cea8b41157594dca Mon Sep 17 00:00:00 2001 From: Michael Vlach Date: Wed, 20 Dec 2023 21:12:27 +0100 Subject: [PATCH 2/2] trigger ci