From 3e36dae50d946e5251bf5aea718292de7ff5b0a6 Mon Sep 17 00:00:00 2001 From: "Nathan.fooo" <86001920+appflowy@users.noreply.github.com> Date: Tue, 26 Mar 2024 21:36:24 +0800 Subject: [PATCH] chore: remove collab ac cache (#420) * chore: remove collab ac cache * chore: update member write workspace role * chore: fix test * chore: clippy --- libs/database/src/collab/collab_storage.rs | 9 ++-- src/biz/casbin/collab_ac.rs | 63 +++++++--------------- src/biz/collab/access_control.rs | 16 +++--- src/biz/collab/storage.rs | 6 ++- 4 files changed, 37 insertions(+), 57 deletions(-) diff --git a/libs/database/src/collab/collab_storage.rs b/libs/database/src/collab/collab_storage.rs index 62b1798b..a8430078 100644 --- a/libs/database/src/collab/collab_storage.rs +++ b/libs/database/src/collab/collab_storage.rs @@ -23,6 +23,7 @@ pub trait CollabStorageAccessControl: Send + Sync + 'static { async fn update_policy(&self, uid: &i64, oid: &str, level: AFAccessLevel) -> Result<(), AppError>; + /// Removes the access level of the user for given collab object. async fn enforce_read_collab( &self, workspace_id: &str, @@ -30,6 +31,7 @@ pub trait CollabStorageAccessControl: Send + Sync + 'static { oid: &str, ) -> Result; + /// Enforce the user's permission to write to the collab object. async fn enforce_write_collab( &self, workspace_id: &str, @@ -37,15 +39,16 @@ pub trait CollabStorageAccessControl: Send + Sync + 'static { oid: &str, ) -> Result; + /// Enforce the user's permission to write to the workspace. + async fn enforce_write_workspace(&self, uid: &i64, workspace_id: &str) -> Result; + + /// Enforce the user's permission to delete the collab object. async fn enforce_delete( &self, workspace_id: &str, uid: &i64, oid: &str, ) -> Result; - - /// Returns the role of the user in the workspace. - async fn enforce_write_workspace(&self, uid: &i64, workspace_id: &str) -> Result; } /// Represents a storage mechanism for collaborations. diff --git a/src/biz/casbin/collab_ac.rs b/src/biz/casbin/collab_ac.rs index 5ffa2185..e8ba7c7b 100644 --- a/src/biz/casbin/collab_ac.rs +++ b/src/biz/casbin/collab_ac.rs @@ -1,15 +1,15 @@ use crate::biz::casbin::access_control::ObjectType; use crate::biz::casbin::access_control::{ - enable_access_control, AccessControl, AccessControlChange, Action, ActionVariant, + enable_access_control, AccessControl, Action, ActionVariant, }; use crate::biz::collab::access_control::CollabAccessControl; use app_error::AppError; use async_trait::async_trait; use collab_rt::RealtimeAccessControl; -use dashmap::DashMap; + use database_entity::dto::AFAccessLevel; -use std::sync::Arc; + use tracing::instrument; #[derive(Clone)] @@ -93,35 +93,25 @@ impl CollabAccessControl for CollabAccessControlImpl { #[derive(Clone)] pub struct RealtimeCollabAccessControlImpl { access_control: AccessControl, - action_by_oid: Arc>, } impl RealtimeCollabAccessControlImpl { pub fn new(access_control: AccessControl) -> Self { - let action_by_oid = Arc::new(DashMap::new()); - let mut sub = access_control.subscribe_change(); - let weak_action_by_oid = Arc::downgrade(&action_by_oid); - - tokio::spawn(async move { - while let Ok(change) = sub.recv().await { - match weak_action_by_oid.upgrade() { - None => break, - Some(action_by_oid) => match change { - AccessControlChange::UpdatePolicy { uid, oid } => { - action_by_oid.remove(&cache_key(uid, &oid)); - }, - AccessControlChange::RemovePolicy { uid, oid } => { - action_by_oid.remove(&cache_key(uid, &oid)); - }, - }, - } - } - }); - - Self { - access_control, - action_by_oid, - } + // let action_by_oid = Arc::new(DashMap::new()); + // let mut sub = access_control.subscribe_change(); + // let weak_action_by_oid = Arc::downgrade(&action_by_oid); + // tokio::spawn(async move { + // while let Ok(change) = sub.recv().await { + // match weak_action_by_oid.upgrade() { + // None => break, + // Some(action_by_oid) => match change { + // AccessControlChange::UpdatePolicy { uid, oid } => {}, + // AccessControlChange::RemovePolicy { uid, oid } => {}, + // }, + // } + // } + // }); + Self { access_control } } async fn can_perform_action( @@ -132,13 +122,6 @@ impl RealtimeCollabAccessControlImpl { required_action: Action, ) -> Result { if enable_access_control() { - let key = cache_key(*uid, oid); - // Check if the action is already cached - if let Some(action) = self.action_by_oid.get(&key) { - return Ok(*action >= required_action); - } - - // Not in cache, enforce access control let is_permitted = self .access_control .enforce( @@ -149,11 +132,6 @@ impl RealtimeCollabAccessControlImpl { ) .await?; - if is_permitted { - // Permission granted, cache the action - self.action_by_oid.insert(key, required_action); - } - Ok(is_permitted) } else { Ok(true) @@ -161,11 +139,6 @@ impl RealtimeCollabAccessControlImpl { } } -#[inline] -fn cache_key(uid: i64, oid: &str) -> String { - format!("{}:{}", uid, oid) -} - #[async_trait] impl RealtimeAccessControl for RealtimeCollabAccessControlImpl { async fn can_write_collab( diff --git a/src/biz/collab/access_control.rs b/src/biz/collab/access_control.rs index 33820297..f6d70f24 100644 --- a/src/biz/collab/access_control.rs +++ b/src/biz/collab/access_control.rs @@ -7,7 +7,7 @@ use actix_web::http::Method; use app_error::AppError; use async_trait::async_trait; use database::collab::CollabStorageAccessControl; -use database_entity::dto::{AFAccessLevel, AFRole}; +use database_entity::dto::AFAccessLevel; use crate::biz::collab::cache::CollabCache; @@ -218,6 +218,13 @@ where .await } + async fn enforce_write_workspace(&self, uid: &i64, workspace_id: &str) -> Result { + self + .workspace_access_control + .enforce_action(uid, workspace_id, Action::Write) + .await + } + async fn enforce_delete( &self, workspace_id: &str, @@ -229,11 +236,4 @@ where .enforce_access_level(workspace_id, uid, oid, AFAccessLevel::FullAccess) .await } - - async fn enforce_write_workspace(&self, uid: &i64, workspace_id: &str) -> Result { - self - .workspace_access_control - .enforce_role(uid, workspace_id, AFRole::Owner) - .await - } } diff --git a/src/biz/collab/storage.rs b/src/biz/collab/storage.rs index 8f2fa3f6..d380767b 100644 --- a/src/biz/collab/storage.rs +++ b/src/biz/collab/storage.rs @@ -172,7 +172,6 @@ where } #[instrument(level = "trace", skip(self, params), oid = %params.oid, err)] - #[allow(clippy::blocks_in_if_conditions)] async fn insert_or_update_collab_with_transaction( &self, workspace_id: &str, @@ -188,6 +187,11 @@ where // When the collab is not exist in the database, and the user passes the permission check, // which means the user has the permission to create the collab, we should update the policy if !is_collab_exist_in_db { + trace!( + "Update policy for user:{} to create collab:{}", + uid, + params.object_id + ); self .access_control .update_policy(uid, ¶ms.object_id, AFAccessLevel::FullAccess)