diff --git a/scripts/translations/.translation_state.json b/scripts/translations/.translation_state.json index e4c6a0dead..4542b910d9 100644 --- a/scripts/translations/.translation_state.json +++ b/scripts/translations/.translation_state.json @@ -375,6 +375,8 @@ "copied": "ea61bc15688d1e48", "copyDetails": "ec7ee2826c849564", "denied": { + "actions": "ad5a1de1c7b8b675", + "askAdmin": "01ecb087fe129ce1", "create": "e64bc3593a603832", "delete": "c346b3c06e9303ed", "edit": "6649052d1364e44e", @@ -382,57 +384,120 @@ "missingIntro": "475cc73919dd1e9d", "more": "5f4f590acbda8ee3", "parts": "8b82b8f1aed3dad5", + "resources": "cd3b36d4f31da787", "showLess": "94ea9b1d33a02975", "view": "27e9f709ef02acf1" }, "resources": { "ai": "11fb682be0a0233d", + "alertFolders": "a6e2acee332f6d23", "alerts": "f62e9267d196c761", + "billingGroup": "3a0cc47e1a82ca70", "cipherKeys": "9d5e36b817bf496e", + "dashboardFolders": "62ec1a77e94fbcb4", "dashboards": "a53bcafb67d960df", + "databaseMonitoring": "8f614378c817c981", "destinations": "72eb63f032e47e34", + "downtimeFolders": "8d359247ac73379d", "downtimes": "5d9972eaed771890", + "enrichmentTables": "271f9c6c3357a0b6", + "evalJobs": "e24b61f0397c85fd", "fleetConfigs": "affc3f6b5f45623a", + "fleetManagement": "734376ce61f58376", "functions": "75e942e5b366227e", "groups": "39bbb719fa2b9d22", + "incidentIntegrations": "28df794d2558a7c3", + "incidents": "bfe868931580fb3b", "llmModelPricing": "794f915f46e1cc64", + "llmProviders": "437dbe28117905aa", + "metrics": "a58da793c7250f1b", + "onCall": "3004e1251d8890eb", + "onCallResponses": "a330bac7444925ef", "organizations": "2730183d6b776b40", "passcode": "df5f51e2e793f3c9", "pipelines": "d1a8fffeb935847e", + "promptFolders": "f92c9201b9ff5bb1", "publicDashboards": "b4bee918655d4e11", "regexPatterns": "36e205848147c947", + "remoteTasks": "10d46c5392001dce", + "reportFolders": "515a865d77faf262", "reports": "dacca3cba3f346a4", "roles": "c25337055464f8d3", + "rumTokens": "d7da49dc65e2dbf8", "savedviews": "81184cb517eda985", + "scoreConfigs": "7d19a0ae2c76ced2", + "scorers": "d8598427499abb65", + "searchJobs": "989ecb5d07fd4c76", "serviceAccounts": "1642d9e225e63659", + "serviceStreams": "e3c04b59334f2bd1", + "settings": "74a883a037bc227f", + "sourceMaps": "b95b75c0ca503146", + "statusPages": "8fe24747e79435f9", "streams": "bfd297afbf9e7430", + "syntheticChecks": "fe7120bc3b396719", + "syntheticEnvironments": "a4b9982963b2f048", + "syntheticFolders": "48bdc3235b5ea3ea", "templates": "56b564b75c7fdb48", - "users": "6b0cc904d081a8c1" + "traces": "998741a58ed2b411", + "users": "6b0cc904d081a8c1", + "workflowFolders": "8e6af10f9d904da1", + "workflows": "e2073645cb0dbbdd" }, "resourcesInline": { "ai": "11fb682be0a0233d", + "alertFolders": "d2a7365a5fcea5bd", "alerts": "deb3e366c077c228", + "billingGroup": "988507f87cfea865", "cipherKeys": "a4ec922695af251c", + "dashboardFolders": "b3505579443c0ff4", "dashboards": "21b2e2095fd38f7d", + "databaseMonitoring": "d70b640c5e7e70cb", "destinations": "1af0500e4d2be663", + "downtimeFolders": "20ee5e166973b24f", "downtimes": "573ef711d75684dd", + "enrichmentTables": "2523e745479b9694", + "evalJobs": "3fcc2bfe3200a445", "fleetConfigs": "fa54fb3d9e25631c", + "fleetManagement": "a3ff8c3b237fa163", "functions": "ef7d009c7e812da1", "groups": "4ed379d418bb8629", + "incidentIntegrations": "c04904d8df7467f6", + "incidents": "a6d1765ab64d82a7", "llmModelPricing": "794f915f46e1cc64", + "llmProviders": "437dbe28117905aa", + "metrics": "177a7ea3611fe6b1", + "onCall": "3004e1251d8890eb", + "onCallResponses": "a330bac7444925ef", "organizations": "f8a665a97769f1d2", "passcode": "583375d251142f0d", "pipelines": "8719e4b9771d340e", + "promptFolders": "ac11560dd26704d4", "publicDashboards": "55ff9bd4571fa4cb", "regexPatterns": "f2677e20964b4314", + "remoteTasks": "63d70d9b7b8e801d", + "reportFolders": "ac9ce060cfb53c7b", "reports": "7f26104f77a7ce47", "roles": "66cf5513b37462be", + "rumTokens": "d7da49dc65e2dbf8", "savedviews": "1b46920dbc530fb7", + "scoreConfigs": "189e152b76d63e8a", + "scorers": "2b0f61e1cf254296", + "searchJobs": "6722117cf5ce7eda", "serviceAccounts": "7c40134810b0f1e1", + "serviceStreams": "8d5894bc8ffa2192", + "settings": "cde0fb0dec1400c5", + "sourceMaps": "08cd5f94f175b0d2", + "statusPages": "449ab3614f7fecf1", "streams": "d5a08b6893ecd1aa", + "syntheticChecks": "6e01d2051842b5a1", + "syntheticEnvironments": "a902961a1e4e24a3", + "syntheticFolders": "b8fe6356a3da347a", "templates": "9c6641c1f9cfb0c6", + "traces": "b5ac69fc467c01f3", "unnamed": "644ebf22501f57b5", - "users": "7dfb4cf67742cb06" + "users": "7dfb4cf67742cb06", + "workflowFolders": "9c0544bd232b9a33", + "workflows": "45b34f16db605a06" } }, "aiAssistant": { @@ -2883,6 +2948,9 @@ "alertTypeAll": "a52ace420f2175d0", "alertTypeAnomaly": "79622828d2d8d988", "alertTypeThreshold": "0da627ada476c179", + "card": { + "customizeTooltip": "c54bbc1df678bf48" + }, "categories": "292c06f0045a45d0", "clearFilters": "7179ea0035fca783", "clearGroupSelection": "65a102085dc79c05", @@ -2935,32 +3003,21 @@ "anomalyUnavailable": "6e0d80036514c77d", "back": "76900f1bfd16c8d4", "cancel": "19766ed6ccb2f4a3", - "clear": "83b12c2216efb4fd", - "confirmLarge": "e0e1d5cc504b8f92", "confirmLargeNoDestination": "c20eb3ff81f4d2fb", - "destinationIntro": "bf8cdafc7243e855", - "destinationLabel": "293d404a500f5f9a", - "destinationsLoadFailed": "4eeeed3a003c5cf3", - "destinationsNoneBody": "cdef20b5bf7cd337", "done": "11a6767d5674c7e4", "folderIntro": "b4e0c86b9e450e0f", "frequency": "24947227759d0aca", "minutes": "1f6fa6f69d185e60", "next": "1ff57a29d7c9d11b", - "openDestinations": "c56a8a631d92885b", - "retry": "d8b8392e2c542950", "retryFailed": "32780945b5f6a491", "run": "bd467a2684768a3d", - "selectAll": "1fc9a387654d410f", - "selectedCount": "30ae395b1b4c7b5c", "silence": "6ce6a4db3720f473", "someFailed": "e0ff076899de444d", "stepAlerts": "f62e9267d196c761", - "stepDestination": "293d404a500f5f9a", "stepFolder": "74ccd43303847f26", "stepInstall": "9fd728c66c9a256b", "stepTune": "22c58d84f08e3a4e", - "subtitle": "f0269d0053de4642", + "subtitle": "a1658f4d1f84e24b", "summaryTuningNone": "cda6ba311e7559b0", "summaryTuningOverride": "4618a77d0da6425f", "title": "4cc111be1713bc4e", @@ -2976,7 +3033,6 @@ "noCollectorTitle": "f2c2ba2f9dafa112", "noMatchesDescription": "e09a161e41ec40f8", "noMatchesTitle": "0106064dcab637bc", - "notIngested": "ba8613ab3f202cb7", "notIngestedHint": "12e99d7888e6aa47", "retry": "942087cc2d41e013", "searchCategories": "d03ec37c5f391830", @@ -3310,6 +3366,7 @@ "alertIfColumn": "598de24632dc3693", "alertIfNoOfEvents": "4e93cac2049f2acc", "alertIfValueIs": "d93436b1c28cdb91", + "alertLibrary": "4c4995d1987d3b7b", "alertName": "60a6c1eb5253724b", "alertNamePlaceholder": "1f84a161ed86d648", "alertRules": "98a30442b31e77d6", @@ -4477,6 +4534,7 @@ "v2GroupConditionsArray": "e7eb3aa890d8d2bb", "vrlFunctionString": "c8fdb15f9cc29aba" }, + "importAlert": "33cc28980e082bc3", "importAlertTitle": "8003b4ed4b9e3e5b", "importedFromPanel": "4e66f1ce917a8daf", "incidents": { @@ -23324,7 +23382,6 @@ "searchField": "7909dc4e707ddddb", "searchHistory": "66d7e4a1c738f0b1", "searchInspect": "bac72e15bee2bc8a", - "searchJobNoPermission": "e0038dd68dcec8eb", "searchOperationCancelled": "2429bc389adabe7f", "searchQueryEmptyOrInvalid": "7682ef6f589ed1ea", "searchSavedFunction": "49c266baaaa70981", diff --git a/src/api/http/src/handler/http/router/mod.rs b/src/api/http/src/handler/http/router/mod.rs index f23dfc72da..f26de02a13 100644 --- a/src/api/http/src/handler/http/router/mod.rs +++ b/src/api/http/src/handler/http/router/mod.rs @@ -2604,7 +2604,7 @@ fn fleet_router() -> Router { .route_layer(middleware::from_fn(fleet_gate)) } -/// §17: with `O2_FLEET_ENABLED` off, OpAMP answers 503 and management reads work but writes do not. +/// §17: with `O2_FLEET_ENABLED` off, auth refuses fleet tokens first (401); writes get 503. #[cfg(feature = "enterprise")] async fn fleet_gate(request: Request, next: Next) -> Response { if openobserve_core::fleet::enabled() || request.method() == Method::GET && !is_opamp(&request) diff --git a/src/api/http/src/handler/http/router/openapi.rs b/src/api/http/src/handler/http/router/openapi.rs index a834b8ab6f..dd3144bfc6 100644 --- a/src/api/http/src/handler/http/router/openapi.rs +++ b/src/api/http/src/handler/http/router/openapi.rs @@ -94,6 +94,7 @@ use crate::{ openobserve_api_search::promql::remote_write, openobserve_api_search::promql::query_get, openobserve_api_search::promql::query_range_get, + openobserve_api_search::promql::query_exemplars_get, openobserve_api_search::promql::metadata, openobserve_api_search::promql::series_get, openobserve_api_search::promql::labels_get, @@ -1714,6 +1715,19 @@ mod tests { } } + #[test] + fn promql_exemplars_is_published_with_its_error_codes() { + let spec = serde_json::to_value(ApiDoc::openapi()).unwrap(); + let op = &spec["paths"]["/api/{org_id}/prometheus/api/v1/query_exemplars"]["get"]; + assert_eq!(op["operationId"], "PrometheusQueryExemplars"); + for status in ["200", "400", "403", "429", "500"] { + assert!( + op["responses"].get(status).is_some(), + "query_exemplars {status}" + ); + } + } + #[test] fn recent_rejections_path_and_schemas_are_registered() { let spec = serde_json::to_value(ApiDoc::openapi()).unwrap(); diff --git a/src/api/management/src/request/eval_jobs/mod.rs b/src/api/management/src/request/eval_jobs/mod.rs index 98e0a18095..93fc44f583 100644 --- a/src/api/management/src/request/eval_jobs/mod.rs +++ b/src/api/management/src/request/eval_jobs/mod.rs @@ -20,13 +20,15 @@ use axum::{ #[cfg(feature = "enterprise")] use openobserve_api_common::extractors::Headers; #[cfg(feature = "enterprise")] -use openobserve_core::auth::UserEmail; +use openobserve_core::auth::{UserEmail, UserReads}; #[cfg(test)] use openobserve_core::llm_evaluations::eval_jobs::EvalJobError; use openobserve_core::llm_evaluations::eval_jobs::{ self, ManualEvalJobRequestBody, ManualEvalJobResponseBody, }; +#[cfg(feature = "enterprise")] +use crate::request::scorers::{MAX_SCORERS_PER_REQUEST, authorize_references}; use crate::{ common::meta::http::HttpResponse as MetaHttpResponse, models::eval_jobs::{ @@ -118,6 +120,10 @@ pub async fn create_eval_job( Err(err) => return MetaHttpResponse::bad_request(err), }; #[cfg(feature = "enterprise")] + if let Err(resp) = limit_scorers(&job.scorers) { + return resp; + } + #[cfg(feature = "enterprise")] let job = match eval_jobs::prepare_new_job(&org_id, job).await { Ok(job) => job, Err(err) => return err.into(), @@ -127,6 +133,10 @@ pub async fn create_eval_job( return resp; } #[cfg(feature = "enterprise")] + if let Err(resp) = guard_scorers(&org_id, &user_email.user_id, &job.scorers).await { + return resp; + } + #[cfg(feature = "enterprise")] let created = eval_jobs::insert_job(&org_id, job).await; #[cfg(not(feature = "enterprise"))] let created = eval_jobs::create_job(&org_id, job).await; @@ -205,6 +215,15 @@ pub async fn update_eval_job( Err(err) => return MetaHttpResponse::bad_request(err), }; #[cfg(feature = "enterprise")] + let scorers_changed = match eval_jobs::get_job(&org_id, &job_id).await { + Ok(existing) => existing.scorers != job.scorers, + Err(_) => true, + }; + #[cfg(feature = "enterprise")] + if scorers_changed && let Err(resp) = limit_scorers(&job.scorers) { + return resp; + } + #[cfg(feature = "enterprise")] let job = match eval_jobs::prepare_job_update(&org_id, &job_id, job).await { Ok(job) => job, Err(err) => return err.into(), @@ -214,6 +233,12 @@ pub async fn update_eval_job( return resp; } #[cfg(feature = "enterprise")] + if scorers_changed + && let Err(resp) = guard_scorers(&org_id, &user_email.user_id, &job.scorers).await + { + return resp; + } + #[cfg(feature = "enterprise")] let updated = eval_jobs::store_job_update(job).await; #[cfg(not(feature = "enterprise"))] let updated = eval_jobs::update_job(&org_id, &job_id, job).await; @@ -485,6 +510,36 @@ async fn guard_job( openobserve_core::background_access::guard_write(org_id, user_id, &sources).await } +#[cfg(feature = "enterprise")] +fn limit_scorers( + scorers: &[config::meta::pipeline::components::ScorerRef], +) -> Result<(), Response> { + if scorers.len() > MAX_SCORERS_PER_REQUEST { + return Err(MetaHttpResponse::bad_request(format!( + "At most {MAX_SCORERS_PER_REQUEST} scorers can be named in one request" + ))); + } + Ok(()) +} + +#[cfg(feature = "enterprise")] +async fn guard_scorers( + org_id: &str, + user_id: &str, + scorers: &[config::meta::pipeline::components::ScorerRef], +) -> Result<(), Response> { + let objects: Vec<_> = scorers + .iter() + .map(|scorer| ("scorers", scorer.id.as_str())) + .collect(); + let definitions = async { + eval_jobs::scorer_definitions(org_id, scorers) + .await + .map_err(Response::from) + }; + authorize_references(&objects, &UserReads { org_id, user_id }, definitions).await +} + /// The transition's own read and checks answer first, so a bad job is never reported as refused. #[cfg(feature = "enterprise")] async fn guard_activation(org_id: &str, user_id: &str, job_id: &str) -> Result<(), Response> { @@ -497,7 +552,8 @@ async fn guard_activation(org_id: &str, user_id: &str, job_id: &str) -> Result<( eval_jobs::check_transition(org_id, &job, "active") .await .map_err(Response::from)?; - guard_job(org_id, user_id, &job).await + guard_job(org_id, user_id, &job).await?; + guard_scorers(org_id, user_id, &job.scorers).await } /// The evaluation's own read and checks answer first, so a bad request is never refused. @@ -517,7 +573,8 @@ async fn guard_manual( eval_jobs::check_manual_evaluate(org_id, &job, body) .await .map_err(Response::from)?; - guard_job(org_id, user_id, &job).await + guard_job(org_id, user_id, &job).await?; + guard_scorers(org_id, user_id, &job.scorers).await } #[cfg(test)] @@ -636,4 +693,31 @@ mod tests { .await .unwrap(); } + + #[cfg(feature = "enterprise")] + fn scorer_refs(count: usize) -> Vec { + (0..count) + .map(|i| config::meta::pipeline::components::ScorerRef { + id: format!("scorer-{i}"), + version: None, + }) + .collect() + } + + #[cfg(feature = "enterprise")] + #[tokio::test] + async fn a_job_naming_a_scorer_the_caller_cannot_read_is_refused() { + let denied = guard_scorers("org1", &denied_user().0.user_id, &scorer_refs(1)) + .await + .unwrap_err(); + assert_eq!(denied.status().as_u16(), 403); + } + + #[cfg(feature = "enterprise")] + #[test] + fn a_job_naming_more_scorers_than_the_maximum_is_refused() { + assert!(limit_scorers(&scorer_refs(MAX_SCORERS_PER_REQUEST)).is_ok()); + let resp = limit_scorers(&scorer_refs(MAX_SCORERS_PER_REQUEST + 1)).unwrap_err(); + assert_eq!(resp.status().as_u16(), 400); + } } diff --git a/src/api/management/src/request/experiments.rs b/src/api/management/src/request/experiments.rs index 878b1e8e3b..764aa1db58 100644 --- a/src/api/management/src/request/experiments.rs +++ b/src/api/management/src/request/experiments.rs @@ -20,13 +20,15 @@ use axum::{ response::{IntoResponse, Response}, }; use db::authz::{remove_ownership, set_ownership}; +use infra::table::scorers::Scorer; use openobserve_api_common::extractors::Headers; use openobserve_core::{ - auth::{UserEmail, is_ofga_object_visible}, + auth::{UserEmail, UserReads, is_ofga_object_visible}, llm_evaluations::{ datasets, experiments::{ - self, ExperimentError, PinnedExperimentScorer, baseline, + self, Experiment, ExperimentError, ExperimentScorerRef, ExperimentTaskConfig, + PinnedExperimentScorer, baseline, dispersion::{self, NormalizationSpans, RowDispersion}, ingest::{self, IngestError}, results::{self, ExperimentResultSlot, ExperimentSlotStatus}, @@ -53,14 +55,41 @@ use crate::{ ExperimentResultRowSortBody, ExperimentResultsResponseBody, ExperimentRowDetailResponseBody, ExperimentRowNavigationBody, ExperimentRowSnapshotBody, ExperimentScoreSummaryBody, ExperimentSlotPageQuery, - ExperimentSlotPageResponseBody, ExperimentSummaryResponseBody, ExperimentTaskBody, + ExperimentSlotPageResponseBody, ExperimentSummaryResponseBody, ExperimentViewResponseBody, ListExperimentsResponseBody, RetryExperimentSlotRequestBody, SubmitExperimentRecordsRequestBody, SubmitExperimentRecordsResponseBody, }, }, + request::scorers::{MAX_SCORERS_PER_REQUEST, authorize_references}, }; +/// The scorers an Experiment pins: named by a request, or kept from a stored Experiment. +enum ScorerSelection<'a> { + Requested(&'a [ExperimentScorerRef]), + Pinned(&'a Experiment), +} + +impl ScorerSelection<'_> { + fn ids(&self) -> Vec<&str> { + match self { + Self::Requested(refs) => refs.iter().map(|scorer| scorer.id.as_str()).collect(), + Self::Pinned(experiment) => experiment + .scorers + .iter() + .map(|scorer| scorer.id.as_str()) + .collect(), + } + } + + async fn definitions(&self, org_id: &str) -> Result, ExperimentError> { + match self { + Self::Requested(refs) => Ok(experiments::pin_scorers(org_id, refs).await?.1), + Self::Pinned(experiment) => experiments::scorer_definitions(org_id, experiment).await, + } + } +} + fn experiment_error_response(error: ExperimentError) -> Response { match error { ExperimentError::Database(error) => { @@ -406,51 +435,84 @@ async fn require_experiment_visibility( Ok(()) } -/// The Remote Task reference is the one place an Experiment reaches outside -/// itself, so the caller must be able to see the Task it names. -/// -/// Without this, a caller who knows a hidden `name@version` can make the -/// platform place that Task's credentialed outbound call and can read its output -/// back through the Experiment results. The Experiment routes authorize the -/// Experiment alone, so nothing else covers this. -async fn require_remote_task_visibility( - org_id: &str, - user_id: &str, - task_ref: &str, -) -> Result<(), Response> { - // The OpenFGA object is the head's `entity_id`, not the `name@version` an - // Experiment carries, so the reference has to be resolved first. An - // unresolvable reference is answered as a denial rather than a 404, so that - // a caller cannot use the difference to probe for hidden Tasks. - let task = remote_tasks::resolve_task_ref(org_id, task_ref) - .await - .map_err(|_| MetaHttpResponse::forbidden("Unauthorized Access"))?; - let permitted_objects = openobserve_api_common::auth::validator::list_objects_for_user( - org_id, - user_id, - "GET", - "remote_task", - ) - .await - .map_err(|error| MetaHttpResponse::forbidden(error.to_string()))?; - if !is_ofga_object_visible( - org_id, - "remote_task", - &task.entity_id, - permitted_objects.as_deref(), - ) { - return Err(MetaHttpResponse::forbidden("Unauthorized Access")); +/// The objects an Experiment reads besides itself and its Remote Task, as `(resource, id)`. +fn referenced_objects<'a>( + dataset_id: &'a str, + scorer_ids: &[&'a str], + task: &'a ExperimentTaskConfig, +) -> Vec<(&'static str, &'a str)> { + let mut objects = vec![("datasets", dataset_id)]; + objects.extend(scorer_ids.iter().map(|id| ("scorers", *id))); + if let Some((prompt_id, ..)) = task.prompt_ref() { + objects.push(("prompts", prompt_id)); } - Ok(()) + if let Some(provider_id) = task.provider_id() { + objects.push(("providers", provider_id)); + } + objects } -/// The `task_ref` of a Remote Task request, or `None` for the task types that -/// never leave the platform. -fn remote_task_ref(task: &ExperimentTaskBody) -> Option<&str> { - match task { - ExperimentTaskBody::Remote { task_ref, .. } => Some(task_ref.as_str()), - _ => None, +async fn require_reference_access( + org_id: &str, + user_id: &str, + dataset_id: &str, + scorers: ScorerSelection<'_>, + task: &ExperimentTaskConfig, +) -> Result<(), Response> { + let scorer_ids = scorers.ids(); + if matches!(scorers, ScorerSelection::Requested(_)) + && scorer_ids.len() > MAX_SCORERS_PER_REQUEST + { + return Err(MetaHttpResponse::bad_request(format!( + "At most {MAX_SCORERS_PER_REQUEST} scorers can be named in one request" + ))); } + // Unresolvable is a denial, not a 404, so hidden Tasks cannot be probed. + let remote_task = match task.remote_task() { + Some((task_ref, _)) => Some( + remote_tasks::resolve_task_ref(org_id, task_ref) + .await + .map_err(|_| MetaHttpResponse::forbidden("Unauthorized Access"))?, + ), + None => None, + }; + let mut objects = referenced_objects(dataset_id, &scorer_ids, task); + if let Some(remote_task) = &remote_task { + objects.push(("remote_tasks", remote_task.entity_id.as_str())); + } + let definitions = async { + scorers + .definitions(org_id) + .await + .map_err(experiment_error_response) + }; + authorize_references(&objects, &UserReads { org_id, user_id }, definitions).await +} + +async fn require_pinned_reference_access( + org_id: &str, + user_id: &str, + experiment: &Experiment, +) -> Result<(), Response> { + require_reference_access( + org_id, + user_id, + &experiment.dataset_id, + ScorerSelection::Pinned(experiment), + &experiment.task, + ) + .await +} + +async fn require_stored_reference_access( + org_id: &str, + experiment_id: &str, + user_id: &str, +) -> Result<(), Response> { + let experiment = experiments::get(org_id, experiment_id) + .await + .map_err(experiment_error_response)?; + require_pinned_reference_access(org_id, user_id, &experiment).await } #[utoipa::path( @@ -467,6 +529,7 @@ fn remote_task_ref(task: &ExperimentTaskBody) -> Option<&str> { responses( (status = 200, body = inline(ExperimentPreviewResponseBody)), (status = 400, description = "Invalid Experiment definition", body = ()), + (status = 403, description = "A referenced Dataset, Scorer, Prompt, Provider or Remote Task is not accessible", body = ()), (status = 404, description = "Dataset or scorer not found", body = ()), ), )] @@ -476,13 +539,19 @@ pub async fn preview_experiment( Headers(user): Headers, axum::Json(body): axum::Json, ) -> Response { - if let Some(task_ref) = remote_task_ref(&body.task) - && let Err(response) = - require_remote_task_visibility(&org_id, &user.user_id, task_ref).await + let input: experiments::CreateExperiment = body.into(); + if let Err(response) = require_reference_access( + &org_id, + &user.user_id, + &input.dataset_id, + ScorerSelection::Requested(&input.scorers), + &input.task, + ) + .await { return response; } - match experiments::preview(&org_id, body.into(), query.sample_size).await { + match experiments::preview(&org_id, input, query.sample_size).await { Ok(preview) => MetaHttpResponse::json(ExperimentPreviewResponseBody::from(preview)), Err(error) => experiment_error_response(error), } @@ -499,6 +568,7 @@ pub async fn preview_experiment( responses( (status = 200, body = inline(CreateExperimentResponseBody)), (status = 400, description = "Invalid Experiment definition", body = ()), + (status = 403, description = "A referenced Dataset, Scorer, Prompt, Provider or Remote Task is not accessible", body = ()), (status = 409, description = "Idempotency conflict", body = ()), (status = 412, description = "Cost estimate is above the warning threshold and was not confirmed", body = ()), ), @@ -508,12 +578,6 @@ pub async fn create_experiment( Headers(user): Headers, axum::Json(body): axum::Json, ) -> Response { - if let Some(task_ref) = remote_task_ref(&body.task) - && let Err(response) = - require_remote_task_visibility(&org_id, &user.user_id, task_ref).await - { - return response; - } // Re-serialized rather than hashed from the raw bytes so two encodings of // the same request agree on their canonical hash. let canonical_request = match serde_json::to_value(&body) { @@ -523,7 +587,19 @@ pub async fn create_experiment( return MetaHttpResponse::internal_error("Internal server error"); } }; - match experiments::create(&org_id, &user.user_id, body.into(), &canonical_request).await { + let input: experiments::CreateExperiment = body.into(); + if let Err(response) = require_reference_access( + &org_id, + &user.user_id, + &input.dataset_id, + ScorerSelection::Requested(&input.scorers), + &input.task, + ) + .await + { + return response; + } + match experiments::create(&org_id, &user.user_id, input, &canonical_request).await { Ok(result) => { set_ownership(&org_id, "experiments", Authz::new(&result.experiment.id)).await; MetaHttpResponse::json(CreateExperimentResponseBody::from(result)) @@ -1337,11 +1413,20 @@ pub async fn delete_experiment( ), responses( (status = 200, description = "Failed Experiment returned to running", body = ExperimentResponseBody), + (status = 403, description = "A referenced Dataset, Scorer, Prompt, Provider or Remote Task is not accessible"), (status = 404, description = "Experiment not found"), (status = 409, description = "Invalid or concurrent lifecycle transition"), ) )] -pub async fn retry_experiment(Path((org_id, experiment_id)): Path<(String, String)>) -> Response { +pub async fn retry_experiment( + Path((org_id, experiment_id)): Path<(String, String)>, + Headers(user): Headers, +) -> Response { + if let Err(response) = + require_stored_reference_access(&org_id, &experiment_id, &user.user_id).await + { + return response; + } match openobserve_core::llm_evaluations::experiments::runner::retry_failed_experiment( &org_id, &experiment_id, @@ -1371,7 +1456,7 @@ pub async fn retry_experiment(Path((org_id, experiment_id)): Path<(String, Strin responses( (status = 202, description = "Selected slot retry queued"), (status = 400, description = "Invalid idempotency key"), - (status = 403, description = "Experiment is not accessible"), + (status = 403, description = "Experiment or one of its references is not accessible"), (status = 404, description = "Experiment, row, or trial not found"), (status = 409, description = "Experiment lifecycle, active slot retry, or idempotency conflict"), ) @@ -1388,6 +1473,11 @@ pub async fn retry_experiment_slot( { return response; } + if let Err(response) = + require_stored_reference_access(&org_id, &experiment_id, &user.user_id).await + { + return response; + } match openobserve_core::llm_evaluations::experiments::runner::queue_slot_retry( &org_id, @@ -1435,6 +1525,7 @@ pub async fn retry_experiment_slot( responses( (status = 200, description = "Pending clone created", body = ExperimentResponseBody), (status = 400, description = "Invalid Experiment definition after overrides", body = ()), + (status = 403, description = "A referenced Dataset, Scorer, Prompt, Provider or Remote Task is not accessible", body = ()), (status = 404, description = "Experiment, Dataset or scorer not found"), (status = 409, description = "Only sealed Experiments can be cloned, or the idempotency key describes a different clone"), (status = 412, description = "Cost estimate is above the warning threshold and was not confirmed", body = ()), @@ -1445,24 +1536,31 @@ pub async fn clone_experiment( Headers(user): Headers, axum::Json(body): axum::Json, ) -> Response { - // The clone reaches whichever Remote Task it ends up pinning — the source's - // when the request overrides nothing, the override's when it does. - // Checking only at create would leave clone as the way around that check. + // Overrides fall back to the source's pins, so check what the clone will actually reference. let source = match experiments::get(&org_id, &experiment_id).await { Ok(source) => source, Err(error) => return experiment_error_response(error), }; - let effective_task_ref = match body.task.as_ref() { - Some(task) => remote_task_ref(task), - None => source.task.remote_task().map(|(task_ref, _)| task_ref), + let overrides: experiments::CloneExperimentOverrides = body.into(); + let scorers = match &overrides.scorers { + Some(refs) => ScorerSelection::Requested(refs), + None => ScorerSelection::Pinned(&source), }; - if let Some(task_ref) = effective_task_ref - && let Err(response) = - require_remote_task_visibility(&org_id, &user.user_id, task_ref).await + if let Err(response) = require_reference_access( + &org_id, + &user.user_id, + overrides + .dataset_id + .as_deref() + .unwrap_or(&source.dataset_id), + scorers, + overrides.task.as_ref().unwrap_or(&source.task), + ) + .await { return response; } - match experiments::clone_sealed(&org_id, &experiment_id, &user.user_id, body.into()).await { + match experiments::clone_sealed(&org_id, &experiment_id, &user.user_id, overrides).await { Ok(experiment) => { set_ownership(&org_id, "experiments", Authz::new(&experiment.id)).await; MetaHttpResponse::json(ExperimentResponseBody::from(experiment)) @@ -1768,63 +1866,194 @@ mod tests { )); } - /// The guard that decides whether the Remote Task check runs at all. If this - /// ever returned `None` for a Remote Task, `create_experiment` would go back - /// to reaching any Task in the org without a permission check, and no other - /// test would notice. + /// Dropping an object from this list would let an Experiment pin it unchecked. #[test] - fn only_a_remote_task_carries_a_reference_that_needs_checking() { + fn every_dataset_scorer_prompt_and_provider_an_experiment_reads_is_checked() { + let prompt_ref = ExperimentTaskConfig::PromptRef { + id: "prompt-1".to_string(), + version: 2, + provider_id: "provider-1".to_string(), + params_overrides: None, + }; assert_eq!( - remote_task_ref(&ExperimentTaskBody::Remote { - task_ref: "grader@3".to_string(), - overrides: None, - }), - Some("grader@3") + referenced_objects("dataset-1", &["scorer-1", "scorer-2"], &prompt_ref), + vec![ + ("datasets", "dataset-1"), + ("scorers", "scorer-1"), + ("scorers", "scorer-2"), + ("prompts", "prompt-1"), + ("providers", "provider-1"), + ] ); + let inline = ExperimentTaskConfig::InlinePrompt { + messages: vec![], + provider_id: "provider-1".to_string(), + model: None, + params: None, + }; assert_eq!( - remote_task_ref(&ExperimentTaskBody::Sdk { - task_fingerprint: "abc".to_string(), - config: serde_json::json!({}), - }), - None + referenced_objects("dataset-1", &[], &inline), + vec![("datasets", "dataset-1"), ("providers", "provider-1")] ); + let remote = ExperimentTaskConfig::Remote { + task_ref: "grader@3".to_string(), + overrides: None, + }; assert_eq!( - remote_task_ref(&ExperimentTaskBody::InlinePrompt { - messages: vec![], - provider_id: "provider-1".to_string(), - model: None, - params: None, - }), - None + referenced_objects("dataset-1", &[], &remote), + vec![("datasets", "dataset-1")] + ); + let sdk = ExperimentTaskConfig::Sdk { + task_fingerprint: "sha256:abc".to_string(), + config: serde_json::json!({}), + }; + assert_eq!( + referenced_objects("dataset-1", &[], &sdk), + vec![("datasets", "dataset-1")] ); } - /// The Remote Task check keys on the head `entity_id`, so it has to accept - /// the same grant shapes the Remote Task list already accepts. - #[test] - fn remote_task_visibility_accepts_exact_or_org_wildcard_and_rejects_other_tasks() { - let exact = vec!["remote_task:task-1".to_string()]; - let wildcard = vec!["remote_task:_all_acme".to_string()]; - let other = vec!["remote_task:task-2".to_string()]; + fn judge_scorer(provider_id: &str) -> Scorer { + let body: crate::models::scorers::ScorerRequestBody = + serde_json::from_value(serde_json::json!({ + "name": "judge", + "scorer": {"type": "llm_judge", "template": "t", "params": {"provider_id": provider_id}} + })) + .unwrap(); + body.into() + } - assert!(is_ofga_object_visible( - "acme", - "remote_task", - "task-1", - Some(&exact) - )); - assert!(is_ofga_object_visible( - "acme", - "remote_task", - "task-1", - Some(&wildcard) - )); - assert!(!is_ofga_object_visible( - "acme", - "remote_task", - "task-1", - Some(&other) - )); + fn unknown_user() -> String { + format!("{}@example.com", config::ider::uuid()) + } + + fn inline_task() -> ExperimentTaskConfig { + ExperimentTaskConfig::InlinePrompt { + messages: vec![], + provider_id: "provider-1".to_string(), + model: None, + params: None, + } + } + + struct DeniesResource(&'static str); + + impl openobserve_core::auth::ReadCheck for DeniesResource { + async fn can_read(&self, resource: &str, _: &str) -> bool { + resource != self.0 + } + } + + #[tokio::test] + async fn an_llm_judge_scorer_s_own_provider_must_be_readable() { + let definitions = vec![judge_scorer("judge-provider")]; + let denied = authorize_references( + &[("datasets", "dataset-1"), ("scorers", "scorer-1")], + &DeniesResource("providers"), + async { Ok(definitions.clone()) }, + ) + .await + .unwrap_err(); + assert_eq!(denied.status().as_u16(), 403); + + assert!( + authorize_references( + &[("datasets", "dataset-1")], + &DeniesResource("none"), + async { Ok(definitions.clone()) }, + ) + .await + .is_ok() + ); + } + + #[tokio::test] + async fn a_scorer_the_caller_cannot_read_is_never_loaded() { + let loaded = std::cell::Cell::new(false); + let denied = authorize_references( + &[("scorers", "scorer-1")], + &DeniesResource("scorers"), + async { + loaded.set(true); + Ok(vec![judge_scorer("judge-provider")]) + }, + ) + .await + .unwrap_err(); + assert_eq!(denied.status().as_u16(), 403); + assert!(!loaded.get()); + } + + #[tokio::test] + async fn naming_more_scorers_than_the_maximum_is_refused_before_any_check() { + let refs: Vec = (0..=MAX_SCORERS_PER_REQUEST) + .map(|i| ExperimentScorerRef { + id: format!("scorer-{i}"), + version: None, + }) + .collect(); + let task = inline_task(); + let user = unknown_user(); + let too_many = require_reference_access( + "org1", + &user, + "dataset-1", + ScorerSelection::Requested(&refs), + &task, + ) + .await + .unwrap_err(); + assert_eq!(too_many.status().as_u16(), 400); + + let at_limit = require_reference_access( + "org1", + &user, + "dataset-1", + ScorerSelection::Requested(&refs[..MAX_SCORERS_PER_REQUEST]), + &task, + ) + .await + .unwrap_err(); + assert_eq!(at_limit.status().as_u16(), 403); + } + + #[tokio::test] + async fn a_retry_rechecks_the_stored_experiment_s_references() { + let experiment = Experiment { + id: "experiment-1".to_string(), + org_id: "org1".to_string(), + name: "e".to_string(), + description: None, + dataset_id: "dataset-1".to_string(), + dataset_version: 1, + dataset_filter: None, + task: inline_task(), + scorers: vec![PinnedExperimentScorer { + id: "scorer-1".to_string(), + version: 1, + }], + trial_count: 1, + metadata: None, + status: experiments::ExperimentStatus::Failed, + status_reason: None, + deadline_at: 0, + completed_at: None, + lifecycle_version: 0, + retry_count: 0, + scores_settled_at: None, + idempotency_key: None, + is_baseline: false, + prompt_id: None, + prompt_name: None, + prompt_version: None, + prompt_content_hash: None, + created_by: "owner@example.com".to_string(), + created_at: 0, + }; + let denied = require_pinned_reference_access("org1", &unknown_user(), &experiment) + .await + .unwrap_err(); + assert_eq!(denied.status().as_u16(), 403); } #[test] diff --git a/src/api/management/src/request/playground.rs b/src/api/management/src/request/playground.rs index 33a6026e48..98f58b77f2 100644 --- a/src/api/management/src/request/playground.rs +++ b/src/api/management/src/request/playground.rs @@ -26,7 +26,7 @@ use db::authz::set_ownership; use futures::StreamExt; use openobserve_api_common::extractors::Headers; use openobserve_core::{ - auth::{UserEmail, is_ofga_object_visible}, + auth::{ReadCheck, UserEmail, UserReads, first_unreadable, is_ofga_object_visible}, llm_evaluations::{ experiments::runner::render_prompt, playground::{self, PlaygroundError}, @@ -46,6 +46,7 @@ use crate::{ PlaygroundScoreResponseBody, PlaygroundSnapshotResponseBody, SharePlaygroundSnapshotRequestBody, }, + request::scorers::{MAX_SCORERS_PER_REQUEST, judge_provider_ids}, }; /// FGA object type for everything under `/playground`. @@ -135,21 +136,15 @@ async fn require_provider_visibility( provider_id: &str, user_id: &str, ) -> Result<(), Response> { - match openobserve_api_common::auth::validator::list_objects_for_user( - org_id, user_id, "GET", "provider", - ) - .await + if (UserReads { org_id, user_id }) + .can_read("providers", provider_id) + .await { - Ok(permitted) => { - if is_ofga_object_visible(org_id, "provider", provider_id, permitted.as_deref()) { - Ok(()) - } else { - Err(MetaHttpResponse::forbidden( - "Not allowed to use this provider", - )) - } - } - Err(error) => Err(MetaHttpResponse::forbidden(error.to_string())), + Ok(()) + } else { + Err(MetaHttpResponse::forbidden( + "Not allowed to use this provider", + )) } } @@ -160,52 +155,38 @@ async fn require_scorer_visibility( scorer_ids: &[String], user_id: &str, ) -> Result<(), Response> { - let permitted = openobserve_api_common::auth::validator::list_objects_for_user( - org_id, user_id, "GET", "scorer", - ) - .await - .map_err(|error| MetaHttpResponse::forbidden(error.to_string()))?; - - if let Some(hidden_id) = scorer_ids.iter().find(|scorer_id| { - !is_ofga_object_visible(org_id, "scorer", scorer_id, permitted.as_deref()) - }) { - return Err(MetaHttpResponse::not_found(format!( - "Scorer '{hidden_id}' not found" + if scorer_ids.len() > MAX_SCORERS_PER_REQUEST { + return Err(MetaHttpResponse::bad_request(format!( + "At most {MAX_SCORERS_PER_REQUEST} scorers can be named in one request" ))); } - Ok(()) + let scorers: Vec<_> = scorer_ids + .iter() + .map(|id| ("scorers", id.as_str())) + .collect(); + match first_unreadable(&scorers, &UserReads { org_id, user_id }).await { + Some((_, hidden_id)) => Err(MetaHttpResponse::not_found(format!( + "Scorer '{hidden_id}' not found" + ))), + None => Ok(()), + } } -/// Confirm the caller may use every provider referenced by an LLM judge -/// scorer. The permission lookup is shared across the whole batch. +/// Confirm the caller may use every provider referenced by an LLM judge scorer. async fn require_scorer_provider_visibility( org_id: &str, scorers: &[infra::table::scorers::Scorer], user_id: &str, ) -> Result<(), Response> { - let provider_ids = scorers - .iter() - .filter(|scorer| scorer.scorer_type == infra::table::scorers::ScorerType::LlmJudge) - .filter_map(|scorer| scorer.params.get("provider_id").and_then(Value::as_str)) - .collect::>(); - if provider_ids.is_empty() { - return Ok(()); - } - - let permitted = openobserve_api_common::auth::validator::list_objects_for_user( - org_id, user_id, "GET", "provider", - ) - .await - .map_err(|error| MetaHttpResponse::forbidden(error.to_string()))?; - - if provider_ids.iter().any(|provider_id| { - !is_ofga_object_visible(org_id, "provider", provider_id, permitted.as_deref()) - }) { - return Err(MetaHttpResponse::forbidden( + let providers: Vec<_> = judge_provider_ids(scorers) + .map(|id| ("providers", id)) + .collect(); + match first_unreadable(&providers, &UserReads { org_id, user_id }).await { + Some(_) => Err(MetaHttpResponse::forbidden( "Not allowed to use a provider configured by this scorer", - )); + )), + None => Ok(()), } - Ok(()) } // --------------------------------------------------------------------------- @@ -291,11 +272,11 @@ fn sse(payload: &Value) -> bytes::Bytes { /// Resolve a provider that belongs to this organization. async fn load_provider(org_id: &str, provider_id: &str) -> Result { - let provider = match infra::table::providers::get(provider_id).await { - Ok(Some(provider)) if provider.org_id == org_id => provider, + let provider = match infra::table::providers::get(org_id, provider_id).await { + Ok(Some(provider)) => provider, // A provider in another organization is reported as absent rather than // forbidden, so its existence cannot be probed. - Ok(_) => return Err(MetaHttpResponse::not_found("Provider not found")), + Ok(None) => return Err(MetaHttpResponse::not_found("Provider not found")), Err(error) => { log::error!("[Playground] failed to load provider {provider_id}: {error}"); return Err(MetaHttpResponse::internal_error("Internal server error")); @@ -613,6 +594,7 @@ pub async fn run_playground_cell( request_body(content = inline(PlaygroundScoreRequestBody)), responses( (status = 200, body = inline(PlaygroundScoreResponseBody)), + (status = 400, description = "Too many scorers named", body = ()), (status = 403, description = "A scorer provider is not visible to the caller", body = ()), (status = 404, description = "A referenced scorer does not exist", body = ()), (status = 500, description = "Scorer resolution failed", body = ()), @@ -749,4 +731,48 @@ mod tests { ); assert_eq!(rendered[1].tool_call_id.as_deref(), Some("call_1")); } + + fn unknown_user() -> String { + format!("{}@example.com", config::ider::uuid()) + } + + #[tokio::test] + async fn providers_and_scorers_are_checked_per_object_whatever_list_filtering_says() { + let user = unknown_user(); + let denied = require_provider_visibility("org1", "provider-1", &user) + .await + .unwrap_err(); + assert_eq!(denied.status().as_u16(), 403); + + let hidden = require_scorer_visibility("org1", &["scorer-1".to_string()], &user) + .await + .unwrap_err(); + assert_eq!(hidden.status().as_u16(), 404); + + let body: crate::models::scorers::ScorerRequestBody = serde_json::from_value(json!({ + "name": "judge", + "scorer": {"type": "llm_judge", "template": "t", "params": {"provider_id": "provider-1"}} + })) + .unwrap(); + let denied = require_scorer_provider_visibility("org1", &[body.into()], &user) + .await + .unwrap_err(); + assert_eq!(denied.status().as_u16(), 403); + } + + #[tokio::test] + async fn naming_more_scorers_than_the_maximum_is_refused_before_any_check() { + let ids: Vec = (0..=MAX_SCORERS_PER_REQUEST) + .map(|i| format!("scorer-{i}")) + .collect(); + let resp = require_scorer_visibility("org1", &ids, &unknown_user()) + .await + .unwrap_err(); + assert_eq!(resp.status().as_u16(), 400); + let resp = + require_scorer_visibility("org1", &ids[..MAX_SCORERS_PER_REQUEST], &unknown_user()) + .await + .unwrap_err(); + assert_eq!(resp.status().as_u16(), 404); + } } diff --git a/src/api/management/src/request/providers/mod.rs b/src/api/management/src/request/providers/mod.rs index 72336e8ee5..8a1702fa81 100644 --- a/src/api/management/src/request/providers/mod.rs +++ b/src/api/management/src/request/providers/mod.rs @@ -17,7 +17,7 @@ use axum::{extract::Path, response::Response}; #[cfg(feature = "enterprise")] use openobserve_api_common::extractors::Headers; #[cfg(feature = "enterprise")] -use openobserve_core::auth::{UserEmail, is_ofga_object_visible}; +use openobserve_core::auth::{ReadCheck, UserEmail, UserReads}; use crate::{ common::meta::http::HttpResponse as MetaHttpResponse, @@ -282,15 +282,10 @@ async fn require_provider_visibility( provider_id: &str, user_id: &str, ) -> Result<(), Response> { - let permitted = openobserve_api_common::auth::validator::list_objects_for_user( - org_id, user_id, "GET", "provider", - ) - .await - .map_err(|error| { - log::warn!("[Provider] failed to load provider permissions: {error}"); - MetaHttpResponse::forbidden("Not allowed to use this provider") - })?; - if is_ofga_object_visible(org_id, "provider", provider_id, permitted.as_deref()) { + if (UserReads { org_id, user_id }) + .can_read("providers", provider_id) + .await + { Ok(()) } else { Err(MetaHttpResponse::forbidden( diff --git a/src/api/management/src/request/scorers/mod.rs b/src/api/management/src/request/scorers/mod.rs index d54ee12f1c..7c902388b6 100644 --- a/src/api/management/src/request/scorers/mod.rs +++ b/src/api/management/src/request/scorers/mod.rs @@ -21,7 +21,9 @@ use db::authz::{remove_ownership, set_ownership}; #[cfg(feature = "enterprise")] use openobserve_api_common::extractors::Headers; #[cfg(feature = "enterprise")] -use openobserve_core::auth::{UserEmail, is_ofga_object_visible}; +use openobserve_core::auth::{ + ReadCheck, UserEmail, UserReads, first_unreadable, is_ofga_object_visible, +}; use crate::{ common::meta::{authz::Authz, http::HttpResponse as MetaHttpResponse}, @@ -37,6 +39,9 @@ use crate::{ }, }; +/// Most scorers one request may name; each costs a permission check before anything runs. +pub(crate) const MAX_SCORERS_PER_REQUEST: usize = 50; + fn scorer_error_response(value: ScorerError) -> Response { match value { ScorerError::InfraError(err) => { @@ -45,6 +50,8 @@ fn scorer_error_response(value: ScorerError) -> Response { } ScorerError::MissingName => MetaHttpResponse::bad_request("Scorer name cannot be empty"), ScorerError::NotFound => MetaHttpResponse::not_found("Scorer not found"), + ScorerError::ProviderNotFound(_) => MetaHttpResponse::not_found(value), + ScorerError::ProviderNotReadable(_) => MetaHttpResponse::forbidden(value), ScorerError::InvalidScorerType(_) | ScorerError::ScorerTypeImmutable | ScorerError::ProducesScoreConfigIdImmutable @@ -131,6 +138,8 @@ pub async fn list_scorers( responses( (status = 200, body = inline(ScorerResponseBody)), (status = 400, description = "Bad Request", body = ()), + (status = 403, description = "Provider not accessible", body = ()), + (status = 404, description = "Provider not found", body = ()), (status = 409, description = "Conflict", body = ()), ), extensions( @@ -139,9 +148,14 @@ pub async fn list_scorers( )] pub async fn create_scorer( Path(org_id): Path, + #[cfg(feature = "enterprise")] Headers(user_email): Headers, axum::Json(body): axum::Json, ) -> Response { let scorer: infra::table::scorers::Scorer = body.into(); + #[cfg(feature = "enterprise")] + if let Err(err) = require_provider_read(&org_id, &user_email.user_id, &scorer).await { + return scorer_error_response(err); + } match scorers::save_scorer(&org_id, scorer).await { Ok(s) => { set_ownership(&org_id, "scorers", Authz::new(&s.entity_id)).await; @@ -232,7 +246,8 @@ pub async fn list_scorer_versions(Path((org_id, entity_id)): Path<(String, Strin request_body(content = inline(ScorerUpdateRequestBody), description = "Scorer payload"), responses( (status = 200, body = inline(ScorerResponseBody)), - (status = 404, description = "Not Found", body = ()), + (status = 403, description = "Provider not accessible", body = ()), + (status = 404, description = "Scorer or provider not found", body = ()), ), extensions( ("x-o2-ratelimit" = json!({"module": "Scorers", "operation": "update"})), @@ -240,9 +255,16 @@ pub async fn list_scorer_versions(Path((org_id, entity_id)): Path<(String, Strin )] pub async fn update_scorer( Path((org_id, entity_id)): Path<(String, String)>, + #[cfg(feature = "enterprise")] Headers(user_email): Headers, axum::Json(body): axum::Json, ) -> Response { let scorer: infra::table::scorers::Scorer = body.into(); + #[cfg(feature = "enterprise")] + if let Err(err) = + require_changed_provider_read(&org_id, &user_email.user_id, &entity_id, &scorer).await + { + return scorer_error_response(err); + } match scorers::update_scorer(&org_id, &entity_id, scorer).await { Ok(s) => { let resp: ScorerResponseBody = s.into(); @@ -300,6 +322,7 @@ pub async fn delete_scorer(Path((org_id, entity_id)): Path<(String, String)>) -> responses( (status = 200, body = inline(ScorerTestResponseBody)), (status = 400, description = "Bad Request", body = ()), + (status = 403, description = "Provider not accessible", body = ()), (status = 404, description = "Not Found", body = ()), ), extensions( @@ -308,9 +331,10 @@ pub async fn delete_scorer(Path((org_id, entity_id)): Path<(String, String)>) -> )] pub async fn test_scorer( Path(org_id): Path, + #[cfg(feature = "enterprise")] Headers(user_email): Headers, axum::Json(body): axum::Json, ) -> Response { - match run_scorer_test(&org_id, &body).await { + match run_scorer_test(&org_id, &user_email.user_id, &body).await { Ok(resp) => MetaHttpResponse::json(resp), Err(err) => scorer_error_response(err), } @@ -348,6 +372,76 @@ pub async fn preview_llm_judge_output_schema( } } +/// The provider each LLM-judge scorer calls. +pub(crate) fn judge_provider_ids( + scorers: &[infra::table::scorers::Scorer], +) -> impl Iterator { + scorers + .iter() + .filter(|scorer| scorer.scorer_type == infra::table::scorers::ScorerType::LlmJudge) + .filter_map(|scorer| scorer.params.get("provider_id")?.as_str()) + .map(str::trim) + .filter(|id| !id.is_empty()) +} + +/// Scorers are loaded only after every named object is readable, then their providers are checked. +#[cfg(feature = "enterprise")] +pub(crate) async fn authorize_references( + objects: &[(&'static str, &str)], + reads: &impl ReadCheck, + scorer_definitions: impl Future, Response>>, +) -> Result<(), Response> { + if first_unreadable(objects, reads).await.is_some() { + return Err(MetaHttpResponse::forbidden("Unauthorized Access")); + } + let definitions = scorer_definitions.await?; + let providers: Vec<_> = judge_provider_ids(&definitions) + .map(|id| ("providers", id)) + .collect(); + if first_unreadable(&providers, reads).await.is_some() { + return Err(MetaHttpResponse::forbidden("Unauthorized Access")); + } + Ok(()) +} + +#[cfg(feature = "enterprise")] +async fn require_provider_read( + org_id: &str, + user_id: &str, + scorer: &infra::table::scorers::Scorer, +) -> Result<(), ScorerError> { + let providers: Vec<_> = judge_provider_ids(std::slice::from_ref(scorer)) + .map(|id| ("providers", id)) + .collect(); + match first_unreadable(&providers, &UserReads { org_id, user_id }).await { + Some((_, id)) => Err(ScorerError::ProviderNotReadable(id.to_string())), + None => Ok(()), + } +} + +/// A new version that keeps its provider needs no read on it; rebinding does. +#[cfg(feature = "enterprise")] +async fn require_changed_provider_read( + org_id: &str, + user_id: &str, + entity_id: &str, + incoming: &infra::table::scorers::Scorer, +) -> Result<(), ScorerError> { + let existing = match scorers::get_scorer(org_id, entity_id).await { + Ok(existing) => Some(existing), + Err(ScorerError::NotFound) => None, + Err(err) => return Err(err), + }; + let unchanged = existing.as_ref().is_some_and(|existing| { + judge_provider_ids(std::slice::from_ref(existing)) + .eq(judge_provider_ids(std::slice::from_ref(incoming))) + }); + if unchanged { + return Ok(()); + } + require_provider_read(org_id, user_id, incoming).await +} + async fn derive_llm_judge_output_schema( org_id: &str, body: &LlmJudgeOutputSchemaRequestBody, @@ -442,11 +536,13 @@ async fn score_config_info_for_test_scorer( async fn run_scorer_test( org_id: &str, + user_id: &str, body: &ScorerTestRequestBody, ) -> Result { use std::collections::HashMap; let mut scorer = scorer_for_test(org_id, body)?; + require_provider_read(org_id, user_id, &scorer).await?; let score_config_info = score_config_info_for_test_scorer(org_id, &mut scorer).await?; let mut attrs: HashMap = HashMap::new(); @@ -460,6 +556,8 @@ async fn run_scorer_test( match &scorer.scorer_type { infra::table::scorers::ScorerType::LlmJudge => { + // Admins pass the read check, so this is what keeps another org's provider out. + scorers::validate_llm_judge_provider_model(org_id, &scorer).await?; let prepared = openobserve_core::llm_evaluations::prepared_scorers::PreparedLlmJudgeScorer::prepare_with_score_config_info( org_id, @@ -558,6 +656,8 @@ mod tests { (ScorerError::ProducesScoreConfigIdImmutable, 400), (ScorerError::InvalidOutputSchema("bad".to_string()), 400), (ScorerError::InvalidProviderConfig("bad".to_string()), 400), + (ScorerError::ProviderNotFound("p".to_string()), 404), + (ScorerError::ProviderNotReadable("p".to_string()), 403), (ScorerError::DuplicateName, 409), (ScorerError::InUseByEvalJob, 409), ]; @@ -625,4 +725,78 @@ mod tests { let err = scorer_for_test("org1", &body).unwrap_err(); assert!(matches!(err, ScorerError::MissingName)); } + + fn denied_user() -> Headers { + Headers(UserEmail { + user_id: format!("{}@example.com", config::ider::uuid()), + }) + } + + fn judge(provider_id: &str) -> serde_json::Value { + serde_json::json!({ + "type": "llm_judge", + "template": "Rate {{output}}", + "params": {"provider_id": provider_id} + }) + } + + #[tokio::test] + async fn testing_a_judge_on_an_unreadable_provider_is_refused() { + let body: ScorerTestRequestBody = serde_json::from_value(serde_json::json!({ + "name": "probe", + "scorer": judge("provider-1"), + "inputVariables": {"output": "hello"} + })) + .unwrap(); + let resp = test_scorer(Path("org1".to_string()), denied_user(), axum::Json(body)).await; + assert_eq!(resp.status().as_u16(), 403); + } + + #[tokio::test] + async fn creating_a_judge_on_an_unreadable_provider_is_refused() { + let body: ScorerRequestBody = serde_json::from_value( + serde_json::json!({"name": "judge", "scorer": judge("provider-1")}), + ) + .unwrap(); + let resp = create_scorer(Path("org1".to_string()), denied_user(), axum::Json(body)).await; + assert_eq!(resp.status().as_u16(), 403); + } + + #[tokio::test] + async fn rebinding_a_judge_needs_read_on_the_new_provider_but_keeping_it_does_not() { + infra::table::scorers::create_table().await.unwrap(); + let org_id = format!("rebind_{}", config::ider::uuid()); + let body: ScorerRequestBody = serde_json::from_value( + serde_json::json!({"name": "judge", "scorer": judge("provider-a")}), + ) + .unwrap(); + let mut existing: infra::table::scorers::Scorer = body.into(); + existing.id = config::ider::uuid(); + existing.entity_id = config::ider::uuid(); + existing.org_id = org_id.clone(); + existing.version = 1; + existing.is_active = true; + infra::table::scorers::add(&existing).await.unwrap(); + + let user = denied_user().0.user_id; + let kept = existing.clone(); + assert!( + require_changed_provider_read(&org_id, &user, &existing.entity_id, &kept) + .await + .is_ok() + ); + + let rebound: ScorerUpdateRequestBody = + serde_json::from_value(serde_json::json!({"scorer": judge("provider-b")})).unwrap(); + let resp = update_scorer( + Path((org_id.clone(), existing.entity_id.clone())), + denied_user(), + axum::Json(rebound), + ) + .await; + assert_eq!(resp.status().as_u16(), 403); + infra::table::scorers::delete_all_by_org(&org_id) + .await + .unwrap(); + } } diff --git a/src/api/search/src/promql/mod.rs b/src/api/search/src/promql/mod.rs index fa5a365cf5..ebc82e67a9 100644 --- a/src/api/search/src/promql/mod.rs +++ b/src/api/search/src/promql/mod.rs @@ -60,6 +60,7 @@ use crate::{ request_body(content = String, description = "prometheus WriteRequest", content_type = "application/x-protobuf"), responses( (status = 200, description = "Success", content_type = "application/json", body = Object, example = json!({"code": 200})), + (status = 400, description = "Not a protobuf body or the payload cannot be ingested", content_type = "application/json", body = ()), (status = 500, description = "Failure", content_type = "application/json", body = ()), ), extensions( @@ -132,6 +133,9 @@ pub async fn remote_write( ] } })), + (status = 400, description = "Invalid query or time", content_type = "application/json", body = ()), + (status = 403, description = "No permission on the metric stream", content_type = "application/json", body = ()), + (status = 429, description = "Search disabled after exceeding the licensed ingestion limit, or rate limited", content_type = "application/json", body = ()), (status = 500, description = "Failure", content_type = "application/json", body = ()), ), extensions( @@ -304,6 +308,9 @@ async fn query( ] } })), + (status = 400, description = "Invalid query, time range, step or search type", content_type = "application/json", body = ()), + (status = 403, description = "No permission on the metric stream", content_type = "application/json", body = ()), + (status = 429, description = "Search disabled after exceeding the licensed ingestion limit, or rate limited", content_type = "application/json", body = ()), (status = 500, description = "Failure", content_type = "application/json", body = ()), ), extensions( @@ -398,6 +405,9 @@ pub async fn query_range_post( } ] })), + (status = 400, description = "Invalid query, time range, step or search type", content_type = "application/json", body = ()), + (status = 403, description = "No permission on the metric stream", content_type = "application/json", body = ()), + (status = 429, description = "Search disabled after exceeding the licensed ingestion limit, or rate limited", content_type = "application/json", body = ()), (status = 500, description = "Failure", content_type = "application/json", body = ()), ), extensions( @@ -626,6 +636,7 @@ async fn query_range( ] } })), + (status = 403, description = "No permission to read metrics", content_type = "application/json", body = ()), (status = 500, description = "Failure", content_type = "application/json", body = ()), ), extensions( @@ -750,6 +761,9 @@ pub async fn metadata( } ] })), + (status = 400, description = "Invalid match[] or time range", content_type = "application/json", body = ()), + (status = 403, description = "No permission on the metric stream", content_type = "application/json", body = ()), + (status = 429, description = "Search disabled after exceeding the licensed ingestion limit, or rate limited", content_type = "application/json", body = ()), (status = 500, description = "Failure", content_type = "application/json", body = ()), ), extensions( @@ -883,6 +897,8 @@ async fn series( "version" ] })), + (status = 400, description = "Invalid match[] or time range", content_type = "application/json", body = ()), + (status = 403, description = "No permission to read metrics", content_type = "application/json", body = ()), (status = 500, description = "Failure", content_type = "application/json", body = ()), ), extensions( @@ -1024,6 +1040,7 @@ async fn labels( ] })), (status = 400, description = "Invalid parameters or match[] does not identify a metric", content_type = "application/json", body = ()), + (status = 403, description = "No permission to read metrics", content_type = "application/json", body = ()), (status = 500, description = "Failure", content_type = "application/json", body = ()), ), extensions( @@ -1246,6 +1263,7 @@ fn validate_range_window(start: i64, end: i64) -> Result<(), String> { "status" : "success", "data" : "foo / bar" })), + (status = 400, description = "The query does not parse", content_type = "application/json", body = ()), (status = 500, description = "Failure", content_type = "application/json", body = ()), ), extensions( diff --git a/src/compaction/src/dump.rs b/src/compaction/src/dump.rs index 1178c751ae..ac183c1bc6 100644 --- a/src/compaction/src/dump.rs +++ b/src/compaction/src/dump.rs @@ -206,6 +206,15 @@ pub async fn dump(job: &DumpJob) -> Result<(), anyhow::Error> { job.offset ); + let dump_stream_name = generate_dump_stream_name(job.stream_type, &job.stream_name); + // the dump key outgrows the source's by 10, so a source near the limit cannot be dumped + if infra_file_list::stream_key_violation(&job.org_id, StreamType::Filelist, &dump_stream_name) + .is_some() + { + infra_file_list::set_job_dumped_status(&[job.job_id], true).await?; + return Ok(()); + } + // check offset let partition_time_level = get_partition_time_level(job.stream_type); let offset_time: DateTime = Utc.timestamp_nanos(job.offset * 1000); @@ -259,7 +268,6 @@ pub async fn dump(job: &DumpJob) -> Result<(), anyhow::Error> { } // check if the schema need to be updated - let dump_stream_name = generate_dump_stream_name(job.stream_type, &job.stream_name); let dump_stream_key = format!( "{}/{}/{}", job.org_id, @@ -820,6 +828,23 @@ mod tests { use super::*; + #[test] + fn test_dump_key_of_a_source_at_the_limit_cannot_be_stored() { + let org_id = "org"; + let source = + "s".repeat(infra::file_list::STREAM_KEY_MAX_LEN - org_id.len() - "/logs/".len()); + assert!(infra_file_list::stream_key_violation(org_id, StreamType::Logs, &source).is_none()); + let bounded = + infra_file_list::stream_key_violation(org_id, StreamType::Logs, &format!("{source}s")) + .is_some(); + let dump_stream = generate_dump_stream_name(StreamType::Logs, &source); + assert_eq!( + infra_file_list::stream_key_violation(org_id, StreamType::Filelist, &dump_stream) + .is_some(), + bounded + ); + } + // ---- DumpJob ---- #[test] diff --git a/src/compaction/src/lib.rs b/src/compaction/src/lib.rs index a8b4634023..0d7ad6f26f 100644 --- a/src/compaction/src/lib.rs +++ b/src/compaction/src/lib.rs @@ -152,6 +152,12 @@ pub async fn run_generate_job(job_type: CompactionJobType) -> Result<(), anyhow: ); continue; } + // a stream created before over-long keys were rejected can never record a job + if infra_file_list::stream_key_violation(&org_id, stream_type, &stream_name) + .is_some() + { + continue; + } match job_type { CompactionJobType::Current => { @@ -245,6 +251,11 @@ pub async fn run_generate_downsampling_job() -> Result<(), anyhow::Error> { ); continue; } + if infra_file_list::stream_key_violation(&org_id, stream_type, &stream_name) + .is_some() + { + continue; + } if let Err(e) = merge::generate_downsampling_job_by_stream_and_rule( &org_id, diff --git a/src/config/src/meta/stream.rs b/src/config/src/meta/stream.rs index ebcc506d93..3e76fdaf03 100644 --- a/src/config/src/meta/stream.rs +++ b/src/config/src/meta/stream.rs @@ -149,7 +149,11 @@ impl StreamType { pub fn support_index(&self) -> bool { matches!( *self, - StreamType::Logs | StreamType::Metrics | StreamType::Traces | StreamType::Metadata + StreamType::Logs + | StreamType::Metrics + | StreamType::Traces + | StreamType::Profiles + | StreamType::Metadata ) } @@ -1716,7 +1720,7 @@ mod tests { assert!(StreamType::Logs.support_index()); assert!(StreamType::Metrics.support_index()); assert!(StreamType::Traces.support_index()); - assert!(!StreamType::Profiles.support_index()); + assert!(StreamType::Profiles.support_index()); assert!(StreamType::Metadata.support_index()); assert!(!StreamType::EnrichmentTables.support_index()); assert!(!StreamType::Filelist.support_index()); diff --git a/src/config/src/utils/inverted_index.rs b/src/config/src/utils/inverted_index.rs index 701e3f62df..14271e4944 100644 --- a/src/config/src/utils/inverted_index.rs +++ b/src/config/src/utils/inverted_index.rs @@ -35,6 +35,7 @@ pub fn to_tantivy_name(from: &str) -> Option { "logs" => StreamType::Logs, "metrics" => StreamType::Metrics, "traces" => StreamType::Traces, + "profiles" => StreamType::Profiles, "metadata" => StreamType::Metadata, _ => return None, }; @@ -87,6 +88,13 @@ mod tests { .to_string(), ), ), + ( + "files/default/profiles/quickstart1/2024/02/16/16/7164299619311026293.parquet", + Some( + "files/default/index/quickstart1_profiles/2024/02/16/16/7164299619311026293.ttv" + .to_string(), + ), + ), ( "files/default/metadata/quickstart1/2024/02/16/16/7164299619311026293.parquet", Some( @@ -129,8 +137,7 @@ mod tests { #[test] fn test_convert_returns_none_for_unknown_stream_type() { - // service_graph / enrichment_tables / file_list are not handled by this - // converter — only logs/metrics/traces/metadata map to a tantivy index. + // only logs/metrics/traces/profiles/metadata map to a tantivy index let input = "files/default/service_graph/x/2024/02/16/16/1.parquet"; assert_eq!(to_tantivy_name(input), None); diff --git a/src/core/src/auth.rs b/src/core/src/auth.rs index ac4f5e5b71..3193b1646f 100644 --- a/src/core/src/auth.rs +++ b/src/core/src/auth.rs @@ -224,6 +224,37 @@ pub struct UserEmail { pub user_id: String, } +/// A per-object GET check, taken as a trait so tests can stand in for OpenFGA. +#[cfg(feature = "enterprise")] +pub trait ReadCheck { + fn can_read(&self, resource: &str, object_id: &str) -> impl Future + Send; +} + +/// One user's reads in one org, checked as the `GET /{org}/{resource}/{id}` routes check them. +#[cfg(feature = "enterprise")] +pub struct UserReads<'a> { + pub org_id: &'a str, + pub user_id: &'a str, +} + +#[cfg(feature = "enterprise")] +impl ReadCheck for UserReads<'_> { + async fn can_read(&self, resource: &str, object_id: &str) -> bool { + check_permissions( + object_id, + self.org_id, + self.user_id, + resource, + "GET", + None, + false, + false, + true, + ) + .await + } +} + #[derive(Debug, PartialEq, Eq, Clone)] pub struct AuthExtractor { pub auth: String, @@ -771,6 +802,22 @@ pub async fn check_permissions( true } +/// The first object `reads` refuses, asking once per distinct trimmed id. +#[cfg(feature = "enterprise")] +pub async fn first_unreadable<'a>( + objects: &[(&'static str, &'a str)], + reads: &impl ReadCheck, +) -> Option<(&'static str, &'a str)> { + let mut asked = std::collections::HashSet::new(); + for &(resource, id) in objects { + let id = id.trim(); + if asked.insert((resource, id)) && !reads.can_read(resource, id).await { + return Some((resource, id)); + } + } + None +} + // OSS has no folder boundary to cross, so this allows where the `check_permissions` // stub above denies: there is no folder ACL to consult, not a failed lookup. #[cfg(not(feature = "enterprise"))] diff --git a/src/core/src/llm_evaluations/eval_jobs/mod.rs b/src/core/src/llm_evaluations/eval_jobs/mod.rs index 174ceefe3e..79dbbbd18f 100644 --- a/src/core/src/llm_evaluations/eval_jobs/mod.rs +++ b/src/core/src/llm_evaluations/eval_jobs/mod.rs @@ -167,6 +167,18 @@ async fn validate_online_scorers( org_id: &str, scorer_refs: &[config::meta::pipeline::components::ScorerRef], ) -> Result<(), EvalJobError> { + for scorer in scorer_definitions(org_id, scorer_refs).await? { + ensure_scorer_is_online_compatible(&scorer)?; + } + Ok(()) +} + +/// The scorer versions a job's references resolve to, in reference order. +pub async fn scorer_definitions( + org_id: &str, + scorer_refs: &[config::meta::pipeline::components::ScorerRef], +) -> Result, EvalJobError> { + let mut definitions = Vec::with_capacity(scorer_refs.len()); for scorer_ref in scorer_refs { let scorer = match scorer_ref.version { Some(version) => { @@ -178,9 +190,9 @@ async fn validate_online_scorers( .ok_or_else(|| { EvalJobError::InvalidJob(format!("Scorer '{}' was not found", scorer_ref.id)) })?; - ensure_scorer_is_online_compatible(&scorer)?; + definitions.push(scorer); } - Ok(()) + Ok(definitions) } #[tracing::instrument(skip(job))] diff --git a/src/core/src/providers.rs b/src/core/src/providers.rs index 1e5e76a03c..3d9f7c9b25 100644 --- a/src/core/src/providers.rs +++ b/src/core/src/providers.rs @@ -137,12 +137,9 @@ pub async fn update_provider( return Err(ProviderError::MissingName); } - let existing = table::providers::get(provider_id) + let existing = table::providers::get(org_id, provider_id) .await? .ok_or(ProviderError::NotFound)?; - if existing.org_id != org_id { - return Err(ProviderError::NotFound); - } provider.auth_config = resolve_updated_auth_config(&provider.auth_config, &existing.auth_config); @@ -190,23 +187,16 @@ pub async fn get_provider( org_id: &str, provider_id: &str, ) -> Result { - let provider = table::providers::get(provider_id) + table::providers::get(org_id, provider_id) .await? - .ok_or(ProviderError::NotFound)?; - if provider.org_id != org_id { - return Err(ProviderError::NotFound); - } - Ok(provider) + .ok_or(ProviderError::NotFound) } #[tracing::instrument()] pub async fn delete_provider(org_id: &str, provider_id: &str) -> Result<(), ProviderError> { - let provider = table::providers::get(provider_id) + table::providers::get(org_id, provider_id) .await? .ok_or(ProviderError::NotFound)?; - if provider.org_id != org_id { - return Err(ProviderError::NotFound); - } let dependent_scorers: Vec<_> = table::scorers::get_all_by_org(org_id) .await? diff --git a/src/infra/src/table/providers.rs b/src/infra/src/table/providers.rs index e611c8631a..4bef6a6006 100644 --- a/src/infra/src/table/providers.rs +++ b/src/infra/src/table/providers.rs @@ -174,10 +174,15 @@ async fn provider_active_model(provider: &Provider) -> Result Result, errors::Error> { +/// A provider in another organization is reported as absent. +pub async fn get(org_id: &str, id: &str) -> Result, errors::Error> { let client = get_orm_client_ro().await; - let model = Entity::find().filter(Column::Id.eq(id)).one(client).await?; + let model = Entity::find() + .filter(Column::OrgId.eq(org_id)) + .filter(Column::Id.eq(id)) + .one(client) + .await?; match model { Some(m) => { diff --git a/tests/ui-testing/pages/alertsPages/alertLibraryPage.js b/tests/ui-testing/pages/alertsPages/alertLibraryPage.js index 9de398e12d..af1260e9a4 100644 --- a/tests/ui-testing/pages/alertsPages/alertLibraryPage.js +++ b/tests/ui-testing/pages/alertsPages/alertLibraryPage.js @@ -1,16 +1,6 @@ -/** - * AlertLibraryPage - Page object for the Alert Library (browse / filter / preview / install). - * - * Covers the gallery page, the left rail (severity + category facets), the - * readiness stat strip, cross-surface selection, the detail drawer and the - * 5-step install wizard. Selectors are the feature's own data-test attributes. - * - * Reuses: OSelect dropdown helper (reka trigger gotcha) and the O2 OInput - * `-field` inner-input gotcha for text/number inputs. - */ +// Selectors and expects for the Alert Library page; specs never inline them. import { expect } from '@playwright/test'; -import { openOSelectDropdown } from './oselectHelpers.js'; const testLogger = require('../../playwright-tests/utils/test-logger.js'); const { getAuthHeaders, getOrgIdentifier } = require('../../playwright-tests/utils/cloud-auth.js'); @@ -55,26 +45,21 @@ export class AlertLibraryPage { drawerCustomize: '[data-test="alert-library-drawer-customize"]', // install wizard dialog: '[data-test="alert-library-install-dialog"]', - destinationStep: '[data-test="alert-library-install-destination-step"]', - destination: '[data-test="alert-library-install-destination"]', - destinationsEmpty: '[data-test="alert-library-install-destinations-empty"]', - destinationsFailed: '[data-test="alert-library-install-destinations-failed"]', - destinationsRetry: '[data-test="alert-library-install-destinations-retry"]', - openDestinations: '[data-test="alert-library-install-open-destinations"]', - installSelectAll: '[data-test="alert-library-install-select-all"]', - installClear: '[data-test="alert-library-install-clear"]', - installCount: '[data-test="alert-library-install-count"]', + folderStep: '[data-test="alert-library-install-folder-step"]', + tuneStep: '[data-test="alert-library-install-tune-step"]', + reviewStep: '[data-test="alert-library-install-review-step"]', tuneToggle: '[data-test="alert-library-install-tune-toggle"]', tuneFrequency: '[data-test="alert-library-install-tune-frequency"]', tuneSilence: '[data-test="alert-library-install-tune-silence"]', summary: '[data-test="alert-library-install-summary"]', confirmLarge: '[data-test="alert-library-install-confirm-large"]', next: '[data-test="alert-library-install-next"]', - back: '[data-test="alert-library-install-back"]', run: '[data-test="alert-library-install-run"]', retry: '[data-test="alert-library-install-retry"]', done: '[data-test="alert-library-install-done"]', cancel: '[data-test="alert-library-install-cancel"]', + // alert editor (the Customize hand-off target) + editorAlertName: '[data-test="add-alert-name-input-value"]', }; } @@ -143,6 +128,8 @@ export class AlertLibraryPage { // ── cards / selection ───────────────────────────────────────────────────── card(id) { return this.page.locator(`[data-test="alert-library-card-${id}"]`); } cardTitle(id) { return this.page.locator(`[data-test="alert-library-card-title-${id}"]`); } + cardAdd(id) { return this.page.locator(`[data-test="alert-library-card-add-${id}"]`); } + cardCustomize(id) { return this.page.locator(`[data-test="alert-library-card-customize-${id}"]`); } needsDataChip(id) { return this.card(id).locator('[data-test="alert-library-card-needs-data"]'); } @@ -154,6 +141,13 @@ export class AlertLibraryPage { return dt.replace('alert-library-card-title-', ''); } + async addFromCard(id) { + await this.cardAdd(id).click(); + await expect(this.page.locator(this.l.dialog)).toBeVisible({ timeout: 15000 }); + } + + async customizeFromCard(id) { await this.cardCustomize(id).click(); } + async selectCard(id) { await this.page.locator(`[data-test="alert-library-select-${id}"]`).click(); } @@ -211,28 +205,7 @@ export class AlertLibraryPage { } // ── install wizard ──────────────────────────────────────────────────────── - async pickDestination(name) { - const root = this.page.locator(this.l.destination); - await root.waitFor({ state: 'visible', timeout: 10000 }); - await openOSelectDropdown(this.page, root); - await this.page.waitForTimeout(500); - // Options are virtualized; narrow via the popover search box (the -search - // data-test IS the input) so the target row is actually rendered. - const search = this.page.locator('[data-test="alert-library-install-destination-search"]'); - if (await search.isVisible({ timeout: 2000 }).catch(() => false)) { - await search.fill(name, { force: true }); - await this.page.waitForTimeout(400); - } - const options = this.page.locator('[data-test$="-popover"] [data-test$="-option"]'); - await expect(options.first()).toBeVisible({ timeout: 8000 }); - // Fail loudly if the named destination is absent — never silently pick another. - const match = options.filter({ hasText: name }).first(); - await expect(match, `destination "${name}" not in the wizard's list`).toBeVisible({ timeout: 8000 }); - await match.click(); - } - async next() { await this.page.locator(this.l.next).click(); } - async back() { await this.page.locator(this.l.back).click(); } async enableTune() { const toggle = this.page.locator(this.l.tuneToggle); @@ -249,11 +222,6 @@ export class AlertLibraryPage { async run() { await this.page.locator(this.l.run).click(); } async retryFailed() { await this.page.locator(this.l.retry).click(); } - async clearInDialog() { await this.page.locator(this.l.installClear).click(); } - async selectAllInDialog() { await this.page.locator(this.l.installSelectAll).click(); } - async toggleAlertInDialog(id) { - await this.page.locator(`[data-test="alert-library-install-alert-${id}"]`).click(); - } async waitResult(id, status = 'installed', timeout = 30000) { await expect(this.page.locator(`[data-test="alert-library-install-result-${id}"]`)) @@ -303,11 +271,33 @@ export class AlertLibraryPage { await expect(this.page.locator(this.l.drawer)).toBeHidden(); } - async expectSummaryDestination(text) { - await expect(this.page.locator(this.l.summary).locator('dd').first()).toHaveText(text); + async expectEditorAlertName(name) { + await expect(this.page.locator(this.l.editorAlertName).first()).toHaveText(name, { timeout: 15000 }); + } + + async expectReadyFacetActive() { + await expect(this.page.locator(this.l.statReady)).toHaveAttribute('aria-pressed', 'true'); + } + async expectCardActions(id, { customize = true } = {}) { + await expect(this.cardAdd(id)).toBeVisible(); + await expect(this.cardCustomize(id)).toHaveCount(customize ? 1 : 0); + } + async expectFolderStep() { await expect(this.page.locator(this.l.folderStep)).toBeVisible(); } + async expectTuneStep() { await expect(this.page.locator(this.l.tuneStep)).toBeVisible(); } + async expectReviewStep() { await expect(this.page.locator(this.l.reviewStep)).toBeVisible(); } + async expectNoDestinationAsked() { + const dialog = this.page.locator(this.l.dialog); + for (const removed of [ + 'alert-library-install-destination-step', + 'alert-library-install-destination', + 'alert-library-install-alerts-step', + 'alert-library-install-count', + 'alert-library-install-select-all', + ]) { + await expect(dialog.locator(`[data-test="${removed}"]`)).toHaveCount(0); + } + await expect(dialog.locator('[data-test^="alert-library-install-alert-"]')).toHaveCount(0); } - async expectNextDisabled() { await expect(this.page.locator(this.l.next)).toBeDisabled(); } - async expectNextEnabled() { await expect(this.page.locator(this.l.next)).toBeEnabled(); } async expectRunDisabled() { await expect(this.page.locator(this.l.run)).toBeDisabled(); } async expectRunEnabled() { await expect(this.page.locator(this.l.run)).toBeEnabled(); } async expectLargeConfirmVisible() { await expect(this.page.locator(this.l.confirmLarge)).toBeVisible(); } @@ -330,6 +320,9 @@ export class AlertLibraryPage { async expectRailCategoryVisible(id) { await expect(this.page.locator(`[data-test="alert-library-rail-category-${id}"]`)).toBeVisible(); } + async expectRailPackHeading(pack) { + await expect(this.page.locator(`[data-test="alert-library-rail-pack-heading-${pack}"]`)).toBeVisible(); + } async expectRailCategoryAbsent(id) { await expect(this.page.locator(`[data-test="alert-library-rail-category-${id}"]`)).toHaveCount(0); } @@ -351,12 +344,6 @@ export class AlertLibraryPage { } async expectRetryVisible() { await expect(this.page.locator(this.l.retry)).toBeVisible(); } - async expectDestinationsFailed() { await expect(this.page.locator(this.l.destinationsFailed)).toBeVisible(); } - async expectDestinationsRetryVisible() { await expect(this.page.locator(this.l.destinationsRetry)).toBeVisible(); } - async clickDestinationsRetry() { await this.page.locator(this.l.destinationsRetry).click(); } - async expectDestinationsEmpty() { await expect(this.page.locator(this.l.destinationsEmpty)).toBeVisible(); } - async expectOpenDestinationsVisible() { await expect(this.page.locator(this.l.openDestinations)).toBeVisible(); } - // ── internals ───────────────────────────────────────────────────────────── async _fillField(rootSelector, value) { // O2 OInput: the data-test is on a wrapper; the real input is the inner node. diff --git a/tests/ui-testing/playwright-tests/Alerts/alert-library.spec.js b/tests/ui-testing/playwright-tests/Alerts/alert-library.spec.js index 5ab6d2b203..c62c56bf14 100644 --- a/tests/ui-testing/playwright-tests/Alerts/alert-library.spec.js +++ b/tests/ui-testing/playwright-tests/Alerts/alert-library.spec.js @@ -21,8 +21,6 @@ test.describe('Alert Library', () => { let lib; let readyStream; let missingStream; - let templateName; - let destinationName; let entries; test.beforeEach(async ({ page }, testInfo) => { @@ -33,20 +31,11 @@ test.describe('Alert Library', () => { const rand = pm.alertsPage.generateRandomString().toLowerCase(); readyStream = `pw_lib_ready_${rand}`.toLowerCase(); missingStream = `pw_lib_absent_${rand}`.toLowerCase(); // never created → "missing" - templateName = `pw_lib_tmpl_${rand}`; - destinationName = `pw_lib_dest_${rand}`; // A stream with data → its cards read "ready" / fresh. await pm.commonActions.initializeAlertTestStream(readyStream); await pm.commonActions.queryStream(readyStream).catch(() => {}); // let it become searchable - // The install picks a real destination (template first). example.com avoids - // the SSRF guard that blocks self-host URLs. - await pm.alertTemplatesPage.ensureTemplateExists(templateName); - await pm.alertDestinationsPage.ensureDestinationExists( - destinationName, 'http://example.com/webhook', templateName, - ); - // All installable names share the pw_lib_ prefix so cleanup.spec.js sweeps // the alerts they create (see apiCleanup alertPrefixes). const ready = makeEntry(1, { @@ -93,9 +82,13 @@ test.describe('Alert Library', () => { const ready = entries[0]; const missing = entries[1]; - // Gallery renders from the mocked manifest. + // Gallery renders from the mocked manifest, opening on "Ready to use". await lib.openViaUrl(); await lib.expectGalleryVisible(); + await lib.expectReadyFacetActive(); + await lib.expectCardVisible(ready.id); + await lib.expectCardAbsent(missing.id); + await lib.filterAll(); // Readiness: missing card shows the "not ingested" chip, ready card doesn't. await lib.expectCardNeedsData(missing.id); @@ -115,18 +108,18 @@ test.describe('Alert Library', () => { await lib.openCard(ready.id); await lib.expectDrawerPreviewVisible(); - // Install wizard: destination → alerts → folder(default) → tune → run. + // Install wizard: folder(default) → tune → add. No destination, no checklist. await lib.installFromDrawer(); - await lib.expectNextEnabled(); // F2: the destination is optional - await lib.pickDestination(destinationName); - await lib.expectNextEnabled(); - await lib.next(); // → alerts - await lib.next(); // → folder + await lib.expectFolderStep(); + await lib.expectNoDestinationAsked(); await lib.next(); // → tune + await lib.expectTuneStep(); await lib.enableTune(); await lib.setFrequency(15); await lib.setSilence(20); - await lib.next(); // → review + await lib.next(); // → add + await lib.expectReviewStep(); + await lib.expectNoDestinationAsked(); await lib.run(); await lib.waitResult(ready.id, 'installed'); await lib.done(); @@ -134,8 +127,8 @@ test.describe('Alert Library', () => { // Verify via API the transformations the UI never shows. const created = await lib.getInstalledAlert(ready.name); expect(created, 'installed alert should be readable via API').toBeTruthy(); - // I2 destination overridden (author's o2_to_slack replaced) - expect(created.destinations).toContain(destinationName); + // I2 the author's destinations (o2_to_slack) are cleared, none added + expect(created.destinations).toEqual([]); // I3 provenance stamped expect(JSON.stringify(created.tags || [])).toContain(`pack:${ready.pack}`); expect(created.context_attributes?.library_id).toBe(ready.id); @@ -149,17 +142,15 @@ test.describe('Alert Library', () => { expect(created.trigger_condition?.silence).toBe(20); }); - test('install with no destination adds the alert without one', async () => { + test('a card adds its alert directly, through Folder, Tune and Add', async () => { const ready = entries[0]; await lib.openViaUrl(); - await lib.openCard(ready.id); - await lib.installFromDrawer(); - await lib.next(); // → alerts, with no destination picked - await lib.next(); // → folder + await lib.expectCardActions(ready.id); + await lib.addFromCard(ready.id); + await lib.expectFolderStep(); await lib.next(); // → tune - await lib.next(); // → review - await lib.expectSummaryDestination('None'); + await lib.next(); // → add await lib.run(); await lib.waitResult(ready.id, 'installed'); await lib.done(); @@ -171,35 +162,21 @@ test.describe('Alert Library', () => { test('bulk select via select-all-in-view triggers the >50 large-batch guard', async () => { await lib.openViaUrl(); + await lib.filterAll(); // Select every card in view (1 ready + 1 missing + 51 bulk + 1 malformed = 54 > 50). await lib.selectAllInViewToggle(); expect(await lib.selectedCountInBar()).toBeGreaterThan(50); await lib.addSelected(); - await lib.pickDestination(destinationName); - await lib.next(); // alerts - await lib.next(); // folder await lib.next(); // tune - await lib.next(); // review + await lib.next(); // add // Guard: confirm checkbox present, Run disabled until it is ticked. await lib.expectLargeConfirmVisible(); await lib.expectRunDisabled(); await lib.confirmLargeBatch(); await lib.expectRunEnabled(); - - // H5: the confirmation auto-resets when the run's shape changes (here, the - // alert selection), so it always describes the run about to happen. - await lib.back(); // → tune - await lib.back(); // → folder - await lib.back(); // → alerts - await lib.clearInDialog(); - await lib.selectAllInDialog(); - await lib.next(); // folder - await lib.next(); // tune - await lib.next(); // review - await lib.expectRunDisabled(); }); test('readiness surfaces on the card and in the drawer (fresh vs missing)', async () => { @@ -207,6 +184,7 @@ test.describe('Alert Library', () => { const missing = entries[1]; await lib.openViaUrl(); + await lib.filterAll(); // Card: missing → "not ingested" chip; fresh → none. await lib.expectCardNeedsData(missing.id); @@ -306,12 +284,32 @@ test.describe('Alert Library', () => { await expect(page).toHaveURL(/prefill=library/, { timeout: 15000 }); }); + test('Customize on a card opens the editor without opening the drawer', async ({ page }) => { + const ready = entries[0]; + + await lib.openViaUrl(); + await lib.customizeFromCard(ready.id); + await expect(page).toHaveURL(/prefill=library/, { timeout: 15000 }); + await lib.expectEditorAlertName(ready.name); + }); + + test('Customize on a card whose stream was never ingested still prefills the editor', async ({ page }) => { + const missing = entries[1]; + + await lib.openViaUrl(); + await lib.filterAll(); + await lib.customizeFromCard(missing.id); + await expect(page).toHaveURL(/prefill=library/, { timeout: 15000 }); + await lib.expectEditorAlertName(missing.name); + }); + test('the rail filters the gallery by severity and by category', async () => { const ready = entries[0]; // critical, category ready-signals const missing = entries[1]; // info, category absent-signals const bulk = entries[2]; // warning, category bulk-signals await lib.openViaUrl(); + await lib.filterAll(); // Severity is single-select: critical → only the critical card. await lib.selectSeverity('critical'); @@ -326,6 +324,14 @@ test.describe('Alert Library', () => { await lib.expectRailCategoryAbsent('absent-signals'); await lib.searchCategories(''); + // Categories sit under their pack heading, and a pack name finds all of its categories. + await lib.expectRailPackHeading('infrastructure'); + await lib.expectRailPackHeading('observability'); + await lib.searchCategories('infrastructure'); + await lib.expectRailCategoryVisible('bulk-signals'); + await lib.expectRailCategoryAbsent('absent-signals'); + await lib.searchCategories(''); + // Category multi-select filters the gallery; clear resets it. await lib.toggleCategory('bulk-signals'); await lib.expectCardVisible(bulk.id); @@ -346,17 +352,8 @@ test.describe('Alert Library', () => { expect(await lib.selectedCountInBar()).toBe(picks.length); await lib.addSelected(); - await lib.pickDestination(destinationName); - await lib.next(); // alerts - // F5: clearing the selection blocks advancing until at least one is re-picked - // (re-check the three specific rows — select-all would pull the whole gallery). - await lib.clearInDialog(); - await lib.expectNextDisabled(); - for (const e of picks) await lib.toggleAlertInDialog(e.id); - await lib.expectNextEnabled(); - await lib.next(); // folder await lib.next(); // tune - await lib.next(); // review + await lib.next(); // add await lib.expectLargeConfirmAbsent(); // <= 50, so no confirm required await lib.run(); for (const e of picks) await lib.waitResult(e.id, 'installed'); @@ -365,7 +362,7 @@ test.describe('Alert Library', () => { // Spot-check one installed alert via API. const created = await lib.getInstalledAlert(picks[0].name); expect(created, 'a bulk-installed alert should be readable via API').toBeTruthy(); - expect(created.destinations).toContain(destinationName); + expect(created.destinations).toEqual([]); expect(created.context_attributes?.library_id).toBe(picks[0].id); }); @@ -426,11 +423,8 @@ test.describe('Alert Library', () => { await lib.selectCard(a.id); await lib.selectCard(b.id); await lib.addSelected(); - await lib.pickDestination(destinationName); - await lib.next(); // alerts - await lib.next(); // folder await lib.next(); // tune - await lib.next(); // review + await lib.next(); // add await lib.run(); // H6: exactly one installed, one failed, with the server message shown. @@ -443,29 +437,4 @@ test.describe('Alert Library', () => { await lib.retryFailed(); await lib.expectInstallResultCount('failed', 1); }); - - test('install explains when the org has no usable destinations', async ({ page }) => { - const ready = entries[0]; - - // F4: destinations fail to load → failed banner + retry. - await page.route('**/api/*/alerts/destinations**', (route) => - route.fulfill({ status: 500, contentType: 'application/json', body: '{}' }), - ); - await lib.openViaUrl(); - await lib.openCard(ready.id); - await lib.installFromDrawer(); - await lib.expectDestinationsFailed(); - await lib.expectDestinationsRetryVisible(); - - // F3: retry against an empty list → the no-destinations banner + open-destinations. - await page.unroute('**/api/*/alerts/destinations**'); - await page.route('**/api/*/alerts/destinations**', (route) => - route.fulfill({ status: 200, contentType: 'application/json', body: '[]' }), - ); - await lib.clickDestinationsRetry(); - await lib.expectDestinationsEmpty(); - await lib.expectOpenDestinationsVisible(); - // Not a blocker: the alerts install without a destination. - await lib.expectNextEnabled(); - }); }); diff --git a/web/src/components/access/OAccessActionPopover.spec.ts b/web/src/components/access/OAccessActionPopover.spec.ts new file mode 100644 index 0000000000..a984a4aab4 --- /dev/null +++ b/web/src/components/access/OAccessActionPopover.spec.ts @@ -0,0 +1,188 @@ +// Copyright 2026 OpenObserve Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { flushPromises, mount, type VueWrapper } from "@vue/test-utils"; +import { shallowRef, type ShallowRef } from "vue"; +import i18n from "@/locales"; +import OAccessActionPopover from "@/components/access/OAccessActionPopover.vue"; +import { viewDenial, type AccessActionNotice } from "@/composables/useAccessStore"; + +const state = vi.hoisted(() => ({ + notice: null as unknown as ShallowRef, + dismissAction: null as unknown as () => void, +})); + +vi.mock("@/composables/useAccessStore", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useAccessBoundary: () => ({ actionNotice: state.notice, dismissAction: state.dismissAction }), + }; +}); + +let wrapper: VueWrapper | null = null; +let container: HTMLElement; +let anchor: HTMLButtonElement; + +const card = () => document.querySelector('[data-test="access-action-error"]'); +const settle = async () => { + await flushPromises(); + await new Promise((resolve) => setTimeout(resolve, 20)); +}; +const press = (el: Element, key: string, init: KeyboardEventInit = {}) => + el.dispatchEvent(new KeyboardEvent("keydown", { key, bubbles: true, cancelable: true, ...init })); + +async function openCard( + denials = [viewDenial("alerts", "delete", ["DELETE /api/default/alerts/a1"])], +): Promise { + wrapper = mount(OAccessActionPopover, { attachTo: document.body, global: { plugins: [i18n] } }); + state.notice.value = { + el: anchor, + permission: denials[0].permission, + method: "DELETE", + resource: denials[0].resource, + denials, + routeKey: "alerts", + fallbacks: [], + }; + await settle(); +} + +beforeEach(() => { + state.notice = shallowRef(null); + state.dismissAction = vi.fn(() => (state.notice.value = null)); + container = document.createElement("div"); + anchor = document.createElement("button"); + anchor.textContent = "Delete"; + container.appendChild(anchor); + document.body.appendChild(container); + anchor.focus(); +}); + +afterEach(() => { + wrapper?.unmount(); + wrapper = null; + container.remove(); +}); + +describe("OAccessActionPopover", () => { + it("keeps focus on the pressed control when the card opens", async () => { + await openCard(); + expect(card()).not.toBeNull(); + expect(document.activeElement).toBe(anchor); + }); + + it("names every refused write the one action sent", async () => { + await openCard([ + viewDenial("roles", "update", ["PUT /api/default/roles/viewer"]), + viewDenial("groups", "update", ["PUT /api/default/groups/ops"]), + ]); + const text = card()!.textContent ?? ""; + expect(text).toContain("Roles"); + expect(text).toContain("Groups"); + }); + + it("moves Tab from the pressed control into the card instead of closing it", async () => { + await openCard(); + press(anchor, "Tab"); + await settle(); + expect(state.dismissAction).not.toHaveBeenCalled(); + expect(card()?.contains(document.activeElement)).toBe(true); + }); + + it("stays open when focus returns to the pressed control", async () => { + await openCard(); + card()!.focus(); + anchor.focus(); + await settle(); + expect(state.dismissAction).not.toHaveBeenCalled(); + expect(card()).not.toBeNull(); + }); + + it("returns to the pressed control when Tab leaves either end of the card", async () => { + await openCard(); + card()!.focus(); + press(card()!, "Tab", { shiftKey: true }); + expect(document.activeElement).toBe(anchor); + expect(state.dismissAction).not.toHaveBeenCalled(); + + const copy = card()!.querySelector('[data-test="access-denied-copy"]')!; + copy.focus(); + press(copy, "Tab"); + await settle(); + expect(document.activeElement).toBe(anchor); + expect(state.dismissAction).toHaveBeenCalledTimes(1); + }); + + it("returns Shift+Tab from the first card button to the pressed control", async () => { + await openCard(); + const copy = card()!.querySelector('[data-test="access-denied-copy"]')!; + copy.focus(); + const event = new KeyboardEvent("keydown", { + key: "Tab", + shiftKey: true, + bubbles: true, + cancelable: true, + }); + copy.dispatchEvent(event); + await settle(); + expect(event.defaultPrevented).toBe(true); + expect(document.activeElement).toBe(anchor); + expect(state.dismissAction).not.toHaveBeenCalled(); + }); + + it("returns Shift+Tab from the first of several card buttons, and leaves the others to the browser", async () => { + await openCard( + ["dashboards", "alerts", "streams", "functions", "pipelines", "destinations"].map((key) => + viewDenial(key, "delete", [`DELETE /api/default/${key}/x`]), + ), + ); + const buttons = [...card()!.querySelectorAll("button")]; + expect(buttons[0].dataset.test).toBe("access-denied-more"); + expect(buttons.length).toBeGreaterThan(1); + buttons[1].focus(); + const inner = new KeyboardEvent("keydown", { + key: "Tab", + shiftKey: true, + bubbles: true, + cancelable: true, + }); + buttons[1].dispatchEvent(inner); + expect(inner.defaultPrevented).toBe(false); + buttons[0].focus(); + press(buttons[0], "Tab", { shiftKey: true }); + await settle(); + expect(document.activeElement).toBe(anchor); + expect(state.dismissAction).not.toHaveBeenCalled(); + }); + + it("closes on Escape and gives focus back to the pressed control", async () => { + await openCard(); + card()!.focus(); + press(card()!, "Escape"); + await settle(); + expect(state.dismissAction).toHaveBeenCalled(); + expect(document.activeElement).toBe(anchor); + }); + + it("stays open while the page under it scrolls", async () => { + await openCard(); + container.dispatchEvent(new Event("scroll")); + await settle(); + expect(state.dismissAction).not.toHaveBeenCalled(); + expect(card()).not.toBeNull(); + }); +}); diff --git a/web/src/components/access/OAccessActionPopover.vue b/web/src/components/access/OAccessActionPopover.vue index 91cfbdd0d0..3c6177dc63 100644 --- a/web/src/components/access/OAccessActionPopover.vue +++ b/web/src/components/access/OAccessActionPopover.vue @@ -22,13 +22,21 @@ along with this program. If not, see . align="end" flip-align arrow + :close-on-scroll="false" :side-offset="-1" :collision-padding="8" content-class="border-0!" @update:open="onOpenChange" @open-auto-focus="keepFocus" + @focus-outside="keepWhileOnAnchor" > - + diff --git a/web/src/components/access/OAccessBoundary.spec.ts b/web/src/components/access/OAccessBoundary.spec.ts new file mode 100644 index 0000000000..cbf7eb254c --- /dev/null +++ b/web/src/components/access/OAccessBoundary.spec.ts @@ -0,0 +1,107 @@ +// Copyright 2026 OpenObserve Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +import { afterEach, describe, expect, it, vi } from "vitest"; +import { flushPromises, mount, type VueWrapper } from "@vue/test-utils"; +import { computed, defineComponent, h, ref, shallowRef } from "vue"; +import { createMemoryHistory, createRouter } from "vue-router"; +import i18n from "@/locales"; +import OAccessBoundary from "@/components/access/OAccessBoundary.vue"; +import OTable from "@/lib/core/Table/OTable.vue"; +import { viewDenial, type AccessSectionEntry } from "@/composables/useAccessStore"; + +const state = vi.hoisted(() => ({ entries: [] as AccessSectionEntry[] })); + +vi.mock("@/composables/useAccessStore", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useAccessBoundary: () => ({ + pageDenied: computed(() => false), + pageDenial: computed(() => null), + pageRequests: () => [], + sectionEntries: computed(() => state.entries), + deepestBoundary: computed(() => -1), + actionNotice: shallowRef(null), + dismissAction: vi.fn(), + }), + }; +}); + +let wrapper: VueWrapper | null = null; + +async function mountTable(entries: AccessSectionEntry[], forbiddenResource?: string) { + state.entries = entries; + const router = createRouter({ + history: createMemoryHistory(), + routes: [{ path: "/", component: { render: () => null } }], + }); + await router.push("/"); + const Page = defineComponent({ + setup: () => () => + h(OTable, { + data: [], + columns: [{ id: "name", header: "Name", accessorKey: "name" }], + forbidden: true, + forbiddenResource, + loading: ref(false).value, + }), + }); + wrapper = mount(OAccessBoundary, { + slots: { default: () => h(Page) }, + global: { plugins: [i18n, router] }, + }); + await flushPromises(); + return wrapper.get('[data-test="o2-table-forbidden"]'); +} + +afterEach(() => { + wrapper?.unmount(); + wrapper = null; +}); + +describe("OAccessBoundary table no-access state", () => { + it("shows the shared lock naming the table's refused list", async () => { + const forbidden = await mountTable( + [ + viewDenial("templates", "list", ["GET /api/default/alerts/templates"]), + viewDenial("alert_folders", "list", ["GET /api/v2/default/alerts"]), + ], + "alert_folders", + ); + expect(forbidden.find('[data-test="access-lock-icon"]').exists()).toBe(true); + expect(forbidden.text()).toContain("You can't view alert folders"); + expect(forbidden.text()).toContain( + "Your role is missing the List permission on Alert folders.", + ); + expect(forbidden.text()).toContain("Copy details"); + }); + + it("keeps the shared lock with the general wording when it cannot tell which list was refused", async () => { + const forbidden = await mountTable([ + viewDenial("templates", "list"), + viewDenial("destinations", "list"), + ]); + expect(forbidden.find('[data-test="access-lock-icon"]').exists()).toBe(true); + expect(forbidden.text()).toContain("You don't have access"); + expect(forbidden.text()).not.toContain("Copy details"); + }); + + it("does not name the page's only refused list on a table that names no resource", async () => { + const forbidden = await mountTable([viewDenial("templates", "list")]); + expect(forbidden.text()).toContain("You don't have access"); + expect(forbidden.text()).not.toContain("templates"); + }); +}); diff --git a/web/src/components/access/OAccessBoundary.vue b/web/src/components/access/OAccessBoundary.vue index e81a075d54..120715f9c1 100644 --- a/web/src/components/access/OAccessBoundary.vue +++ b/web/src/components/access/OAccessBoundary.vue @@ -37,12 +37,22 @@ along with this program. If not, see . diff --git a/web/src/components/access/OAccessDeniedCard.vue b/web/src/components/access/OAccessDeniedCard.vue index 2f19e99d54..671c36e99f 100644 --- a/web/src/components/access/OAccessDeniedCard.vue +++ b/web/src/components/access/OAccessDeniedCard.vue @@ -16,7 +16,7 @@ along with this program. If not, see . - +

+ {{ t("accessBoundary.denied.askAdmin") }} +

. + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { flushPromises, mount, type VueWrapper } from "@vue/test-utils"; +import { computed, shallowRef, type ShallowRef } from "vue"; +import i18n from "@/locales"; +import OAccessHeaderChip from "@/components/access/OAccessHeaderChip.vue"; +import { viewDenial, type AccessSectionEntry } from "@/composables/useAccessStore"; + +const state = vi.hoisted(() => ({ + entries: null as unknown as ShallowRef, +})); + +const screen = vi.hoisted(() => ({ lgUp: true })); +vi.mock("@/composables/useBreakpoint", () => ({ + default: () => ({ lgUp: computed(() => screen.lgUp) }), +})); + +vi.mock("@/composables/useAccessStore", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useAccessBoundary: () => ({ sectionEntries: state.entries }) }; +}); + +let wrapper: VueWrapper | null = null; + +const announcer = () => wrapper!.find('[data-test="access-header-chip-announcer"]'); +const chipName = () => wrapper!.find('[data-test="access-header-chip"]').attributes("aria-label"); +const wait = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); + +beforeEach(() => { + screen.lgUp = true; + state.entries = shallowRef([viewDenial("templates", "list", ["GET /api/default/templates"])]); + wrapper = mount(OAccessHeaderChip, { attachTo: document.body, global: { plugins: [i18n] } }); +}); + +afterEach(() => { + wrapper?.unmount(); + wrapper = null; +}); + +describe("OAccessHeaderChip", () => { + it("announces the chip once through a polite live region", async () => { + const region = announcer(); + expect(region.attributes("aria-live")).toBe("polite"); + expect(region.attributes("role")).toBe("status"); + expect(region.text()).toBe(""); + await wait(400); + expect(region.text()).toBe(chipName()); + expect(region.text()).toContain("templates"); + }); + + it("does not re-announce when the same denials render again", async () => { + await wait(400); + const changes: MutationRecord[] = []; + const observer = new MutationObserver((records) => changes.push(...records)); + observer.observe(announcer().element, { childList: true, characterData: true, subtree: true }); + state.entries.value = [viewDenial("templates", "list", ["GET /api/default/templates"])]; + await flushPromises(); + await wait(400); + observer.disconnect(); + expect(changes).toHaveLength(0); + }); + + it("announces again when the denials change", async () => { + await wait(400); + state.entries.value = [ + viewDenial("templates", "list", ["GET /api/default/templates"]), + viewDenial("destinations", "list", ["GET /api/default/destinations"]), + ]; + await flushPromises(); + expect(announcer().text()).toBe(chipName()); + expect(announcer().text()).not.toContain("templates"); + }); + + it("asks for an administrator, naming no permission, for a read no role grant lifts", async () => { + state.entries.value = [ + { + resource: { + key: "_unnamed/synthetics", + labelKey: "accessBoundary.resourcesInline.unnamed", + path: "/api/default/synthetics/variables", + grantable: false, + }, + permission: "list", + requests: ["GET /api/default/synthetics/variables"], + }, + ]; + await flushPromises(); + await wrapper!.find('[data-test="access-header-chip"]').trigger("click"); + await flushPromises(); + const card = document.querySelector('[data-test="access-header-popover"]')!; + expect(card.querySelector('[data-test="access-denied-title"]')?.textContent).toBe( + "You can't view this resource", + ); + expect(card.querySelector('[data-test="access-denied-ask-admin"]')?.textContent).toBe( + "Ask your administrator for access.", + ); + expect(card.textContent).not.toContain("permission"); + }); + + it("is no taller than the title it sits beside below lg", async () => { + wrapper!.unmount(); + screen.lgUp = false; + wrapper = mount(OAccessHeaderChip, { attachTo: document.body, global: { plugins: [i18n] } }); + const chip = wrapper.find('[data-test="access-header-chip"]'); + expect(chip.classes()).toContain("h-6"); + }); +}); diff --git a/web/src/components/access/OAccessHeaderChip.vue b/web/src/components/access/OAccessHeaderChip.vue index 2cdbd7b502..14f42ab425 100644 --- a/web/src/components/access/OAccessHeaderChip.vue +++ b/web/src/components/access/OAccessHeaderChip.vue @@ -30,13 +30,13 @@ along with this program. If not, see . . + {{ + announcement + }} diff --git a/web/src/components/alertLibrary/LibraryDrawer.vue b/web/src/components/alertLibrary/LibraryDrawer.vue index 2ff5641e05..1a57a5c698 100644 --- a/web/src/components/alertLibrary/LibraryDrawer.vue +++ b/web/src/components/alertLibrary/LibraryDrawer.vue @@ -240,19 +240,16 @@ import { computed, nextTick, ref, watch } from "vue"; import AlertQueryPreview from "@/components/alerts/AlertQueryPreview.vue"; import { ANOMALY_DIRECTION_KEYS } from "@/components/anomaly_detection/steps/AnomalyDetectionConfig.schema"; import PreviewAlert from "@/components/alerts/PreviewAlert.vue"; -import { useAlertCreation } from "@/composables/alerts/useAlertCreation"; import { useAlertLibrary } from "@/composables/alerts/useAlertLibrary"; import OTag from "@/lib/core/Badge/OTag.vue"; import OButton from "@/lib/core/Button/OButton.vue"; import OIcon from "@/lib/core/Icon/OIcon.vue"; import OBanner from "@/lib/feedback/Banner/OBanner.vue"; import OSkeleton from "@/lib/feedback/Skeleton/OSkeleton.vue"; -import { toast } from "@/lib/feedback/Toast/useToast"; import ODrawer from "@/lib/overlay/Drawer/ODrawer.vue"; import type { AlertLibraryEntry, AlertLibraryFile, StreamDataState } from "@/types/alertLibrary"; import { formatDistanceToNowStrict } from "date-fns"; import { raw, useI18nTyped, type I18nKey } from "@/types/i18n"; -import { buildPrefillFromLibrary } from "@/utils/alerts/prefill/fromLibrary"; import { alertTypeOf, @@ -262,7 +259,8 @@ import { severityBadgeValue, severityLabel, } from "./libraryFacets"; -import { readTunables, applyTunables } from "./libraryTunables"; +import { readTunables, applyTunables, fileForInstall } from "./libraryTunables"; +import { useLibraryCustomize } from "./useLibraryCustomize"; const props = defineProps<{ open: boolean; @@ -284,7 +282,7 @@ const emit = defineEmits<{ const { t } = useI18nTyped(); const { loadAlertFile } = useAlertLibrary(); -const { openAlertCreation } = useAlertCreation(); +const { customizeInEditor } = useLibraryCustomize(); const file = ref(null); const isLoading = ref(false); @@ -548,27 +546,13 @@ watch( ); // ── actions ──────────────────────────────────────────────────────────────── -const customize = () => { +const customize = async () => { if (!props.entry || !file.value) return; - - const opened = openAlertCreation( - buildPrefillFromLibrary({ entry: props.entry, file: file.value }), - ); - if (!opened) { - toast({ variant: "error", message: t("alert_library.drawer.customizeFailed") }); - return; - } - emit("update:open", false); + if (await customizeInEditor(props.entry, file.value)) emit("update:open", false); }; const install = () => { if (!props.entry || !file.value) return; - // The threshold floors would write a trigger_condition into a body that must not have one. - if (isAnomaly.value) { - emit("install", { entry: props.entry, file: file.value }); - return; - } - if (!tunedFile.value) return; - emit("install", { entry: props.entry, file: tunedFile.value }); + emit("install", { entry: props.entry, file: fileForInstall(props.entry, file.value) }); }; diff --git a/web/src/components/alertLibrary/LibraryInstallDialog.spec.ts b/web/src/components/alertLibrary/LibraryInstallDialog.spec.ts index 722ecd9e6d..edac864586 100644 --- a/web/src/components/alertLibrary/LibraryInstallDialog.spec.ts +++ b/web/src/components/alertLibrary/LibraryInstallDialog.spec.ts @@ -27,28 +27,16 @@ import { anomalyKeys } from "@/services/anomaly_detection.querykeys"; const mocks = vi.hoisted(() => ({ loadAlertFile: vi.fn(), - listDestinations: vi.fn(), createAlert: vi.fn(), createAnomaly: vi.fn(), getFoldersListByType: vi.fn(), - validateCredentials: vi.fn(() => ({ isValid: true, errors: {} })), - testDestination: vi.fn(), - createDestination: vi.fn(), - updateDestination: vi.fn(), - fetchSystemTemplates: vi.fn(), toast: vi.fn(), - resolveRoute: vi.fn(), })); // Every partial mock below spreads the REAL module and overrides only what the // test drives. A hand-listed subset silently removes the other members, so an // implementation that touches one dies at mount and reads as a component bug // rather than as the missing test double it is. -vi.mock("vue-router", async () => { - const actual = await vi.importActual>("vue-router"); - return { ...actual, useRouter: () => ({ resolve: mocks.resolveRoute }) }; -}); - vi.mock("@/composables/alerts/useAlertLibrary", async () => { const actual = await vi.importActual>( "@/composables/alerts/useAlertLibrary", @@ -62,13 +50,6 @@ vi.mock("@/composables/alerts/useAlertLibrary", async () => { }; }); -vi.mock("@/services/alert_destination", async () => { - const actual = await vi.importActual<{ default: Record }>( - "@/services/alert_destination", - ); - return { ...actual, default: { ...actual.default, list: mocks.listDestinations } }; -}); - vi.mock("@/services/alerts", async () => { const actual = await vi.importActual<{ default: Record }>("@/services/alerts"); return { ...actual, default: { ...actual.default, create_by_alert_id: mocks.createAlert } }; @@ -86,26 +67,6 @@ vi.mock("@/utils/commons", async () => { return { ...actual, getFoldersListByType: mocks.getFoldersListByType }; }); -// Safe to call the real factory: `fetchSystemTemplates` is only ever invoked -// from inside the other methods, never at construction, so nothing reaches the -// network just by building the composable. -vi.mock("@/composables/usePrebuiltDestinations", async () => { - const actual = await vi.importActual>( - "@/composables/usePrebuiltDestinations", - ); - return { - ...actual, - usePrebuiltDestinations: () => ({ - ...(actual.usePrebuiltDestinations as () => Record)(), - fetchSystemTemplates: mocks.fetchSystemTemplates, - validateCredentials: mocks.validateCredentials, - testDestination: mocks.testDestination, - createDestination: mocks.createDestination, - updateDestination: mocks.updateDestination, - }), - }; -}); - vi.mock("@/lib/feedback/Toast/useToast", () => ({ toast: mocks.toast })); vi.mock("@/services/product_analytics", () => ({ default: { track: vi.fn() } })); @@ -121,18 +82,6 @@ const ODialogStub = { template: '
', }; -const OSelectStub = { - name: "OSelect", - props: ["modelValue", "options", "label", "size", "placeholder", "errorMessage"], - emits: ["update:modelValue"], - template: - '", -}; - const OInputStub = { name: "OInput", props: [ @@ -170,20 +119,11 @@ const SelectFolderStub = { template: '
', }; -const PrebuiltSelectorStub = { - name: "PrebuiltDestinationSelector", - props: ["modelValue"], - emits: ["update:modelValue", "select"], - template: '
', -}; - const stubs = { ODialog: ODialogStub, - OSelect: OSelectStub, OInput: OInputStub, OCheckbox: OCheckboxStub, SelectFolderDropDown: SelectFolderStub, - PrebuiltDestinationSelector: PrebuiltSelectorStub, }; // ── fixtures ─────────────────────────────────────────────────────────────── @@ -241,7 +181,6 @@ const mountDialog = async (props: Record = {}) => { const wrapper = mount(LibraryInstallDialog, { props: { open: true, - entries: entries(), preselect: [], seed: { entry: seedEntry(), file: libraryFile("pod-oom-killed") }, ...props, @@ -257,26 +196,21 @@ const click = async (wrapper: any, test: string) => { await flushPromises(); }; -/** Walk Destination → Alerts → Folder → Tune → Install with the defaults. */ +/** The gallery's three-alert selection, with no drawer seed — the batch cases. */ +const mountBatch = (props: Record = {}) => + mountDialog({ seed: null, preselect: entries(), ...props }); + +/** Walk Folder → Tune → Add with the defaults; an anomaly-only batch has no Tune. */ const advanceToInstall = async (wrapper: any) => { - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await flushPromises(); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); + for (let guard = 0; guard < 3; guard += 1) { + if (wrapper.find('[data-test="alert-library-install-run"]').exists()) return; + await click(wrapper, "alert-library-install-next"); + } }; -/** Same walk, but installing all three — the batch cases. */ -const advanceAllToInstall = async (wrapper: any) => { - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await flushPromises(); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-select-all"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); -}; +/** Step titles as the stepper shows them, in order. */ +const stepTitles = (wrapper: any): string[] => + wrapper.findAllComponents({ name: "OStep" }).map((step: any) => String(step.props("title"))); /** Per-alert outcome as the results list reports it. */ const statusOf = (wrapper: any, id: string) => @@ -301,8 +235,6 @@ const drain = async (releases: Array<(value: unknown) => void>) => { const payloadFor = (name: string) => mocks.createAlert.mock.calls.find((call) => call[1].name === name)?.[1] as Record; -let windowOpen: ReturnType; - describe("LibraryInstallDialog", () => { beforeEach(() => { // Reset OUR mocks only. `clearAllMocks` cannot drain an unconsumed `…Once` @@ -313,290 +245,108 @@ describe("LibraryInstallDialog", () => { // strips their implementations for the rest of the file. Object.values(mocks).forEach((mock) => mock.mockReset()); vi.mocked(analytics.track).mockClear(); - mocks.listDestinations.mockResolvedValue({ - data: [{ name: "ops-slack" }, { name: "oncall-pagerduty" }], - }); mocks.loadAlertFile.mockImplementation((e: AlertLibraryEntry) => Promise.resolve(libraryFile(e.name)), ); mocks.createAlert.mockResolvedValue({ data: {} }); mocks.getFoldersListByType.mockResolvedValue([]); - mocks.validateCredentials.mockReturnValue({ isValid: true, errors: {} }); - mocks.createDestination.mockResolvedValue(undefined); - mocks.testDestination.mockResolvedValue({ success: true }); - mocks.resolveRoute.mockReturnValue({ href: "/web/alerts/destinations?action=add" }); - windowOpen = vi.spyOn(window, "open").mockReturnValue(null); }); - afterEach(() => { - windowOpen.mockRestore(); - }); - - describe("destination step", () => { - it("offers the destinations this org actually has", async () => { + describe("steps", () => { + it("asks only Folder, Tune and Add for a batch with a threshold alert", async () => { const wrapper = await mountDialog(); - expect(mocks.listDestinations).toHaveBeenCalledWith( - expect.objectContaining({ org_identifier: "default", module: "alert" }), - ); - // Membership, not order: sorting or grouping the picker is a display - // choice, and pinning the API's order forbids it for no reason. - const options = wrapper - .find('[data-test="alert-library-install-destination"]') - .attributes("data-options") - ?.split(","); - expect(options).toHaveLength(2); - expect(options).toEqual(expect.arrayContaining(["ops-slack", "oncall-pagerduty"])); + expect(stepTitles(wrapper)).toEqual(["Folder", "Tune", "Add"]); + expect(wrapper.find('[data-test="alert-library-install-folder-step"]').exists()).toBe(true); }); - it("advances without a destination, which is optional", async () => { + it("never asks for a destination", async () => { const wrapper = await mountDialog(); - expect( - wrapper.find('[data-test="alert-library-install-next"]').attributes("disabled"), - ).toBeUndefined(); + expect(wrapper.text()).not.toContain("Destination"); + expect(wrapper.find('[data-test="alert-library-install-destination"]').exists()).toBe(false); + await advanceToInstall(wrapper); + expect(wrapper.find('[data-test="alert-library-install-summary"]').text()).not.toContain( + "Destination", + ); }); - it("installs with no destination when none is chosen", async () => { - const wrapper = await mountDialog(); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); + it("offers no per-alert checklist — the page already chose the alerts", async () => { + const wrapper = await mountBatch(); + expect(wrapper.find('[data-test^="alert-library-install-alert-"]').exists()).toBe(false); + await advanceToInstall(wrapper); + expect(wrapper.find('[data-test="alert-library-install-run"]').text()).toContain("3"); + }); - expect(wrapper.find('[data-test="alert-library-install-summary"]').text()).toContain( - (i18n.global.t as any)("common.none"), - ); + it("installs every alert passed in, with no destination", async () => { + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); - expect(mocks.createAlert).toHaveBeenCalledWith( - "default", - expect.objectContaining({ destinations: [] }), - "default", - ); + expect(mocks.createAlert.mock.calls.map((call) => call[1].name)).toEqual([ + "pod-oom-killed", + "node-disk-pressure", + "cert-expiring", + ]); + for (const call of mocks.createAlert.mock.calls) expect(call[1].destinations).toEqual([]); }); - it("does not mistake a failed request for an org with no destinations", async () => { - // A transient GET failure says nothing about how many destinations exist. - // Forcing create mode on it TRAPS the user: the way back out is hidden - // exactly when the list is empty, and Back is hidden on step 1 — so an - // org with fifty destinations could only create a duplicate or quit. - mocks.listDestinations.mockRejectedValue({ response: { status: 500 } }); - const wrapper = await mountDialog(); - - expect(wrapper.find('[data-test="alert-library-install-destinations-failed"]').exists()).toBe( - true, - ); - expect(wrapper.find('[data-test="alert-library-install-destination"]').exists()).toBe(true); - // NOT the empty-org state: the list is unknown, not known to be empty. - expect(wrapper.find('[data-test="alert-library-install-destinations-empty"]').exists()).toBe( - false, - ); - // And a way forward either way. - expect(wrapper.find('[data-test="alert-library-install-destinations-retry"]').exists()).toBe( - true, - ); - expect(wrapper.find('[data-test="alert-library-install-open-destinations"]').exists()).toBe( - true, - ); - }); - - it("says so plainly when the org genuinely has no destinations", async () => { - // A legitimate first-run condition, not an error — and no longer - // something this dialog can fix, so it has to point somewhere real. - mocks.listDestinations.mockResolvedValue({ data: [] }); - const wrapper = await mountDialog(); - - expect(wrapper.find('[data-test="alert-library-install-destinations-empty"]').exists()).toBe( - true, - ); - expect(wrapper.find('[data-test="alert-library-install-destinations-failed"]').exists()).toBe( - false, - ); - expect(wrapper.find('[data-test="alert-library-install-open-destinations"]').exists()).toBe( - true, - ); - // The alerts install without one, so the wizard still moves on. - expect( - wrapper.find('[data-test="alert-library-install-next"]').attributes("disabled"), - ).toBeUndefined(); - }); - - it("routes to the Destinations page for every type, not just a custom one", async () => { - const wrapper = await mountDialog(); - await click(wrapper, "alert-library-install-open-destinations"); - - expect(mocks.resolveRoute).toHaveBeenCalledWith( - expect.objectContaining({ name: "alertDestinations" }), - ); - expect(windowOpen).toHaveBeenCalledWith( - "/web/alerts/destinations?action=add", - "_blank", - expect.stringContaining("noopener"), - ); - }); - - it("offers a retry that recovers the list", async () => { - mocks.listDestinations.mockRejectedValue({ response: { status: 500 } }); - const wrapper = await mountDialog(); - - mocks.listDestinations.mockResolvedValue({ data: [{ name: "ops-slack" }] }); - await click(wrapper, "alert-library-install-destinations-retry"); - - expect(wrapper.find('[data-test="alert-library-install-destinations-failed"]').exists()).toBe( - false, - ); - expect( - wrapper.find('[data-test="alert-library-install-destination"]').attributes("data-options"), - ).toBe("ops-slack"); - }); - }); - - describe("alerts step", () => { - it("arrives with the drawer's alert already chosen and the rest offered", async () => { - const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - const checked = (id: string) => - ( - wrapper.find(`[data-test="alert-library-install-alert-${id}"]`) - .element as HTMLInputElement - ).checked; - expect(checked("k8s/pod-oom-killed")).toBe(true); - expect(checked("k8s/node-disk-pressure")).toBe(false); - expect(checked("k8s/cert-expiring")).toBe(false); - }); - - it("cannot continue with nothing selected", async () => { - const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - await wrapper - .find('[data-test="alert-library-install-alert-k8s/pod-oom-killed"]') - .setValue(false); - await flushPromises(); - expect( - wrapper.find('[data-test="alert-library-install-next"]').attributes("disabled"), - ).toBeDefined(); - }); - - it("survives an empty candidate list without offering a dead Next", async () => { - const wrapper = await mountDialog({ entries: [], seed: null }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - expect(wrapper.find('[data-test="alert-library-install-alerts-step"]').exists()).toBe(true); - expect( - wrapper.find('[data-test="alert-library-install-next"]').attributes("disabled"), - ).toBeDefined(); - }); - - it("offers the seeded alert even when the gallery filter excludes it", async () => { - // Real: `entries` is the gallery's FILTERED view, and the user may have - // narrowed it after opening the drawer. Dropping the seed would silently - // install nothing, or install the wrong alert. - const wrapper = await mountDialog({ - entries: [secondEntry(), thirdEntry()], - seed: { entry: seedEntry(), file: libraryFile("pod-oom-killed") }, - }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - const seedRow = wrapper.find('[data-test="alert-library-install-alert-k8s/pod-oom-killed"]'); - expect(seedRow.exists()).toBe(true); - expect((seedRow.element as HTMLInputElement).checked).toBe(true); - - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); + it("installs the drawer's single alert, whatever else the gallery had selected", async () => { + const wrapper = await mountDialog({ preselect: [secondEntry(), thirdEntry()] }); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(mocks.createAlert).toHaveBeenCalledTimes(1); expect(mocks.createAlert.mock.calls[0][1].name).toBe("pod-oom-killed"); }); - it("goes back to the destination step without losing the choice", async () => { - const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-back"); - - expect(wrapper.find('[data-test="alert-library-install-destination-step"]').exists()).toBe( - true, - ); + it("offers no run for an empty batch", async () => { + const wrapper = await mountDialog({ seed: null, preselect: [] }); + await advanceToInstall(wrapper); expect( - wrapper.find('[data-test="alert-library-install-destination"]').attributes("data-value"), - ).toBe("ops-slack"); + wrapper.find('[data-test="alert-library-install-run"]').attributes("disabled"), + ).toBeDefined(); }); - it("selects and clears every offered alert in one click", async () => { + it("goes back from Add to Tune to Folder", async () => { const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - // Read the numbers as data, not as prose. `toContain("3")` was worse than - // useless — "0 of 3 selected" contains "3" — but exact text couples the - // test to the copy AND to source formatting, since VTU's `text()` trims - // the ends without collapsing interior whitespace, so a Prettier rewrap - // of the template would fail a correct component. - const count = () => wrapper.find('[data-test="alert-library-install-count"]'); - - await click(wrapper, "alert-library-install-select-all"); - expect(count().attributes("data-selected")).toBe("3"); - expect(count().attributes("data-total")).toBe("3"); - - await click(wrapper, "alert-library-install-clear"); - expect(count().attributes("data-selected")).toBe("0"); - expect(count().attributes("data-total")).toBe("3"); + await advanceToInstall(wrapper); + await click(wrapper, "alert-library-install-back"); + expect(wrapper.find('[data-test="alert-library-install-tune-step"]').exists()).toBe(true); + await click(wrapper, "alert-library-install-back"); + expect(wrapper.find('[data-test="alert-library-install-folder-step"]').exists()).toBe(true); }); }); describe("folder step", () => { - it("refreshes the alert folder list on reaching the step, not before", async () => { - const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - // WHEN is the whole claim: SelectFolderDropDown refreshes itself only on - // `onActivated`, which a dialog never fires. Asserting only that it was - // called eventually would pass on a fetch at mount, which is the bug. + it("refreshes the alert folder list when the dialog opens, since Folder is the first step", async () => { + // SelectFolderDropDown refreshes itself only on `onActivated`, which a dialog never fires. + await mountDialog({ open: false }); expect(mocks.getFoldersListByType).not.toHaveBeenCalled(); - await click(wrapper, "alert-library-install-next"); + const wrapper = await mountDialog(); expect(mocks.getFoldersListByType).toHaveBeenCalledWith(expect.anything(), "alerts"); expect(wrapper.find('[data-test="folder-picker"]').attributes("data-type")).toBe("alerts"); }); it("installs into the folder the picker reports", async () => { const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - wrapper .findComponent({ name: "SelectFolderDropDown" }) .vm.$emit("folder-selected", { label: "Kubernetes", value: "folder-42" }); await flushPromises(); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(mocks.createAlert).toHaveBeenCalledWith("default", expect.anything(), "folder-42"); }); it("still lets the batch install when the folder list cannot be read", async () => { - // As plausible as the destination 403 already covered. "default" always - // exists, so a failed refresh must degrade to it rather than trap the run. + // "default" always exists, so a failed refresh must degrade to it rather than trap the run. mocks.getFoldersListByType.mockRejectedValue({ response: { status: 403 } }); const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - expect(wrapper.find('[data-test="alert-library-install-folder-step"]').exists()).toBe(true); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); + expect(wrapper.find('[data-test="folder-picker"]').exists()).toBe(true); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(mocks.createAlert).toHaveBeenCalledWith("default", expect.anything(), "default"); @@ -615,10 +365,8 @@ describe("LibraryInstallDialog", () => { await click(wrapper, "alert-library-install-run"); const payload = mocks.createAlert.mock.calls[0][1] as Record; - // Wizard-supplied: the chosen destination, the folder, and the store - // identity/timezone. `owner: undefined` serialises away silently, so - // nothing else in either spec would notice these were never read. - expect(payload.destinations).toEqual(["ops-slack"]); + // `owner: undefined` serialises away silently, so only this read-back proves the wizard fed it. + expect(payload.destinations).toEqual([]); expect(payload.folder_id).toBe("default"); expect(mocks.createAlert.mock.calls[0][2]).toBe("default"); expect(payload.owner).toBe("example@gmail.com"); @@ -629,8 +377,8 @@ describe("LibraryInstallDialog", () => { }); it("builds each alert from its own entry, not from the seed's", async () => { - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(payloadFor("pod-oom-killed").context_attributes).toMatchObject({ @@ -658,25 +406,38 @@ describe("LibraryInstallDialog", () => { expect(payload.trigger_condition.silence).toBe(45); }); - it("fetches the file for an alert the drawer never opened, and only those", async () => { - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + it("fetches the file of every alert in a gallery batch, which no drawer opened", async () => { + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); - expect(mocks.loadAlertFile).toHaveBeenCalledTimes(2); expect(mocks.loadAlertFile.mock.calls.map((call) => call[0].id)).toEqual([ + "k8s/pod-oom-killed", "k8s/node-disk-pressure", "k8s/cert-expiring", ]); expect(mocks.createAlert).toHaveBeenCalledTimes(3); }); + it("floors a fetched batch file's zero threshold and period, as the card and drawer do", async () => { + mocks.loadAlertFile.mockImplementation((e: AlertLibraryEntry) => + Promise.resolve({ + ...libraryFile(e.name), + trigger_condition: { period: 0, operator: ">=", threshold: 0, frequency: 5, silence: 30 }, + }), + ); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); + await click(wrapper, "alert-library-install-run"); + + for (const name of ["pod-oom-killed", "node-disk-pressure", "cert-expiring"]) { + expect(payloadFor(name).trigger_condition.threshold).toBe(1); + expect(payloadFor(name).trigger_condition.period).toBe(1); + } + }); + it("applies the bulk-tuned pair to every alert in the batch", async () => { - const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-select-all"); - await click(wrapper, "alert-library-install-next"); + const wrapper = await mountBatch(); await click(wrapper, "alert-library-install-next"); await wrapper.find('[data-test="alert-library-install-tune-toggle"]').setValue(true); @@ -712,9 +473,6 @@ describe("LibraryInstallDialog", () => { // `Number("")` is 0, which silently turns "run every N minutes" into an // evaluation that never comes round. The drawer already solved this. const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); await click(wrapper, "alert-library-install-next"); await wrapper.find('[data-test="alert-library-install-tune-toggle"]').setValue(true); @@ -734,9 +492,6 @@ describe("LibraryInstallDialog", () => { it("keeps a silence of zero, which means 'never suppress'", async () => { const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); await click(wrapper, "alert-library-install-next"); await wrapper.find('[data-test="alert-library-install-tune-toggle"]').setValue(true); @@ -769,8 +524,8 @@ describe("LibraryInstallDialog", () => { const releases: Array<(value: unknown) => void> = []; mocks.createAlert.mockImplementation(() => new Promise((resolve) => releases.push(resolve))); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(mocks.createAlert).toHaveBeenCalledTimes(1); @@ -803,8 +558,8 @@ describe("LibraryInstallDialog", () => { const releases: Array<(value: unknown) => void> = []; mocks.createAlert.mockImplementation(() => new Promise((resolve) => releases.push(resolve))); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(mocks.createAlert).toHaveBeenCalledTimes(1); @@ -827,8 +582,8 @@ describe("LibraryInstallDialog", () => { .mockRejectedValueOnce({ response: { data: { message: "alert name already exists" } } }) .mockResolvedValueOnce({ data: {} }); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); // The third create is the whole point: it only happens if the second @@ -853,8 +608,8 @@ describe("LibraryInstallDialog", () => { .mockRejectedValueOnce({ response: { data: { message: "boom" } } }) .mockResolvedValueOnce({ data: {} }); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); // Times(1) matters: `toHaveBeenCalledWith` scans EVERY call, so an @@ -865,18 +620,23 @@ describe("LibraryInstallDialog", () => { }); it("reports a file that could not be fetched as that alert's failure only", async () => { - mocks.loadAlertFile.mockRejectedValue(new Error("network down")); + mocks.loadAlertFile.mockImplementation((e: AlertLibraryEntry) => + e.name === "node-disk-pressure" + ? Promise.reject(new Error("network down")) + : Promise.resolve(libraryFile(e.name)), + ); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); - // The seeded alert still installs; the two that needed a fetch do not. - expect(mocks.createAlert).toHaveBeenCalledTimes(1); - expect(mocks.createAlert.mock.calls[0][1].name).toBe("pod-oom-killed"); + expect(mocks.createAlert.mock.calls.map((call) => call[1].name)).toEqual([ + "pod-oom-killed", + "cert-expiring", + ]); expect(statusOf(wrapper, "k8s/pod-oom-killed")).toBe("installed"); expect(statusOf(wrapper, "k8s/node-disk-pressure")).toBe("failed"); - expect(statusOf(wrapper, "k8s/cert-expiring")).toBe("failed"); + expect(statusOf(wrapper, "k8s/cert-expiring")).toBe("installed"); }); it("retries only what failed, and keeps the successes reported", async () => { @@ -885,8 +645,8 @@ describe("LibraryInstallDialog", () => { .mockRejectedValueOnce({ response: { data: { message: "boom" } } }) .mockResolvedValueOnce({ data: {} }); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); mocks.createAlert.mockClear(); @@ -908,8 +668,8 @@ describe("LibraryInstallDialog", () => { .mockRejectedValueOnce({ response: { data: { message: "boom" } } }) .mockResolvedValueOnce({ data: {} }); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); const releases: Array<(value: unknown) => void> = []; @@ -937,8 +697,8 @@ describe("LibraryInstallDialog", () => { .mockRejectedValueOnce({ response: { data: { message: "boom" } } }) .mockResolvedValueOnce({ data: {} }); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); const installed = wrapper.emitted("installed"); @@ -954,8 +714,8 @@ describe("LibraryInstallDialog", () => { .mockRejectedValueOnce({ response: { data: { message: "boom" } } }) .mockResolvedValueOnce({ data: {} }); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(analytics.track).toHaveBeenCalledTimes(1); @@ -968,8 +728,8 @@ describe("LibraryInstallDialog", () => { it("does not track library_alert_installed when every install fails", async () => { mocks.createAlert.mockRejectedValue({ response: { data: { message: "boom" } } }); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(analytics.track).not.toHaveBeenCalled(); @@ -1034,18 +794,9 @@ describe("LibraryInstallDialog", () => { it("installs a repeated manifest id exactly once", async () => { // The manifest is fetched content; a duplicated entry would otherwise be // walked twice and create the alert twice. - const wrapper = await mountDialog({ entries: [seedEntry(), seedEntry(), secondEntry()] }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-select-all"); - - expect(wrapper.find('[data-test="alert-library-install-count"]').attributes("data-total")).toBe( - "2", - ); - - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); + const wrapper = await mountBatch({ preselect: [seedEntry(), seedEntry(), secondEntry()] }); + await advanceToInstall(wrapper); + expect(wrapper.find('[data-test="alert-library-install-run"]').text()).toContain("2"); await click(wrapper, "alert-library-install-run"); expect(mocks.createAlert).toHaveBeenCalledTimes(2); @@ -1065,11 +816,7 @@ describe("LibraryInstallDialog", () => { return Promise.resolve(file); }); - const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-select-all"); - await click(wrapper, "alert-library-install-next"); + const wrapper = await mountBatch(); await click(wrapper, "alert-library-install-next"); await click(wrapper, "alert-library-install-next"); await click(wrapper, "alert-library-install-run"); @@ -1093,8 +840,8 @@ describe("LibraryInstallDialog", () => { .mockRejectedValueOnce({ response: { data: { message: "boom" } } }) .mockResolvedValueOnce({ data: {} }); - const wrapper = await mountDialog(); - await advanceAllToInstall(wrapper); + const wrapper = await mountBatch(); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); mocks.createAlert.mockResolvedValue({ data: {} }); @@ -1108,17 +855,14 @@ describe("LibraryInstallDialog", () => { it("starts from the first step again when reopened", async () => { const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); await click(wrapper, "alert-library-install-next"); - expect(wrapper.find('[data-test="alert-library-install-alerts-step"]').exists()).toBe(true); + expect(wrapper.find('[data-test="alert-library-install-tune-step"]').exists()).toBe(true); await wrapper.setProps({ open: false }); await wrapper.setProps({ open: true }); await flushPromises(); - expect(wrapper.find('[data-test="alert-library-install-destination-step"]').exists()).toBe( - true, - ); + expect(wrapper.find('[data-test="alert-library-install-folder-step"]').exists()).toBe(true); }); it("forgets the previous run when reopened", async () => { @@ -1134,27 +878,16 @@ describe("LibraryInstallDialog", () => { expect(wrapper.findAll('[data-test^="alert-library-install-result-"]')).toHaveLength(0); }); - it("reseeds the selection from the alert the drawer handed over", async () => { + it("installs the alert the drawer handed over on the latest open", async () => { const wrapper = await mountDialog(); await wrapper.setProps({ open: false }); await wrapper.setProps({ seed: { entry: thirdEntry(), file: libraryFile("cert-expiring") } }); await wrapper.setProps({ open: true }); await flushPromises(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - expect( - ( - wrapper.find('[data-test="alert-library-install-alert-k8s/cert-expiring"]') - .element as HTMLInputElement - ).checked, - ).toBe(true); - expect( - ( - wrapper.find('[data-test="alert-library-install-alert-k8s/pod-oom-killed"]') - .element as HTMLInputElement - ).checked, - ).toBe(false); + await advanceToInstall(wrapper); + await click(wrapper, "alert-library-install-run"); + expect(mocks.createAlert.mock.calls.map((call) => call[1].name)).toEqual(["cert-expiring"]); }); // The stub above renders the panel inline and has no close button, which makes @@ -1182,35 +915,6 @@ describe("LibraryInstallDialog", () => { document.body.innerHTML = ""; }); - it("gives every alert checkbox an accessible name", async () => { - // OCheckbox names itself from `label` alone. Without it a screen-reader - // user hears "checkbox, unchecked" once per alert with no way to tell - // which one they are about to install. - const checkboxStubs = { ...realStubs } as Record; - checkboxStubs.ODialog = ODialogStub; - delete checkboxStubs.OCheckbox; - - wrapper = mount(LibraryInstallDialog, { - props: { - open: true, - entries: entries(), - preselect: [], - seed: { entry: seedEntry(), file: libraryFile("pod-oom-killed") }, - }, - global: { plugins: [i18n, store], stubs: checkboxStubs }, - }); - await flushPromises(); - - const select = wrapper.find('[data-test="alert-library-install-destination"]'); - await select.setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - const row = wrapper.find('[data-test="alert-library-install-alert-k8s/pod-oom-killed"]'); - expect(row.exists()).toBe(true); - expect(row.text()).toContain("Pod OOM Killed"); - expect(row.find('[role="checkbox"]').exists()).toBe(true); - }); - it("takes the close button away while creates are in flight", async () => { let release: (value: unknown) => void = () => {}; mocks.createAlert.mockReturnValue( @@ -1222,7 +926,6 @@ describe("LibraryInstallDialog", () => { wrapper = mount(LibraryInstallDialog, { props: { open: true, - entries: entries(), preselect: [], seed: { entry: seedEntry(), file: libraryFile("pod-oom-killed") }, }, @@ -1233,13 +936,6 @@ describe("LibraryInstallDialog", () => { expect(inBody("o-dialog-close-btn")).not.toBeNull(); - const select = inBody("alert-library-install-destination") as HTMLSelectElement; - select.value = "ops-slack"; - select.dispatchEvent(new Event("change")); - await flushPromises(); - - await clickInBody("alert-library-install-next"); - await clickInBody("alert-library-install-next"); await clickInBody("alert-library-install-next"); await clickInBody("alert-library-install-next"); await clickInBody("alert-library-install-run"); @@ -1255,18 +951,14 @@ describe("LibraryInstallDialog", () => { }); }); - it("loads no destinations while closed", async () => { + it("loads nothing while closed", async () => { await mountDialog({ open: false }); - expect(mocks.listDestinations).not.toHaveBeenCalled(); expect(mocks.loadAlertFile).not.toHaveBeenCalled(); expect(mocks.getFoldersListByType).not.toHaveBeenCalled(); }); describe("the tune step's hint", () => { const hintOf = async () => { const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); await click(wrapper, "alert-library-install-next"); return wrapper.get('[data-test="alert-library-install-tune-hint"]').text(); }; @@ -1281,9 +973,6 @@ describe("LibraryInstallDialog", () => { it("takes those names from the field labels, so they cannot drift apart", async () => { const wrapper = await mountDialog(); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); await click(wrapper, "alert-library-install-next"); await wrapper.find('[data-test="alert-library-install-tune-toggle"]').setValue(true); await flushPromises(); @@ -1301,132 +990,42 @@ describe("LibraryInstallDialog", () => { // ── preselection from the gallery ───────────────────────────────────────── describe("preselect", () => { - const ticked = (wrapper: any) => - wrapper - .findAll('[data-test^="alert-library-install-alert-"]') - .filter((box: any) => box.element.checked) - .map((box: any) => - box.attributes("data-test")!.replace("alert-library-install-alert-", ""), - ); - - it("ticks exactly the alerts the gallery handed over", async () => { - const wrapper = await mountDialog({ - seed: null, - preselect: [seedEntry(), thirdEntry()], - }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - expect(ticked(wrapper).sort()).toEqual(["k8s/cert-expiring", "k8s/pod-oom-killed"]); - expect( - wrapper.find('[data-test="alert-library-install-count"]').attributes("data-selected"), - ).toBe("2"); - }); - - it("offers a preselected alert the current filters exclude", async () => { - // `entries` is the gallery's FILTERED view. A batch built before the user - // narrowed it would otherwise report a count for alerts not in the list. - const offView = entry({ + const offView = () => + entry({ id: "kafka/consumer-lag", name: "consumer-lag", pack: "kafka", title: "Consumer Lag Growing", content_hash: "hash-9", }); - const wrapper = await mountDialog({ seed: null, preselect: [offView] }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - expect( - wrapper.find('[data-test="alert-library-install-alert-kafka/consumer-lag"]').exists(), - ).toBe(true); - expect(ticked(wrapper)).toEqual(["kafka/consumer-lag"]); - expect( - wrapper.find('[data-test="alert-library-install-count"]').attributes("data-total"), - ).toBe("4"); + it("installs exactly the alerts the gallery handed over", async () => { + const wrapper = await mountDialog({ seed: null, preselect: [seedEntry(), thirdEntry()] }); + await advanceToInstall(wrapper); + await click(wrapper, "alert-library-install-run"); + + expect(mocks.createAlert.mock.calls.map((call) => call[1].name)).toEqual([ + "pod-oom-killed", + "cert-expiring", + ]); }); - it("creates the off-view alert, not just lists it", async () => { - const offView = entry({ - id: "kafka/consumer-lag", - name: "consumer-lag", - pack: "kafka", - title: "Consumer Lag Growing", - content_hash: "hash-9", - }); + it("creates an alert the gallery's current filters hide", async () => { + // The batch outlives the filters: it was frozen when the user pressed Add. mocks.loadAlertFile.mockResolvedValue(libraryFile("consumer-lag")); - mocks.createAlert.mockResolvedValue({ data: {} }); - - const wrapper = await mountDialog({ seed: null, preselect: [offView] }); + const wrapper = await mountDialog({ seed: null, preselect: [offView()] }); await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); - await flushPromises(); expect(statusOf(wrapper, "kafka/consumer-lag")).toBe("installed"); }); - it("lists the seed first, then the off-view batch, then the visible set", async () => { - const offView = entry({ id: "kafka/consumer-lag", name: "consumer-lag", pack: "kafka" }); - const wrapper = await mountDialog({ preselect: [offView] }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - const order = wrapper - .findAll('[data-test^="alert-library-install-alert-"]') - .map((b: any) => b.attributes("data-test")!.replace("alert-library-install-alert-", "")); - expect(order[0]).toBe("k8s/pod-oom-killed"); - expect(order[1]).toBe("kafka/consumer-lag"); - }); - - it("lets the drawer's seed win — one alert, whatever the gallery had selected", async () => { - const wrapper = await mountDialog({ preselect: [secondEntry(), thirdEntry()] }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - expect(ticked(wrapper)).toEqual(["k8s/pod-oom-killed"]); - }); - - it("scopes an untick to this run — it tells the gallery nothing", async () => { - // The gallery selection is the user's shelf; only a successful add takes - // an alert off it. A modal editing that state behind its own backdrop is - // the two-selection-models failure this design exists to avoid. + it("tells the gallery nothing until an alert is actually added", async () => { const wrapper = await mountDialog({ seed: null, preselect: [seedEntry(), secondEntry()] }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - await wrapper - .find('[data-test="alert-library-install-alert-k8s/pod-oom-killed"]') - .setValue(false); - await flushPromises(); + await advanceToInstall(wrapper); expect(Object.keys(wrapper.emitted())).not.toContain("installed"); expect(Object.keys(wrapper.emitted())).not.toContain("update:preselect"); - // The row is off for this run, and the dialog said nothing to anyone. - expect( - wrapper.find('[data-test="alert-library-install-count"]').attributes("data-selected"), - ).toBe("1"); - }); - - it("still widens a preselected batch from step 2", async () => { - const wrapper = await mountDialog({ seed: null, preselect: [seedEntry()] }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-select-all"); - - expect( - wrapper.find('[data-test="alert-library-install-count"]').attributes("data-selected"), - ).toBe("3"); - }); - - it("never lists an alert twice when the gallery preselects a visible one", async () => { - const wrapper = await mountDialog({ seed: null, preselect: [secondEntry()] }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - - const rows = wrapper.findAll( - '[data-test="alert-library-install-alert-k8s/node-disk-pressure"]', - ); - expect(rows).toHaveLength(1); }); }); @@ -1447,7 +1046,7 @@ describe("LibraryInstallDialog", () => { it("asks before a run big enough to reshape the org's alerting", async () => { // The catalog is ~1,242 alerts and select-all-in-view is one click, so // the largest batches must be deliberate rather than merely reachable. - const wrapper = await mountDialog({ seed: null, entries: many(51), preselect: many(51) }); + const wrapper = await mountDialog({ seed: null, preselect: many(51) }); await advanceToInstall(wrapper); const confirm = wrapper.find('[data-test="alert-library-install-confirm-large"]'); @@ -1463,7 +1062,7 @@ describe("LibraryInstallDialog", () => { }); it("stays out of the way for an ordinary batch", async () => { - const wrapper = await mountDialog({ seed: null, entries: many(50), preselect: many(50) }); + const wrapper = await mountDialog({ seed: null, preselect: many(50) }); await advanceToInstall(wrapper); expect(wrapper.find('[data-test="alert-library-install-confirm-large"]').exists()).toBe( @@ -1472,102 +1071,35 @@ describe("LibraryInstallDialog", () => { expect(runButton(wrapper).attributes("disabled")).toBeUndefined(); }); - it("re-arms the confirmation when a narrowed batch is widened again", async () => { - // An acknowledgement given at 51, kept through a dip to 50 and a climb - // back to 51, would let the largest run through unacknowledged. - const wrapper = await mountDialog({ seed: null, entries: many(51), preselect: many(51) }); - const row = '[data-test="alert-library-install-alert-k8s/bulk-0"]'; - const backToAlerts = async () => { - await click(wrapper, "alert-library-install-back"); - await click(wrapper, "alert-library-install-back"); - await click(wrapper, "alert-library-install-back"); - }; - const forwardToRun = async () => { - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - }; - + it("drops the acknowledgement when the batch changes under it", async () => { + const wrapper = await mountDialog({ seed: null, preselect: many(51) }); await advanceToInstall(wrapper); await wrapper.find('[data-test="alert-library-install-confirm-large"]').setValue(true); await flushPromises(); expect(runButton(wrapper).attributes("disabled")).toBeUndefined(); - await backToAlerts(); - await wrapper.find(row).setValue(false); + await wrapper.setProps({ preselect: many(52) }); await flushPromises(); - await forwardToRun(); - expect(wrapper.find('[data-test="alert-library-install-confirm-large"]').exists()).toBe( - false, - ); - - await backToAlerts(); - await wrapper.find(row).setValue(true); - await flushPromises(); - await forwardToRun(); - - const confirm = wrapper.find('[data-test="alert-library-install-confirm-large"]'); - expect(confirm.exists()).toBe(true); - expect((confirm.element as HTMLInputElement).checked).toBe(false); - expect(runButton(wrapper).attributes("disabled")).toBeDefined(); - }); - - it("drops the acknowledgement when the batch grows instead of shrinking", async () => { - // The dangerous path: acknowledge 51, go back, Select all — which is what - // B14 exists to stop — and return to a run button that never re-armed. - const wrapper = await mountDialog({ seed: null, entries: many(80), preselect: many(51) }); - await advanceToInstall(wrapper); - await wrapper.find('[data-test="alert-library-install-confirm-large"]').setValue(true); - await flushPromises(); - expect(runButton(wrapper).attributes("disabled")).toBeUndefined(); - - await click(wrapper, "alert-library-install-back"); - await click(wrapper, "alert-library-install-back"); - await click(wrapper, "alert-library-install-back"); - await click(wrapper, "alert-library-install-select-all"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - - const confirm = wrapper.find('[data-test="alert-library-install-confirm-large"]'); - expect((confirm.element as HTMLInputElement).checked).toBe(false); - expect(runButton(wrapper).attributes("disabled")).toBeDefined(); - }); - - it("states the folder and destination it is asking about", async () => { - const wrapper = await mountDialog({ seed: null, entries: many(51), preselect: many(51) }); - await advanceToInstall(wrapper); - - const label = wrapper - .find('[data-test="alert-library-install-confirm-large"]') - .attributes("data-label"); - expect(label).toContain("51"); - expect(label).toContain("ops-slack"); - }); - - it("drops the acknowledgement when the destination changes under it", async () => { - // The tick is a statement about a specific folder and destination — the - // two parameters that decide who gets paged 51 times. - const wrapper = await mountDialog({ seed: null, entries: many(51), preselect: many(51) }); - await advanceToInstall(wrapper); - await wrapper.find('[data-test="alert-library-install-confirm-large"]').setValue(true); - await flushPromises(); - expect(runButton(wrapper).attributes("disabled")).toBeUndefined(); - - for (let back = 0; back < 4; back += 1) await click(wrapper, "alert-library-install-back"); - await wrapper - .get('[data-test="alert-library-install-destination"]') - .setValue("oncall-pagerduty"); - await flushPromises(); - for (let next = 0; next < 4; next += 1) await click(wrapper, "alert-library-install-next"); const confirm = wrapper.get('[data-test="alert-library-install-confirm-large"]'); expect((confirm.element as HTMLInputElement).checked).toBe(false); expect(runButton(wrapper).attributes("disabled")).toBeDefined(); }); + it("states the count and folder it is asking about, and that none notify yet", async () => { + const wrapper = await mountDialog({ seed: null, preselect: many(51) }); + await advanceToInstall(wrapper); + + const label = wrapper + .find('[data-test="alert-library-install-confirm-large"]') + .attributes("data-label"); + expect(label).toContain("51"); + expect(label).toContain("default"); + expect(label).toContain("none of which notify"); + }); + it("drops the acknowledgement when the folder changes under it", async () => { - const wrapper = await mountDialog({ seed: null, entries: many(51), preselect: many(51) }); + const wrapper = await mountDialog({ seed: null, preselect: many(51) }); await advanceToInstall(wrapper); await wrapper.find('[data-test="alert-library-install-confirm-large"]').setValue(true); await flushPromises(); @@ -1587,7 +1119,7 @@ describe("LibraryInstallDialog", () => { }); it("forgets the acknowledgement when the dialog is reopened", async () => { - const wrapper = await mountDialog({ seed: null, entries: many(51), preselect: many(51) }); + const wrapper = await mountDialog({ seed: null, preselect: many(51) }); await advanceToInstall(wrapper); await wrapper.find('[data-test="alert-library-install-confirm-large"]').setValue(true); await flushPromises(); @@ -1602,15 +1134,11 @@ describe("LibraryInstallDialog", () => { expect(runButton(wrapper).attributes("disabled")).toBeDefined(); }); - it("lists every alert in a large batch — the count is not capped", async () => { - const wrapper = await mountDialog({ seed: null, entries: many(100), preselect: many(100) }); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); + it("counts every alert in a large batch — the count is not capped", async () => { + const wrapper = await mountDialog({ seed: null, preselect: many(100) }); + await advanceToInstall(wrapper); - expect(wrapper.findAll('[data-test^="alert-library-install-alert-"]')).toHaveLength(100); - expect( - wrapper.find('[data-test="alert-library-install-count"]').attributes("data-selected"), - ).toBe("100"); + expect(runButton(wrapper).text()).toContain("100"); }); }); @@ -1686,7 +1214,6 @@ describe("LibraryInstallDialog", () => { it("sends each kind of a mixed batch through its own create", async () => { const wrapper = await mountDialog({ seed: null, - entries: [seedEntry(), anomalyEntry()], preselect: [seedEntry(), anomalyEntry()], }); await advanceToInstall(wrapper); @@ -1699,7 +1226,7 @@ describe("LibraryInstallDialog", () => { expect(anomalyPayload()).toMatchObject({ alert_type: "anomaly_detection", enabled: true, - destinations: ["ops-slack"], + destinations: [], folder_id: "default", priority: 3, }); @@ -1717,11 +1244,8 @@ describe("LibraryInstallDialog", () => { const invalidate = vi.spyOn(queryClient, "invalidateQueries"); const wrapper = await mountDialog({ seed: { entry: anomalyEntry(), file: anomalyFile() }, - entries: [anomalyEntry()], }); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(invalidate).toHaveBeenCalledWith({ queryKey: anomalyKeys.all("default") }); @@ -1732,22 +1256,16 @@ describe("LibraryInstallDialog", () => { it("drops anomaly alerts from the batch when the feature turns off mid-dialog", async () => { const wrapper = await mountDialog({ seed: null, - entries: [seedEntry(), anomalyEntry()], preselect: [seedEntry(), anomalyEntry()], }); - const count = () => - wrapper.find('[data-test="alert-library-install-count"]').attributes("data-selected"); - await wrapper.find('[data-test="alert-library-install-destination"]').setValue("ops-slack"); - await click(wrapper, "alert-library-install-next"); - expect(count()).toBe("2"); + const runLabel = () => wrapper.find('[data-test="alert-library-install-run"]').text(); + await advanceToInstall(wrapper); + expect(runLabel()).toContain("2"); setAccess("true", false); await flushPromises(); - expect(count()).toBe("1"); + expect(runLabel()).toContain("1"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); await click(wrapper, "alert-library-install-run"); expect(mocks.createAnomaly).not.toHaveBeenCalled(); expect(mocks.createAlert).toHaveBeenCalledTimes(1); @@ -1756,11 +1274,8 @@ describe("LibraryInstallDialog", () => { it("disables Run when the feature turns off under an anomaly-only batch", async () => { const wrapper = await mountDialog({ seed: { entry: anomalyEntry(), file: anomalyFile() }, - entries: [anomalyEntry()], }); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); + await advanceToInstall(wrapper); setAccess("true", false); await flushPromises(); @@ -1775,12 +1290,9 @@ describe("LibraryInstallDialog", () => { it("says the Tune step applies to threshold alerts only in a mixed batch", async () => { const wrapper = await mountDialog({ seed: null, - entries: [seedEntry(), anomalyEntry()], preselect: [seedEntry(), anomalyEntry()], }); await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); expect(wrapper.find('[data-test="alert-library-install-tune-threshold-only"]').exists()).toBe( true, @@ -1790,12 +1302,10 @@ describe("LibraryInstallDialog", () => { it("skips the Tune step for an anomaly-only batch, both ways", async () => { const wrapper = await mountDialog({ seed: { entry: anomalyEntry(), file: anomalyFile() }, - entries: [anomalyEntry()], }); expect(wrapper.find('[data-test="alert-library-install-tune-step"]').exists()).toBe(false); + expect(stepTitles(wrapper)).toEqual(["Folder", "Add"]); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); await click(wrapper, "alert-library-install-next"); expect(wrapper.find('[data-test="alert-library-install-run"]').exists()).toBe(true); expect(wrapper.find('[data-test="alert-library-install-summary"]').text()).not.toContain( @@ -1803,7 +1313,7 @@ describe("LibraryInstallDialog", () => { ); await click(wrapper, "alert-library-install-back"); - expect(wrapper.find('[data-test="alert-library-install-next"]').exists()).toBe(true); + expect(wrapper.find('[data-test="alert-library-install-folder-step"]').exists()).toBe(true); await click(wrapper, "alert-library-install-next"); expect(wrapper.find('[data-test="alert-library-install-run"]').exists()).toBe(true); }); @@ -1811,12 +1321,9 @@ describe("LibraryInstallDialog", () => { it("notes after the run that anomaly alerts wait for training", async () => { const wrapper = await mountDialog({ seed: { entry: anomalyEntry(), file: anomalyFile() }, - entries: [anomalyEntry()], }); expect(wrapper.find('[data-test="alert-library-install-anomaly-note"]').exists()).toBe(false); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); - await click(wrapper, "alert-library-install-next"); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(wrapper.find('[data-test="alert-library-install-anomaly-note"]').exists()).toBe(true); @@ -1826,27 +1333,25 @@ describe("LibraryInstallDialog", () => { setAccess("false", true); const wrapper = await mountDialog({ seed: null, - entries: [seedEntry(), anomalyEntry()], preselect: [seedEntry(), anomalyEntry()], }); - await click(wrapper, "alert-library-install-next"); + await advanceToInstall(wrapper); + await click(wrapper, "alert-library-install-run"); + expect(mocks.createAnomaly).not.toHaveBeenCalled(); + expect(mocks.createAlert).toHaveBeenCalledTimes(1); expect( wrapper - .find('[data-test="alert-library-install-alert-apm/traces_server_error_ratio_anomaly"]') + .find('[data-test="alert-library-install-result-apm/traces_server_error_ratio_anomaly"]') .exists(), ).toBe(false); - await click(wrapper, "alert-library-install-select-all"); - expect( - wrapper.find('[data-test="alert-library-install-count"]').attributes("data-selected"), - ).toBe("1"); }); it("refuses to send a file declaring an anomaly alert when the feature is off", async () => { setAccess("true", false); mocks.loadAlertFile.mockResolvedValue(anomalyFile()); - const wrapper = await mountDialog({ seed: null, entries: [secondEntry()] }); - await advanceAllToInstall(wrapper); + const wrapper = await mountDialog({ seed: null, preselect: [secondEntry()] }); + await advanceToInstall(wrapper); await click(wrapper, "alert-library-install-run"); expect(mocks.createAnomaly).not.toHaveBeenCalled(); diff --git a/web/src/components/alertLibrary/LibraryInstallDialog.vue b/web/src/components/alertLibrary/LibraryInstallDialog.vue index e1da69b19e..568e340916 100644 --- a/web/src/components/alertLibrary/LibraryInstallDialog.vue +++ b/web/src/components/alertLibrary/LibraryInstallDialog.vue @@ -14,19 +14,7 @@ You should have received a copy of the GNU Affero General Public License along with this program. If not, see . --> - +