Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/add findex redis v7 #20

Merged
merged 1 commit into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build_all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ jobs:
archive-name: ${{ matrix.archive-name }}
target: ${{ matrix.target }}
debug_or_release: ${{ inputs.debug_or_release }}
skip_services_tests: --skip test_findex --skip test_all_authentications --skip test_server_auth_matrix --skip test_datasets
skip_services_tests: --skip test_findex --skip test_all_authentications --skip test_server_auth_matrix --skip test_datasets --skip test_permissions

windows-2022:
if: inputs.debug_or_release == 'release'
Expand Down
3 changes: 1 addition & 2 deletions crate/server/src/config/params/db_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ impl DbParams {
}
}
impl Default for DbParams {
#[allow(clippy::expect_used)]
#[allow(clippy::expect_used)] // Won't panic because the URL is valid
fn default() -> Self {
Self::Redis(Url::parse("redis://localhost:6379").expect("Invalid default URL"))
}
Expand Down Expand Up @@ -44,7 +44,6 @@ fn redact_url(original: &Url) -> Url {
url.set_password(Some("****"))
.expect("masking password failed");
}

url
}

Expand Down
2 changes: 1 addition & 1 deletion crate/server/src/database/database_traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ pub(crate) trait FindexMemoryTrait:
//
}

#[allow(dead_code)] // code is actually used
#[allow(dead_code)] // false positive, used in crate/server/src/database/redis/mod.rs
#[async_trait]
pub(crate) trait DatabaseTraits:
PermissionsTrait + DatasetsTrait + FindexMemoryTrait
Expand Down
246 changes: 244 additions & 2 deletions crate/server/src/database/redis/permissions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ impl PermissionsTrait for Redis<WORD_LENGTH> {
Ok(permission.clone())
}

#[allow(dependency_on_unit_never_type_fallback)]
async fn grant_permission(
&self,
user_id: &str,
Expand All @@ -149,7 +148,8 @@ impl PermissionsTrait for Redis<WORD_LENGTH> {
};

let mut pipe = pipe();
pipe.atomic()
let () = pipe
.atomic()
.set(&redis_key, permissions.serialize()?.as_slice())
.query_async(&mut self.manager.clone())
.await
Expand Down Expand Up @@ -194,3 +194,245 @@ impl PermissionsTrait for Redis<WORD_LENGTH> {
Ok(())
}
}

