[server] Disallow changing owner's db role #900

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": [

agdb_server/src/db_pool.rs

@@ -455,26 +455,6 @@ impl DbPool {
             .id)
     }
 
-    pub(crate) fn db_admins(&self, db: DbId) -> ServerResult<Vec<DbId>> {
-        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<DbId> {
         Ok(self
             .db()?

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,16 +18,29 @@ 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(
     _admin: AdminId,
     State(db_pool): State<DbPool>,
     Json(request): Json<DbUser>,
 ) -> 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,27 +86,31 @@ 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(
     _admin: AdminId,
     State(db_pool): State<DbPool>,
     Json(request): Json<RemoveDbUser>,
 ) -> 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)

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,26 +40,39 @@ 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(
     user: UserId,
     State(db_pool): State<DbPool>,
     Json(request): Json<DbUser>,
 ) -> 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,24 +116,36 @@ 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(
     user: UserId,
     State(db_pool): State<DbPool>,
     Json(request): Json<RemoveDbUser>,
 ) -> 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",
         ));
     }
 

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?;

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::<Vec<DbWithRole>>(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?;