From 1da05140758956cb8d7e77e30930acf6d0ea62db Mon Sep 17 00:00:00 2001 From: konac-hamza Date: Tue, 10 Feb 2026 23:39:21 +0300 Subject: [PATCH 1/4] feat: Create revoke event after revoke grant --- .../project/user/role/revoke.rs | 136 ++++++++++++++++++ 1 file changed, 136 insertions(+) diff --git a/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs b/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs index a73c27c4..090ad3cc 100644 --- a/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs +++ b/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs @@ -32,6 +32,7 @@ use crate::{ assignment::{AssignmentApi, types::AssignmentBuilder, types::AssignmentType}, identity::IdentityApi, resource::ResourceApi, + revoke::{RevocationEventCreate, RevokeApi}, }; /// Revoke role from user on project @@ -132,6 +133,28 @@ pub(super) async fn revoke( .revoke_grant(&state, grant) .await?; + // Create revocation event + let revocation_event = RevocationEventCreate { + domain_id: None, + project_id: Some(project.id.clone()), + user_id: Some(user.id.clone()), + role_id: Some(role.id.clone()), + trust_id: None, + consumer_id: None, + access_token_id: None, + issued_before: chrono::Utc::now(), + expires_at: None, + audit_id: None, + audit_chain_id: None, + revoked_at: chrono::Utc::now(), + }; + + state + .provider + .get_revoke_provider() + .create_revocation_event(&state, revocation_event) + .await?; + Ok(StatusCode::NO_CONTENT.into_response()) } @@ -151,6 +174,7 @@ mod tests { use crate::identity::{MockIdentityProvider, types::*}; use crate::provider::Provider; use crate::resource::{MockResourceProvider, types::Project}; + use crate::revoke::{MockRevokeProvider, types::RevocationEventCreate}; #[tokio::test] #[traced_test] @@ -401,4 +425,116 @@ mod tests { assert_eq!(response.status(), StatusCode::NOT_FOUND); } + + #[tokio::test] + #[traced_test] + async fn test_revoke_creates_revocation_event() { + let mut identity_mock = MockIdentityProvider::default(); + identity_mock + .expect_get_user() + .withf(|_, id: &'_ str| id == "user_id") + .returning(|_, _| { + Ok(Some( + UserResponseBuilder::default() + .id("user_id") + .domain_id("did") + .enabled(true) + .name("uname") + .build() + .unwrap(), + )) + }); + + 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() + })) + }); + + // Mock revoke_grant to succeed + assignment_mock + .expect_revoke_grant() + .withf(|_, grant: &Assignment| { + grant.actor_id == "user_id" + && grant.target_id == "project_id" + && grant.role_id == "role_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() + })) + }); + + // Mock revoke provider to verify revocation event is created + let mut revoke_mock = MockRevokeProvider::default(); + revoke_mock + .expect_create_revocation_event() + .withf(|_, event: &RevocationEventCreate| { + event.user_id == Some("user_id".to_string()) + && event.project_id == Some("project_id".to_string()) + && event.role_id == Some("role_id".to_string()) + && event.domain_id.is_none() + && event.trust_id.is_none() + }) + .times(1) // Verify it's called exactly once + .returning(|_, _| { + Ok(crate::revoke::types::RevocationEvent { + domain_id: None, + project_id: Some("project_id".to_string()), + user_id: Some("user_id".to_string()), + role_id: Some("role_id".to_string()), + trust_id: None, + consumer_id: None, + access_token_id: None, + issued_before: chrono::Utc::now(), + expires_at: None, + revoked_at: chrono::Utc::now(), + audit_id: None, + audit_chain_id: None, + }) + }); + + let provider_builder = Provider::mocked_builder() + .assignment(assignment_mock) + .identity(identity_mock) + .resource(resource_mock) + .revoke(revoke_mock); // Add revoke provider mock + + let state = get_mocked_state(provider_builder, true); // Policy 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(); + + // Should succeed + assert_eq!(response.status(), StatusCode::NO_CONTENT); + } } From 249a7cf26930b2266de552b99b085237f3d3b198 Mon Sep 17 00:00:00 2001 From: konac-hamza Date: Wed, 11 Feb 2026 22:56:51 +0300 Subject: [PATCH 2/4] fix: Move event creation from api to provider --- crates/keystone/src/assignment/backend.rs | 2 +- crates/keystone/src/assignment/backend/sql.rs | 2 +- .../backend/sql/assignment/delete.rs | 16 +++--- crates/keystone/src/assignment/error.rs | 8 +++ crates/keystone/src/assignment/mod.rs | 49 ++++++++++++++++++- 5 files changed, 66 insertions(+), 11 deletions(-) diff --git a/crates/keystone/src/assignment/backend.rs b/crates/keystone/src/assignment/backend.rs index aec134d5..306aefc5 100644 --- a/crates/keystone/src/assignment/backend.rs +++ b/crates/keystone/src/assignment/backend.rs @@ -91,6 +91,6 @@ pub trait AssignmentBackend: Send + Sync { async fn revoke_grant( &self, state: &ServiceState, - params: Assignment, + params: &Assignment, ) -> Result<(), AssignmentProviderError>; } diff --git a/crates/keystone/src/assignment/backend/sql.rs b/crates/keystone/src/assignment/backend/sql.rs index ed984434..13ba6827 100644 --- a/crates/keystone/src/assignment/backend/sql.rs +++ b/crates/keystone/src/assignment/backend/sql.rs @@ -136,7 +136,7 @@ impl AssignmentBackend for SqlBackend { async fn revoke_grant( &self, state: &ServiceState, - grant: Assignment, + grant: &Assignment, ) -> Result<(), AssignmentProviderError> { Ok(assignment::delete(&state.db, grant).await?) } diff --git a/crates/keystone/src/assignment/backend/sql/assignment/delete.rs b/crates/keystone/src/assignment/backend/sql/assignment/delete.rs index 8e3a87b8..0a3d436c 100644 --- a/crates/keystone/src/assignment/backend/sql/assignment/delete.rs +++ b/crates/keystone/src/assignment/backend/sql/assignment/delete.rs @@ -24,7 +24,7 @@ use crate::error::DbContextExt; /// Delete assignment grant. pub async fn delete( db: &DatabaseConnection, - grant: Assignment, + grant: &Assignment, ) -> Result<(), AssignmentDatabaseError> { let rows_affected = match &grant.r#type { AssignmentType::GroupDomain @@ -96,7 +96,7 @@ mod tests { implied_via: None, }; - delete(&db, grant).await.unwrap(); + delete(&db, &grant).await.unwrap(); // Update expected SQL to match delete_by_id order assert_eq!( @@ -134,7 +134,7 @@ mod tests { implied_via: None, }; - delete(&db, grant).await.unwrap(); + delete(&db, &grant).await.unwrap(); assert_eq!( db.into_transaction_log(), @@ -171,7 +171,7 @@ mod tests { implied_via: None, }; - delete(&db, grant).await.unwrap(); + delete(&db, &grant).await.unwrap(); assert_eq!( db.into_transaction_log(), @@ -208,7 +208,7 @@ mod tests { implied_via: None, }; - delete(&db, grant).await.unwrap(); + delete(&db, &grant).await.unwrap(); assert_eq!( db.into_transaction_log(), @@ -245,7 +245,7 @@ mod tests { implied_via: None, }; - delete(&db, grant).await.unwrap(); + delete(&db, &grant).await.unwrap(); assert_eq!( db.into_transaction_log(), @@ -282,7 +282,7 @@ mod tests { implied_via: None, }; - delete(&db, grant).await.unwrap(); + delete(&db, &grant).await.unwrap(); assert_eq!( db.into_transaction_log(), @@ -319,7 +319,7 @@ mod tests { implied_via: None, }; - let result = delete(&db, grant).await; + let result = delete(&db, &grant).await; assert!(result.is_err()); diff --git a/crates/keystone/src/assignment/error.rs b/crates/keystone/src/assignment/error.rs index b17efce7..4a66ef39 100644 --- a/crates/keystone/src/assignment/error.rs +++ b/crates/keystone/src/assignment/error.rs @@ -17,6 +17,7 @@ use thiserror::Error; use crate::assignment::backend::error::*; use crate::identity::error::IdentityProviderError; use crate::resource::error::ResourceProviderError; +use crate::revoke::error::RevokeProviderError; /// Assignment provider error. #[derive(Error, Debug)] @@ -47,6 +48,13 @@ pub enum AssignmentProviderError { source: ResourceProviderError, }, + /// Revoke provider error. + #[error(transparent)] + RevokeProvider { + #[from] + source: RevokeProviderError, + }, + /// Role not found. #[error("role {0} not found")] RoleNotFound(String), diff --git a/crates/keystone/src/assignment/mod.rs b/crates/keystone/src/assignment/mod.rs index 87f27b28..43c5e13a 100644 --- a/crates/keystone/src/assignment/mod.rs +++ b/crates/keystone/src/assignment/mod.rs @@ -64,6 +64,7 @@ use crate::identity::IdentityApi; use crate::keystone::ServiceState; use crate::plugin_manager::PluginManager; use crate::resource::ResourceApi; +use crate::revoke::{RevokeApi, types::RevocationEventCreate}; #[cfg(test)] pub use mock::MockAssignmentProvider; @@ -238,6 +239,52 @@ impl AssignmentApi for AssignmentProvider { state: &ServiceState, grant: Assignment, ) -> Result<(), AssignmentProviderError> { - self.backend_driver.revoke_grant(state, grant).await + // Call backend with reference (no move) + self.backend_driver.revoke_grant(state, &grant).await?; + + // Determine user_id or group_id + let user_id = match &grant.r#type { + AssignmentType::UserDomain + | AssignmentType::UserProject + | AssignmentType::UserSystem => Some(grant.actor_id.clone()), + + AssignmentType::GroupDomain + | AssignmentType::GroupProject + | AssignmentType::GroupSystem => None, + }; + + // Determine project_id or domain_id + let (project_id, domain_id) = match &grant.r#type { + AssignmentType::UserProject | AssignmentType::GroupProject => { + (Some(grant.target_id.clone()), None) + } + AssignmentType::UserDomain | AssignmentType::GroupDomain => { + (None, Some(grant.target_id.clone())) + } + AssignmentType::UserSystem | AssignmentType::GroupSystem => (None, None), + }; + + let revocation_event = RevocationEventCreate { + domain_id, + project_id, + user_id, + role_id: Some(grant.role_id.clone()), + trust_id: None, + consumer_id: None, + access_token_id: None, + issued_before: chrono::Utc::now(), + expires_at: None, + audit_id: None, + audit_chain_id: None, + revoked_at: chrono::Utc::now(), + }; + + state + .provider + .get_revoke_provider() + .create_revocation_event(state, revocation_event) + .await?; + + Ok(()) } } From d1d8b49b1cd3ca6bb6cc92d7b93267fa891a4e7d Mon Sep 17 00:00:00 2001 From: konac-hamza Date: Thu, 12 Feb 2026 01:51:04 +0300 Subject: [PATCH 3/4] fix: Remove event creation from api --- .../project/user/role/revoke.rs | 135 ------------------ 1 file changed, 135 deletions(-) diff --git a/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs b/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs index 090ad3cc..d2cf128b 100644 --- a/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs +++ b/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs @@ -32,7 +32,6 @@ use crate::{ assignment::{AssignmentApi, types::AssignmentBuilder, types::AssignmentType}, identity::IdentityApi, resource::ResourceApi, - revoke::{RevocationEventCreate, RevokeApi}, }; /// Revoke role from user on project @@ -133,28 +132,6 @@ pub(super) async fn revoke( .revoke_grant(&state, grant) .await?; - // Create revocation event - let revocation_event = RevocationEventCreate { - domain_id: None, - project_id: Some(project.id.clone()), - user_id: Some(user.id.clone()), - role_id: Some(role.id.clone()), - trust_id: None, - consumer_id: None, - access_token_id: None, - issued_before: chrono::Utc::now(), - expires_at: None, - audit_id: None, - audit_chain_id: None, - revoked_at: chrono::Utc::now(), - }; - - state - .provider - .get_revoke_provider() - .create_revocation_event(&state, revocation_event) - .await?; - Ok(StatusCode::NO_CONTENT.into_response()) } @@ -425,116 +402,4 @@ mod tests { assert_eq!(response.status(), StatusCode::NOT_FOUND); } - - #[tokio::test] - #[traced_test] - async fn test_revoke_creates_revocation_event() { - let mut identity_mock = MockIdentityProvider::default(); - identity_mock - .expect_get_user() - .withf(|_, id: &'_ str| id == "user_id") - .returning(|_, _| { - Ok(Some( - UserResponseBuilder::default() - .id("user_id") - .domain_id("did") - .enabled(true) - .name("uname") - .build() - .unwrap(), - )) - }); - - 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() - })) - }); - - // Mock revoke_grant to succeed - assignment_mock - .expect_revoke_grant() - .withf(|_, grant: &Assignment| { - grant.actor_id == "user_id" - && grant.target_id == "project_id" - && grant.role_id == "role_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() - })) - }); - - // Mock revoke provider to verify revocation event is created - let mut revoke_mock = MockRevokeProvider::default(); - revoke_mock - .expect_create_revocation_event() - .withf(|_, event: &RevocationEventCreate| { - event.user_id == Some("user_id".to_string()) - && event.project_id == Some("project_id".to_string()) - && event.role_id == Some("role_id".to_string()) - && event.domain_id.is_none() - && event.trust_id.is_none() - }) - .times(1) // Verify it's called exactly once - .returning(|_, _| { - Ok(crate::revoke::types::RevocationEvent { - domain_id: None, - project_id: Some("project_id".to_string()), - user_id: Some("user_id".to_string()), - role_id: Some("role_id".to_string()), - trust_id: None, - consumer_id: None, - access_token_id: None, - issued_before: chrono::Utc::now(), - expires_at: None, - revoked_at: chrono::Utc::now(), - audit_id: None, - audit_chain_id: None, - }) - }); - - let provider_builder = Provider::mocked_builder() - .assignment(assignment_mock) - .identity(identity_mock) - .resource(resource_mock) - .revoke(revoke_mock); // Add revoke provider mock - - let state = get_mocked_state(provider_builder, true); // Policy 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(); - - // Should succeed - assert_eq!(response.status(), StatusCode::NO_CONTENT); - } } From 363c6be788e8e6caf7202454314a4d05745eae23 Mon Sep 17 00:00:00 2001 From: konac-hamza Date: Thu, 12 Feb 2026 21:06:53 +0300 Subject: [PATCH 4/4] fix: Remove unused imports --- .../src/api/v3/role_assignment/project/user/role/revoke.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs b/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs index d2cf128b..0ff8ecec 100644 --- a/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs +++ b/crates/keystone/src/api/v3/role_assignment/project/user/role/revoke.rs @@ -151,8 +151,6 @@ mod tests { use crate::identity::{MockIdentityProvider, types::*}; use crate::provider::Provider; use crate::resource::{MockResourceProvider, types::Project}; - use crate::revoke::{MockRevokeProvider, types::RevocationEventCreate}; - #[tokio::test] #[traced_test] async fn test_revoke_success() {