#[cfg(test)]
#[allow(
clippy::unwrap_used,
clippy::expect_used,
clippy::match_same_arms,
clippy::option_if_let_else
)]
mod tests {

use std::{env, sync::Arc};

use super::*;
use crate::config::{DBConfig, DatabaseType};

use tokio;
use uuid::Uuid;

fn redis_db_config() -> DBConfig {
let url = if let Ok(var_env) = env::var("REDIS_HOST") {
format!("redis://{var_env}:6379")
} else {
"redis://localhost:6379".to_owned()
};
trace!("TESTS: using redis on {url}");
DBConfig {
database_type: DatabaseType::Redis,
clear_database: false,
database_url: url,
}
}

fn get_db_config() -> DBConfig {
env::var_os("FINDEX_TEST_DB").map_or_else(redis_db_config, |v| {
match v.to_str().unwrap_or("") {
"redis" => redis_db_config(),
_ => redis_db_config(),
}
})
}

async fn setup_test_db() -> Redis<WORD_LENGTH> {
let url = get_db_config().database_url;
Redis::instantiate(url.as_str(), false)
.await
.expect("Test failed to instantiate Redis")
}

#[tokio::test]
async fn test_permissions_create_index_id() {
let db = setup_test_db().await;
let user_id = "test_user";

// Create new index
let index_id = db
.create_index_id(user_id)
.await
.expect("Failed to create index");

// Verify permissions were created
let permissions = db
.get_permissions(user_id)
.await
.expect("Failed to get permissions");

assert!(permissions.get_permission(&index_id).is_some());
assert_eq!(
permissions.get_permission(&index_id).unwrap(),
&Permission::Admin
);
}

#[tokio::test]
async fn test_permissions_grant_and_revoke_permissions() {
let db = setup_test_db().await;
let user_id = "test_user_2";
let index_id = Uuid::new_v4();

// Grant Read permission
db.grant_permission(user_id, Permission::Read, &index_id)
.await
.expect("Failed to grant permission");

// Verify permission was granted
let permission = db
.get_permission(user_id, &index_id)
.await
.expect("Failed to get permission");
assert_eq!(permission, Permission::Read);

// Grant Read, then update to Admin
db.grant_permission(user_id, Permission::Admin, &index_id)
.await
.unwrap();

let permission = db.get_permission(user_id, &index_id).await.unwrap();
assert_eq!(permission, Permission::Admin);

// Revoke permission
db.revoke_permission(user_id, &index_id)
.await
.expect("Failed to revoke permission");

// Verify permission was revoked
let result = db.get_permission(user_id, &index_id).await;
result.unwrap_err();
}

#[tokio::test]
#[allow(clippy::unwrap_used, clippy::assertions_on_result_states)]
async fn test_permissions_revoke_permission() {
let db = setup_test_db().await;
let other_user_id = "another_user";
let test_user_id = "main_user";

// Create new index by another user
let (admin_index_id, write_index_id, read_index_id) = (
db.create_index_id(other_user_id)
.await
.expect("Failed to create index"),
db.create_index_id(other_user_id)
.await
.expect("Failed to create index"),
db.create_index_id(other_user_id)
.await
.expect("Failed to create index"),
);
let permission_kinds = [Permission::Admin, Permission::Write, Permission::Read];
for (index_id, permission_kind) in vec![admin_index_id, write_index_id, read_index_id]
.into_iter()
.zip(permission_kinds.into_iter())
{
// Grant permission
db.grant_permission(test_user_id, permission_kind.clone(), &index_id)
.await
.expect("Failed to grant permission {permission_kind}");

// Verify permission was granted
let permission = db
.get_permission(test_user_id, &index_id)
.await
.expect("Failed to get permission {permission_kind}");
assert_eq!(permission, permission_kind);

// Revoke permission
db.revoke_permission(test_user_id, &index_id)
.await
.expect("Failed to revoke permission {permission_kind}");

// Verify permission was revoked
let result = db.get_permission(test_user_id, &index_id).await;
result.unwrap_err();
}

// Now, we create two indexes for the test_user, we revoke the permission for one of them and we check that the other one is still there
let (index_id1, index_id2) = (
db.create_index_id(test_user_id)
.await
.expect("Failed to create index"),
db.create_index_id(test_user_id)
.await
.expect("Failed to create index"),
);

// revoke permission for index_id1
db.revoke_permission(test_user_id, &index_id1)
.await
.expect("Failed to revoke permission");

// Verify permission of index_id2 is still there
let permission = db
.get_permission(test_user_id, &index_id2)
.await
.expect("Failed to get permission");
assert_eq!(permission, Permission::Admin);
}

#[tokio::test]
async fn test_permissions_nonexistent_user_and_permission() {
let db = setup_test_db().await;
let user_id = "nonexistent_user";
let index_id = Uuid::new_v4();

// Try to get permissions for nonexistent user
let result = db.get_permissions(user_id).await;
assert!(result.unwrap().permissions.is_empty());

// Try to get specific permission
let result = db.get_permission(user_id, &index_id).await;
result.unwrap_err();

// Revoke a non existent permission, should not fail
db.revoke_permission("someone", &Uuid::new_v4())
.await
.unwrap();
}

/// Test that concurrent permission operations of the same user on different indexes work correctly
#[tokio::test]
async fn test_permissions_concurrent_permission_operations() {
let db_init = setup_test_db().await;
let db_arc = Arc::new(db_init);
let user_id = "concurrent_user";
let num_tasks = 10;
let mut handles = vec![];

for _ in 0..num_tasks {
let db = Arc::clone(&db_arc);
let user_id_clone = user_id.to_owned();
let handle = tokio::spawn(async move {
let index_id = db
.create_index_id(&user_id_clone)
.await
.expect("Failed to create index");

db.grant_permission(&user_id_clone, Permission::Read, &index_id)
.await
.expect("Failed to grant permission");

let permission = db
.get_permission(&user_id_clone, &index_id)
.await
.expect("Failed to get permission");

assert_eq!(permission, Permission::Read);

db.revoke_permission(&user_id_clone, &index_id)
.await
.expect("Failed to revoke permission");

let result = db.get_permission(&user_id_clone, &index_id).await;

result.unwrap_err();
});
handles.push(handle);
}

for handle in handles {
handle.await.expect("Task panicked");
}
}
}
2 changes: 1 addition & 1 deletion crate/server/src/error/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ macro_rules! findex_server_bail {
};
}

