diff --git a/python_tests/test_multi_tenant_acl.py b/python_tests/test_multi_tenant_acl.py index e2df05e..ed73286 100644 --- a/python_tests/test_multi_tenant_acl.py +++ b/python_tests/test_multi_tenant_acl.py @@ -341,6 +341,25 @@ def test_dataset_management_requires_a_blanket_write_grant(env): assert "manage" in excinfo.value.message or "all-datasets" in excinfo.value.message +def test_a_dataset_node_via_resources_needs_the_blanket_write_grant(env): + """The same rule, reached by type-label through ``resources.create``. + + Worth a Python test of its own where the ``/datasets`` one is barely worth keeping: this is a + *different* server-side check, and it arrives through a different binding path — the typed + node union that ``resources.create`` accepts. What is asserted here is that a 403 from that + path still surfaces as a typed ``DataHubException`` carrying ``status_code``, rather than + whatever a conversion failure in the union would raise. + """ + writer = client_for(env, "MT_WRITEONLY") + + external_id = unique_id("mt_acl_res_ds") + with pytest.raises(intellistream_datahub_sdk.DataHubException) as excinfo: + writer.resources.create([ + intellistream_datahub_sdk.Dataset(external_id=external_id, name=external_id) + ]) + assert excinfo.value.status_code == 403 + + def test_search_omits_denied_rows_rather_than_raising(env, acl_dataset_id): """A denied search is 200-with-nothing, not 403. diff --git a/src/multi_tenant_integration.rs b/src/multi_tenant_integration.rs index 90ee926..e95af56 100644 --- a/src/multi_tenant_integration.rs +++ b/src/multi_tenant_integration.rs @@ -207,7 +207,7 @@ use crate::generic::{DataWrapper, IdAndExtId, SearchAndFilterForm}; use crate::graph_data_wrapper::GraphDataWrapper; use crate::http::ResponseError; use crate::resources::{RelatedResourcesForm, Resource}; -use crate::nodes::Node; +use crate::nodes::{Node, NodeType, Policy}; use crate::tests::cleanup::{cleanup_datasets_as, cleanup_resources_as, cleanup_timeseries_as}; use crate::{ApiService, TimeSeries}; use chrono::Utc; @@ -1115,6 +1115,164 @@ async fn acl_orphan_entities_need_a_blanket_grant() -> Result<(), ResponseError> Ok(()) } +// --------------------------------------------------------------------------------------------- +// Who may create a data set, and by which route +// --------------------------------------------------------------------------------------------- +// +// Two endpoints reach the same rule from different directions, and they are checked separately +// server-side. `/datasets/create` has always demanded the blanket grant. `/resources/create` +// only started demanding it once a node's type became something a caller picks with a label: +// before that, a per-dataset writer could mint a `DATASET`-labelled node and keep mutating it +// under a grant that was never meant to cover data-set management. Both routes are covered here, +// and the allow side as well as the deny side — a rule that only ever denies is indistinguishable +// from one that denies everything. + +/// The blanket grant **is** sufficient, not merely necessary. +/// +/// [`acl_dataset_management_requires_a_blanket_write_grant`] proves a per-dataset grant is not +/// enough. On its own that is satisfied by a backend that refuses everyone, and the suite would +/// stay green through a change that made data sets uncreatable — the failures would land in +/// unrelated fixtures and read as fixture breakage. +#[tokio::test] +#[ignore] +async fn acl_a_blanket_write_grant_can_create_a_dataset() -> Result<(), ResponseError> { + const TEST: &str = "acl_a_blanket_write_grant_can_create_a_dataset"; + let Some(admin) = principal(TEST, "MT_ORG_A", Some(SCOPE_ALL_ORGS)) else { + return Ok(()); + }; + + let dataset = Dataset::new(unique_id("acl_allow_ds")); + let ext_id = dataset.external_id.clone(); + let mut guard = cleanup_datasets_as(admin.config.clone(), vec![ext_id.clone()]); + + admin.service.datasets.create(&dataset).await?; + assert!( + admin.datasets_by_external_id(&ext_id).await?.is_some(), + "the data set should exist after a create by a principal holding /datasets/*/write", + ); + + admin.service.datasets.delete(&by_external_id(&ext_id)).await?; + guard.disarm(); + Ok(()) +} + +/// Read-only and no-grant principals are refused too, not just the per-dataset writer. +/// +/// The existing denial test uses `MT_WRITEONLY`, which is the interesting near-miss — it holds a +/// *write* grant, just a scoped one. These two are the plain cases, and they are what would catch +/// a check that keyed on "holds any write grant" rather than on the blanket one. +#[tokio::test] +#[ignore] +async fn acl_a_lesser_grant_cannot_create_a_dataset() -> Result<(), ResponseError> { + const TEST: &str = "acl_a_lesser_grant_cannot_create_a_dataset"; + let (Some(reader), Some(nogrant)) = ( + principal(TEST, "MT_READONLY", Some(SCOPE_ALL_ORGS)), + principal(TEST, "MT_NOGRANT", Some(SCOPE_ALL_ORGS)), + ) else { + return Ok(()); + }; + + for who in [&reader, &nogrant] { + let dataset = Dataset::new(unique_id("acl_deny_ds")); + let guard = cleanup_datasets_as(who.config.clone(), vec![dataset.external_id.clone()]); + let error = assert_status( + who.service.datasets.create(&dataset).await, + 403, + &format!("{} creating a data set", who.label), + ); + assert!( + error.get_message().contains("manage") + || error.get_message().contains("all-datasets"), + "{}: the denial should say a blanket grant is required — got: {}", + who.label, + error.get_message() + ); + drop(guard); + } + Ok(()) +} + +/// The same rule, reached through `/resources/create` by type-label rather than `/datasets`. +/// +/// This is the route the typed node surface makes natural — `resources.create(vec![Dataset::new +/// (..)])` — and it is gated by its own check server-side, so proving `/datasets/create` is +/// guarded says nothing about it. A `POLICY` node is covered in the same pass because it shares +/// the gate, and the lowercase label is deliberate: the type-label is canonicalised before it is +/// matched, so a check that compared the raw string would let `dataset` through. +#[tokio::test] +#[ignore] +async fn acl_a_dataset_or_policy_node_via_resources_needs_the_blanket_grant( +) -> Result<(), ResponseError> { + const TEST: &str = "acl_a_dataset_or_policy_node_via_resources_needs_the_blanket_grant"; + let (Some(writer), Some(admin)) = ( + principal(TEST, "MT_WRITEONLY", Some(SCOPE_ALL_ORGS)), + principal(TEST, "MT_ORG_A", Some(SCOPE_ALL_ORGS)), + ) else { + return Ok(()); + }; + + // (node, description) — the third is the same data set type named in lower case. + let cases: Vec<(Node, &str)> = vec![ + (Dataset::new(unique_id("acl_res_ds")).into(), "a DATASET node"), + ( + { + // A policy needs its `type`: bean validation runs *before* the ACL gate, so an + // otherwise-invalid body earns a 400 and never reaches the permission check. + // Without this the test would "pass" against a backend with no gate at all. + let mut p = Policy::new(&unique_id("acl_res_policy"), "acl probe policy"); + p.policy_type = Some("IS_WRITE_PROTECTED".to_string()); + p.value = Some(serde_json::Value::String("TRUE".to_string())); + p.into() + }, + "a POLICY node", + ), + ( + { + let mut r = Resource::new(); + r.external_id = unique_id("acl_res_lower"); + r.name = "acl probe lowercase".to_string(); + r.labels = Some(vec!["dataset".to_string()]); + r.into() + }, + "a node labelled `dataset` in lower case", + ), + ]; + + for (node, what) in cases { + let guard = + cleanup_resources_as(writer.config.clone(), vec![node.external_id().to_string()]); + assert_status( + writer.service.resources.create(vec![node], vec![]).await, + 403, + &format!("a per-data-set write grant creating {what} through /resources"), + ); + drop(guard); + } + + // ...and the blanket grant gets through the same door. + let allowed = Dataset::new(unique_id("acl_res_ds_ok")); + let ext_id = allowed.external_id.clone(); + let mut guard = cleanup_resources_as(admin.config.clone(), vec![ext_id.clone()]); + let created = admin + .service + .resources + .create(vec![Node::from(allowed)], vec![]) + .await?; + assert_eq!( + created.nodes().unwrap_or_default().first().map(Node::kind), + Some(NodeType::Dataset), + "the create echo should come back typed as a data set", + ); + + admin + .service + .resources + .delete(&by_external_id(&ext_id)) + .await?; + guard.disarm(); + Ok(()) +} + // --------------------------------------------------------------------------------------------- // How an ACL denial interacts with durable ingest buffering // --------------------------------------------------------------------------------------------- diff --git a/src/nodes.rs b/src/nodes.rs index 89518a0..644938a 100644 --- a/src/nodes.rs +++ b/src/nodes.rs @@ -260,6 +260,10 @@ pub struct Policy { #[serde(default, skip_serializing_if = "Option::is_none")] pub description: Option, /// The policy kind, e.g. `IS_WRITE_PROTECTED`. Named `type` on the wire. + /// + /// Optional here because reads never carry it, but **required on a create**: the api declares + /// it `@NotNull`, and bean validation runs before the permission check, so a policy without + /// one is a 400 naming `nodes[N].type` rather than whatever the caller was testing for. #[serde(rename = "type", default, skip_serializing_if = "Option::is_none")] pub policy_type: Option, /// The policy's value. The api declares it `Object`, so it is any JSON scalar.