From e534ebeaa9ce88f469c098930ef3346eb703fce0 Mon Sep 17 00:00:00 2001 From: konac-hamza Date: Tue, 20 Jan 2026 01:45:42 +0300 Subject: [PATCH 1/4] feat: Implement grant deletion provider api --- .../v3/role_assignment/project/user/role.rs | 3 +- .../project/user/role/grant.rs | 6 +- .../project/user/role/revoke.rs | 455 ++++++++++++++++++ src/assignment/backend.rs | 7 + src/assignment/backend/sql.rs | 10 + src/assignment/backend/sql/assignment.rs | 2 + .../backend/sql/assignment/delete.rs | 305 ++++++++++++ src/assignment/mock.rs | 6 + src/assignment/mod.rs | 10 + src/assignment/types/assignment.rs | 62 +++ src/assignment/types/provider_api.rs | 7 + 11 files changed, 869 insertions(+), 4 deletions(-) create mode 100644 src/api/v3/role_assignment/project/user/role/revoke.rs create mode 100644 src/assignment/backend/sql/assignment/delete.rs diff --git a/src/api/v3/role_assignment/project/user/role.rs b/src/api/v3/role_assignment/project/user/role.rs index 3afb8aec..c972d76f 100644 --- a/src/api/v3/role_assignment/project/user/role.rs +++ b/src/api/v3/role_assignment/project/user/role.rs @@ -18,7 +18,8 @@ use crate::keystone::ServiceState; mod check; mod grant; +mod revoke; pub(crate) fn openapi_router() -> OpenApiRouter { - OpenApiRouter::new().routes(routes!(check::check, grant::grant)) + OpenApiRouter::new().routes(routes!(check::check, grant::grant, revoke::revoke)) } diff --git a/src/api/v3/role_assignment/project/user/role/grant.rs b/src/api/v3/role_assignment/project/user/role/grant.rs index e7b7f536..5829403f 100644 --- a/src/api/v3/role_assignment/project/user/role/grant.rs +++ b/src/api/v3/role_assignment/project/user/role/grant.rs @@ -33,15 +33,15 @@ use crate::{ resource::ResourceApi, }; -/// Assign role to group on project +/// Assign role to user on project /// -/// Assigns a role to a group on a project. +/// Assigns a role to a user on a project. #[utoipa::path( put, path = "/projects/{project_id}/users/{user_id}/roles/{role_id}", operation_id = "/project/user/role:put", params( - ("role_id" = String, Path, description = "The user ID."), + ("role_id" = String, Path, description = "The role ID."), ("project_id" = String, Path, description = "The project ID."), ("user_id" = String, Path, description = "The user ID.") ), diff --git a/src/api/v3/role_assignment/project/user/role/revoke.rs b/src/api/v3/role_assignment/project/user/role/revoke.rs new file mode 100644 index 00000000..af501d1b --- /dev/null +++ b/src/api/v3/role_assignment/project/user/role/revoke.rs @@ -0,0 +1,455 @@ +// 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. +// +// SPDX-License-Identifier: Apache-2.0 + +//! Project user role: delete + +use axum::{ + extract::{Path, State}, + http::StatusCode, + response::IntoResponse, +}; +use mockall_double::double; +use serde_json::json; +use tracing::info; + +use crate::api::error::KeystoneApiError; +use crate::keystone::ServiceState; +#[double] +use crate::policy::Policy; +use crate::{ + api::auth::Auth, + assignment::{AssignmentApi, types::AssignmentRevoke}, + identity::IdentityApi, + resource::ResourceApi, +}; + +/// Revoke role from user on project +/// +/// Remove a role assignment for a user on a specific project. +#[utoipa::path( + delete, + path = "/projects/{project_id}/users/{user_id}/roles/{role_id}", + operation_id = "/project/user/role:delete", + params( + ("role_id" = String, Path, description = "The role ID."), + ("project_id" = String, Path, description = "The project ID."), + ("user_id" = String, Path, description = "The user ID."), + ), + responses( + (status = 204, description = "Role revoked successfully"), + (status = 401, description = "Unauthorized"), + (status = 403, description = "Forbidden"), + (status = 404, description = "Grant not found", example = json!(KeystoneApiError::NotFound(String::from("id = 1")))) + ), + security( + ("X-Auth-Token" = [])), + tag="Role Assignment" +)] +#[tracing::instrument( + name = "api::v3:project_user_role_revoke", + level = "debug", + skip(state, user_auth, policy), + err(Debug) +)] + +pub(super) async fn revoke( + Auth(user_auth): Auth, + mut policy: Policy, + Path((project_id, user_id, role_id)): Path<(String, String, String)>, + State(state): State, +) -> Result { + // Use join instead of try_join to have more constant latency preventing timing + // attacks. + let (user, role, project) = tokio::join!( + state + .provider + .get_identity_provider() + .get_user(&state, &user_id), + state + .provider + .get_assignment_provider() + .get_role(&state, &role_id), + state + .provider + .get_resource_provider() + .get_project(&state, &project_id) + ); + let user = user?.ok_or_else(|| { + info!("User {} was not found", user_id); + KeystoneApiError::NotFound { + resource: "grant".into(), + identifier: "".into(), + } + })?; + let project = project?.ok_or_else(|| { + info!("Project {} was not found", project_id); + KeystoneApiError::NotFound { + resource: "grant".into(), + identifier: "".into(), + } + })?; + let role = role?.ok_or_else(|| { + info!("Role {} was not found", role_id); + KeystoneApiError::NotFound { + resource: "grant".into(), + identifier: "".into(), + } + })?; + + policy + .enforce( + "identity/project/user/role/check", + &user_auth, + json!({"user": user, "role": role, "project": project}), + None, + ) + .await?; + + state + .provider + .get_assignment_provider() + .revoke_grant( + &state, + &AssignmentRevoke::user_project(user.id, project.id, role.id, false), + ) + .await?; + + Ok(StatusCode::NO_CONTENT.into_response()) +} + +#[cfg(test)] +mod tests { + use axum::{ + body::Body, + http::{Request, StatusCode}, + }; + use tower::ServiceExt; + use tower_http::trace::TraceLayer; + use tracing_test::traced_test; + + use crate::api::tests::get_mocked_state; + use crate::api::v3::role_assignment::openapi_router; + use crate::assignment::{MockAssignmentProvider, types::*}; + use crate::identity::{MockIdentityProvider, types::*}; + use crate::provider::Provider; + use crate::resource::{MockResourceProvider, types::Project}; + + #[tokio::test] + #[traced_test] + async fn test_revoke_success() { + let mut identity_mock = MockIdentityProvider::default(); + identity_mock + .expect_get_user() + .withf(|_, id: &'_ str| id == "user_id") + .returning(|_, _| { + Ok(Some(UserResponse { + id: "user_id".into(), + ..Default::default() + })) + }); + + let mut assignment_mock = MockAssignmentProvider::default(); + assignment_mock + .expect_get_role() + .withf(|_, rid: &'_ str| rid == "role_id") + .returning(|_, _| { + Ok(Some(Role { + id: "role_id".into(), + name: "test_role".into(), + ..Default::default() + })) + }); + assignment_mock + .expect_revoke_grant() + .withf(|_, grant: &AssignmentRevoke| { + grant.role_id == "role_id" + && grant.actor_id == "user_id" + && grant.target_id == "project_id" + && grant.r#type == AssignmentType::UserProject + && !grant.inherited + }) + .returning(|_, _| Ok(())); + + let mut resource_mock = MockResourceProvider::default(); + resource_mock + .expect_get_project() + .withf(|_, pid: &'_ str| pid == "project_id") + .returning(|_, id: &'_ str| { + Ok(Some(Project { + id: id.to_string(), + domain_id: "project_domain_id".into(), + ..Default::default() + })) + }); + + let provider_builder = Provider::mocked_builder() + .assignment(assignment_mock) + .identity(identity_mock) + .resource(resource_mock); + let state = get_mocked_state(provider_builder, true); + let mut api = openapi_router() + .layer(TraceLayer::new_for_http()) + .with_state(state); + + let response = api + .as_service() + .oneshot( + Request::builder() + .method("DELETE") + .uri("/projects/project_id/users/user_id/roles/role_id") + .header("x-auth-token", "foo") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::NO_CONTENT); + } + + #[tokio::test] + #[traced_test] + async fn test_revoke_forbidden() { + let mut identity_mock = MockIdentityProvider::default(); + identity_mock + .expect_get_user() + .withf(|_, id: &'_ str| id == "user_id") + .returning(|_, _| { + Ok(Some(UserResponse { + id: "user_id".into(), + ..Default::default() + })) + }); + + let mut assignment_mock = MockAssignmentProvider::default(); + assignment_mock + .expect_get_role() + .withf(|_, rid: &'_ str| rid == "role_id") + .returning(|_, _| { + Ok(Some(Role { + id: "role_id".into(), + name: "test_role".into(), + ..Default::default() + })) + }); + + let mut resource_mock = MockResourceProvider::default(); + resource_mock + .expect_get_project() + .withf(|_, pid: &'_ str| pid == "project_id") + .returning(|_, id: &'_ str| { + Ok(Some(Project { + id: id.to_string(), + domain_id: "project_domain_id".into(), + ..Default::default() + })) + }); + + let provider_builder = Provider::mocked_builder() + .assignment(assignment_mock) + .identity(identity_mock) + .resource(resource_mock); + let state = get_mocked_state(provider_builder, false); // Policy NOT allowed + let mut api = openapi_router() + .layer(TraceLayer::new_for_http()) + .with_state(state); + + let response = api + .as_service() + .oneshot( + Request::builder() + .method("DELETE") + .uri("/projects/project_id/users/user_id/roles/role_id") + .header("x-auth-token", "foo") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::FORBIDDEN); + } + + #[tokio::test] + #[traced_test] + async fn test_revoke_user_not_found() { + let mut identity_mock = MockIdentityProvider::default(); + identity_mock + .expect_get_user() + .withf(|_, id: &'_ str| id == "user_id") + .returning(|_, _| Ok(None)); // User not found + + let mut assignment_mock = MockAssignmentProvider::default(); + assignment_mock + .expect_get_role() + .withf(|_, rid: &'_ str| rid == "role_id") + .returning(|_, _| { + Ok(Some(Role { + id: "role_id".into(), + name: "test_role".into(), + ..Default::default() + })) + }); + + let mut resource_mock = MockResourceProvider::default(); + resource_mock + .expect_get_project() + .withf(|_, pid: &'_ str| pid == "project_id") + .returning(|_, id: &'_ str| { + Ok(Some(Project { + id: id.to_string(), + domain_id: "project_domain_id".into(), + ..Default::default() + })) + }); + + let provider_builder = Provider::mocked_builder() + .assignment(assignment_mock) + .identity(identity_mock) + .resource(resource_mock); + let state = get_mocked_state(provider_builder, true); + let mut api = openapi_router() + .layer(TraceLayer::new_for_http()) + .with_state(state); + + let response = api + .as_service() + .oneshot( + Request::builder() + .method("DELETE") + .uri("/projects/project_id/users/user_id/roles/role_id") + .header("x-auth-token", "foo") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::NOT_FOUND); + } + + #[tokio::test] + #[traced_test] + async fn test_revoke_project_not_found() { + let mut identity_mock = MockIdentityProvider::default(); + identity_mock + .expect_get_user() + .withf(|_, id: &'_ str| id == "user_id") + .returning(|_, _| { + Ok(Some(UserResponse { + id: "user_id".into(), + ..Default::default() + })) + }); + + let mut assignment_mock = MockAssignmentProvider::default(); + assignment_mock + .expect_get_role() + .withf(|_, rid: &'_ str| rid == "role_id") + .returning(|_, _| { + Ok(Some(Role { + id: "role_id".into(), + name: "test_role".into(), + ..Default::default() + })) + }); + + let mut resource_mock = MockResourceProvider::default(); + resource_mock + .expect_get_project() + .withf(|_, pid: &'_ str| pid == "project_id") + .returning(|_, _| Ok(None)); // Project not found + + let provider_builder = Provider::mocked_builder() + .assignment(assignment_mock) + .identity(identity_mock) + .resource(resource_mock); + let state = get_mocked_state(provider_builder, true); + let mut api = openapi_router() + .layer(TraceLayer::new_for_http()) + .with_state(state); + + let response = api + .as_service() + .oneshot( + Request::builder() + .method("DELETE") + .uri("/projects/project_id/users/user_id/roles/role_id") + .header("x-auth-token", "foo") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::NOT_FOUND); + } + + #[tokio::test] + #[traced_test] + async fn test_revoke_role_not_found() { + let mut identity_mock = MockIdentityProvider::default(); + identity_mock + .expect_get_user() + .withf(|_, id: &'_ str| id == "user_id") + .returning(|_, _| { + Ok(Some(UserResponse { + id: "user_id".into(), + ..Default::default() + })) + }); + + let mut assignment_mock = MockAssignmentProvider::default(); + assignment_mock + .expect_get_role() + .withf(|_, rid: &'_ str| rid == "role_id") + .returning(|_, _| Ok(None)); // Role not found + + let mut resource_mock = MockResourceProvider::default(); + resource_mock + .expect_get_project() + .withf(|_, pid: &'_ str| pid == "project_id") + .returning(|_, id: &'_ str| { + Ok(Some(Project { + id: id.to_string(), + domain_id: "project_domain_id".into(), + ..Default::default() + })) + }); + + let provider_builder = Provider::mocked_builder() + .assignment(assignment_mock) + .identity(identity_mock) + .resource(resource_mock); + let state = get_mocked_state(provider_builder, true); + let mut api = openapi_router() + .layer(TraceLayer::new_for_http()) + .with_state(state); + + let response = api + .as_service() + .oneshot( + Request::builder() + .method("DELETE") + .uri("/projects/project_id/users/user_id/roles/role_id") + .header("x-auth-token", "foo") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::NOT_FOUND); + } +} diff --git a/src/assignment/backend.rs b/src/assignment/backend.rs index 17dd8313..fa086c3f 100644 --- a/src/assignment/backend.rs +++ b/src/assignment/backend.rs @@ -86,4 +86,11 @@ pub trait AssignmentBackend: Send + Sync { state: &ServiceState, params: &RoleListParameters, ) -> Result, AssignmentProviderError>; + + /// Revoke assignment grant. + async fn revoke_grant( + &self, + state: &ServiceState, + params: &AssignmentRevoke, + ) -> Result<(), AssignmentProviderError>; } diff --git a/src/assignment/backend/sql.rs b/src/assignment/backend/sql.rs index 5611e29a..97d29682 100644 --- a/src/assignment/backend/sql.rs +++ b/src/assignment/backend/sql.rs @@ -130,4 +130,14 @@ impl AssignmentBackend for SqlBackend { ) -> Result, AssignmentProviderError> { Ok(assignment::list_for_multiple_actors_and_targets(&state.db, params).await?) } + + /// Revoke assignment grant. + #[tracing::instrument(level = "info", skip(self, state))] + async fn revoke_grant( + &self, + state: &ServiceState, + grant: &AssignmentRevoke, + ) -> Result<(), AssignmentProviderError> { + Ok(assignment::delete(&state.db, grant).await?) + } } diff --git a/src/assignment/backend/sql/assignment.rs b/src/assignment/backend/sql/assignment.rs index 98b4cdee..dd1960ea 100644 --- a/src/assignment/backend/sql/assignment.rs +++ b/src/assignment/backend/sql/assignment.rs @@ -23,10 +23,12 @@ use crate::db::entity::{ mod check; mod create; +mod delete; mod list; pub use check::check; pub use create::create; +pub use delete::delete; pub use list::list; pub use list::list_for_multiple_actors_and_targets; diff --git a/src/assignment/backend/sql/assignment/delete.rs b/src/assignment/backend/sql/assignment/delete.rs new file mode 100644 index 00000000..88e3c40a --- /dev/null +++ b/src/assignment/backend/sql/assignment/delete.rs @@ -0,0 +1,305 @@ +// 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. +// +// SPDX-License-Identifier: Apache-2.0 + +use sea_orm::entity::*; +use sea_orm::{DatabaseConnection, EntityTrait, QueryFilter}; + +use crate::assignment::backend::error::AssignmentDatabaseError; +use crate::assignment::types::*; +use crate::db::entity::{ + assignment as db_assignment, sea_orm_active_enums::Type as DbAssignmentType, + system_assignment as db_system_assignment, +}; +use crate::error::DbContextExt; + +/// Delete assignment grant. +pub async fn delete( + db: &DatabaseConnection, + grant: &AssignmentRevoke, +) -> Result<(), AssignmentDatabaseError> { + if grant.inherited { + // Cannot delete inherited assignments directly + return Ok(()); + } + + match &grant.r#type { + AssignmentType::GroupDomain + | AssignmentType::GroupProject + | AssignmentType::UserDomain + | AssignmentType::UserProject => { + db_assignment::Entity::delete_many() + .filter(db_assignment::Column::RoleId.eq(&grant.role_id)) + .filter(db_assignment::Column::TargetId.eq(&grant.target_id)) + .filter(db_assignment::Column::ActorId.eq(&grant.actor_id)) + .filter(db_assignment::Column::Type.eq(DbAssignmentType::try_from(&grant.r#type)?)) + .filter(db_assignment::Column::Inherited.eq(false)) + .exec(db) + .await + .context("deleting assignment")?; + } + AssignmentType::GroupSystem | AssignmentType::UserSystem => { + db_system_assignment::Entity::delete_many() + .filter(db_system_assignment::Column::RoleId.eq(&grant.role_id)) + .filter(db_system_assignment::Column::TargetId.eq(&grant.target_id)) + .filter(db_system_assignment::Column::ActorId.eq(&grant.actor_id)) + .filter(db_system_assignment::Column::Type.eq(grant.r#type.to_string())) + .filter(db_system_assignment::Column::Inherited.eq(false)) + .exec(db) + .await + .context("deleting system assignment")?; + } + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use sea_orm::{DatabaseBackend, MockDatabase, MockExecResult, Transaction}; + + use super::*; + + #[tokio::test] + async fn test_delete_user_project() { + // Create MockDatabase with mock query results + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_exec_results([MockExecResult { + last_insert_id: 0, + rows_affected: 1, // 1 row deleted + }]) + .into_connection(); + + let grant = AssignmentRevoke { + role_id: "role_id".into(), + actor_id: "actor_id".into(), + target_id: "target_id".into(), + r#type: AssignmentType::UserProject, + inherited: false, + }; + + delete(&db, &grant).await.unwrap(); + + // Checking transaction log + assert_eq!( + db.into_transaction_log(), + [Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"DELETE FROM "assignment" WHERE "assignment"."role_id" = $1 AND "assignment"."target_id" = $2 AND "assignment"."actor_id" = $3 AND "assignment"."type" = (CAST($4 AS "type")) AND "assignment"."inherited" = $5"#, + [ + "role_id".into(), + "target_id".into(), + "actor_id".into(), + "UserProject".into(), + false.into(), + ] + ),] + ); + } + + #[tokio::test] + async fn test_delete_group_project() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_exec_results([MockExecResult { + last_insert_id: 0, + rows_affected: 1, + }]) + .into_connection(); + + let grant = AssignmentRevoke { + role_id: "role_id".into(), + actor_id: "group_id".into(), + target_id: "project_id".into(), + r#type: AssignmentType::GroupProject, + inherited: false, + }; + + delete(&db, &grant).await.unwrap(); + + assert_eq!( + db.into_transaction_log(), + [Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"DELETE FROM "assignment" WHERE "assignment"."role_id" = $1 AND "assignment"."target_id" = $2 AND "assignment"."actor_id" = $3 AND "assignment"."type" = (CAST($4 AS "type")) AND "assignment"."inherited" = $5"#, + [ + "role_id".into(), + "project_id".into(), + "group_id".into(), + "GroupProject".into(), + false.into(), + ] + ),] + ); + } + + #[tokio::test] + async fn test_delete_user_domain() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_exec_results([MockExecResult { + last_insert_id: 0, + rows_affected: 1, + }]) + .into_connection(); + + let grant = AssignmentRevoke { + role_id: "role_id".into(), + actor_id: "user_id".into(), + target_id: "domain_id".into(), + r#type: AssignmentType::UserDomain, + inherited: false, + }; + + delete(&db, &grant).await.unwrap(); + + assert_eq!( + db.into_transaction_log(), + [Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"DELETE FROM "assignment" WHERE "assignment"."role_id" = $1 AND "assignment"."target_id" = $2 AND "assignment"."actor_id" = $3 AND "assignment"."type" = (CAST($4 AS "type")) AND "assignment"."inherited" = $5"#, + [ + "role_id".into(), + "domain_id".into(), + "user_id".into(), + "UserDomain".into(), + false.into(), + ] + ),] + ); + } + + #[tokio::test] + async fn test_delete_user_system() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_exec_results([MockExecResult { + last_insert_id: 0, + rows_affected: 1, + }]) + .into_connection(); + + let grant = AssignmentRevoke { + role_id: "role_id".into(), + actor_id: "user_id".into(), + target_id: "system".into(), + r#type: AssignmentType::UserSystem, + inherited: false, + }; + + delete(&db, &grant).await.unwrap(); + + assert_eq!( + db.into_transaction_log(), + [Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"DELETE FROM "system_assignment" WHERE "system_assignment"."role_id" = $1 AND "system_assignment"."target_id" = $2 AND "system_assignment"."actor_id" = $3 AND "system_assignment"."type" = $4 AND "system_assignment"."inherited" = $5"#, + [ + "role_id".into(), + "system".into(), + "user_id".into(), + "UserSystem".into(), + false.into(), + ] + ),] + ); + } + + #[tokio::test] + async fn test_delete_group_system() { + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_exec_results([MockExecResult { + last_insert_id: 0, + rows_affected: 1, + }]) + .into_connection(); + + let grant = AssignmentRevoke { + role_id: "role_id".into(), + actor_id: "group_id".into(), + target_id: "system".into(), + r#type: AssignmentType::GroupSystem, + inherited: false, + }; + + delete(&db, &grant).await.unwrap(); + + assert_eq!( + db.into_transaction_log(), + [Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"DELETE FROM "system_assignment" WHERE "system_assignment"."role_id" = $1 AND "system_assignment"."target_id" = $2 AND "system_assignment"."actor_id" = $3 AND "system_assignment"."type" = $4 AND "system_assignment"."inherited" = $5"#, + [ + "role_id".into(), + "system".into(), + "group_id".into(), + "GroupSystem".into(), + false.into(), + ] + ),] + ); + } + + #[tokio::test] + async fn test_delete_inherited_succeeds_without_deletion() { + // Inherited grants should be silently skipped + let db = MockDatabase::new(DatabaseBackend::Postgres).into_connection(); + + let grant = AssignmentRevoke { + role_id: "role_id".into(), + actor_id: "user_id".into(), + target_id: "project_id".into(), + r#type: AssignmentType::UserProject, + inherited: true, // ← Inherited + }; + + delete(&db, &grant).await.unwrap(); + + // No SQL should be executed for inherited grants + assert_eq!(db.into_transaction_log(), []); + } + + #[tokio::test] + async fn test_delete_not_found_succeeds() { + // Deleting non-existent grant should succeed (idempotent) + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_exec_results([MockExecResult { + last_insert_id: 0, + rows_affected: 0, // ← 0 rows deleted + }]) + .into_connection(); + + let grant = AssignmentRevoke { + role_id: "nonexistent_role".into(), + actor_id: "user_id".into(), + target_id: "project_id".into(), + r#type: AssignmentType::UserProject, + inherited: false, + }; + + // Should succeed even though nothing was deleted + delete(&db, &grant).await.unwrap(); + + assert_eq!( + db.into_transaction_log(), + [Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"DELETE FROM "assignment" WHERE "assignment"."role_id" = $1 AND "assignment"."target_id" = $2 AND "assignment"."actor_id" = $3 AND "assignment"."type" = (CAST($4 AS "type")) AND "assignment"."inherited" = $5"#, + [ + "nonexistent_role".into(), + "project_id".into(), + "user_id".into(), + "UserProject".into(), + false.into(), + ] + ),] + ); + } +} diff --git a/src/assignment/mock.rs b/src/assignment/mock.rs index b24bb7fb..d00501c1 100644 --- a/src/assignment/mock.rs +++ b/src/assignment/mock.rs @@ -63,5 +63,11 @@ mock! { state: &ServiceState, params: &RoleAssignmentListParameters, ) -> Result, AssignmentProviderError>; + + async fn revoke_grant( + &self, + state: &ServiceState, + params: &AssignmentRevoke, + ) -> Result<(), AssignmentProviderError>; } } diff --git a/src/assignment/mod.rs b/src/assignment/mod.rs index aae1debd..9bacd86b 100644 --- a/src/assignment/mod.rs +++ b/src/assignment/mod.rs @@ -230,4 +230,14 @@ impl AssignmentApi for AssignmentProvider { .list_assignments_for_multiple_actors_and_targets(state, &request.build()?) .await } + + /// Revoke grant + #[tracing::instrument(level = "info", skip(self, state))] + async fn revoke_grant( + &self, + state: &ServiceState, + grant: &AssignmentRevoke, + ) -> Result<(), AssignmentProviderError> { + self.backend_driver.revoke_grant(state, &grant).await + } } diff --git a/src/assignment/types/assignment.rs b/src/assignment/types/assignment.rs index 37665ec5..adf709cd 100644 --- a/src/assignment/types/assignment.rs +++ b/src/assignment/types/assignment.rs @@ -293,3 +293,65 @@ pub enum RoleAssignmentTargetType { /// System ID. System, } + +#[derive(Builder, Clone, Debug, Deserialize, PartialEq, Serialize, Validate)] +#[builder(build_fn(error = "BuilderError"))] +#[builder(setter(strip_option, into))] +pub struct AssignmentRevoke { + /// The actor id. + #[validate(length(max = 64))] + pub actor_id: String, + + /// The role ID. + #[validate(length(max = 64))] + pub role_id: String, + + /// The target id. + #[validate(length(max = 64))] + pub target_id: String, + + /// The assignment type. + pub r#type: AssignmentType, + + /// Inherited flag. + pub inherited: bool, +} +impl AssignmentRevoke { + /// Instantiate new assignment revoke. + pub fn new( + actor_id: A, + target_id: T, + role_id: R, + r#type: AssignmentType, + inherited: bool, + ) -> Self + where + A: Into, + T: Into, + R: Into, + { + Self { + actor_id: actor_id.into(), + target_id: target_id.into(), + role_id: role_id.into(), + r#type, + inherited, + } + } + + /// Instantiate UserProject assignment. + pub fn user_project(actor_id: A, target_id: T, role_id: R, inherited: bool) -> Self + where + A: Into, + T: Into, + R: Into, + { + Self::new( + actor_id, + target_id, + role_id, + AssignmentType::UserProject, + inherited, + ) + } +} diff --git a/src/assignment/types/provider_api.rs b/src/assignment/types/provider_api.rs index 5714e7c1..bf2b8efd 100644 --- a/src/assignment/types/provider_api.rs +++ b/src/assignment/types/provider_api.rs @@ -62,4 +62,11 @@ pub trait AssignmentApi: Send + Sync { state: &ServiceState, params: &RoleAssignmentListParameters, ) -> Result, AssignmentProviderError>; + + /// Revoke assignment grant. + async fn revoke_grant( + &self, + state: &ServiceState, + params: &AssignmentRevoke, + ) -> Result<(), AssignmentProviderError>; } From e4b554a9c890ae54d0515221707206a32a82b0af Mon Sep 17 00:00:00 2001 From: konac-hamza Date: Tue, 20 Jan 2026 02:00:37 +0300 Subject: [PATCH 2/4] fix: Fix reference warning --- .../role_assignment/project/user/role/revoke.rs | 2 +- src/assignment/backend.rs | 2 +- src/assignment/backend/sql.rs | 2 +- src/assignment/backend/sql/assignment/delete.rs | 16 ++++++++-------- src/assignment/mock.rs | 2 +- src/assignment/mod.rs | 4 ++-- src/assignment/types/provider_api.rs | 2 +- 7 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/api/v3/role_assignment/project/user/role/revoke.rs b/src/api/v3/role_assignment/project/user/role/revoke.rs index af501d1b..0da684e1 100644 --- a/src/api/v3/role_assignment/project/user/role/revoke.rs +++ b/src/api/v3/role_assignment/project/user/role/revoke.rs @@ -121,7 +121,7 @@ pub(super) async fn revoke( .get_assignment_provider() .revoke_grant( &state, - &AssignmentRevoke::user_project(user.id, project.id, role.id, false), + AssignmentRevoke::user_project(user.id, project.id, role.id, false), ) .await?; diff --git a/src/assignment/backend.rs b/src/assignment/backend.rs index fa086c3f..6132e8c6 100644 --- a/src/assignment/backend.rs +++ b/src/assignment/backend.rs @@ -91,6 +91,6 @@ pub trait AssignmentBackend: Send + Sync { async fn revoke_grant( &self, state: &ServiceState, - params: &AssignmentRevoke, + params: AssignmentRevoke, ) -> Result<(), AssignmentProviderError>; } diff --git a/src/assignment/backend/sql.rs b/src/assignment/backend/sql.rs index 97d29682..a8d934f3 100644 --- a/src/assignment/backend/sql.rs +++ b/src/assignment/backend/sql.rs @@ -136,7 +136,7 @@ impl AssignmentBackend for SqlBackend { async fn revoke_grant( &self, state: &ServiceState, - grant: &AssignmentRevoke, + grant: AssignmentRevoke, ) -> Result<(), AssignmentProviderError> { Ok(assignment::delete(&state.db, grant).await?) } diff --git a/src/assignment/backend/sql/assignment/delete.rs b/src/assignment/backend/sql/assignment/delete.rs index 88e3c40a..777f850d 100644 --- a/src/assignment/backend/sql/assignment/delete.rs +++ b/src/assignment/backend/sql/assignment/delete.rs @@ -26,7 +26,7 @@ use crate::error::DbContextExt; /// Delete assignment grant. pub async fn delete( db: &DatabaseConnection, - grant: &AssignmentRevoke, + grant: AssignmentRevoke, ) -> Result<(), AssignmentDatabaseError> { if grant.inherited { // Cannot delete inherited assignments directly @@ -88,7 +88,7 @@ mod tests { inherited: false, }; - delete(&db, &grant).await.unwrap(); + delete(&db, grant).await.unwrap(); // Checking transaction log assert_eq!( @@ -124,7 +124,7 @@ mod tests { inherited: false, }; - delete(&db, &grant).await.unwrap(); + delete(&db, grant).await.unwrap(); assert_eq!( db.into_transaction_log(), @@ -159,7 +159,7 @@ mod tests { inherited: false, }; - delete(&db, &grant).await.unwrap(); + delete(&db, grant).await.unwrap(); assert_eq!( db.into_transaction_log(), @@ -194,7 +194,7 @@ mod tests { inherited: false, }; - delete(&db, &grant).await.unwrap(); + delete(&db, grant).await.unwrap(); assert_eq!( db.into_transaction_log(), @@ -229,7 +229,7 @@ mod tests { inherited: false, }; - delete(&db, &grant).await.unwrap(); + delete(&db, grant).await.unwrap(); assert_eq!( db.into_transaction_log(), @@ -260,7 +260,7 @@ mod tests { inherited: true, // ← Inherited }; - delete(&db, &grant).await.unwrap(); + delete(&db, grant).await.unwrap(); // No SQL should be executed for inherited grants assert_eq!(db.into_transaction_log(), []); @@ -285,7 +285,7 @@ mod tests { }; // Should succeed even though nothing was deleted - delete(&db, &grant).await.unwrap(); + delete(&db, grant).await.unwrap(); assert_eq!( db.into_transaction_log(), diff --git a/src/assignment/mock.rs b/src/assignment/mock.rs index d00501c1..cdf58cf6 100644 --- a/src/assignment/mock.rs +++ b/src/assignment/mock.rs @@ -67,7 +67,7 @@ mock! { async fn revoke_grant( &self, state: &ServiceState, - params: &AssignmentRevoke, + params: AssignmentRevoke, ) -> Result<(), AssignmentProviderError>; } } diff --git a/src/assignment/mod.rs b/src/assignment/mod.rs index 9bacd86b..d099b57e 100644 --- a/src/assignment/mod.rs +++ b/src/assignment/mod.rs @@ -236,8 +236,8 @@ impl AssignmentApi for AssignmentProvider { async fn revoke_grant( &self, state: &ServiceState, - grant: &AssignmentRevoke, + grant: AssignmentRevoke, ) -> Result<(), AssignmentProviderError> { - self.backend_driver.revoke_grant(state, &grant).await + self.backend_driver.revoke_grant(state, grant).await } } diff --git a/src/assignment/types/provider_api.rs b/src/assignment/types/provider_api.rs index bf2b8efd..d45f5452 100644 --- a/src/assignment/types/provider_api.rs +++ b/src/assignment/types/provider_api.rs @@ -67,6 +67,6 @@ pub trait AssignmentApi: Send + Sync { async fn revoke_grant( &self, state: &ServiceState, - params: &AssignmentRevoke, + params: AssignmentRevoke, ) -> Result<(), AssignmentProviderError>; } From 8273dd0c350f686f804da3d1b03c06bbf402eba5 Mon Sep 17 00:00:00 2001 From: konac-hamza Date: Thu, 22 Jan 2026 01:50:00 +0300 Subject: [PATCH 3/4] fix: Use Assignment struct to obtain clean design Add policy to revoke role from user --- policy/project/user/role/revoke.rego | 27 ++++++++ policy/project/user/role/revoke_test.rego | 20 ++++++ .../project/user/role/revoke.rs | 19 +++--- src/assignment/backend.rs | 2 +- src/assignment/backend/sql.rs | 2 +- .../backend/sql/assignment/delete.rs | 30 ++++++--- src/assignment/mock.rs | 2 +- src/assignment/mod.rs | 2 +- src/assignment/types/assignment.rs | 62 ------------------- src/assignment/types/provider_api.rs | 2 +- 10 files changed, 86 insertions(+), 82 deletions(-) create mode 100644 policy/project/user/role/revoke.rego create mode 100644 policy/project/user/role/revoke_test.rego diff --git a/policy/project/user/role/revoke.rego b/policy/project/user/role/revoke.rego new file mode 100644 index 00000000..e4b4cf57 --- /dev/null +++ b/policy/project/user/role/revoke.rego @@ -0,0 +1,27 @@ +package identity.project.user.role.revoke + +import data.identity +import data.identity.assignment + +# Revoke user a role on a project + +default allow := false + +allow if { + "admin" in input.credentials.roles +} + +allow if { + "manager" in input.credentials.roles + assignment.project_role_domain_matches +} + + +violation contains {"field": "domain_id", "msg": "revoking a role from a user on a project requires admin or manager role in the domain scope."} if { + not "admin" in input.credentials.roles + not "manager" in input.credentials.roles +} + +violation contains {"field": "domain_id", "msg": "revoking a role from a user on a project requires domain scope matching the domain_id of the target project and role (or a global role)."} if { + assignment.project_role_domain_matches +} diff --git a/policy/project/user/role/revoke_test.rego b/policy/project/user/role/revoke_test.rego new file mode 100644 index 00000000..fd3149bd --- /dev/null +++ b/policy/project/user/role/revoke_test.rego @@ -0,0 +1,20 @@ +package test_project_user_role_revoke + +import data.identity.project.user.role.revoke + +test_allowed if { + revoke.allow with input as {"credentials": {"roles": ["admin"]}} + revoke.allow with input as {"credentials": {"roles": ["manager"], "domain_id": "foo"}, "target": {"user": {"domain_id": "foo"}, "project": {"domain_id": "foo"}, "role": {"domain_id": null}}} + revoke.allow with input as {"credentials": {"roles": ["manager"], "domain_id": "foo"}, "target": {"user": {"domain_id": "foo"}, "project": {"domain_id": "foo"}, "role": {"domain_id": "foo"}}} +} + +test_forbidden if { + not revoke.allow with input as {"credentials": {"roles": []}} + not revoke.allow with input as {"credentials": {"roles": ["reader"], "system": "foo"}} + not revoke.allow with input as {"credentials": {"roles": ["reader"], "domain_id": "foo"}, "target": {"user": {"domain_id": "foo1"}, "project": {"domain_id": "foo"}, "role": {"domain_id": "foo"}}} + not revoke.allow with input as {"credentials": {"roles": ["reader"], "domain_id": "foo"}, "target": {"user": {"domain_id": "foo1"}, "project": {"domain_id": "foo1"}, "role": {"domain_id": "foo1"}}} + not revoke.allow with input as {"credentials": {"roles": ["reader"], "domain_id": "foo"}, "target": {"user": {"domain_id": "foo"}, "project": {"domain_id": "foo1"}, "role": {"domain_id": "foo1"}}} + not revoke.allow with input as {"credentials": {"roles": ["reader"], "domain_id": "foo"}, "target": {"user": {"domain_id": "foo1"}, "project": {"domain_id": "foo1"}, "role": {"domain_id": "foo1"}}} + not revoke.allow with input as {"credentials": {"roles": ["reader"], "domain_id": "foo"}, "target": {"project": {"domain_id": "foo"}, "role": {"domain_id": "foo"}}} + not revoke.allow with input as {"credentials": {"roles": ["member"], "domain_id": "foo"}, "target": {"project": {"domain_id": "foo"}, "role": {"domain_id": "foo"}, "user": {"domain_id": "foo"}}} +} diff --git a/src/api/v3/role_assignment/project/user/role/revoke.rs b/src/api/v3/role_assignment/project/user/role/revoke.rs index 0da684e1..a242b988 100644 --- a/src/api/v3/role_assignment/project/user/role/revoke.rs +++ b/src/api/v3/role_assignment/project/user/role/revoke.rs @@ -29,7 +29,7 @@ use crate::keystone::ServiceState; use crate::policy::Policy; use crate::{ api::auth::Auth, - assignment::{AssignmentApi, types::AssignmentRevoke}, + assignment::{AssignmentApi, types::AssignmentBuilder, types::AssignmentType}, identity::IdentityApi, resource::ResourceApi, }; @@ -109,20 +109,25 @@ pub(super) async fn revoke( policy .enforce( - "identity/project/user/role/check", + "identity/project/user/role/revoke", &user_auth, json!({"user": user, "role": role, "project": project}), None, ) .await?; + let grant = AssignmentBuilder::default() + .actor_id(user_id) + .role_id(role_id) + .target_id(project_id) + .r#type(AssignmentType::UserProject) + .inherited(false) + .build()?; + state .provider .get_assignment_provider() - .revoke_grant( - &state, - AssignmentRevoke::user_project(user.id, project.id, role.id, false), - ) + .revoke_grant(&state, grant) .await?; Ok(StatusCode::NO_CONTENT.into_response()) @@ -172,7 +177,7 @@ mod tests { }); assignment_mock .expect_revoke_grant() - .withf(|_, grant: &AssignmentRevoke| { + .withf(|_, grant: &Assignment| { grant.role_id == "role_id" && grant.actor_id == "user_id" && grant.target_id == "project_id" diff --git a/src/assignment/backend.rs b/src/assignment/backend.rs index 6132e8c6..aec134d5 100644 --- a/src/assignment/backend.rs +++ b/src/assignment/backend.rs @@ -91,6 +91,6 @@ pub trait AssignmentBackend: Send + Sync { async fn revoke_grant( &self, state: &ServiceState, - params: AssignmentRevoke, + params: Assignment, ) -> Result<(), AssignmentProviderError>; } diff --git a/src/assignment/backend/sql.rs b/src/assignment/backend/sql.rs index a8d934f3..ed984434 100644 --- a/src/assignment/backend/sql.rs +++ b/src/assignment/backend/sql.rs @@ -136,7 +136,7 @@ impl AssignmentBackend for SqlBackend { async fn revoke_grant( &self, state: &ServiceState, - grant: AssignmentRevoke, + grant: Assignment, ) -> Result<(), AssignmentProviderError> { Ok(assignment::delete(&state.db, grant).await?) } diff --git a/src/assignment/backend/sql/assignment/delete.rs b/src/assignment/backend/sql/assignment/delete.rs index 777f850d..3cb38374 100644 --- a/src/assignment/backend/sql/assignment/delete.rs +++ b/src/assignment/backend/sql/assignment/delete.rs @@ -26,7 +26,7 @@ use crate::error::DbContextExt; /// Delete assignment grant. pub async fn delete( db: &DatabaseConnection, - grant: AssignmentRevoke, + grant: Assignment, ) -> Result<(), AssignmentDatabaseError> { if grant.inherited { // Cannot delete inherited assignments directly @@ -80,12 +80,14 @@ mod tests { }]) .into_connection(); - let grant = AssignmentRevoke { + let grant = Assignment { role_id: "role_id".into(), actor_id: "actor_id".into(), target_id: "target_id".into(), r#type: AssignmentType::UserProject, inherited: false, + role_name: None, + implied_via: None, }; delete(&db, grant).await.unwrap(); @@ -116,12 +118,14 @@ mod tests { }]) .into_connection(); - let grant = AssignmentRevoke { + let grant = Assignment { role_id: "role_id".into(), actor_id: "group_id".into(), target_id: "project_id".into(), r#type: AssignmentType::GroupProject, inherited: false, + role_name: None, + implied_via: None, }; delete(&db, grant).await.unwrap(); @@ -151,12 +155,14 @@ mod tests { }]) .into_connection(); - let grant = AssignmentRevoke { + let grant = Assignment { role_id: "role_id".into(), actor_id: "user_id".into(), target_id: "domain_id".into(), r#type: AssignmentType::UserDomain, inherited: false, + role_name: None, + implied_via: None, }; delete(&db, grant).await.unwrap(); @@ -186,12 +192,14 @@ mod tests { }]) .into_connection(); - let grant = AssignmentRevoke { + let grant = Assignment { role_id: "role_id".into(), actor_id: "user_id".into(), target_id: "system".into(), r#type: AssignmentType::UserSystem, inherited: false, + role_name: None, + implied_via: None, }; delete(&db, grant).await.unwrap(); @@ -221,12 +229,14 @@ mod tests { }]) .into_connection(); - let grant = AssignmentRevoke { + let grant = Assignment { role_id: "role_id".into(), actor_id: "group_id".into(), target_id: "system".into(), r#type: AssignmentType::GroupSystem, inherited: false, + role_name: None, + implied_via: None, }; delete(&db, grant).await.unwrap(); @@ -252,12 +262,14 @@ mod tests { // Inherited grants should be silently skipped let db = MockDatabase::new(DatabaseBackend::Postgres).into_connection(); - let grant = AssignmentRevoke { + let grant = Assignment { role_id: "role_id".into(), actor_id: "user_id".into(), target_id: "project_id".into(), r#type: AssignmentType::UserProject, inherited: true, // ← Inherited + role_name: None, + implied_via: None, }; delete(&db, grant).await.unwrap(); @@ -276,12 +288,14 @@ mod tests { }]) .into_connection(); - let grant = AssignmentRevoke { + let grant = Assignment { role_id: "nonexistent_role".into(), actor_id: "user_id".into(), target_id: "project_id".into(), r#type: AssignmentType::UserProject, inherited: false, + role_name: None, + implied_via: None, }; // Should succeed even though nothing was deleted diff --git a/src/assignment/mock.rs b/src/assignment/mock.rs index cdf58cf6..9092d85c 100644 --- a/src/assignment/mock.rs +++ b/src/assignment/mock.rs @@ -67,7 +67,7 @@ mock! { async fn revoke_grant( &self, state: &ServiceState, - params: AssignmentRevoke, + params: Assignment, ) -> Result<(), AssignmentProviderError>; } } diff --git a/src/assignment/mod.rs b/src/assignment/mod.rs index d099b57e..87f27b28 100644 --- a/src/assignment/mod.rs +++ b/src/assignment/mod.rs @@ -236,7 +236,7 @@ impl AssignmentApi for AssignmentProvider { async fn revoke_grant( &self, state: &ServiceState, - grant: AssignmentRevoke, + grant: Assignment, ) -> Result<(), AssignmentProviderError> { self.backend_driver.revoke_grant(state, grant).await } diff --git a/src/assignment/types/assignment.rs b/src/assignment/types/assignment.rs index adf709cd..37665ec5 100644 --- a/src/assignment/types/assignment.rs +++ b/src/assignment/types/assignment.rs @@ -293,65 +293,3 @@ pub enum RoleAssignmentTargetType { /// System ID. System, } - -#[derive(Builder, Clone, Debug, Deserialize, PartialEq, Serialize, Validate)] -#[builder(build_fn(error = "BuilderError"))] -#[builder(setter(strip_option, into))] -pub struct AssignmentRevoke { - /// The actor id. - #[validate(length(max = 64))] - pub actor_id: String, - - /// The role ID. - #[validate(length(max = 64))] - pub role_id: String, - - /// The target id. - #[validate(length(max = 64))] - pub target_id: String, - - /// The assignment type. - pub r#type: AssignmentType, - - /// Inherited flag. - pub inherited: bool, -} -impl AssignmentRevoke { - /// Instantiate new assignment revoke. - pub fn new( - actor_id: A, - target_id: T, - role_id: R, - r#type: AssignmentType, - inherited: bool, - ) -> Self - where - A: Into, - T: Into, - R: Into, - { - Self { - actor_id: actor_id.into(), - target_id: target_id.into(), - role_id: role_id.into(), - r#type, - inherited, - } - } - - /// Instantiate UserProject assignment. - pub fn user_project(actor_id: A, target_id: T, role_id: R, inherited: bool) -> Self - where - A: Into, - T: Into, - R: Into, - { - Self::new( - actor_id, - target_id, - role_id, - AssignmentType::UserProject, - inherited, - ) - } -} diff --git a/src/assignment/types/provider_api.rs b/src/assignment/types/provider_api.rs index d45f5452..3cf5a3aa 100644 --- a/src/assignment/types/provider_api.rs +++ b/src/assignment/types/provider_api.rs @@ -67,6 +67,6 @@ pub trait AssignmentApi: Send + Sync { async fn revoke_grant( &self, state: &ServiceState, - params: AssignmentRevoke, + params: Assignment, ) -> Result<(), AssignmentProviderError>; } From ac579adcfc00c8c18458343978382b59229c35f1 Mon Sep 17 00:00:00 2001 From: konac-hamza Date: Fri, 23 Jan 2026 03:16:30 +0300 Subject: [PATCH 4/4] feat: Set inherited by inherited_to_projects Define AssignmentNotFound error type --- .../project/user/role/revoke.rs | 8 +- src/assignment/backend/error.rs | 4 + .../backend/sql/assignment/delete.rs | 122 ++++++++++++++---- src/assignment/error.rs | 7 + 4 files changed, 112 insertions(+), 29 deletions(-) diff --git a/src/api/v3/role_assignment/project/user/role/revoke.rs b/src/api/v3/role_assignment/project/user/role/revoke.rs index a242b988..bad92720 100644 --- a/src/api/v3/role_assignment/project/user/role/revoke.rs +++ b/src/api/v3/role_assignment/project/user/role/revoke.rs @@ -15,7 +15,7 @@ //! Project user role: delete use axum::{ - extract::{Path, State}, + extract::{Path, Request, State}, http::StatusCode, response::IntoResponse, }; @@ -54,7 +54,7 @@ use crate::{ ), security( ("X-Auth-Token" = [])), - tag="Role Assignment" + tag="role_assignments" )] #[tracing::instrument( name = "api::v3:project_user_role_revoke", @@ -68,7 +68,9 @@ pub(super) async fn revoke( mut policy: Policy, Path((project_id, user_id, role_id)): Path<(String, String, String)>, State(state): State, + request: Request, ) -> Result { + let inherited = request.uri().path().contains("inherited_to_projects"); // Use join instead of try_join to have more constant latency preventing timing // attacks. let (user, role, project) = tokio::join!( @@ -121,7 +123,7 @@ pub(super) async fn revoke( .role_id(role_id) .target_id(project_id) .r#type(AssignmentType::UserProject) - .inherited(false) + .inherited(inherited) .build()?; state diff --git a/src/assignment/backend/error.rs b/src/assignment/backend/error.rs index 8aebb315..bfc750b4 100644 --- a/src/assignment/backend/error.rs +++ b/src/assignment/backend/error.rs @@ -19,6 +19,10 @@ use crate::error::{BuilderError, DatabaseError}; /// Database driver for the assignments provider. #[derive(Error, Debug)] pub enum AssignmentDatabaseError { + /// Assignment not found. + #[error("assignment not found: {0}")] + AssignmentNotFound(String), + /// Database error. #[error(transparent)] Database { diff --git a/src/assignment/backend/sql/assignment/delete.rs b/src/assignment/backend/sql/assignment/delete.rs index 3cb38374..2f9b581f 100644 --- a/src/assignment/backend/sql/assignment/delete.rs +++ b/src/assignment/backend/sql/assignment/delete.rs @@ -28,12 +28,7 @@ pub async fn delete( db: &DatabaseConnection, grant: Assignment, ) -> Result<(), AssignmentDatabaseError> { - if grant.inherited { - // Cannot delete inherited assignments directly - return Ok(()); - } - - match &grant.r#type { + let rows_affected = match &grant.r#type { AssignmentType::GroupDomain | AssignmentType::GroupProject | AssignmentType::UserDomain @@ -43,10 +38,11 @@ pub async fn delete( .filter(db_assignment::Column::TargetId.eq(&grant.target_id)) .filter(db_assignment::Column::ActorId.eq(&grant.actor_id)) .filter(db_assignment::Column::Type.eq(DbAssignmentType::try_from(&grant.r#type)?)) - .filter(db_assignment::Column::Inherited.eq(false)) + .filter(db_assignment::Column::Inherited.eq(grant.inherited)) .exec(db) .await - .context("deleting assignment")?; + .context("deleting assignment")? + .rows_affected } AssignmentType::GroupSystem | AssignmentType::UserSystem => { db_system_assignment::Entity::delete_many() @@ -54,11 +50,19 @@ pub async fn delete( .filter(db_system_assignment::Column::TargetId.eq(&grant.target_id)) .filter(db_system_assignment::Column::ActorId.eq(&grant.actor_id)) .filter(db_system_assignment::Column::Type.eq(grant.r#type.to_string())) - .filter(db_system_assignment::Column::Inherited.eq(false)) + .filter(db_system_assignment::Column::Inherited.eq(grant.inherited)) .exec(db) .await - .context("deleting system assignment")?; + .context("deleting system assignment")? + .rows_affected } + }; + + if rows_affected == 0 { + return Err(AssignmentDatabaseError::AssignmentNotFound(format!( + "actor={}, target={}, role={}, type={:?}, inherited={}", + grant.actor_id, grant.target_id, grant.role_id, grant.r#type, grant.inherited + ))); } Ok(()) @@ -71,7 +75,7 @@ mod tests { use super::*; #[tokio::test] - async fn test_delete_user_project() { + async fn test_delete_user_project_success() { // Create MockDatabase with mock query results let db = MockDatabase::new(DatabaseBackend::Postgres) .append_exec_results([MockExecResult { @@ -90,9 +94,10 @@ mod tests { implied_via: None, }; + // Should succeed delete(&db, grant).await.unwrap(); - // Checking transaction log + // Verify SQL was executed correctly assert_eq!( db.into_transaction_log(), [Transaction::from_sql_and_values( @@ -110,7 +115,7 @@ mod tests { } #[tokio::test] - async fn test_delete_group_project() { + async fn test_delete_group_project_success() { let db = MockDatabase::new(DatabaseBackend::Postgres) .append_exec_results([MockExecResult { last_insert_id: 0, @@ -147,7 +152,7 @@ mod tests { } #[tokio::test] - async fn test_delete_user_domain() { + async fn test_delete_user_domain_success() { let db = MockDatabase::new(DatabaseBackend::Postgres) .append_exec_results([MockExecResult { last_insert_id: 0, @@ -184,7 +189,7 @@ mod tests { } #[tokio::test] - async fn test_delete_user_system() { + async fn test_delete_user_system_success() { let db = MockDatabase::new(DatabaseBackend::Postgres) .append_exec_results([MockExecResult { last_insert_id: 0, @@ -221,7 +226,7 @@ mod tests { } #[tokio::test] - async fn test_delete_group_system() { + async fn test_delete_group_system_success() { let db = MockDatabase::new(DatabaseBackend::Postgres) .append_exec_results([MockExecResult { last_insert_id: 0, @@ -258,29 +263,48 @@ mod tests { } #[tokio::test] - async fn test_delete_inherited_succeeds_without_deletion() { - // Inherited grants should be silently skipped - let db = MockDatabase::new(DatabaseBackend::Postgres).into_connection(); + async fn test_delete_inherited_assignment_success() { + // Inherited assignments CAN now be deleted (if they exist in DB) + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_exec_results([MockExecResult { + last_insert_id: 0, + rows_affected: 1, // ← 1 row deleted + }]) + .into_connection(); let grant = Assignment { role_id: "role_id".into(), actor_id: "user_id".into(), target_id: "project_id".into(), r#type: AssignmentType::UserProject, - inherited: true, // ← Inherited + inherited: true, // ← Inherited assignment role_name: None, implied_via: None, }; + // Should succeed delete(&db, grant).await.unwrap(); - // No SQL should be executed for inherited grants - assert_eq!(db.into_transaction_log(), []); + // Verify correct SQL with inherited=true + assert_eq!( + db.into_transaction_log(), + [Transaction::from_sql_and_values( + DatabaseBackend::Postgres, + r#"DELETE FROM "assignment" WHERE "assignment"."role_id" = $1 AND "assignment"."target_id" = $2 AND "assignment"."actor_id" = $3 AND "assignment"."type" = (CAST($4 AS "type")) AND "assignment"."inherited" = $5"#, + [ + "role_id".into(), + "project_id".into(), + "user_id".into(), + "UserProject".into(), + true.into(), // ← inherited=true + ] + ),] + ); } #[tokio::test] - async fn test_delete_not_found_succeeds() { - // Deleting non-existent grant should succeed (idempotent) + async fn test_delete_not_found_returns_error() { + // Deleting non-existent grant should return AssignmentNotFound error let db = MockDatabase::new(DatabaseBackend::Postgres) .append_exec_results([MockExecResult { last_insert_id: 0, @@ -298,9 +322,22 @@ mod tests { implied_via: None, }; - // Should succeed even though nothing was deleted - delete(&db, grant).await.unwrap(); + // Should return error + let result = delete(&db, grant).await; + + assert!(result.is_err()); + // Verify it's the correct error type + match result { + Err(AssignmentDatabaseError::AssignmentNotFound(msg)) => { + assert!(msg.contains("nonexistent_role")); + assert!(msg.contains("user_id")); + assert!(msg.contains("project_id")); + } + _ => panic!("Expected AssignmentNotFound error, got: {:?}", result), + } + + // Verify SQL was still executed assert_eq!( db.into_transaction_log(), [Transaction::from_sql_and_values( @@ -316,4 +353,37 @@ mod tests { ),] ); } + + #[tokio::test] + async fn test_delete_inherited_not_found_returns_error() { + // Deleting non-existent inherited grant should also return error + let db = MockDatabase::new(DatabaseBackend::Postgres) + .append_exec_results([MockExecResult { + last_insert_id: 0, + rows_affected: 0, // ← 0 rows deleted + }]) + .into_connection(); + + let grant = Assignment { + role_id: "role_id".into(), + actor_id: "user_id".into(), + target_id: "project_id".into(), + r#type: AssignmentType::UserProject, + inherited: true, // ← Inherited + role_name: None, + implied_via: None, + }; + + // Should return error + let result = delete(&db, grant).await; + + assert!(result.is_err()); + + match result { + Err(AssignmentDatabaseError::AssignmentNotFound(msg)) => { + assert!(msg.contains("inherited=true")); + } + _ => panic!("Expected AssignmentNotFound error"), + } + } } diff --git a/src/assignment/error.rs b/src/assignment/error.rs index 977349ac..00979434 100644 --- a/src/assignment/error.rs +++ b/src/assignment/error.rs @@ -21,6 +21,10 @@ use crate::resource::error::ResourceProviderError; /// Assignment provider error. #[derive(Error, Debug)] pub enum AssignmentProviderError { + /// Assignment not found. + #[error("assignment not found: {0}")] + AssignmentNotFound(String), + /// Assignment provider error #[error(transparent)] Backend { source: AssignmentDatabaseError }, @@ -71,6 +75,9 @@ pub enum AssignmentProviderError { impl From for AssignmentProviderError { fn from(source: AssignmentDatabaseError) -> Self { match source { + AssignmentDatabaseError::AssignmentNotFound(msg) => { + AssignmentProviderError::AssignmentNotFound(msg) + } AssignmentDatabaseError::Database { source } => match source { cfl @ crate::error::DatabaseError::Conflict { .. } => { Self::Conflict(cfl.to_string())