#[allow(clippy::expect_used)]
#[allow(clippy::expect_used)] // ok in tests
#[cfg(test)]
mod tests {
use super::FindexServerError;
Expand Down
2 changes: 1 addition & 1 deletion crate/server/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,6 @@ pub mod findex_server;
pub mod middlewares;
pub mod routes;

#[allow(clippy::panic, clippy::unwrap_used, clippy::expect_used, unsafe_code)]
#[allow(clippy::panic, clippy::unwrap_used, clippy::expect_used)]
#[cfg(test)]
mod tests;
1 change: 0 additions & 1 deletion crate/server/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ const FINDEX_SERVER_CONF: &str = "/etc/cosmian/findex_server.toml";
/// This function sets up the necessary environment variables and logging
/// options, then parses the command line arguments using [`ClapConfig::parse()`](https://docs.rs/clap/latest/clap/struct.ClapConfig.html#method.parse).
#[tokio::main]
#[allow(clippy::needless_return)]
async fn main() -> FResult<()> {
// Set up environment variables and logging options
if std::env::var("RUST_BACKTRACE").is_err() {
Expand Down
1 change: 0 additions & 1 deletion crate/server/src/middlewares/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ where
S::Future: 'static,
{
type Error = Error;
#[allow(clippy::type_complexity)]
type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>>>>;
type Response = ServiceResponse<EitherBody<B, BoxBody>>;

Expand Down
1 change: 0 additions & 1 deletion crate/server/src/middlewares/ssl_auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ where
S::Future: 'static,
{
type Error = Error;
#[allow(clippy::type_complexity)]
type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>>>>;
type Response = ServiceResponse<EitherBody<B, BoxBody>>;

Expand Down
10 changes: 6 additions & 4 deletions crate/server/src/routes/findex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::{
routes::{check_permission, error::ResponseBytes},
};

// todo(hatem): reduce cloning
// TODO(hatem): reduce cloning

#[allow(clippy::indexing_slicing)]
fn prepend_index_id(
Expand Down Expand Up @@ -78,6 +78,7 @@ pub(crate) async fn findex_guarded_write(
findex_server: Data<Arc<FindexServer>>,
) -> ResponseBytes {
const OPERATION_NAME: &str = "guarded_write";
const INVALID_REQUEST: &str = "Invalid request.";
let user = findex_server.get_user(&req);

trace!("user {user}: POST /indexes/{index_id}/guarded_write");
Expand All @@ -101,9 +102,10 @@ pub(crate) async fn findex_guarded_write(
}
}
} else {
return Err(FindexServerError::InvalidRequest(format!(
"{error_prefix} Invalid discriminant flag. Expected 0 or 1, found None"
)));
trace!("{error_prefix} Invalid discriminant flag. Expected 0 or 1, found None");
return Err(FindexServerError::InvalidRequest(
INVALID_REQUEST.to_owned(),
));
Comment on lines +105 to +108
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
trace!("{error_prefix} Invalid discriminant flag. Expected 0 or 1, found None");
return Err(FindexServerError::InvalidRequest(
INVALID_REQUEST.to_owned(),
));
return Err(FindexServerError::InvalidRequest(format!(
"{error_prefix} Invalid discriminant flag. Expected 0 or 1, found None"
)));

};

let guard = Guard::deserialize(bytes.get(..guard_len).ok_or_else(|| {
Expand Down