From a845cd888005c49f421b74ff5453b8a12fd28a1e Mon Sep 17 00:00:00 2001 From: Maxime Legendre Date: Wed, 15 Dec 2021 14:52:33 +0100 Subject: [PATCH] Fix(auth): Forbid index creation on alternates routes Forbid index creation on alternates routes when the action `index.create` is not given fix #2024 --- meilisearch-auth/src/lib.rs | 16 ++- .../src/routes/indexes/documents.rs | 6 + .../src/routes/indexes/settings.rs | 9 ++ meilisearch-http/tests/auth/authorization.rs | 132 ++++++++++++++++++ meilisearch-http/tests/auth/mod.rs | 19 +-- meilisearch-http/tests/common/server.rs | 27 ++++ .../index_resolver/mod.txt | 1 + .../index_controller/dump_actor/compat/v3.rs | 2 + meilisearch-lib/src/index_controller/mod.rs | 6 + meilisearch-lib/src/index_resolver/mod.rs | 16 ++- meilisearch-lib/src/tasks/task.rs | 2 + 11 files changed, 213 insertions(+), 23 deletions(-) diff --git a/meilisearch-auth/src/lib.rs b/meilisearch-auth/src/lib.rs index 0f476ac16..ccd5cfca2 100644 --- a/meilisearch-auth/src/lib.rs +++ b/meilisearch-auth/src/lib.rs @@ -69,6 +69,11 @@ impl AuthController { if !key.indexes.iter().any(|i| i.as_str() == "*") { filters.indexes = Some(key.indexes); } + + filters.allow_index_creation = key + .actions + .iter() + .any(|&action| action == Action::IndexesAdd || action == Action::All); } Ok(filters) @@ -118,9 +123,18 @@ impl AuthController { } } -#[derive(Default)] pub struct AuthFilter { pub indexes: Option>, + pub allow_index_creation: bool, +} + +impl Default for AuthFilter { + fn default() -> Self { + Self { + indexes: None, + allow_index_creation: true, + } + } } pub fn generate_key(master_key: &[u8], uid: &str) -> String { diff --git a/meilisearch-http/src/routes/indexes/documents.rs b/meilisearch-http/src/routes/indexes/documents.rs index 293b996a1..d18c600af 100644 --- a/meilisearch-http/src/routes/indexes/documents.rs +++ b/meilisearch-http/src/routes/indexes/documents.rs @@ -173,6 +173,7 @@ pub async fn add_documents( &req, ); + let allow_index_creation = meilisearch.filters().allow_index_creation; let task = document_addition( extract_mime_type(&req)?, meilisearch, @@ -180,6 +181,7 @@ pub async fn add_documents( params.primary_key, body, IndexDocumentsMethod::ReplaceDocuments, + allow_index_creation, ) .await?; @@ -203,6 +205,7 @@ pub async fn update_documents( &req, ); + let allow_index_creation = meilisearch.filters().allow_index_creation; let task = document_addition( extract_mime_type(&req)?, meilisearch, @@ -210,6 +213,7 @@ pub async fn update_documents( params.into_inner().primary_key, body, IndexDocumentsMethod::UpdateDocuments, + allow_index_creation, ) .await?; @@ -223,6 +227,7 @@ async fn document_addition( primary_key: Option, body: Payload, method: IndexDocumentsMethod, + allow_index_creation: bool, ) -> Result { let format = match mime_type .as_ref() @@ -250,6 +255,7 @@ async fn document_addition( primary_key, method, format, + allow_index_creation, }; let task = meilisearch.register_update(index_uid, update).await?.into(); diff --git a/meilisearch-http/src/routes/indexes/settings.rs b/meilisearch-http/src/routes/indexes/settings.rs index 45412a928..8b38072c4 100644 --- a/meilisearch-http/src/routes/indexes/settings.rs +++ b/meilisearch-http/src/routes/indexes/settings.rs @@ -34,9 +34,12 @@ macro_rules! make_setting_route { $attr: Setting::Reset, ..Default::default() }; + + let allow_index_creation = meilisearch.filters().allow_index_creation; let update = Update::Settings { settings, is_deletion: true, + allow_index_creation, }; let task: SummarizedTaskView = meilisearch .register_update(index_uid.into_inner(), update) @@ -66,9 +69,11 @@ macro_rules! make_setting_route { ..Default::default() }; + let allow_index_creation = meilisearch.filters().allow_index_creation; let update = Update::Settings { settings, is_deletion: false, + allow_index_creation, }; let task: SummarizedTaskView = meilisearch .register_update(index_uid.into_inner(), update) @@ -272,9 +277,11 @@ pub async fn update_all( Some(&req), ); + let allow_index_creation = meilisearch.filters().allow_index_creation; let update = Update::Settings { settings, is_deletion: false, + allow_index_creation, }; let task: SummarizedTaskView = meilisearch .register_update(index_uid.into_inner(), update) @@ -300,9 +307,11 @@ pub async fn delete_all( ) -> Result { let settings = Settings::cleared().into_unchecked(); + let allow_index_creation = data.filters().allow_index_creation; let update = Update::Settings { settings, is_deletion: true, + allow_index_creation, }; let task: SummarizedTaskView = data .register_update(index_uid.into_inner(), update) diff --git a/meilisearch-http/tests/auth/authorization.rs b/meilisearch-http/tests/auth/authorization.rs index d8ae8b51e..85b37bdc2 100644 --- a/meilisearch-http/tests/auth/authorization.rs +++ b/meilisearch-http/tests/auth/authorization.rs @@ -496,3 +496,135 @@ async fn list_authorized_tasks_no_index_restriction() { // key should have access on `test` index. assert!(response.iter().any(|task| task["indexUid"] == "test")); } + +#[actix_rt::test] +async fn error_creating_index_without_action() { + let mut server = Server::new_auth().await; + server.use_api_key("MASTER_KEY"); + + // create key with access on all indexes. + let content = json!({ + "indexes": ["*"], + "actions": ALL_ACTIONS.iter().cloned().filter(|a| *a != "indexes.create").collect::>(), + "expiresAt": "2050-11-13T00:00:00Z" + }); + let (response, code) = server.add_api_key(content).await; + assert_eq!(code, 201); + assert!(response["key"].is_string()); + + // use created key. + let key = response["key"].as_str().unwrap(); + server.use_api_key(&key); + + let expected_error = json!({ + "message": "Index `test` not found.", + "code": "index_not_found", + "type": "invalid_request", + "link": "https://docs.meilisearch.com/errors#index_not_found" + }); + + // try to create a index via add documents route + let index = server.index("test"); + let documents = json!([ + { + "id": 1, + "content": "foo", + } + ]); + + let (response, code) = index.add_documents(documents, None).await; + assert_eq!(code, 202, "{:?}", response); + let task_id = response["uid"].as_u64().unwrap(); + + let response = index.wait_task(task_id).await; + assert_eq!(response["status"], "failed"); + assert_eq!(response["error"], expected_error.clone()); + + // try to create a index via add settings route + let settings = json!({ "distinctAttribute": "test"}); + + let (response, code) = index.update_settings(settings).await; + assert_eq!(code, 202); + let task_id = response["uid"].as_u64().unwrap(); + + let response = index.wait_task(task_id).await; + + assert_eq!(response["status"], "failed"); + assert_eq!(response["error"], expected_error.clone()); + + // try to create a index via add specialized settings route + let (response, code) = index.update_distinct_attribute(json!("test")).await; + assert_eq!(code, 202); + let task_id = response["uid"].as_u64().unwrap(); + + let response = index.wait_task(task_id).await; + + assert_eq!(response["status"], "failed"); + assert_eq!(response["error"], expected_error.clone()); +} + +#[actix_rt::test] +async fn lazy_create_index() { + let mut server = Server::new_auth().await; + server.use_api_key("MASTER_KEY"); + + // create key with access on all indexes. + let content = json!({ + "indexes": ["*"], + "actions": ["*"], + "expiresAt": "2050-11-13T00:00:00Z" + }); + + let (response, code) = server.add_api_key(content).await; + assert_eq!(code, 201); + assert!(response["key"].is_string()); + + // use created key. + let key = response["key"].as_str().unwrap(); + server.use_api_key(&key); + + // try to create a index via add documents route + let index = server.index("test"); + let documents = json!([ + { + "id": 1, + "content": "foo", + } + ]); + + let (response, code) = index.add_documents(documents, None).await; + assert_eq!(code, 202, "{:?}", response); + let task_id = response["uid"].as_u64().unwrap(); + + index.wait_task(task_id).await; + + let (response, code) = index.get_task(task_id).await; + assert_eq!(code, 200); + assert_eq!(response["status"], "succeeded"); + + // try to create a index via add settings route + let index = server.index("test1"); + let settings = json!({ "distinctAttribute": "test"}); + + let (response, code) = index.update_settings(settings).await; + assert_eq!(code, 202); + let task_id = response["uid"].as_u64().unwrap(); + + index.wait_task(task_id).await; + + let (response, code) = index.get_task(task_id).await; + assert_eq!(code, 200); + assert_eq!(response["status"], "succeeded"); + + // try to create a index via add specialized settings route + let index = server.index("test2"); + let (response, code) = index.update_distinct_attribute(json!("test")).await; + assert_eq!(code, 202); + let task_id = response["uid"].as_u64().unwrap(); + + index.wait_task(task_id).await; + + let (response, code) = index.get_task(task_id).await; + assert_eq!(code, 200); + assert_eq!(response["status"], "succeeded"); +} diff --git a/meilisearch-http/tests/auth/mod.rs b/meilisearch-http/tests/auth/mod.rs index 4d5d043bd..a7ae7c592 100644 --- a/meilisearch-http/tests/auth/mod.rs +++ b/meilisearch-http/tests/auth/mod.rs @@ -2,29 +2,12 @@ mod api_keys; mod authorization; mod payload; -use crate::common::server::default_settings; -use crate::common::server::TEST_TEMP_DIR; use crate::common::Server; use actix_web::http::StatusCode; + use serde_json::{json, Value}; -use tempfile::TempDir; impl Server { - pub async fn new_auth() -> Self { - let dir = TempDir::new().unwrap(); - - if cfg!(windows) { - std::env::set_var("TMP", TEST_TEMP_DIR.path()); - } else { - std::env::set_var("TMPDIR", TEST_TEMP_DIR.path()); - } - - let mut options = default_settings(dir.path()); - options.master_key = Some("MASTER_KEY".to_string()); - - Self::new_with_options(options).await - } - pub fn use_api_key(&mut self, api_key: impl AsRef) { self.service.api_key = Some(api_key.as_ref().to_string()); } diff --git a/meilisearch-http/tests/common/server.rs b/meilisearch-http/tests/common/server.rs index 115b4650c..0fb5eacfb 100644 --- a/meilisearch-http/tests/common/server.rs +++ b/meilisearch-http/tests/common/server.rs @@ -50,6 +50,33 @@ impl Server { } } + pub async fn new_auth() -> Self { + let dir = TempDir::new().unwrap(); + + if cfg!(windows) { + std::env::set_var("TMP", TEST_TEMP_DIR.path()); + } else { + std::env::set_var("TMPDIR", TEST_TEMP_DIR.path()); + } + + let mut options = default_settings(dir.path()); + options.master_key = Some("MASTER_KEY".to_string()); + + let meilisearch = setup_meilisearch(&options).unwrap(); + let auth = AuthController::new(&options.db_path, &options.master_key).unwrap(); + let service = Service { + meilisearch, + auth, + options, + api_key: None, + }; + + Server { + service, + _dir: Some(dir), + } + } + pub async fn new_with_options(options: Opt) -> Self { let meilisearch = setup_meilisearch(&options).unwrap(); let auth = AuthController::new(&options.db_path, &options.master_key).unwrap(); diff --git a/meilisearch-lib/proptest-regressions/index_resolver/mod.txt b/meilisearch-lib/proptest-regressions/index_resolver/mod.txt index 553b8f1d5..583db4918 100644 --- a/meilisearch-lib/proptest-regressions/index_resolver/mod.txt +++ b/meilisearch-lib/proptest-regressions/index_resolver/mod.txt @@ -17,3 +17,4 @@ cc 3a01c78db082434b8a4f8914abf0d1059d39f4426d16df20d72e1bd7ebb94a6a # shrinks to cc c450806df3921d1e6fe9b6af93d999e8196d0175b69b64f1810802582421e94a # shrinks to task = Task { id: 0, index_uid: IndexUid("a"), content: CreateIndex { primary_key: Some("") }, events: [] }, index_exists = false, index_op_fails = false, any_int = 0 cc fb6b98947cbdbdee05ed3c0bf2923aad2c311edc276253642eb43a0c0ec4888a # shrinks to task = Task { id: 0, index_uid: IndexUid("A"), content: CreateIndex { primary_key: Some("") }, events: [] }, index_exists = false, index_op_fails = true, any_int = 0 cc 1aa59d8e22484e9915efbb5818e1e1ab684aa61b166dc82130d6221663ba00bf # shrinks to task = Task { id: 0, index_uid: IndexUid("a"), content: DocumentDeletion(Clear), events: [] }, index_exists = true, index_op_fails = false, any_int = 0 +cc 2e8644e6397b5f76e0b79f961fa125e2f45f42f26e03c453c9a174dfb427500d # shrinks to task = Task { id: 0, index_uid: IndexUid("0"), content: SettingsUpdate { settings: Settings { displayed_attributes: NotSet, searchable_attributes: NotSet, filterable_attributes: NotSet, sortable_attributes: NotSet, ranking_rules: NotSet, stop_words: NotSet, synonyms: NotSet, distinct_attribute: NotSet, _kind: PhantomData }, is_deletion: false, allow_index_creation: false }, events: [] }, index_exists = false, index_op_fails = false, any_int = 0 diff --git a/meilisearch-lib/src/index_controller/dump_actor/compat/v3.rs b/meilisearch-lib/src/index_controller/dump_actor/compat/v3.rs index a7faf4c1b..597c11fe0 100644 --- a/meilisearch-lib/src/index_controller/dump_actor/compat/v3.rs +++ b/meilisearch-lib/src/index_controller/dump_actor/compat/v3.rs @@ -74,11 +74,13 @@ impl From for TaskContent { primary_key, // document count is unknown for legacy updates documents_count: 0, + allow_index_creation: true, }, Update::Settings(settings) => TaskContent::SettingsUpdate { settings, // There is no way to know now, so we assume it isn't is_deletion: false, + allow_index_creation: true, }, Update::ClearDocuments => TaskContent::DocumentDeletion(DocumentDeletion::Clear), } diff --git a/meilisearch-lib/src/index_controller/mod.rs b/meilisearch-lib/src/index_controller/mod.rs index 001bbdbd7..5e50ff281 100644 --- a/meilisearch-lib/src/index_controller/mod.rs +++ b/meilisearch-lib/src/index_controller/mod.rs @@ -119,6 +119,7 @@ pub enum Update { settings: Settings, /// Indicates whether the update was a deletion is_deletion: bool, + allow_index_creation: bool, }, DocumentAddition { #[derivative(Debug = "ignore")] @@ -126,6 +127,7 @@ pub enum Update { primary_key: Option, method: IndexDocumentsMethod, format: DocumentAdditionFormat, + allow_index_creation: bool, }, DeleteIndex, CreateIndex { @@ -340,15 +342,18 @@ where Update::Settings { settings, is_deletion, + allow_index_creation, } => TaskContent::SettingsUpdate { settings, is_deletion, + allow_index_creation, }, Update::DocumentAddition { mut payload, primary_key, format, method, + allow_index_creation, } => { let mut buffer = Vec::new(); while let Some(bytes) = payload.next().await { @@ -380,6 +385,7 @@ where merge_strategy: method, primary_key, documents_count, + allow_index_creation, } } Update::DeleteIndex => TaskContent::IndexDeletion, diff --git a/meilisearch-lib/src/index_resolver/mod.rs b/meilisearch-lib/src/index_resolver/mod.rs index 79493407e..7b61983fe 100644 --- a/meilisearch-lib/src/index_resolver/mod.rs +++ b/meilisearch-lib/src/index_resolver/mod.rs @@ -188,13 +188,18 @@ where content_uuid, merge_strategy, primary_key, + allow_index_creation, .. } => { let primary_key = primary_key.clone(); let content_uuid = *content_uuid; let method = *merge_strategy; - let index = self.get_or_create_index(index_uid, task.id).await?; + let index = if *allow_index_creation { + self.get_or_create_index(index_uid, task.id).await? + } else { + self.get_index(index_uid.into_inner()).await? + }; let file_store = self.file_store.clone(); let result = spawn_blocking(move || { index.update_documents(method, content_uuid, primary_key, file_store) @@ -227,8 +232,9 @@ where TaskContent::SettingsUpdate { settings, is_deletion, + allow_index_creation, } => { - let index = if *is_deletion { + let index = if *is_deletion || !*allow_index_creation { self.get_index(index_uid.into_inner()).await? } else { self.get_or_create_index(index_uid, task.id).await? @@ -503,8 +509,8 @@ mod test { match &task.content { // an unexisting index should trigger an index creation in the folllowing cases: - TaskContent::DocumentAddition { .. } - | TaskContent::SettingsUpdate { is_deletion: false, .. } + TaskContent::DocumentAddition { allow_index_creation: true, .. } + | TaskContent::SettingsUpdate { allow_index_creation: true, is_deletion: false, .. } | TaskContent::IndexCreation { .. } if !index_exists => { index_store .expect_create() @@ -566,6 +572,8 @@ mod test { || (!index_exists && matches!(task.content, TaskContent::IndexDeletion | TaskContent::DocumentDeletion(_) | TaskContent::SettingsUpdate { is_deletion: true, ..} + | TaskContent::SettingsUpdate { allow_index_creation: false, ..} + | TaskContent::DocumentAddition { allow_index_creation: false, ..} | TaskContent::IndexUpdate { .. } )) { assert!(result.is_err(), "{:?}", result); diff --git a/meilisearch-lib/src/tasks/task.rs b/meilisearch-lib/src/tasks/task.rs index 028814136..f5dd8d9be 100644 --- a/meilisearch-lib/src/tasks/task.rs +++ b/meilisearch-lib/src/tasks/task.rs @@ -134,12 +134,14 @@ pub enum TaskContent { merge_strategy: IndexDocumentsMethod, primary_key: Option, documents_count: usize, + allow_index_creation: bool, }, DocumentDeletion(DocumentDeletion), SettingsUpdate { settings: Settings, /// Indicates whether the task was a deletion is_deletion: bool, + allow_index_creation: bool, }, IndexDeletion, IndexCreation {