From 4686abd8d6c4440690cd7a8dc16d8be66c2373e8 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Fri, 14 Aug 2026 12:42:56 -0400 Subject: [PATCH 1/7] feat(auth): authorize query-auth reads and carry the grant on the split --- crates/paimon/src/api/api_response.rs | 17 +- crates/paimon/src/spec/schema.rs | 29 +- .../src/table/batch_vector_search_builder.rs | 9 +- crates/paimon/src/table/cow_writer.rs | 4 +- crates/paimon/src/table/format_table_read.rs | 10 +- crates/paimon/src/table/format_table_scan.rs | 16 +- .../src/table/full_text_search_builder.rs | 21 +- .../paimon/src/table/hybrid_search_builder.rs | 14 +- crates/paimon/src/table/incremental_scan.rs | 20 +- .../src/table/lumina_index_build_builder.rs | 4 +- crates/paimon/src/table/mod.rs | 138 ++++++ crates/paimon/src/table/query_auth.rs | 321 ++++++++++++ crates/paimon/src/table/read_builder.rs | 35 +- crates/paimon/src/table/rest_env.rs | 78 ++- .../sorted_global_index_build_builder.rs | 4 +- crates/paimon/src/table/source.rs | 77 +++ crates/paimon/src/table/table_read.rs | 388 ++++++++++++++- crates/paimon/src/table/table_scan.rs | 143 +++++- crates/paimon/src/table/vector_scan.rs | 22 +- .../paimon/src/table/vector_search_builder.rs | 27 +- .../src/table/vindex_index_build_builder.rs | 4 +- crates/paimon/tests/mock_server.rs | 149 +++++- crates/paimon/tests/rest_catalog_test.rs | 459 ++++++++++++++++++ 23 files changed, 1909 insertions(+), 80 deletions(-) create mode 100644 crates/paimon/src/table/query_auth.rs diff --git a/crates/paimon/src/api/api_response.rs b/crates/paimon/src/api/api_response.rs index 20d1b7b77..f8bd2cfdd 100644 --- a/crates/paimon/src/api/api_response.rs +++ b/crates/paimon/src/api/api_response.rs @@ -487,8 +487,11 @@ pub struct GetTableTokenResponse { /// Response for auth table query: the per-user row filter and column masking the /// client must enforce at read time for a `query-auth.enabled` table. +/// +/// Unknown fields are rejected: an absent one reads as "no rule", so protocol +/// drift would look like an unrestricted grant. #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase")] +#[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct AuthTableQueryResponse { /// JSON-serialized row-filter predicates, ANDed together. Empty/None = no filter. pub filter: Option>, @@ -525,6 +528,18 @@ impl ListPermissionsResponse { #[cfg(test)] mod tests { + + #[test] + fn test_auth_table_query_response_rejects_unknown_fields() { + let drifted = r#"{"rowFilter":["restricted"]}"#; + assert!( + serde_json::from_str::(drifted).is_err(), + "an auth response this client does not understand must not parse" + ); + assert!(serde_json::from_str::("{}") + .unwrap() + .is_unrestricted()); + } use super::*; #[test] diff --git a/crates/paimon/src/spec/schema.rs b/crates/paimon/src/spec/schema.rs index 1e020effb..93151a05e 100644 --- a/crates/paimon/src/spec/schema.rs +++ b/crates/paimon/src/spec/schema.rs @@ -638,22 +638,27 @@ impl TableSchema { } } -/// Reject column names reserved for system use, mirroring Java `SpecialFields`: -/// the five `SYSTEM_FIELD_NAMES` and the `_KEY_` key-field prefix. +/// Whether `name` is one Paimon reserves for a system column. Java +/// `SpecialFields.SYSTEM_FIELD_NAMES` plus the `_KEY_` key-field prefix. +pub(crate) fn is_reserved_system_field_name(name: &str) -> bool { + name.starts_with(KEY_FIELD_PREFIX) || SYSTEM_FIELD_NAMES.contains(&name) +} + +// Java SpecialFields.SYSTEM_FIELD_NAMES. +const SYSTEM_FIELD_NAMES: [&str; 5] = [ + SEQUENCE_NUMBER_FIELD_NAME, + VALUE_KIND_FIELD_NAME, + "_LEVEL", + ROW_KIND_FIELD_NAME, + ROW_ID_FIELD_NAME, +]; +const KEY_FIELD_PREFIX: &str = "_KEY_"; + +/// Reject column names reserved for system use, mirroring Java `SpecialFields`. /// /// A user column colliding with a system field is otherwise excluded from the /// physical read and silently filled with the system value. fn validate_no_reserved_field_names(fields: &[DataField]) -> crate::Result<()> { - // Java SpecialFields.SYSTEM_FIELD_NAMES. - const SYSTEM_FIELD_NAMES: [&str; 5] = [ - SEQUENCE_NUMBER_FIELD_NAME, - VALUE_KIND_FIELD_NAME, - "_LEVEL", - ROW_KIND_FIELD_NAME, - ROW_ID_FIELD_NAME, - ]; - const KEY_FIELD_PREFIX: &str = "_KEY_"; - for field in fields { let name = field.name(); if name.starts_with(KEY_FIELD_PREFIX) || SYSTEM_FIELD_NAMES.contains(&name) { diff --git a/crates/paimon/src/table/batch_vector_search_builder.rs b/crates/paimon/src/table/batch_vector_search_builder.rs index f80a7b248..5d42e791e 100644 --- a/crates/paimon/src/table/batch_vector_search_builder.rs +++ b/crates/paimon/src/table/batch_vector_search_builder.rs @@ -114,6 +114,8 @@ impl<'a> BatchVectorSearchBuilder<'a> { self.filter.as_ref(), self.include_row_ids.as_ref(), self.prepared_filter.as_ref(), + // Nothing delegates to the batch builder, so it always asks. + false, ) } @@ -151,8 +153,13 @@ impl<'a> BatchVectorSearchBuilder<'a> { /// Search every query against one plan, including empty per-query results. pub async fn execute(&self) -> crate::Result> { + // Before any validation or fast path, and once: the scan is told so. + self.table + .ensure_read_authorized_live("a vector search") + .await?; let read = self.new_read()?; - read.read(self.new_scan()?.plan().await?).await + let scan = self.new_scan()?.assume_authorized(); + read.read(scan.plan().await?).await } fn column(&self) -> crate::Result<&str> { diff --git a/crates/paimon/src/table/cow_writer.rs b/crates/paimon/src/table/cow_writer.rs index d0913e26d..062234513 100644 --- a/crates/paimon/src/table/cow_writer.rs +++ b/crates/paimon/src/table/cow_writer.rs @@ -207,7 +207,9 @@ impl CopyOnWriteMergeWriter { #[must_use = "commit messages must be passed to TableCommit"] pub async fn prepare_commit(self) -> Result> { // A copy-on-write rewrite reads the rows it replaces. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table + .ensure_read_authorized_live("a copy-on-write rewrite") + .await?; if self.affected_files.is_empty() { return Ok(Vec::new()); diff --git a/crates/paimon/src/table/format_table_read.rs b/crates/paimon/src/table/format_table_read.rs index 9f0814d6f..3b81012a1 100644 --- a/crates/paimon/src/table/format_table_read.rs +++ b/crates/paimon/src/table/format_table_read.rs @@ -103,7 +103,15 @@ impl<'a> FormatTableRead<'a> { data_splits: &[DataSplit], ) -> crate::Result { let core_options = self.table.schema().core_options(); - core_options.ensure_read_authorized()?; + core_options.ensure_type_paimon_served(&self.table.identifier().full_name())?; + // Sync, so the marker stands in for asking the server. + if core_options.query_auth_enabled() + || data_splits.iter().any(|split| split.query_auth_required()) + { + return Err(super::query_auth::unsupported( + "a format table cannot apply a row filter or column masking", + )); + } // Mapping the conjunct onto the data fields drops it, so the read would // silently ignore the filter. Guard on the read path, not the builder: // `TableRead` is public and can be constructed and filtered directly. diff --git a/crates/paimon/src/table/format_table_scan.rs b/crates/paimon/src/table/format_table_scan.rs index b002e88a8..e53b8a365 100644 --- a/crates/paimon/src/table/format_table_scan.rs +++ b/crates/paimon/src/table/format_table_scan.rs @@ -64,20 +64,28 @@ impl<'a> FormatTableScan<'a> { } pub(crate) async fn plan(&self) -> crate::Result { - self.ensure_query_auth_allowed()?; + self.ensure_query_auth_allowed().await?; self.plan_inner(None).await } pub(crate) async fn plan_with_trace(&self) -> crate::Result<(Plan, ScanTrace)> { - self.ensure_query_auth_allowed()?; + self.ensure_query_auth_allowed().await?; let mut trace = ScanTrace::default(); let plan = self.plan_inner(Some(&mut trace)).await?; trace.planned_data_file_bytes = plan.planned_data_file_bytes(); Ok((plan, trace)) } - fn ensure_query_auth_allowed(&self) -> crate::Result<()> { - CoreOptions::new(self.table.schema().options()).ensure_read_authorized() + /// Refused outright. Asks the server: the option can be set after a load. + async fn ensure_query_auth_allowed(&self) -> crate::Result<()> { + let core_options = CoreOptions::new(self.table.schema().options()); + core_options.ensure_type_paimon_served(&self.table.identifier().full_name())?; + if self.table.server_query_auth_enabled().await? { + return Err(super::query_auth::unsupported( + "a format table cannot apply a row filter or column masking", + )); + } + Ok(()) } async fn plan_inner(&self, trace: Option<&mut ScanTrace>) -> crate::Result { diff --git a/crates/paimon/src/table/full_text_search_builder.rs b/crates/paimon/src/table/full_text_search_builder.rs index fac4db4a6..5bd9be62f 100644 --- a/crates/paimon/src/table/full_text_search_builder.rs +++ b/crates/paimon/src/table/full_text_search_builder.rs @@ -63,6 +63,8 @@ const FULL_TEXT_INDEX_SEARCH_CONCURRENCY: usize = 8; /// Reference: `org.apache.paimon.table.source.FullTextSearchBuilder` pub struct FullTextSearchBuilder<'a> { table: &'a Table, + /// Set when the caller already asked, so a delegated search does not repeat it. + authorized: bool, text_column: Option, query_text: Option, limit: Option, @@ -70,8 +72,15 @@ pub struct FullTextSearchBuilder<'a> { } impl<'a> FullTextSearchBuilder<'a> { + /// The caller already asked the server for this operation. + pub(crate) fn assume_authorized(mut self) -> Self { + self.authorized = true; + self + } + pub(crate) fn new(table: &'a Table) -> Self { Self { + authorized: false, table, text_column: None, query_text: None, @@ -117,7 +126,11 @@ impl<'a> FullTextSearchBuilder<'a> { pub async fn execute_scored(&self) -> crate::Result { // Fail closed: returns data-derived row ranges outside `TableScan`/`TableRead`. let core = CoreOptions::new(self.table.schema().options()); - core.ensure_read_authorized()?; + if !self.authorized { + self.table + .ensure_read_authorized_live("a full-text search") + .await?; + } let text_column = self.text_column .as_deref() @@ -204,7 +217,11 @@ impl<'a> FullTextSearchBuilder<'a> { pub async fn execute_read(&self) -> crate::Result { // Fail closed: returns data outside `TableScan`/`TableRead`. let core = CoreOptions::new(self.table.schema().options()); - core.ensure_read_authorized()?; + if !self.authorized { + self.table + .ensure_read_authorized_live("a full-text search") + .await?; + } let text_column = self.text_column .as_deref() diff --git a/crates/paimon/src/table/hybrid_search_builder.rs b/crates/paimon/src/table/hybrid_search_builder.rs index 2ef9be1ae..1515ae0f2 100644 --- a/crates/paimon/src/table/hybrid_search_builder.rs +++ b/crates/paimon/src/table/hybrid_search_builder.rs @@ -286,7 +286,9 @@ impl<'a> HybridSearchBuilder<'a> { pub async fn execute_scored(&self) -> crate::Result { let core = CoreOptions::new(self.table.schema().options()); - core.ensure_read_authorized()?; + self.table + .ensure_read_authorized_live("a hybrid search") + .await?; let limit = self.limit.ok_or_else(|| crate::Error::ConfigInvalid { message: "Limit must be set via with_limit()".to_string(), })?; @@ -317,7 +319,7 @@ impl<'a> HybridSearchBuilder<'a> { for route in &self.routes { let result = match route.kind { HybridSearchRouteKind::Vector => { - let mut builder = self.table.new_vector_search_builder(); + let mut builder = self.table.new_vector_search_builder().assume_authorized(); builder .with_vector_column(&route.field_name) .with_query_vector(route.vector.clone().expect("validated vector route")) @@ -350,7 +352,9 @@ impl<'a> HybridSearchBuilder<'a> { /// `execute`/`execute_scored`. Mirrors Java `HybridSearchBuilderImpl` PK path. pub async fn execute_read(&self) -> crate::Result { let core = CoreOptions::new(self.table.schema().options()); - core.ensure_read_authorized()?; + self.table + .ensure_read_authorized_live("a hybrid search") + .await?; let limit = self.limit.ok_or_else(|| crate::Error::ConfigInvalid { message: "Limit must be set via with_limit()".to_string(), })?; @@ -567,7 +571,7 @@ impl<'a> HybridSearchBuilder<'a> { route: &HybridSearchRoute, ) -> crate::Result { let vector = route.vector.as_deref().expect("validated vector route"); - let mut builder = table.new_vector_search_builder(); + let mut builder = table.new_vector_search_builder().assume_authorized(); builder .with_vector_column(&route.field_name) .with_query_vector(vector.to_vec()) @@ -894,7 +898,7 @@ async fn execute_full_text_route( table: &Table, route: &HybridSearchRoute, ) -> crate::Result { - let mut builder = table.new_full_text_search_builder(); + let mut builder = table.new_full_text_search_builder().assume_authorized(); builder .with_text_column(&route.field_name) .with_query_text( diff --git a/crates/paimon/src/table/incremental_scan.rs b/crates/paimon/src/table/incremental_scan.rs index 747278836..e9ac52b81 100644 --- a/crates/paimon/src/table/incremental_scan.rs +++ b/crates/paimon/src/table/incremental_scan.rs @@ -149,6 +149,18 @@ impl IncrementalPlan { &self.splits } + /// Whether any underlying split came from a query-auth plan. Unlike + /// [`Self::data_splits`] this sees the diff pairs too. + pub(crate) fn any_query_auth_required(&self) -> bool { + self.splits.iter().any(|split| match split { + IncrementalSplit::Data(split) => split.query_auth_required(), + IncrementalSplit::DiffPair { before, after } => before + .iter() + .chain(after) + .any(DataSplit::query_auth_required), + }) + } + pub fn data_splits(&self) -> Vec { self.splits .iter() @@ -244,7 +256,13 @@ impl<'a> IncrementalScan<'a> { } pub async fn plan(&self) -> crate::Result { - crate::spec::CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + let core_options = crate::spec::CoreOptions::new(self.table.schema().options()); + core_options.ensure_type_paimon_served(&self.table.identifier().full_name())?; + if self.table.server_query_auth_enabled().await? { + return Err(super::query_auth::unsupported( + "an incremental read cannot apply a row filter or column masking", + )); + } let mode = self.resolve_mode(); self.validate_snapshot_range(mode).await?; if self.start_exclusive == self.end_inclusive { diff --git a/crates/paimon/src/table/lumina_index_build_builder.rs b/crates/paimon/src/table/lumina_index_build_builder.rs index e50a7311c..de846f31e 100644 --- a/crates/paimon/src/table/lumina_index_build_builder.rs +++ b/crates/paimon/src/table/lumina_index_build_builder.rs @@ -70,7 +70,9 @@ impl<'a> LuminaIndexBuildBuilder<'a> { pub async fn execute(&self) -> Result { // Building the index scans the table's rows. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table + .ensure_read_authorized_live("building an index") + .await?; self.table.ensure_not_branch_reference_for_write()?; diff --git a/crates/paimon/src/table/mod.rs b/crates/paimon/src/table/mod.rs index cc2e0c629..9371e2e7e 100644 --- a/crates/paimon/src/table/mod.rs +++ b/crates/paimon/src/table/mod.rs @@ -90,6 +90,7 @@ mod postpone_fixed_bucket_router; mod postpone_fixed_bucket_write; mod postpone_fixed_bucket_write_builder; mod prepared_files; +mod query_auth; mod read_builder; pub mod referenced_files; pub(crate) mod rest_env; @@ -200,6 +201,9 @@ pub struct Table { schema_manager: SchemaManager, branch: String, branch_reference: bool, + /// Minted only by [`RESTEnv::build_table`], so a handle assembled with the + /// public [`Table::new`] cannot replay a grant. + query_auth_session: Option, rest_env: Option, /// True when this table copy was switched to a historical schema by /// [`Table::copy_with_time_travel`]. Such a copy is read-only. @@ -229,6 +233,7 @@ impl Table { schema_manager, branch, branch_reference: false, + query_auth_session: None, rest_env, time_traveled: false, travel_snapshot: None, @@ -269,6 +274,7 @@ impl Table { schema_manager, branch, branch_reference, + query_auth_session: None, rest_env: None, time_traveled: false, travel_snapshot: None, @@ -352,6 +358,109 @@ impl Table { } } + /// The live counterpart of [`CoreOptions::ensure_read_authorized`], which + /// reads the schema this handle was loaded with. Paths that can await but + /// cannot apply the server's rules must ask instead. + pub(crate) async fn ensure_read_authorized_live(&self, path: &str) -> Result<()> { + let local = CoreOptions::new(self.schema.options()); + local.ensure_type_paimon_served(&self.identifier.full_name())?; + if self.server_query_auth_enabled().await? { + return Err(query_auth::unsupported(&format!( + "{path} reads index files directly and cannot apply a row filter or column masking" + ))); + } + Ok(()) + } + + /// Whether the server says this table is `query-auth.enabled` right now: the + /// handle's schema is a snapshot, and a cached `false` would skip the check. + pub(crate) async fn server_query_auth_enabled(&self) -> Result { + let local = CoreOptions::new(self.schema.options()).query_auth_enabled(); + let Some(rest_env) = &self.rest_env else { + return Ok(local); + }; + // Only ever strengthens: the name can be re-created over this handle's + // files, so the answer may be about a different table. + if local { + return Ok(true); + } + match rest_env.current_table().await?.schema.as_ref() { + Some(schema) => Ok(CoreOptions::new(schema.options()).query_auth_enabled()), + None => Ok(true), + } + } + + /// Whether this user may read this table; `None` when it is not + /// `query-auth.enabled`. `server_query_auth` is the caller's already-fetched + /// [`Self::server_query_auth_enabled`], so planning asks the server once. + pub(crate) async fn authorize_read( + &self, + server_query_auth: bool, + ) -> Result>> { + let local = CoreOptions::new(self.schema.options()); + // Ask the selector too: `copy_with_options` adds one without the flag. + let travels = local.try_time_travel_selector()?.is_some(); + // A `$branch_x` or `$files` handle authorizes against the decorated + // name while its managers read the base table's own files. + let decorated = self.identifier.branch_name()?.is_some() + || self.identifier.system_table_name()?.is_some(); + if (travels || self.time_traveled || self.branch_reference || decorated) + && local.query_auth_enabled() + { + return Err(query_auth::unsupported( + "a time-travelled or branch read authorizes against the table's current schema, \ + which is not the one it reads", + )); + } + + let Some(rest_env) = &self.rest_env else { + // Only a REST catalog can authorize. + return if local.query_auth_enabled() { + Err(query_auth::unsupported( + "it requires a REST catalog to authorize the query", + )) + } else { + Ok(None) + }; + }; + + // No freshness assertion yet — an ordinary table must not inherit one. + if !server_query_auth { + return Ok(None); + } + if travels || self.time_traveled || self.branch_reference || decorated { + return Err(query_auth::unsupported( + "a time-travelled or branch read authorizes against the table's current schema, \ + which is not the one it reads", + )); + } + + // Before any RPC: only the catalog mints a session, so a handle the + // caller assembled stops here whatever name or files it wears. + let session = self.query_auth_session.ok_or_else(|| { + query_auth::unsupported("this table handle was assembled rather than loaded") + })?; + + // Naming a system column here would fail the server's column check. + let response = rest_env + .table_query_auth(&self.branch, self.schema.id(), None) + .await?; + Ok(Some(std::sync::Arc::new(query_auth::QueryAuthGrant::new( + response, session, + )))) + } + + /// Handed out once per catalog-loaded table; wraps only after 2^64 loads. + pub(crate) fn with_query_auth_session(mut self) -> Self { + static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + self.query_auth_session = Some(NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed)); + self + } + + pub(crate) fn query_auth_session(&self) -> Option { + self.query_auth_session + } + /// Get the REST environment, if this table was loaded from a REST catalog. pub fn rest_env(&self) -> Option<&RESTEnv> { self.rest_env.as_ref() @@ -460,6 +569,7 @@ impl Table { schema_manager: self.schema_manager.clone(), branch: self.branch.clone(), branch_reference: self.branch_reference, + query_auth_session: self.query_auth_session, rest_env: self.rest_env.clone(), time_traveled: self.time_traveled, travel_snapshot: if selector_changed { @@ -508,6 +618,7 @@ impl Table { schema_manager: self.schema_manager.clone(), branch: self.branch.clone(), branch_reference: self.branch_reference, + query_auth_session: self.query_auth_session, rest_env: self.rest_env.clone(), time_traveled: true, travel_snapshot: Some(snapshot.clone()), @@ -669,6 +780,7 @@ impl Table { schema_manager, branch, branch_reference: true, + query_auth_session: self.query_auth_session, rest_env: self.rest_env.clone(), time_traveled: false, travel_snapshot: None, @@ -703,6 +815,32 @@ pub(crate) fn find_field_id_by_name(fields: &[DataField], name: &str) -> Option< fields.iter().find(|f| f.name() == name).map(|f| f.id()) } +/// A `query-auth.enabled` table wired to its own REST session. +#[cfg(test)] +pub(crate) async fn rest_query_auth_table() -> Table { + use crate::api::rest_api::RESTApi; + use crate::common::{CatalogOptions, Options}; + + let mut options = Options::default(); + options.set(CatalogOptions::URI, "http://127.0.0.1:1"); + options.set("token.provider", "bear"); + options.set("token", "test_token"); + let api = std::sync::Arc::new(RESTApi::new(options.clone(), false).await.unwrap()); + let table = query_auth_table(); + Table { + rest_env: Some(RESTEnv::new( + table.identifier.clone(), + "uuid-1".to_string(), + api, + options, + false, + None, + )), + ..table + } + .with_query_auth_session() +} + /// A minimal table with `query-auth.enabled = true`, for the fail-closed read guard. #[cfg(test)] pub(crate) fn query_auth_table() -> Table { diff --git a/crates/paimon/src/table/query_auth.rs b/crates/paimon/src/table/query_auth.rs new file mode 100644 index 000000000..721e10b8b --- /dev/null +++ b/crates/paimon/src/table/query_auth.rs @@ -0,0 +1,321 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! What the REST server authorized a user to read from one table. + +use crate::api::AuthTableQueryResponse; + +/// The server's answer for one user on one table, kept unparsed. +/// +/// `session` pins it to the handle that asked: `to_arrow` is public and the +/// response names neither table nor principal. Routing options are unbound on +/// purpose — sound only while unrestricted grants authorize. +#[derive(Debug, PartialEq)] +pub(crate) struct QueryAuthGrant { + response: AuthTableQueryResponse, + session: u64, +} + +impl QueryAuthGrant { + pub(crate) fn new(response: AuthTableQueryResponse, session: u64) -> Self { + Self { response, session } + } + + /// The only case this client can serve. + pub(crate) fn is_unrestricted(&self) -> bool { + self.response.is_unrestricted() + } + + /// Travelled and branch views read a schema the server did not rule on. + /// Everything else follows from the session, which only the catalog mints. + pub(crate) fn matches_table(&self, table: &super::Table) -> bool { + !table.is_time_traveled() + && !table.is_branch_reference() + && table.query_auth_session() == Some(self.session) + } +} + +/// `value_stats` and `write_cols` are public on every split and an older file +/// can name a dropped column. Refused rather than scrubbed: rewriting encoded +/// stats is how bounds get mismatched. +pub(crate) async fn reject_unauthorized_stats( + plan: &super::Plan, + current: &crate::spec::TableSchema, + schemas: &super::schema_manager::SchemaManager, +) -> crate::Result<()> { + let refuse = |column: &str| { + Err(unsupported(&format!( + "a data file still carries statistics for '{column}', which the current schema — the \ + one the server authorized — does not have" + ))) + }; + let named = |name: &String| current.fields().iter().any(|f| f.name() == name); + let mut checked = std::collections::HashSet::new(); + for split in plan.splits() { + for file in split.data_files() { + for column in file + .value_stats_cols + .iter() + .chain(file.write_cols.iter()) + .flatten() + { + if !named(column) { + return refuse(column); + } + } + // The file's own schema is the authority: a name can be dropped and + // re-added under a new id, and the lists may be absent entirely. + if file.schema_id == current.id() || !checked.insert(file.schema_id) { + continue; + } + let older = schemas.schema(file.schema_id).await?; + if let Some(gone) = older.fields().iter().find(|f| { + !current.fields().iter().any(|c| { + c.id() == f.id() && c.name() == f.name() && c.data_type() == f.data_type() + }) + }) { + return refuse(gone.name()); + } + } + } + Ok(()) +} + +/// A refusal naming the option, so callers never match on prose. +pub(crate) fn unsupported(reason: &str) -> crate::Error { + crate::Error::Unsupported { + message: format!( + "reading a table with 'query-auth.enabled' = true is not supported: {reason}" + ), + } +} + +/// Column permissions cover real schema fields, so the server can neither grant +/// nor refuse `_ROW_ID` and friends. +pub(crate) fn reject_system_columns<'a>( + names: impl IntoIterator, +) -> crate::Result<()> { + for name in names { + if crate::spec::is_reserved_system_field_name(name) { + return Err(unsupported(&format!( + "the system column '{name}' is not one the server can authorize: column \ + permissions are granted over table columns" + ))); + } + } + Ok(()) +} + +/// The read resolves older files by field id, so a non-canonical `(id, name)` +/// pair reads as something no grant covered. System fields have no entry. +pub(crate) fn reject_noncanonical_fields( + read_type: &[crate::spec::DataField], + schema_fields: &[crate::spec::DataField], +) -> crate::Result<()> { + for field in read_type { + if crate::spec::is_reserved_system_field_name(field.name()) { + continue; + } + // The whole type: an older field can keep `(id, name)` and carry an extra + // nested child. Nested ids are unassigned, so a legitimate read type + // carries the schema field's shape whole. + let canonical = schema_fields.iter().any(|f| { + f.id() == field.id() && f.name() == field.name() && f.data_type() == field.data_type() + }); + if !canonical { + return Err(unsupported(&format!( + "'{}' (field id {}) is not a column of the current schema, which is what the \ + server authorized", + field.name(), + field.id() + ))); + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::reject_system_columns; + use crate::table::query_auth_table; + + #[tokio::test] + async fn test_a_grant_is_pinned_to_the_handle_that_obtained_it() { + let a = crate::table::rest_query_auth_table().await; + let b = crate::table::rest_query_auth_table().await; + let grant = super::QueryAuthGrant::new( + crate::api::AuthTableQueryResponse::default(), + a.query_auth_session().unwrap(), + ); + assert!(grant.matches_table(&a)); + assert!( + !grant.matches_table(&b), + "another handle — another principal or another table — must not reuse it" + ); + } + + #[tokio::test] + async fn test_a_time_travel_selector_alone_is_refused() { + for selector in [ + "scan.snapshot-id", + "scan.version", + "scan.tag-name", + "scan.timestamp-millis", + "scan.watermark", + ] { + let table = query_auth_table().copy_with_options(std::collections::HashMap::from([( + selector.to_string(), + "1".to_string(), + )])); + assert!(!table.is_time_traveled(), "{selector} sets no flag"); + let err = table.authorize_read(true).await.unwrap_err(); + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("time-travelled or branch read")), + "{selector}: {err:?}" + ); + } + } + + #[tokio::test] + async fn test_a_grant_does_not_cross_into_a_travelled_or_branch_view() { + let table = crate::table::rest_query_auth_table().await; + let grant = super::QueryAuthGrant::new( + crate::api::AuthTableQueryResponse::default(), + table.query_auth_session().unwrap(), + ); + assert!(grant.matches_table(&table)); + + let mut travelled = table.copy_with_options(std::collections::HashMap::new()); + travelled.time_traveled = true; + assert!( + !grant.matches_table(&travelled), + "an older schema is not the one the server ruled on" + ); + + let assembled = crate::table::Table::new( + table.file_io().clone(), + table.identifier().clone(), + "/tmp/somewhere-else".to_string(), + table.schema().clone(), + table.rest_env().cloned(), + ); + assert!( + !grant.matches_table(&assembled), + "an assembled handle must not replay a grant" + ); + + let mut branch = table.copy_with_options(std::collections::HashMap::new()); + branch.branch_reference = true; + assert!( + !grant.matches_table(&branch), + "a branch view is refused even when its schema id coincides" + ); + } + + #[tokio::test] + async fn test_stats_for_a_dropped_column_are_refused() { + let table = query_auth_table(); + let file = + |cols: Option>, written: Option>| crate::spec::DataFileMeta { + file_name: "f.parquet".to_string(), + file_size: 1, + row_count: 1, + min_key: Vec::new(), + max_key: Vec::new(), + key_stats: crate::spec::stats::BinaryTableStats::empty(), + value_stats: crate::spec::stats::BinaryTableStats::empty(), + min_sequence_number: 0, + max_sequence_number: 0, + schema_id: table.schema().id(), + level: 0, + extra_files: Vec::new(), + creation_time: None, + delete_row_count: Some(0), + embedded_index: None, + file_source: None, + value_stats_cols: cols.map(|c| c.iter().map(|s| s.to_string()).collect()), + external_path: None, + first_row_id: None, + write_cols: written.map(|c| c.iter().map(|s| s.to_string()).collect()), + column_max_sequence_numbers: None, + }; + let plan_of = |meta| { + crate::table::Plan::new(vec![crate::table::DataSplitBuilder::new() + .with_snapshot(1) + .with_partition(crate::spec::BinaryRowBuilder::new(0).build()) + .with_bucket(0) + .with_bucket_path("p".to_string()) + .with_total_buckets(1) + .with_data_files(vec![meta]) + .with_raw_convertible(false) + .build() + .unwrap()]) + }; + let schemas = table.schema_manager(); + + for meta in [ + file(Some(vec!["id", "gone"]), None), + file(None, Some(vec!["id", "gone"])), + file(Some(vec!["id"]), Some(vec!["id", "gone"])), + ] { + let err = super::reject_unauthorized_stats(&plan_of(meta), table.schema(), schemas) + .await + .unwrap_err(); + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("statistics for 'gone'")), + "{err:?}" + ); + } + + assert!(super::reject_unauthorized_stats( + &plan_of(file(Some(vec!["id"]), Some(vec!["id"]))), + table.schema(), + schemas + ) + .await + .is_ok()); + } + + #[test] + fn test_a_system_column_read_is_refused() { + let err = reject_system_columns(["id", crate::spec::ROW_ID_FIELD_NAME]).unwrap_err(); + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("system column '_ROW_ID'")), + "{err:?}" + ); + assert!(reject_system_columns(["id", "name"]).is_ok()); + } + + #[tokio::test] + async fn test_time_travelled_or_branch_read_is_refused() { + let mut travelled = query_auth_table(); + travelled.time_traveled = true; + let err = travelled.authorize_read(true).await.unwrap_err(); + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("time-travelled or branch read")), + "got {err:?}" + ); + + let mut branch = query_auth_table(); + branch.branch_reference = true; + assert!(branch.authorize_read(true).await.is_err()); + } +} diff --git a/crates/paimon/src/table/read_builder.rs b/crates/paimon/src/table/read_builder.rs index ec8ef966e..2e0eb638c 100644 --- a/crates/paimon/src/table/read_builder.rs +++ b/crates/paimon/src/table/read_builder.rs @@ -501,10 +501,12 @@ impl<'a> PaimonReadBuilder<'a> { /// Create a table read for consuming splits (e.g. from a scan plan). pub fn new_read(&self) -> Result> { - // Fail closed at read construction so bindings that short-circuit before - // `to_arrow` (e.g. an empty-splits fast path) can't bypass the guard. - let core_options = self.table.schema.core_options(); - core_options.ensure_read_authorized()?; + // Stays here: a table's declared type is known without a grant. Only + // query-auth moved to `to_arrow`, where the split's grant is visible. + self.table + .schema + .core_options() + .ensure_type_paimon_served(&self.table.identifier().full_name())?; let read_type = match self.resolve_read_type()? { None => self.table.schema.fields().to_vec(), Some(fields) => fields, @@ -946,14 +948,30 @@ mod tests { #[test] fn test_read_fails_closed_when_query_auth_enabled() { let table = query_auth_table(); - // `new_read` fails closed, so bindings that short-circuit before `to_arrow` can't bypass. - let err = table.new_read_builder().new_read().unwrap_err(); + let read = table.new_read_builder().new_read().unwrap(); + let err = ungranted_read_error(&read); assert!( matches!(err, crate::Error::Unsupported { ref message } if message.contains("query-auth.enabled")), - "building a read for a query-auth.enabled table must fail closed" + "reading a query-auth.enabled table without a grant must fail closed" ); } + fn ungranted_read_error(read: &crate::table::TableRead<'_>) -> crate::Error { + let split = DataSplitBuilder::new() + .with_snapshot(1) + .with_partition(BinaryRow::new(0)) + .with_bucket(0) + .with_bucket_path("memory:/t/bucket-0".to_string()) + .with_total_buckets(1) + .with_data_files(Vec::new()) + .build() + .unwrap(); + match read.to_arrow(&[split]) { + Ok(_) => panic!("reading without a grant must fail closed"), + Err(err) => err, + } + } + #[test] fn test_dynamic_option_cannot_disable_query_auth() { // Copying the table with the option off must not weaken a stored `true`. @@ -961,7 +979,8 @@ mod tests { "query-auth.enabled".to_string(), "false".to_string(), )])); - let err = table.new_read_builder().new_read().unwrap_err(); + let read = table.new_read_builder().new_read().unwrap(); + let err = ungranted_read_error(&read); assert!( matches!(err, crate::Error::Unsupported { ref message } if message.contains("query-auth.enabled")), "a dynamic override must not disable query-auth" diff --git a/crates/paimon/src/table/rest_env.rs b/crates/paimon/src/table/rest_env.rs index 4e8a1f7c5..ef83530b1 100644 --- a/crates/paimon/src/table/rest_env.rs +++ b/crates/paimon/src/table/rest_env.rs @@ -19,6 +19,7 @@ use crate::api::rest_api::RESTApi; use crate::api::rest_error::RestError; +use crate::api::GetTableResponse; use crate::catalog::{Identifier, RESTTokenFileIO}; use crate::common::Options; use crate::error::Error; @@ -81,6 +82,80 @@ impl RESTEnv { &self.api } + /// Bracketed by a freshness check: the response names no table, so a drop + /// and re-create in between would let a replacement's grant serve this one. + pub(crate) async fn table_query_auth( + &self, + branch: &str, + schema_id: i64, + select: Option>, + ) -> Result { + self.current_table_checked(schema_id).await?; + let response = self + .api + .auth_table_query(&self.branch_identifier(branch), select) + .await?; + self.current_table_checked(schema_id).await?; + Ok(response) + } + + /// Asserts nothing about identity: an ordinary table must not inherit a + /// freshness restriction. + pub(crate) async fn current_table(&self) -> Result { + self.api.get_table(&self.identifier).await + } + + /// Refused unless the name still resolves to the loaded table — a missing + /// identity too, which checks nothing. + pub(crate) async fn current_table_checked(&self, schema_id: i64) -> Result { + let response = self.current_table().await?; + let name = self.identifier.full_name(); + let drifted = |what: &str, from: String, to: String| crate::Error::DataInvalid { + message: format!( + "table '{name}' now resolves to {what} {to}, not the {from} this handle was \ + loaded with; re-load the table before reading it" + ), + source: None, + }; + match response.id.as_deref() { + Some(uuid) if uuid == self.uuid => {} + Some(uuid) => return Err(drifted("uuid", self.uuid.clone(), uuid.to_string())), + None => { + return Err(drifted( + "uuid", + self.uuid.clone(), + "nothing the server reports".to_string(), + )) + } + } + match response.schema_id { + Some(id) if id == schema_id => Ok(response), + Some(id) => Err(drifted("schema", schema_id.to_string(), id.to_string())), + None => Err(drifted( + "schema", + schema_id.to_string(), + "nothing the server reports".to_string(), + )), + } + } + + /// `db.table$branch_`, as Java names a branch. Only the auth call uses it. + fn branch_identifier(&self, branch: &str) -> Identifier { + if branch == crate::catalog::DEFAULT_MAIN_BRANCH { + return self.identifier.clone(); + } + Identifier::new( + self.identifier.database(), + format!( + "{}{}{}{}", + self.identifier.object(), + crate::catalog::SYSTEM_TABLE_SPLITTER, + crate::catalog::SYSTEM_BRANCH_PREFIX, + branch + ), + ) + } + /// Get the table identifier. pub fn identifier(&self) -> &Identifier { &self.identifier @@ -219,7 +294,8 @@ impl RESTEnv { table_path, table_schema, Some(rest_env), - )) + ) + .with_query_auth_session()) } pub(crate) async fn build_object_table( diff --git a/crates/paimon/src/table/sorted_global_index_build_builder.rs b/crates/paimon/src/table/sorted_global_index_build_builder.rs index b83761f66..c0a7c8dd4 100644 --- a/crates/paimon/src/table/sorted_global_index_build_builder.rs +++ b/crates/paimon/src/table/sorted_global_index_build_builder.rs @@ -100,7 +100,9 @@ impl<'a> SortedGlobalIndexBuildBuilder<'a> { pub async fn execute(&self) -> Result { // Building the index scans the table's rows. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table + .ensure_read_authorized_live("building an index") + .await?; self.table.ensure_not_branch_reference_for_write()?; diff --git a/crates/paimon/src/table/source.rs b/crates/paimon/src/table/source.rs index aaaf66cfc..1f676997c 100644 --- a/crates/paimon/src/table/source.rs +++ b/crates/paimon/src/table/source.rs @@ -20,6 +20,7 @@ //! Reference: [org.apache.paimon.table.source](https://github.com/apache/paimon/blob/master/paimon-core/src/main/java/org/apache/paimon/table/source/). use crate::spec::{BinaryRow, DataFileMeta, DataFileMetaRowLayout}; +use crate::table::query_auth::QueryAuthGrant; use crate::table::stats_filter::group_by_overlapping_row_id; use serde::{Deserialize, Serialize}; use std::sync::Arc; @@ -500,9 +501,33 @@ pub struct DataSplit { /// physical rows are exactly its logical rows (modulo deletion files). /// Mirrors Java `DataSplit#rawConvertible`. raw_convertible: bool, + /// Mirrors Java `QueryAuthSplit`, but is dropped by serialization, so a + /// plan must be read where it was made. + #[serde(skip)] + query_auth_grant: Option>, + /// That this split came from a `query-auth.enabled` table. Unlike the grant + /// it survives serialization, so a round-tripped split fails closed. + #[serde(default)] + query_auth_required: bool, } impl DataSplit { + /// Marks the split as needing authorization whether or not a grant came + /// with it, so a plan that produced none still refuses at the read. + pub(crate) fn planned(mut self, grant: Option>) -> Self { + self.query_auth_required = true; + self.query_auth_grant = grant; + self + } + + pub(crate) fn query_auth_required(&self) -> bool { + self.query_auth_required + } + + pub(crate) fn query_auth_grant(&self) -> Option<&Arc> { + self.query_auth_grant.as_ref() + } + pub fn snapshot_id(&self) -> i64 { self.snapshot_id } @@ -683,10 +708,23 @@ impl DataSplit { DataSplitBuilder::new() } + /// The Java-compatible frames have no field for the marker, so a reader would + /// rebuild the split without it. Serde keeps it; these two must refuse. + fn ensure_serializable_without_grant(&self) -> crate::Result<()> { + if self.query_auth_required { + return Err(crate::table::query_auth::unsupported( + "a split of such a table cannot be serialized to the cross-language \ + format, which has no field to carry the authorization with it", + )); + } + Ok(()) + } + /// Serialize the DataSplit fields to Java `DataSplit#serialize` (version 9) binary. /// Byte-compatible with `compatibility/datasplit-v9`. Row ranges are not part of the /// format; `serialize_split_v1` wraps a row-range split as an `IndexedSplit` instead. pub fn serialize(&self) -> crate::Result> { + self.ensure_serializable_without_grant()?; let mut out = Vec::new(); out.extend_from_slice(&SPLIT_MAGIC.to_be_bytes()); out.extend_from_slice(&SPLIT_VERSION.to_be_bytes()); @@ -848,6 +886,7 @@ impl DataSplit { /// `IndexedSplit` (type 3) wrapping the DataSplit body plus the ranges. Byte-compatible with /// `compatibility/split-v1-data` / `split-v1-indexed`. pub fn serialize_split_v1(&self) -> crate::Result> { + self.ensure_serializable_without_grant()?; let mut out = Vec::new(); out.extend_from_slice(&SPLIT_SER_MAGIC.to_be_bytes()); out.extend_from_slice(&SPLIT_SER_VERSION.to_be_bytes()); @@ -1274,6 +1313,8 @@ impl DataSplitBuilder { } } Ok(DataSplit { + query_auth_grant: None, + query_auth_required: false, snapshot_id: self.snapshot_id, partition: Arc::new(partition), bucket: self.bucket, @@ -1311,6 +1352,17 @@ impl Plan { &self.splits } + /// Stamps the grant every split of this plan was authorized under. + pub(crate) fn planned(mut self, grant: Option>) -> Self { + if grant.is_some() { + self.splits = std::mem::take(&mut self.splits) + .into_iter() + .map(|split| split.planned(grant.clone())) + .collect(); + } + self + } + /// Sum of data-file bytes referenced by this plan. /// /// Negative file sizes are treated as unknown and do not contribute. The @@ -1984,6 +2036,31 @@ mod tests { } // Same hardening for the IndexedSplit row-ranges count in the SPLIT_V1 frame. + #[test] + fn test_a_marked_split_refuses_the_cross_language_formats() { + let split = DataSplitBuilder::new() + .with_snapshot(1) + .with_partition(crate::spec::BinaryRowBuilder::new(0).build()) + .with_bucket(0) + .with_bucket_path("p".to_string()) + .with_total_buckets(1) + .with_data_files(vec![]) + .with_raw_convertible(false) + .build() + .unwrap() + .planned(None); + for bytes in [split.serialize(), split.serialize_split_v1()] { + let Err(err) = bytes else { + panic!("the marker has nowhere to go in these formats") + }; + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("query-auth.enabled")), + "{err:?}" + ); + } + } + #[test] fn deserialize_split_v1_rejects_huge_ranges_count_without_aborting() { let split = DataSplitBuilder::new() diff --git a/crates/paimon/src/table/table_read.rs b/crates/paimon/src/table/table_read.rs index acaebeeb1..190745408 100644 --- a/crates/paimon/src/table/table_read.rs +++ b/crates/paimon/src/table/table_read.rs @@ -183,7 +183,7 @@ impl<'a> TableRead<'a> { &self, plan: &IncrementalPlan, ) -> crate::Result { - self.ensure_query_auth_allowed()?; + self.ensure_query_auth_allowed(plan)?; plan.validate()?; match &self.0 { TableReadKind::Paimon(read) => read.to_incremental_arrow(plan), @@ -203,7 +203,7 @@ impl<'a> TableRead<'a> { &self, plan: &IncrementalPlan, ) -> crate::Result { - self.ensure_query_auth_allowed()?; + self.ensure_query_auth_allowed(plan)?; plan.validate()?; match &self.0 { TableReadKind::Paimon(read) => read.to_audit_log_arrow(plan), @@ -213,8 +213,33 @@ impl<'a> TableRead<'a> { } } - fn ensure_query_auth_allowed(&self) -> crate::Result<()> { - CoreOptions::new(self.table().schema().options()).ensure_read_authorized() + /// Sync, so the split's marker stands in for asking the server. + fn ensure_query_auth_allowed(&self, plan: &IncrementalPlan) -> crate::Result<()> { + let core_options = CoreOptions::new(self.table().schema().options()); + core_options.ensure_type_paimon_served(&self.table().identifier().full_name())?; + if core_options.query_auth_enabled() || plan.any_query_auth_required() { + return Err(super::query_auth::unsupported( + "an incremental read cannot apply a row filter or column masking", + )); + } + Ok(()) + } +} + +/// Every leaf's column name. Unlike the index-based walks this sees system +/// columns, whose leaf index is only a placeholder. +fn collect_leaf_column_names(predicate: &Predicate, out: &mut std::collections::HashSet) { + match predicate { + Predicate::Leaf { column, .. } => { + out.insert(column.clone()); + } + Predicate::And(children) | Predicate::Or(children) => { + children + .iter() + .for_each(|child| collect_leaf_column_names(child, out)); + } + Predicate::Not(inner) => collect_leaf_column_names(inner, out), + Predicate::AlwaysTrue | Predicate::AlwaysFalse => {} } } @@ -714,12 +739,70 @@ impl<'a> PaimonTableRead<'a> { reader.read(splits) } + /// Allowed only if the splits carry a grant saying the server imposed + /// nothing. Never fetched here, so a split without one fails closed. + fn ensure_authorized_by_splits( + &self, + core_options: &CoreOptions, + data_splits: &[DataSplit], + ) -> crate::Result<()> { + // Unconditional: unrelated to query-auth. + core_options.ensure_type_paimon_served(&self.table.identifier().full_name())?; + // Decided at plan time, as in Java. Known limitation: a split predating + // the option, or built by hand, has neither flag and is read on the + // caller's word — re-plan after an authorization change. + let required = core_options.query_auth_enabled() + || data_splits.iter().any(|s| s.query_auth_required()); + if !required { + return Ok(()); + } + // The read's own scope: a caller can plan clean, then read differently. + let mut filter_columns = std::collections::HashSet::new(); + for predicate in &self.data_predicates { + collect_leaf_column_names(predicate, &mut filter_columns); + } + super::query_auth::reject_system_columns( + self.read_type + .iter() + .map(|f| f.name()) + .chain(filter_columns.iter().map(String::as_str)), + )?; + // By id AND name: older files resolve by id, so a dropped field passed + // through the public `with_read_type` returns an uncovered column. + super::query_auth::reject_noncanonical_fields( + &self.read_type, + self.table.schema().fields(), + )?; + // Per split, as Java binds one `QueryAuthSplit` each: lists get + // concatenated and the first grant must not cover the rest. + for split in data_splits { + let Some(grant) = split.query_auth_grant() else { + return Err(super::query_auth::unsupported( + "the split carries no authorization; it was built directly, or serialized, \ + which drops the grant — re-plan the scan", + )); + }; + if !grant.matches_table(self.table) { + return Err(super::query_auth::unsupported( + "the grant was issued for a different table, schema or session; re-plan the \ + scan", + )); + } + if !grant.is_unrestricted() { + return Err(super::query_auth::unsupported( + "this client cannot apply a row filter or column masking, so it refuses \ + rather than return unfiltered rows", + )); + } + } + Ok(()) + } + /// Returns an [`ArrowRecordBatchStream`]. pub fn to_arrow(&self, data_splits: &[DataSplit]) -> crate::Result { let has_primary_keys = !self.table.schema.primary_keys().is_empty(); let core_options = self.table.schema.core_options(); - // Fail closed for a direct `TableRead` (bypassing `ReadBuilder::new_read`). - core_options.ensure_read_authorized()?; + self.ensure_authorized_by_splits(&core_options, data_splits)?; let merge_engine = core_options.merge_engine()?; // Route supported PK merge engines through the split-aware reader. @@ -1688,15 +1771,302 @@ mod tests { )); } + #[test] + fn test_incremental_and_audit_log_reads_refuse_a_query_auth_table() { + let table = query_auth_table(); + let read = TableRead::new(&table, table.schema.fields().to_vec(), Vec::new()); + let plan = IncrementalPlan::new(IncrementalScanMode::Delta, Vec::new()); + for err in [ + read.to_incremental_arrow(&plan).err(), + read.to_audit_log_arrow(&plan).err(), + ] { + let err = err.expect("both must refuse a query-auth.enabled table"); + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("query-auth.enabled")), + "got {err:?}" + ); + } + } + + fn stale_handle(name: &str, options: &[(&str, &str)]) -> Table { + let mut builder = crate::spec::Schema::builder().column( + "id", + crate::spec::DataType::Int(crate::spec::IntType::new()), + ); + for (key, value) in options { + builder = builder.option(*key, *value); + } + Table::new( + FileIOBuilder::new("file").build().unwrap(), + Identifier::new("default", name), + format!("/tmp/test-{name}"), + crate::spec::TableSchema::new(0, &builder.build().unwrap()), + None, + ) + } + + #[test] + fn test_a_marked_split_refuses_the_format_and_incremental_reads() { + let stamped = split_with_grant(None); + + let format = stale_handle( + "fmt", + &[("type", "format-table"), ("file.format", "parquet")], + ); + let read = + TableRead::new_format(&format, format.schema().fields().to_vec(), Vec::new(), None); + let Err(err) = read.to_arrow(std::slice::from_ref(&stamped)) else { + panic!("a marked split must refuse a format read") + }; + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("query-auth.enabled")), + "{err:?}" + ); + + let paimon = stale_handle("inc", &[]); + let read = TableRead::new(&paimon, paimon.schema().fields().to_vec(), Vec::new()); + let plan = IncrementalPlan::new( + IncrementalScanMode::Delta, + vec![IncrementalSplit::Data(stamped)], + ); + let Err(err) = read.to_incremental_arrow(&plan) else { + panic!("a marked split must refuse an incremental read") + }; + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("query-auth.enabled")), + "{err:?}" + ); + } + + fn split_with_grant( + grant: Option, + ) -> crate::table::DataSplit { + crate::table::DataSplitBuilder::new() + .with_snapshot(1) + .with_partition(crate::spec::BinaryRow::new(0)) + .with_bucket(0) + .with_bucket_path("memory:/t/bucket-0".to_string()) + .with_total_buckets(1) + .with_data_files(Vec::new()) + .build() + .unwrap() + .planned(grant.map(std::sync::Arc::new)) + } + + fn grant_for(table: &Table, restricted: bool) -> crate::table::query_auth::QueryAuthGrant { + crate::table::query_auth::QueryAuthGrant::new( + crate::api::AuthTableQueryResponse { + filter: restricted.then(|| vec!["{}".to_string()]), + column_masking: None, + }, + table + .query_auth_session() + .expect("a catalog-loaded table has a session"), + ) + } + + #[tokio::test] + async fn test_one_unrestricted_grant_does_not_cover_the_other_splits() { + let table = crate::table::rest_query_auth_table().await; + let read = TableRead::new(&table, table.schema.fields().to_vec(), Vec::new()); + let allowed = split_with_grant(Some(grant_for(&table, false))); + for other in [ + split_with_grant(Some(grant_for(&table, true))), + split_with_grant(None), + ] { + assert!( + read.to_arrow(&[allowed.clone(), other]).is_err(), + "every split must be authorized on its own" + ); + } + } + + #[test] + fn test_engine_served_table_is_refused_at_the_read_boundary() { + let schema = crate::spec::Schema::builder() + .column( + "id", + crate::spec::DataType::Int(crate::spec::IntType::new()), + ) + .option("type", "iceberg-table") + .build() + .unwrap(); + let table = Table::new( + crate::io::FileIOBuilder::new("file").build().unwrap(), + crate::catalog::Identifier::new("default", "ice_t"), + "/tmp/test-engine-served-read".to_string(), + crate::spec::TableSchema::new(0, &schema), + None, + ); + let read = TableRead::new(&table, table.schema.fields().to_vec(), Vec::new()); + let err = match read.to_arrow(&[split_with_grant(None)]) { + Ok(_) => panic!("an engine-served table must not be read as Paimon"), + Err(err) => err, + }; + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("cannot be served as a Paimon table")), + "got {err:?}" + ); + } + + #[tokio::test] + async fn test_a_row_id_filter_configured_on_the_read_is_refused() { + let table = crate::table::rest_query_auth_table().await; + let row_id = Predicate::Leaf { + index: 0, + column: crate::spec::ROW_ID_FIELD_NAME.to_string(), + data_type: crate::spec::DataType::BigInt(crate::spec::BigIntType::new()), + op: crate::spec::PredicateOperator::GtEq, + literals: vec![crate::spec::Datum::Long(1)], + }; + let read = TableRead::new(&table, table.schema.fields().to_vec(), vec![row_id]); + let split = split_with_grant(Some(grant_for(&table, false))); + let err = match read.to_arrow(&[split]) { + Ok(_) => panic!("a system-column filter must be refused"), + Err(err) => err, + }; + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("system column '_ROW_ID'")), + "got {err:?}" + ); + } + + #[tokio::test] + async fn test_a_field_outside_the_current_schema_is_refused() { + let table = crate::table::rest_query_auth_table().await; + let dropped = crate::spec::DataField::new( + 999, + "dropped".to_string(), + crate::spec::DataType::Int(crate::spec::IntType::new()), + ); + let real = table.schema().fields()[0].clone(); + let reshaped = crate::spec::DataField::new( + real.id(), + real.name().to_string(), + crate::spec::DataType::Row(crate::spec::RowType::new(vec![ + crate::spec::DataField::new( + 1, + "hidden".to_string(), + crate::spec::DataType::Int(crate::spec::IntType::new()), + ), + ])), + ); + + for field in [dropped, reshaped] { + let read = TableRead::new(&table, vec![field], Vec::new()); + let split = split_with_grant(Some(grant_for(&table, false))); + let err = match read.to_arrow(&[split]) { + Ok(_) => panic!("a field outside the current schema must be refused"), + Err(err) => err, + }; + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("not a column of the current schema")), + "got {err:?}" + ); + } + } + + #[test] + fn test_a_serialized_split_still_demands_authorization() { + let stamped = split_with_grant(None); + let bytes = serde_json::to_vec(&stamped).unwrap(); + let restored: crate::table::DataSplit = serde_json::from_slice(&bytes).unwrap(); + assert!( + restored.query_auth_grant().is_none(), + "the grant is dropped" + ); + assert!( + restored.query_auth_required(), + "but the demand for authorization survives" + ); + + let stale = Table::new( + crate::io::FileIOBuilder::new("file").build().unwrap(), + crate::catalog::Identifier::new("default", "stale"), + "/tmp/test-stale-handle".to_string(), + crate::spec::TableSchema::new( + 0, + &crate::spec::Schema::builder() + .column( + "id", + crate::spec::DataType::Int(crate::spec::IntType::new()), + ) + .build() + .unwrap(), + ), + None, + ); + assert!(!stale.schema().core_options().query_auth_enabled()); + let read = TableRead::new(&stale, stale.schema().fields().to_vec(), Vec::new()); + assert!( + read.to_arrow(&[restored]).is_err(), + "a round-tripped split must fail closed even on a handle that predates the option" + ); + } + + #[tokio::test] + async fn test_a_grant_from_another_handle_refuses_the_read() { + let table = crate::table::rest_query_auth_table().await; + let other = crate::table::rest_query_auth_table().await; + let read = TableRead::new(&table, table.schema.fields().to_vec(), Vec::new()); + let err = match read.to_arrow(&[split_with_grant(Some(grant_for(&other, false)))]) { + Ok(_) => panic!("a grant obtained elsewhere must not authorize this read"), + Err(err) => err, + }; + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("different table, schema or session")), + "got {err:?}" + ); + } + + #[tokio::test] + async fn test_restricted_grant_on_a_split_refuses_the_read() { + let table = crate::table::rest_query_auth_table().await; + let read = TableRead::new(&table, table.schema.fields().to_vec(), Vec::new()); + let split = split_with_grant(Some(grant_for(&table, true))); + assert!( + matches!( + read.to_arrow(&[split]), + Err(crate::Error::Unsupported { ref message }) if message.contains("query-auth.enabled") + ), + "a row filter this client cannot apply must refuse the read" + ); + } + + #[tokio::test] + async fn test_unrestricted_grant_on_a_split_allows_the_read() { + let table = crate::table::rest_query_auth_table().await; + let read = TableRead::new(&table, table.schema.fields().to_vec(), Vec::new()); + let split = split_with_grant(Some(grant_for(&table, false))); + assert!( + read.to_arrow(&[split]).is_ok(), + "an unrestricted grant must let the read through" + ); + } + #[test] fn test_direct_table_read_fails_closed_when_query_auth_enabled() { let table = query_auth_table(); - // Bypass `ReadBuilder` by constructing `TableRead` directly; the `to_arrow` guard - // still fails closed. let read = TableRead::new(&table, table.schema.fields().to_vec(), Vec::new()); + let split = crate::table::DataSplitBuilder::new() + .with_snapshot(1) + .with_partition(crate::spec::BinaryRow::new(0)) + .with_bucket(0) + .with_bucket_path("memory:/t/bucket-0".to_string()) + .with_total_buckets(1) + .with_data_files(Vec::new()) + .build() + .unwrap(); assert!( matches!( - read.to_arrow(&[]), + read.to_arrow(&[split]), Err(crate::Error::Unsupported { ref message }) if message.contains("query-auth.enabled") ), "directly-constructed read of a query-auth.enabled table must fail closed" diff --git a/crates/paimon/src/table/table_scan.rs b/crates/paimon/src/table/table_scan.rs index 37139e58a..b379aeb22 100644 --- a/crates/paimon/src/table/table_scan.rs +++ b/crates/paimon/src/table/table_scan.rs @@ -1097,43 +1097,110 @@ impl<'a> PaimonTableScan<'a> { /// for `scan.version`; the strict selectors mirror Java's typed /// `scan.snapshot-id` / `scan.tag-name` handling. pub async fn plan(&self) -> crate::Result { - self.ensure_query_auth_allowed()?; + let grant = self.authorize_query().await?; let data_evolution_read_field_ids = self.projected_read_field_ids()?; - let snapshot = match super::time_travel::resolve_snapshot(self.table).await? { - Some(snapshot) => snapshot, - None => return Ok(Plan::new(Vec::new())), + let plan = match super::time_travel::resolve_snapshot(self.table).await? { + Some(snapshot) => { + self.plan_snapshot(snapshot, data_evolution_read_field_ids.as_ref(), None) + .await? + } + None => Plan::new(Vec::new()), }; - self.plan_snapshot(snapshot, data_evolution_read_field_ids.as_ref(), None) - .await + self.check_planned_files(&plan, grant.is_some()).await?; + Ok(plan.planned(grant)) } /// Plan the full scan and return metadata-pruning trace counters. pub async fn plan_with_trace(&self) -> crate::Result<(Plan, ScanTrace)> { - self.ensure_query_auth_allowed()?; + let grant = self.authorize_query().await?; let mut trace = ScanTrace { limit: self.limit, ..Default::default() }; let data_evolution_read_field_ids = self.projected_read_field_ids()?; - let snapshot = match super::time_travel::resolve_snapshot(self.table).await? { - Some(snapshot) => snapshot, - None => return Ok((Plan::new(Vec::new()), trace)), + let plan = match super::time_travel::resolve_snapshot(self.table).await? { + Some(snapshot) => { + trace.snapshot_id = Some(snapshot.id()); + let plan = self + .plan_snapshot( + snapshot, + data_evolution_read_field_ids.as_ref(), + Some(&mut trace), + ) + .await?; + trace.planned_data_file_bytes = plan.planned_data_file_bytes(); + plan + } + None => Plan::new(Vec::new()), }; - trace.snapshot_id = Some(snapshot.id()); - let plan = self - .plan_snapshot( - snapshot, - data_evolution_read_field_ids.as_ref(), - Some(&mut trace), - ) - .await?; - trace.planned_data_file_bytes = plan.planned_data_file_bytes(); - Ok((plan, trace)) + self.check_planned_files(&plan, grant.is_some()).await?; + Ok((plan.planned(grant), trace)) + } + + /// The grant predates the manifest read, so the table can have been re-created + /// at the same path in between. Also refuses a plan whose files carry + /// statistics the current schema no longer covers. + async fn check_planned_files(&self, plan: &Plan, query_auth: bool) -> crate::Result<()> { + if !query_auth { + return Ok(()); + } + if let Some(rest_env) = self.table.rest_env() { + rest_env + .current_table_checked(self.table.schema().id()) + .await?; + } + super::query_auth::reject_unauthorized_stats( + plan, + self.table.schema(), + self.table.schema_manager(), + ) + .await + } + + /// Authorize this scan and return the grant for the caller to stamp. + async fn authorize_query( + &self, + ) -> crate::Result>> { + let core_options = CoreOptions::new(self.table.schema().options()); + // Unconditional: unrelated to query-auth. + core_options.ensure_type_paimon_served(&self.table.identifier().full_name())?; + + // File paths and stats, not table columns: the endpoint cannot rule on them. + let query_auth = self.table.server_query_auth_enabled().await?; + if self.scan_all_files { + return if query_auth { + Err(super::query_auth::unsupported( + "`$files` and friends are file paths and stats, not table columns, so the \ + auth endpoint can never rule on them", + )) + } else { + Ok(None) + }; + } + // A predicate or a row-range slice reads `_ROW_ID` unprojected. + let touches_row_id = self.row_ranges.is_some() + || self + .data_predicates + .iter() + .any(super::row_id_predicate::references_row_id); + if query_auth && touches_row_id { + super::query_auth::reject_system_columns([ROW_ID_FIELD_NAME])?; + } + + let grant = self.table.authorize_read(query_auth).await?; + // A plan carries row counts and bounds that answer COUNT/MIN/MAX without + // reading a row. + if grant.as_ref().is_some_and(|g| !g.is_unrestricted()) { + return Err(super::query_auth::unsupported( + "a plan already carries file paths, row counts and column bounds that a row \ + filter or column masking must not expose", + )); + } + Ok(grant) } - /// Fail closed for a `query-auth.enabled` table: scan planning — including - /// `with_scan_all_files`, which read-facing system tables like `files` use — - /// exposes file paths, row counts, and stats the client can't authorize. + /// Fail closed on planning paths that do not authorize, including + /// `with_scan_all_files`: it exposes stats the client cannot check. fn ensure_query_auth_allowed(&self) -> crate::Result<()> { CoreOptions::new(self.table.schema().options()).ensure_read_authorized() } @@ -2193,6 +2260,36 @@ mod tests { use chrono::{DateTime, Utc}; use std::collections::{HashMap, HashSet}; + #[tokio::test] + async fn test_engine_served_table_is_refused_at_plan() { + let schema = crate::spec::Schema::builder() + .column( + "id", + crate::spec::DataType::Int(crate::spec::IntType::new()), + ) + .option("type", "iceberg-table") + .build() + .unwrap(); + let table = Table::new( + crate::io::FileIOBuilder::new("file").build().unwrap(), + crate::catalog::Identifier::new("default", "ice_t"), + "/tmp/test-engine-served".to_string(), + crate::spec::TableSchema::new(0, &schema), + None, + ); + let err = table + .new_read_builder() + .new_scan() + .plan() + .await + .expect_err("an engine-served table must not plan as Paimon"); + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("cannot be served as a Paimon table")), + "got {err:?}" + ); + } + /// Helper to build a DataFileMeta with data evolution fields. fn make_evo_file( name: &str, diff --git a/crates/paimon/src/table/vector_scan.rs b/crates/paimon/src/table/vector_scan.rs index 396833522..3c2477369 100644 --- a/crates/paimon/src/table/vector_scan.rs +++ b/crates/paimon/src/table/vector_scan.rs @@ -122,8 +122,10 @@ impl PlanContext { /// Creates query-independent plans for DE or primary-key vector search. pub struct VectorScan { + table: Table, context: PlanContext, scan: VectorScanKind, + authorized: bool, } enum VectorScanKind { @@ -138,6 +140,7 @@ impl VectorScan { filter: Option<&Predicate>, include_row_ids: Option<&Arc>, prepared: Option<&PreparedVectorSearchFilter>, + authorized: bool, ) -> crate::Result { let context = PlanContext::new(table, column, filter, include_row_ids, prepared)?; let core = CoreOptions::new(table.schema().options()); @@ -164,10 +167,27 @@ impl VectorScan { prepared, ))) }; - Ok(Self { context, scan }) + Ok(Self { + table: table.clone(), + context, + scan, + authorized, + }) + } + + /// The caller already asked the server for this operation. + pub(crate) fn assume_authorized(mut self) -> Self { + self.authorized = true; + self } pub async fn plan(&self) -> crate::Result { + // The option can be set after a load. + if !self.authorized { + self.table + .ensure_read_authorized_live("a vector search") + .await?; + } let work = match &self.scan { VectorScanKind::DataEvolution(scan) => { VectorScanWork::DataEvolution(Box::new(scan.plan().await?)) diff --git a/crates/paimon/src/table/vector_search_builder.rs b/crates/paimon/src/table/vector_search_builder.rs index 176d75f9c..7116a3eb5 100644 --- a/crates/paimon/src/table/vector_search_builder.rs +++ b/crates/paimon/src/table/vector_search_builder.rs @@ -31,6 +31,8 @@ pub struct VectorSearchBuilder<'a> { limit: Option, options: HashMap, filter: Option, + /// Set when the caller already asked, so a delegated search does not repeat it. + authorized: bool, } impl<'a> VectorSearchBuilder<'a> { @@ -42,9 +44,16 @@ impl<'a> VectorSearchBuilder<'a> { limit: None, options: HashMap::new(), filter: None, + authorized: false, } } + /// The caller already asked the server for this operation. + pub(crate) fn assume_authorized(mut self) -> Self { + self.authorized = true; + self + } + pub fn with_vector_column(&mut self, name: &str) -> &mut Self { self.vector_column = Some(name.to_string()); self @@ -92,7 +101,14 @@ impl<'a> VectorSearchBuilder<'a> { .ok_or_else(|| crate::Error::ConfigInvalid { message: "Vector column must be set via with_vector_column()".to_string(), })?; - VectorScan::new(self.table, column, self.filter.as_ref(), None, None) + VectorScan::new( + self.table, + column, + self.filter.as_ref(), + None, + None, + self.authorized, + ) } /// Create an owned reader; query errors are reported before planning. @@ -115,8 +131,15 @@ impl<'a> VectorSearchBuilder<'a> { /// Search locally using the same Scan -> Plan -> Read API exposed to engines. /// Use the result's `new_read_builder()` to materialize projected columns. pub async fn execute(&self) -> crate::Result { + // Before any validation or fast path, and once: the scan is told so. + if !self.authorized { + self.table + .ensure_read_authorized_live("a vector search") + .await?; + } let read = self.new_read()?; - read.read(self.new_scan()?.plan().await?).await + let scan = self.new_scan()?.assume_authorized(); + read.read(scan.plan().await?).await } fn query(&self) -> crate::Result<(&str, &[f32], usize)> { diff --git a/crates/paimon/src/table/vindex_index_build_builder.rs b/crates/paimon/src/table/vindex_index_build_builder.rs index 9e06ff9a8..d1ac933c3 100644 --- a/crates/paimon/src/table/vindex_index_build_builder.rs +++ b/crates/paimon/src/table/vindex_index_build_builder.rs @@ -61,7 +61,9 @@ impl<'a> VindexIndexBuildBuilder<'a> { pub async fn execute(&self) -> Result { // Building the index scans the table's rows. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table + .ensure_read_authorized_live("building an index") + .await?; self.table.ensure_not_branch_reference_for_write()?; diff --git a/crates/paimon/tests/mock_server.rs b/crates/paimon/tests/mock_server.rs index 929e16500..781ea12c9 100644 --- a/crates/paimon/tests/mock_server.rs +++ b/crates/paimon/tests/mock_server.rs @@ -34,10 +34,10 @@ use std::sync::{Arc, Mutex}; use tokio::task::JoinHandle; use paimon::api::{ - AlterDatabaseRequest, AlterTableRequest, AuditRESTResponse, ConfigResponse, - CreateFunctionRequest, CreatePartitionsRequest, CreateTagRequest, CreateViewRequest, - DropPartitionsRequest, ErrorResponse, GetDatabaseResponse, GetFunctionResponse, - GetTableResponse, GetTagResponse, GetViewResponse, ListDatabasesResponse, + AlterDatabaseRequest, AlterTableRequest, AuditRESTResponse, AuthTableQueryResponse, + ConfigResponse, CreateFunctionRequest, CreatePartitionsRequest, CreateTagRequest, + CreateViewRequest, DropPartitionsRequest, ErrorResponse, GetDatabaseResponse, + GetFunctionResponse, GetTableResponse, GetTagResponse, GetViewResponse, ListDatabasesResponse, ListFunctionsResponse, ListPartitionsByFilterRequest, ListPartitionsByNamesRequest, ListPartitionsResponse, ListPermissionsResponse, ListTablesResponse, ListViewsResponse, PermissionAssignment, PermissionResource, RenameTableRequest, ResourcePaths, ResourceType, @@ -78,6 +78,10 @@ struct MockState { grant_permission_bodies: Vec, revoke_permission_bodies: Vec, grant_permission_error_status: Option, + auth_responses: HashMap, + column_auth: HashMap>, + uuid_after_auth: HashMap, + uuid_after_calls: HashMap, /// ECS metadata role name (for token loader testing) ecs_role_name: Option, /// ECS metadata token (for token loader testing) @@ -175,6 +179,7 @@ pub struct RESTServer { warehouse: String, _data_path: String, config: ConfigResponse, + get_table_calls: Arc, inner: Arc>, resource_paths: ResourcePaths, addr: Option, @@ -211,6 +216,7 @@ impl RESTServer { _data_path, config, warehouse, + get_table_calls: Arc::new(std::sync::atomic::AtomicUsize::new(0)), inner: Arc::new(Mutex::new(MockState { databases, ..Default::default() @@ -767,7 +773,10 @@ impl RESTServer { Path((db, table)): Path<(String, String)>, Extension(state): Extension>, ) -> impl IntoResponse { - let s = state.inner.lock().unwrap(); + state + .get_table_calls + .fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let mut s = state.inner.lock().unwrap(); let key = format!("{db}.{table}"); if s.no_permission_tables.contains(&key) { @@ -780,6 +789,18 @@ impl RESTServer { return (StatusCode::FORBIDDEN, Json(err)).into_response(); } + if let Some((uuid, remaining)) = s.uuid_after_calls.get_mut(&key) { + if *remaining == 0 { + let uuid = uuid.clone(); + s.uuid_after_calls.remove(&key); + if let Some(t) = s.tables.get_mut(&key) { + t.id = Some(uuid); + } + } else { + *remaining -= 1; + } + } + if let Some(response) = s.tables.get(&key) { return (StatusCode::OK, Json(response.clone())).into_response(); } @@ -803,6 +824,78 @@ impl RESTServer { (StatusCode::NOT_FOUND, Json(err)).into_response() } + pub async fn auth_table_query( + Path((db, table)): Path<(String, String)>, + Extension(state): Extension>, + Json(request): Json, + ) -> impl IntoResponse { + let s = state.inner.lock().unwrap(); + let key = format!("{db}.{table}"); + + // Mirrors the reference server: a null select means the real schema + // fields, and any column outside the grant denies the query. + if let Some(allowed) = s.column_auth.get(&key) { + let requested = request.select.clone().unwrap_or_else(|| { + s.tables + .get(&key) + .and_then(|t| t.schema.as_ref()) + .map(|schema| { + schema + .fields() + .iter() + .map(|f| f.name().to_string()) + .collect() + }) + .unwrap_or_default() + }); + if let Some(denied) = requested.iter().find(|c| !allowed.contains(c)) { + return ( + StatusCode::FORBIDDEN, + Json(ErrorResponse::new( + Some("table".to_string()), + Some(denied.clone()), + Some(format!("no permission for column '{denied}'")), + Some(403), + )), + ) + .into_response(); + } + } + + let response = s.auth_responses.get(&key).cloned().unwrap_or_default(); + drop(s); + let mut s = state.inner.lock().unwrap(); + if let Some(uuid) = s.uuid_after_auth.remove(&key) { + if let Some(existing) = s.tables.get_mut(&key) { + existing.id = Some(uuid); + } + } + (StatusCode::OK, Json(response)).into_response() + } + + pub fn set_table_uuid_after_calls( + &self, + database: &str, + table: &str, + uuid: &str, + after: usize, + ) { + let mut s = self.inner.lock().unwrap(); + s.uuid_after_calls + .insert(format!("{database}.{table}"), (uuid.to_string(), after)); + } + + pub fn set_table_uuid_after_auth(&self, database: &str, table: &str, uuid: &str) { + let mut s = self.inner.lock().unwrap(); + s.uuid_after_auth + .insert(format!("{database}.{table}"), uuid.to_string()); + } + + pub fn set_column_auth(&self, database: &str, table: &str, columns: Vec) { + let mut s = self.inner.lock().unwrap(); + s.column_auth.insert(format!("{database}.{table}"), columns); + } + /// Handle DELETE /databases/:db/tables/:table - drop a table. pub async fn drop_table( Path((db, table)): Path<(String, String)>, @@ -1617,6 +1710,48 @@ impl RESTServer { ); } + #[allow(dead_code)] + pub fn get_table_calls(&self) -> usize { + self.get_table_calls + .load(std::sync::atomic::Ordering::Relaxed) + } + + pub fn clear_table_identity(&self, database: &str, table: &str) { + let mut s = self.inner.lock().unwrap(); + if let Some(existing) = s.tables.get_mut(&format!("{database}.{table}")) { + existing.id = None; + existing.schema_id = None; + } + } + + pub fn set_table_uuid(&self, database: &str, table: &str, uuid: &str) { + let mut s = self.inner.lock().unwrap(); + if let Some(existing) = s.tables.get_mut(&format!("{database}.{table}")) { + existing.id = Some(uuid.to_string()); + } + } + + pub fn set_table_schema_id( + &self, + database: &str, + table: &str, + schema: paimon::spec::Schema, + schema_id: i64, + ) { + let mut s = self.inner.lock().unwrap(); + let key = format!("{database}.{table}"); + if let Some(existing) = s.tables.get_mut(&key) { + existing.schema_id = Some(schema_id); + existing.schema = Some(schema); + } + } + + pub fn set_auth_response(&self, database: &str, table: &str, response: AuthTableQueryResponse) { + let mut s = self.inner.lock().unwrap(); + s.auth_responses + .insert(format!("{database}.{table}"), response); + } + /// Add a no-permission table to the server state. pub fn add_no_permission_table(&self, database: &str, table: &str) { let mut s = self.inner.lock().unwrap(); @@ -1949,6 +2084,10 @@ pub async fn start_mock_server( &format!("{prefix}/databases/:db/functions/:function"), get(RESTServer::get_function), ) + .route( + &format!("{prefix}/databases/:db/tables/:table/auth"), + post(RESTServer::auth_table_query), + ) .route( &format!("{prefix}/tables/rename"), post(RESTServer::rename_table), diff --git a/crates/paimon/tests/rest_catalog_test.rs b/crates/paimon/tests/rest_catalog_test.rs index 3d7805e0a..284d8fab1 100644 --- a/crates/paimon/tests/rest_catalog_test.rs +++ b/crates/paimon/tests/rest_catalog_test.rs @@ -2513,6 +2513,465 @@ async fn test_load_table_rejects_unknown_declared_type() { ); } +// Skipped on Windows for the same opendal `fs` StripPrefixError as the +// blob-view regression above: it writes through FileSystemCatalog. +#[cfg(not(windows))] +#[tokio::test] +async fn test_query_auth_unrestricted_user_can_read() { + let tmp = tempfile::tempdir().unwrap(); + let warehouse = format!("file://{}", tmp.path().display()); + let mut fs_options = Options::new(); + fs_options.set(CatalogOptions::WAREHOUSE, &warehouse); + let fs_catalog = FileSystemCatalog::new(fs_options).expect("create filesystem catalog"); + fs_catalog + .create_database("default", true, HashMap::new()) + .await + .unwrap(); + let identifier = Identifier::new("default", "guarded"); + let columns = Schema::builder() + .column("id", DataType::Int(IntType::new())) + .build() + .unwrap(); + fs_catalog + .create_table(&identifier, columns, false) + .await + .unwrap(); + let plain = fs_catalog.get_table(&identifier).await.unwrap(); + let batch = RecordBatch::try_new( + Arc::new(ArrowSchema::new(vec![ArrowField::new( + "id", + ArrowDataType::Int32, + false, + )])), + vec![Arc::new(Int32Array::from(vec![1, 2, 3]))], + ) + .unwrap(); + write_batch(&plain, batch, "query-auth-fixture").await; + + let ctx = setup_catalog(vec!["default"]).await; + let guarded_schema = Schema::builder() + .column("id", DataType::Int(IntType::new())) + .option("query-auth.enabled", "true") + .build() + .unwrap(); + ctx.server + .add_table_with_schema("default", "guarded", guarded_schema, plain.location()); + + let table = ctx.catalog.get_table(&identifier).await.unwrap(); + let read_builder = table.new_read_builder(); + let plan = read_builder.new_scan().plan().await.unwrap(); + assert!( + !plan.splits().is_empty(), + "the fixture must produce a split, or the read below proves nothing" + ); + + let batches = read_builder + .new_read() + .unwrap() + .to_arrow(plan.splits()) + .expect("an unrestricted user must be allowed to read") + .try_collect::>() + .await + .expect("and the rows must decode"); + assert_eq!( + batches.iter().map(|b| b.num_rows()).sum::(), + 3, + "every written row must come back" + ); +} + +fn schema_of(columns: &[&str], options: &[(&str, &str)]) -> Schema { + let mut builder = Schema::builder(); + for name in columns { + builder = builder.column(*name, DataType::Int(IntType::new())); + } + for (key, value) in options { + builder = builder.option(*key, *value); + } + builder.build().unwrap() +} + +const GUARDED: &[(&str, &str)] = &[("query-auth.enabled", "true")]; + +struct Guarded { + ctx: TestContext, + table: Table, + identifier: Identifier, + _tmp: tempfile::TempDir, +} + +async fn guarded(name: &str, columns: &[&str]) -> Guarded { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + ctx.server + .add_table_with_schema("default", name, schema_of(columns, GUARDED), &path); + let identifier = Identifier::new("default", name); + let table = ctx.catalog.get_table(&identifier).await.unwrap(); + Guarded { + ctx, + table, + identifier, + _tmp: tmp, + } +} + +async fn plan_err(table: &Table, why: &str) -> paimon::Error { + table + .new_read_builder() + .new_scan() + .plan() + .await + .expect_err(why) +} + +#[track_caller] +fn assert_refused(err: paimon::Error) { + assert!( + matches!(err, paimon::Error::Unsupported { ref message } + if message.contains("query-auth.enabled")), + "{err:?}" + ); +} + +#[track_caller] +fn assert_drifted(err: paimon::Error, what: &str) { + assert!( + matches!(err, paimon::Error::DataInvalid { ref message, .. } + if message.contains(what)), + "{err:?}" + ); +} + +fn restricted() -> paimon::api::AuthTableQueryResponse { + paimon::api::AuthTableQueryResponse { + filter: Some(vec!["{\"field\":\"id\"}".to_string()]), + column_masking: None, + } +} + +#[tokio::test] +async fn test_query_auth_restricted_user_is_refused_at_plan_time() { + let g = guarded("restricted", &["id"]).await; + g.ctx + .server + .set_auth_response("default", "restricted", restricted()); + + assert_refused( + plan_err( + &g.table, + "a restricted user must be refused before a plan exists", + ) + .await, + ); +} + +#[tokio::test] +async fn test_query_auth_refuses_a_stale_handle() { + let g = guarded("drifting", &["id"]).await; + g.ctx.server.set_table_schema_id( + "default", + "drifting", + schema_of(&["id", "extra"], GUARDED), + 7, + ); + + assert_drifted( + plan_err(&g.table, "a handle whose schema drifted must be refused").await, + "now resolves to schema", + ); +} + +#[tokio::test] +async fn test_query_auth_refuses_a_recreated_table() { + let g = guarded("recreated", &["id"]).await; + g.ctx + .server + .set_table_uuid("default", "recreated", "uuid-of-the-replacement"); + + assert_drifted( + plan_err( + &g.table, + "a re-created table must not reuse this handle's grant", + ) + .await, + "now resolves to uuid", + ); +} + +#[tokio::test] +async fn test_query_auth_refuses_a_table_recreated_while_planning() { + let g = guarded("planned", &["id"]).await; + g.ctx + .server + .set_table_uuid_after_calls("default", "planned", "uuid-of-the-replacement", 2); + + assert_drifted( + plan_err(&g.table, "the files just planned belong to the replacement").await, + "now resolves to uuid", + ); +} + +#[tokio::test] +async fn test_query_auth_refuses_a_table_recreated_mid_exchange() { + let g = guarded("swapped", &["id"]).await; + g.ctx + .server + .set_table_uuid_after_auth("default", "swapped", "uuid-after-the-exchange"); + + assert_drifted( + plan_err(&g.table, "a table replaced mid-exchange must be refused").await, + "now resolves to uuid", + ); +} + +#[tokio::test] +async fn test_query_auth_refuses_a_server_that_reports_no_identity() { + let g = guarded("anonymous", &["id"]).await; + g.ctx.server.clear_table_identity("default", "anonymous"); + + assert_drifted( + plan_err( + &g.table, + "a check that cannot establish the table has not checked anything", + ) + .await, + "nothing the server reports", + ); +} + +#[tokio::test] +async fn test_query_auth_user_granted_all_business_columns_can_read() { + let g = guarded("granted", &["id", "name"]).await; + g.ctx.server.set_column_auth( + "default", + "granted", + vec!["id".to_string(), "name".to_string()], + ); + + g.table + .new_read_builder() + .new_scan() + .plan() + .await + .expect("a user granted every column must be authorized"); +} + +#[tokio::test] +async fn test_query_auth_enabled_after_a_handle_was_loaded_is_still_enforced() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + ctx.server + .add_table_with_schema("default", "later", schema_of(&["id"], &[]), &path); + let table = ctx + .catalog + .get_table(&Identifier::new("default", "later")) + .await + .unwrap(); + + ctx.server + .set_table_schema_id("default", "later", schema_of(&["id"], GUARDED), 0); + ctx.server + .set_auth_response("default", "later", restricted()); + + assert_refused( + plan_err( + &table, + "a handle loaded before the option was set must still be authorized", + ) + .await, + ); +} + +#[tokio::test] +async fn test_query_auth_is_not_weakened_by_a_table_recreated_under_the_same_name() { + let g = guarded("guarded", &["id"]).await; + g.ctx + .server + .set_table_schema_id("default", "guarded", schema_of(&["id"], &[]), 0); + + let err = g + .table + .new_read_builder() + .new_scan() + .with_scan_all_files() + .plan() + .await + .expect_err("the answer is now about a different table over the same files"); + assert_refused(err); +} + +#[tokio::test] +async fn test_query_auth_refuses_a_decorated_handle() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + for name in ["guarded$branch_dev", "guarded$files"] { + ctx.server + .add_table_with_schema("default", name, schema_of(&["id"], GUARDED), &path); + let table = ctx + .catalog + .get_table(&Identifier::new("default", name)) + .await + .unwrap(); + assert_refused( + plan_err( + &table, + "the decorated endpoint rules on files this handle does not read", + ) + .await, + ); + } +} + +#[tokio::test] +async fn test_query_auth_enabled_after_a_load_still_refuses_searches() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + ctx.server + .add_table_with_schema("default", "searched", schema_of(&["id"], &[]), &path); + let table = ctx + .catalog + .get_table(&Identifier::new("default", "searched")) + .await + .unwrap(); + ctx.server + .set_table_schema_id("default", "searched", schema_of(&["id"], GUARDED), 0); + ctx.server + .set_auth_response("default", "searched", restricted()); + + let err = table + .new_vector_search_builder() + .execute() + .await + .expect_err("a search reads index files and cannot apply the server's rules"); + assert_refused(err); +} + +#[tokio::test] +async fn test_a_search_entry_asks_the_server_once() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + ctx.server + .add_table_with_schema("default", "searchable", schema_of(&["id"], &[]), &path); + let table = ctx + .catalog + .get_table(&Identifier::new("default", "searchable")) + .await + .unwrap(); + + let before = ctx.server.get_table_calls(); + let _ = table.new_vector_search_builder().execute().await; + assert_eq!( + ctx.server.get_table_calls() - before, + 1, + "the search entry itself must ask exactly once" + ); +} + +#[tokio::test] +async fn test_query_auth_refuses_an_assembled_handle() { + let g = guarded("guarded", &["id"]).await; + let elsewhere = tempfile::tempdir().unwrap(); + for (schema, location) in [ + (g.table.schema().clone(), g.table.location().to_string()), + ( + paimon::spec::TableSchema::new( + g.table.schema().id(), + &schema_of(&["id", "dropped"], GUARDED), + ), + g.table.location().to_string(), + ), + ( + g.table.schema().clone(), + format!("file://{}", elsewhere.path().display()), + ), + ] { + let assembled = paimon::table::Table::new( + g.table.file_io().clone(), + g.identifier.clone(), + location, + schema, + g.table.rest_env().cloned(), + ); + assert_refused(plan_err(&assembled, "an assembled handle carries no session").await); + } +} + +#[tokio::test] +async fn test_planning_an_ordinary_rest_table_asks_the_server_once() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + ctx.server + .add_table_with_schema("default", "plain", schema_of(&["id"], &[]), &path); + let table = ctx + .catalog + .get_table(&Identifier::new("default", "plain")) + .await + .unwrap(); + + let before = ctx.server.get_table_calls(); + table.new_read_builder().new_scan().plan().await.unwrap(); + assert_eq!( + ctx.server.get_table_calls() - before, + 1, + "planning must not repeat the query-auth lookup" + ); +} + +#[tokio::test] +async fn test_query_auth_refuses_scan_all_files_and_format_tables() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + ctx.server + .add_table_with_schema("default", "metadata", schema_of(&["id"], &[]), &path); + let table = ctx + .catalog + .get_table(&Identifier::new("default", "metadata")) + .await + .unwrap(); + ctx.server + .set_table_schema_id("default", "metadata", schema_of(&["id"], GUARDED), 0); + + let err = table + .new_read_builder() + .new_scan() + .with_scan_all_files() + .plan() + .await + .expect_err("file metadata is not something the auth endpoint can rule on"); + assert_refused(err); + + let format = &[("type", "format-table"), ("file.format", "parquet")]; + ctx.server + .add_table_with_schema("default", "fmt", schema_of(&["id"], format), &path); + let fmt = ctx + .catalog + .get_table(&Identifier::new("default", "fmt")) + .await + .unwrap(); + let mut guarded_format = format.to_vec(); + guarded_format.push(("query-auth.enabled", "true")); + ctx.server + .set_table_schema_id("default", "fmt", schema_of(&["id"], &guarded_format), 0); + + assert_refused(plan_err(&fmt, "a format table cannot apply the server's rules").await); + + assert_refused( + table + .new_read_builder() + .new_incremental_scan(paimon::table::IncrementalScanMode::Delta, 0, 1) + .plan() + .await + .expect_err("an incremental read cannot apply the server's rules"), + ); +} + #[tokio::test] async fn test_rest_catalog_manages_permissions_end_to_end() { let ctx = setup_catalog(vec!["default"]).await; From ab6f29296435d280af1bee15ced635170f15a26e Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Tue, 1 Sep 2026 12:15:43 -0400 Subject: [PATCH 2/7] feat(auth): ask the server on more read paths and cover nested and old-schema drift --- .../paimon/src/table/data_evolution_writer.rs | 8 +- crates/paimon/src/table/query_auth.rs | 146 +++++++++++++----- crates/paimon/tests/rest_catalog_test.rs | 91 ++++++++++- 3 files changed, 197 insertions(+), 48 deletions(-) diff --git a/crates/paimon/src/table/data_evolution_writer.rs b/crates/paimon/src/table/data_evolution_writer.rs index d28553485..17666b2a4 100644 --- a/crates/paimon/src/table/data_evolution_writer.rs +++ b/crates/paimon/src/table/data_evolution_writer.rs @@ -160,7 +160,9 @@ impl DataEvolutionWriter { #[must_use = "commit messages must be passed to TableCommit"] pub async fn prepare_commit(self) -> Result> { // A row-id update reads the original rows it rewrites. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table + .ensure_read_authorized_live("a row-id update") + .await?; let total_matched: usize = self.matched_batches.iter().map(|b| b.num_rows()).sum(); if total_matched == 0 { @@ -478,7 +480,9 @@ impl DataEvolutionDeleteWriter { #[must_use = "commit messages must be passed to TableCommit"] pub async fn prepare_commit(mut self) -> Result> { // A row-id delete reads the files it rewrites. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table + .ensure_read_authorized_live("a row-id delete") + .await?; dedup_i64_in_place(&mut self.row_ids); if self.row_ids.is_empty() { diff --git a/crates/paimon/src/table/query_auth.rs b/crates/paimon/src/table/query_auth.rs index 721e10b8b..271b47b45 100644 --- a/crates/paimon/src/table/query_auth.rs +++ b/crates/paimon/src/table/query_auth.rs @@ -227,51 +227,62 @@ mod tests { ); } + fn data_file_for_stats( + schema_id: i64, + cols: Option>, + written: Option>, + ) -> crate::spec::DataFileMeta { + crate::spec::DataFileMeta { + file_name: "f.parquet".to_string(), + file_size: 1, + row_count: 1, + min_key: Vec::new(), + max_key: Vec::new(), + key_stats: crate::spec::stats::BinaryTableStats::empty(), + value_stats: crate::spec::stats::BinaryTableStats::empty(), + min_sequence_number: 0, + max_sequence_number: 0, + schema_id, + level: 0, + extra_files: Vec::new(), + creation_time: None, + delete_row_count: Some(0), + embedded_index: None, + file_source: None, + value_stats_cols: cols.map(|c| c.iter().map(|s| s.to_string()).collect()), + external_path: None, + first_row_id: None, + write_cols: written.map(|c| c.iter().map(|s| s.to_string()).collect()), + column_max_sequence_numbers: None, + } + } + + fn plan_of(meta: crate::spec::DataFileMeta) -> crate::table::Plan { + crate::table::Plan::new(vec![crate::table::DataSplitBuilder::new() + .with_snapshot(1) + .with_partition(crate::spec::BinaryRowBuilder::new(0).build()) + .with_bucket(0) + .with_bucket_path("p".to_string()) + .with_total_buckets(1) + .with_data_files(vec![meta]) + .with_raw_convertible(false) + .build() + .unwrap()]) + } + #[tokio::test] async fn test_stats_for_a_dropped_column_are_refused() { let table = query_auth_table(); - let file = - |cols: Option>, written: Option>| crate::spec::DataFileMeta { - file_name: "f.parquet".to_string(), - file_size: 1, - row_count: 1, - min_key: Vec::new(), - max_key: Vec::new(), - key_stats: crate::spec::stats::BinaryTableStats::empty(), - value_stats: crate::spec::stats::BinaryTableStats::empty(), - min_sequence_number: 0, - max_sequence_number: 0, - schema_id: table.schema().id(), - level: 0, - extra_files: Vec::new(), - creation_time: None, - delete_row_count: Some(0), - embedded_index: None, - file_source: None, - value_stats_cols: cols.map(|c| c.iter().map(|s| s.to_string()).collect()), - external_path: None, - first_row_id: None, - write_cols: written.map(|c| c.iter().map(|s| s.to_string()).collect()), - column_max_sequence_numbers: None, - }; - let plan_of = |meta| { - crate::table::Plan::new(vec![crate::table::DataSplitBuilder::new() - .with_snapshot(1) - .with_partition(crate::spec::BinaryRowBuilder::new(0).build()) - .with_bucket(0) - .with_bucket_path("p".to_string()) - .with_total_buckets(1) - .with_data_files(vec![meta]) - .with_raw_convertible(false) - .build() - .unwrap()]) - }; let schemas = table.schema_manager(); for meta in [ - file(Some(vec!["id", "gone"]), None), - file(None, Some(vec!["id", "gone"])), - file(Some(vec!["id"]), Some(vec!["id", "gone"])), + data_file_for_stats(table.schema().id(), Some(vec!["id", "gone"]), None), + data_file_for_stats(table.schema().id(), None, Some(vec!["id", "gone"])), + data_file_for_stats( + table.schema().id(), + Some(vec!["id"]), + Some(vec!["id", "gone"]), + ), ] { let err = super::reject_unauthorized_stats(&plan_of(meta), table.schema(), schemas) .await @@ -284,7 +295,11 @@ mod tests { } assert!(super::reject_unauthorized_stats( - &plan_of(file(Some(vec!["id"]), Some(vec!["id"]))), + &plan_of(data_file_for_stats( + table.schema().id(), + Some(vec!["id"]), + Some(vec!["id"]) + )), table.schema(), schemas ) @@ -292,6 +307,57 @@ mod tests { .is_ok()); } + #[tokio::test] + async fn test_an_old_schema_whose_column_changed_type_is_refused() { + let tmp = tempfile::tempdir().unwrap(); + let location = tmp.path().display().to_string(); + let column = |ty| { + crate::spec::Schema::builder() + .column("id", ty) + .option("query-auth.enabled", "true") + .build() + .unwrap() + }; + let table = crate::table::Table::new( + crate::io::FileIOBuilder::new("file").build().unwrap(), + crate::catalog::Identifier::new("default", "evolved"), + location, + crate::spec::TableSchema::new( + 0, + &column(crate::spec::DataType::Int(crate::spec::IntType::new())), + ), + None, + ); + + // Same field id and name, a different type: the server ruled on the + // current one, so the older file's stats are not covered. + let older = crate::spec::TableSchema::new( + 1, + &column(crate::spec::DataType::BigInt(crate::spec::BigIntType::new())), + ); + let schemas = table.schema_manager(); + table + .file_io() + .new_output(&schemas.schema_path(1)) + .unwrap() + .write(serde_json::to_vec(&older).unwrap().into()) + .await + .unwrap(); + + let err = super::reject_unauthorized_stats( + &plan_of(data_file_for_stats(1, None, None)), + table.schema(), + schemas, + ) + .await + .unwrap_err(); + assert!( + matches!(err, crate::Error::Unsupported { ref message } + if message.contains("statistics for 'id'")), + "{err:?}" + ); + } + #[test] fn test_a_system_column_read_is_refused() { let err = reject_system_columns(["id", crate::spec::ROW_ID_FIELD_NAME]).unwrap_err(); diff --git a/crates/paimon/tests/rest_catalog_test.rs b/crates/paimon/tests/rest_catalog_test.rs index 284d8fab1..d2f81c51e 100644 --- a/crates/paimon/tests/rest_catalog_test.rs +++ b/crates/paimon/tests/rest_catalog_test.rs @@ -2802,6 +2802,55 @@ async fn test_query_auth_is_not_weakened_by_a_table_recreated_under_the_same_nam assert_refused(err); } +#[tokio::test] +async fn test_query_auth_refuses_a_read_type_with_an_extra_nested_field() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + let nested = |extra: bool| { + let mut children = vec![paimon::spec::DataField::new( + 1, + "a".to_string(), + DataType::Int(IntType::new()), + )]; + if extra { + children.push(paimon::spec::DataField::new( + 2, + "hidden".to_string(), + DataType::Int(IntType::new()), + )); + } + DataType::Row(paimon::spec::RowType::new(children)) + }; + let served = Schema::builder() + .column("info", nested(false)) + .option("query-auth.enabled", "true") + .build() + .unwrap(); + ctx.server + .add_table_with_schema("default", "nested", served, &path); + + let table = ctx + .catalog + .get_table(&Identifier::new("default", "nested")) + .await + .unwrap(); + let plan = table.new_read_builder().new_scan().plan().await.unwrap(); + + // Same field id and name as the authorized column, one nested child more. + let forged = paimon::spec::DataField::new( + table.schema().fields()[0].id(), + "info".to_string(), + nested(true), + ); + let mut builder = table.new_read_builder(); + builder.with_read_type(vec![forged]); + let Err(err) = builder.new_read().unwrap().to_arrow(plan.splits()) else { + panic!("a nested child the server never ruled on must be refused") + }; + assert_refused(err); +} + #[tokio::test] async fn test_query_auth_refuses_a_decorated_handle() { let ctx = setup_catalog(vec!["default"]).await; @@ -2842,12 +2891,42 @@ async fn test_query_auth_enabled_after_a_load_still_refuses_searches() { ctx.server .set_auth_response("default", "searched", restricted()); - let err = table - .new_vector_search_builder() - .execute() - .await - .expect_err("a search reads index files and cannot apply the server's rules"); - assert_refused(err); + assert_refused( + table + .new_vector_search_builder() + .execute() + .await + .expect_err("a vector search reads index files directly"), + ); + #[cfg(feature = "fulltext")] + assert_refused( + table + .new_full_text_search_builder() + .execute() + .await + .expect_err("a full-text search reads index files directly"), + ); + assert_refused( + table + .new_hybrid_search_builder() + .execute() + .await + .expect_err("a hybrid search reads index files directly"), + ); + assert_refused( + table + .new_batch_vector_search_builder() + .execute() + .await + .expect_err("the batch path is reachable without the outer builder"), + ); + assert_refused( + table + .new_lumina_index_build_builder() + .execute() + .await + .expect_err("building an index scans the table's rows"), + ); } #[tokio::test] From 223011d377f810717f80a1f69fe76058448bc440 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Tue, 1 Sep 2026 20:08:37 -0400 Subject: [PATCH 3/7] feat(auth): ask the server on metadata paths --- .../paimon/src/catalog/partition_listing.rs | 4 +- .../src/table/global_index_drop_builder.rs | 4 +- crates/paimon/src/table/mod.rs | 7 +- crates/paimon/src/table/partition_stat.rs | 3 +- crates/paimon/src/table/query_auth.rs | 68 +++++++++++++++++-- crates/paimon/src/table/table_commit.rs | 10 +-- crates/paimon/src/table/table_scan.rs | 4 +- crates/paimon/tests/rest_catalog_test.rs | 32 +++++++++ 8 files changed, 112 insertions(+), 20 deletions(-) diff --git a/crates/paimon/src/catalog/partition_listing.rs b/crates/paimon/src/catalog/partition_listing.rs index 9bb28d309..75e439c7c 100644 --- a/crates/paimon/src/catalog/partition_listing.rs +++ b/crates/paimon/src/catalog/partition_listing.rs @@ -33,7 +33,9 @@ use crate::Result; /// matching the shape catalogs would otherwise return from a metastore. pub async fn list_partitions_from_file_system(table: &Table) -> Result> { // Manifests carry partition values and per-column stats. - crate::spec::CoreOptions::new(table.schema().options()).ensure_read_authorized()?; + table + .ensure_read_authorized_live("listing partitions") + .await?; let file_io = table.file_io(); let snapshot_sm = table.snapshot_manager(); let manifest_sm = SnapshotManager::new(file_io.clone(), table.location().to_string()); diff --git a/crates/paimon/src/table/global_index_drop_builder.rs b/crates/paimon/src/table/global_index_drop_builder.rs index 170e7097b..6b0dcf03d 100644 --- a/crates/paimon/src/table/global_index_drop_builder.rs +++ b/crates/paimon/src/table/global_index_drop_builder.rs @@ -51,7 +51,9 @@ impl<'a> GlobalIndexDropBuilder<'a> { pub async fn execute(&self) -> Result { // Dropping an index reads the index manifest. - crate::spec::CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table + .ensure_read_authorized_live("dropping an index") + .await?; self.table.ensure_not_branch_reference_for_write()?; diff --git a/crates/paimon/src/table/mod.rs b/crates/paimon/src/table/mod.rs index 9371e2e7e..ea7a3de52 100644 --- a/crates/paimon/src/table/mod.rs +++ b/crates/paimon/src/table/mod.rs @@ -359,8 +359,7 @@ impl Table { } /// The live counterpart of [`CoreOptions::ensure_read_authorized`], which - /// reads the schema this handle was loaded with. Paths that can await but - /// cannot apply the server's rules must ask instead. + /// reads the schema this handle was loaded with. pub(crate) async fn ensure_read_authorized_live(&self, path: &str) -> Result<()> { let local = CoreOptions::new(self.schema.options()); local.ensure_type_paimon_served(&self.identifier.full_name())?; @@ -391,8 +390,8 @@ impl Table { } /// Whether this user may read this table; `None` when it is not - /// `query-auth.enabled`. `server_query_auth` is the caller's already-fetched - /// [`Self::server_query_auth_enabled`], so planning asks the server once. + /// `query-auth.enabled`. `server_query_auth` is the caller's, so planning + /// asks the server once. pub(crate) async fn authorize_read( &self, server_query_auth: bool, diff --git a/crates/paimon/src/table/partition_stat.rs b/crates/paimon/src/table/partition_stat.rs index caa8c4a74..d78b62e49 100644 --- a/crates/paimon/src/table/partition_stat.rs +++ b/crates/paimon/src/table/partition_stat.rs @@ -65,7 +65,8 @@ impl Table { /// Returns an empty Vec when the table has no snapshots yet. pub async fn partition_stats(&self) -> crate::Result> { // Manifests carry partition values and per-column stats. - CoreOptions::new(self.schema().options()).ensure_read_authorized()?; + self.ensure_read_authorized_live("partition statistics") + .await?; let sm = SnapshotManager::new(self.file_io().clone(), self.location().to_string()); let snapshot = match sm.get_latest_snapshot().await? { Some(s) => s, diff --git a/crates/paimon/src/table/query_auth.rs b/crates/paimon/src/table/query_auth.rs index 271b47b45..14fafa186 100644 --- a/crates/paimon/src/table/query_auth.rs +++ b/crates/paimon/src/table/query_auth.rs @@ -50,7 +50,7 @@ impl QueryAuthGrant { } /// `value_stats` and `write_cols` are public on every split and an older file -/// can name a dropped column. Refused rather than scrubbed: rewriting encoded +/// can name a dropped column. Refused rather than scrubbed — rewriting encoded /// stats is how bounds get mismatched. pub(crate) async fn reject_unauthorized_stats( plan: &super::Plan, @@ -85,7 +85,9 @@ pub(crate) async fn reject_unauthorized_stats( let older = schemas.schema(file.schema_id).await?; if let Some(gone) = older.fields().iter().find(|f| { !current.fields().iter().any(|c| { - c.id() == f.id() && c.name() == f.name() && c.data_type() == f.data_type() + c.id() == f.id() + && c.name() == f.name() + && shape(c.data_type()) == shape(f.data_type()) }) }) { return refuse(gone.name()); @@ -95,6 +97,36 @@ pub(crate) async fn reject_unauthorized_stats( Ok(()) } +/// The physical shape, descriptions stripped: `DataField` equality includes them, +/// so a comment-only edit would otherwise read as an unauthorized column. +fn shape(ty: &crate::spec::DataType) -> crate::spec::DataType { + use crate::spec::{ArrayType, DataType, MapType, MultisetType, RowType}; + match ty { + DataType::Row(row) => DataType::Row(RowType::new( + row.fields() + .iter() + .map(|f| { + crate::spec::DataField::new(f.id(), f.name().to_string(), shape(f.data_type())) + }) + .collect(), + )), + DataType::Array(a) => DataType::Array(ArrayType::with_nullable( + ty.is_nullable(), + shape(a.element_type()), + )), + DataType::Multiset(m) => DataType::Multiset(MultisetType::with_nullable( + ty.is_nullable(), + shape(m.element_type()), + )), + DataType::Map(m) => DataType::Map(MapType::with_nullable( + ty.is_nullable(), + shape(m.key_type()), + shape(m.value_type()), + )), + other => other.clone(), + } +} + /// A refusal naming the option, so callers never match on prose. pub(crate) fn unsupported(reason: &str) -> crate::Error { crate::Error::Unsupported { @@ -130,11 +162,12 @@ pub(crate) fn reject_noncanonical_fields( if crate::spec::is_reserved_system_field_name(field.name()) { continue; } - // The whole type: an older field can keep `(id, name)` and carry an extra - // nested child. Nested ids are unassigned, so a legitimate read type - // carries the schema field's shape whole. + // The whole shape: an older field can keep `(id, name)` and carry an + // extra nested child. let canonical = schema_fields.iter().any(|f| { - f.id() == field.id() && f.name() == field.name() && f.data_type() == field.data_type() + f.id() == field.id() + && f.name() == field.name() + && shape(f.data_type()) == shape(field.data_type()) }); if !canonical { return Err(unsupported(&format!( @@ -358,6 +391,29 @@ mod tests { ); } + #[test] + fn test_a_comment_only_change_is_not_a_different_column() { + use crate::spec::{DataField, DataType, IntType, RowType}; + let child = |desc: Option<&str>| { + let f = DataField::new(1, "a".to_string(), DataType::Int(IntType::new())); + match desc { + Some(d) => f.with_description(Some(d.to_string())), + None => f, + } + }; + let row = |desc| DataType::Row(RowType::new(vec![child(desc)])); + assert_ne!( + row(None), + row(Some("why")), + "equality includes descriptions" + ); + assert_eq!( + super::shape(&row(None)), + super::shape(&row(Some("why"))), + "but a comment is not a column the server did not authorize" + ); + } + #[test] fn test_a_system_column_read_is_refused() { let err = reject_system_columns(["id", crate::spec::ROW_ID_FIELD_NAME]).unwrap_err(); diff --git a/crates/paimon/src/table/table_commit.rs b/crates/paimon/src/table/table_commit.rs index 676d53587..8a3735d6c 100644 --- a/crates/paimon/src/table/table_commit.rs +++ b/crates/paimon/src/table/table_commit.rs @@ -205,7 +205,7 @@ impl TableCommit { filter_committed: bool, ) -> Result<()> { // A commit validates against the existing snapshot. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table.ensure_read_authorized_live("a commit").await?; self.table.ensure_not_branch_reference_for_write()?; validate_fixed_bucket_commit_mode(&commit_messages, false)?; validate_bucket_ownership(&commit_messages)?; @@ -252,7 +252,7 @@ impl TableCommit { commit_identifier: i64, ) -> Result<()> { // A commit validates against the existing snapshot. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table.ensure_read_authorized_live("a commit").await?; self.table.ensure_not_branch_reference_for_write()?; validate_fixed_bucket_commit_mode(&commit_messages, false)?; validate_bucket_ownership(&commit_messages)?; @@ -337,7 +337,7 @@ impl TableCommit { filter_committed: bool, ) -> Result<()> { // A commit validates against the existing snapshot. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table.ensure_read_authorized_live("a commit").await?; self.table.ensure_not_branch_reference_for_write()?; validate_fixed_bucket_commit_mode(&commit_messages, true)?; validate_bucket_ownership(&commit_messages)?; @@ -593,7 +593,7 @@ impl TableCommit { filter_committed: bool, ) -> Result<()> { // A commit validates against the existing snapshot. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table.ensure_read_authorized_live("a commit").await?; self.table.ensure_not_branch_reference_for_write()?; if partitions.is_empty() { @@ -674,7 +674,7 @@ impl TableCommit { filter_committed: bool, ) -> Result<()> { // A commit validates against the existing snapshot. - CoreOptions::new(self.table.schema().options()).ensure_read_authorized()?; + self.table.ensure_read_authorized_live("a commit").await?; self.table.ensure_not_branch_reference_for_write()?; self.try_commit( diff --git a/crates/paimon/src/table/table_scan.rs b/crates/paimon/src/table/table_scan.rs index b379aeb22..5137cef33 100644 --- a/crates/paimon/src/table/table_scan.rs +++ b/crates/paimon/src/table/table_scan.rs @@ -1138,8 +1138,8 @@ impl<'a> PaimonTableScan<'a> { } /// The grant predates the manifest read, so the table can have been re-created - /// at the same path in between. Also refuses a plan whose files carry - /// statistics the current schema no longer covers. + /// at the same path in between. Also refuses statistics the current schema + /// no longer covers. async fn check_planned_files(&self, plan: &Plan, query_auth: bool) -> crate::Result<()> { if !query_auth { return Ok(()); diff --git a/crates/paimon/tests/rest_catalog_test.rs b/crates/paimon/tests/rest_catalog_test.rs index d2f81c51e..6aced8b15 100644 --- a/crates/paimon/tests/rest_catalog_test.rs +++ b/crates/paimon/tests/rest_catalog_test.rs @@ -2874,6 +2874,38 @@ async fn test_query_auth_refuses_a_decorated_handle() { } } +#[tokio::test] +async fn test_query_auth_enabled_after_a_load_still_refuses_metadata_and_writes() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + ctx.server + .add_table_with_schema("default", "meta", schema_of(&["id"], &[]), &path); + let table = ctx + .catalog + .get_table(&Identifier::new("default", "meta")) + .await + .unwrap(); + ctx.server + .set_table_schema_id("default", "meta", schema_of(&["id"], GUARDED), 0); + ctx.server + .set_auth_response("default", "meta", restricted()); + + assert_refused( + table + .partition_stats() + .await + .expect_err("partition stats expose partition values, row counts and sizes"), + ); + assert_refused( + table + .new_global_index_drop_builder() + .execute() + .await + .expect_err("dropping an index is not something a restricted user may do"), + ); +} + #[tokio::test] async fn test_query_auth_enabled_after_a_load_still_refuses_searches() { let ctx = setup_catalog(vec!["default"]).await; From 24967015589a194b3ec5f95e16bc0f577f50a7e2 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Fri, 11 Sep 2026 10:44:51 -0400 Subject: [PATCH 4/7] feat(auth): ask the server before the first write, verify the uuid behind a live false, and ask the branch --- .../datafusion/src/system_tables/branches.rs | 1 + .../datafusion/src/system_tables/consumers.rs | 1 + .../datafusion/src/system_tables/files.rs | 1 + .../datafusion/src/system_tables/manifests.rs | 1 + .../datafusion/src/system_tables/mod.rs | 8 + .../datafusion/src/system_tables/options.rs | 1 + .../src/system_tables/partitions.rs | 1 + .../src/system_tables/physical_files_size.rs | 1 + .../system_tables/referenced_files_size.rs | 1 + .../datafusion/src/system_tables/schemas.rs | 1 + .../datafusion/src/system_tables/snapshots.rs | 1 + .../src/system_tables/table_indexes.rs | 1 + .../datafusion/src/system_tables/tags.rs | 1 + crates/paimon-rest-server/src/lib.rs | 7 +- crates/paimon-rest-server/tests/e2e.rs | 170 ++++++++ crates/paimon/src/catalog/filesystem.rs | 145 ++++++- crates/paimon/src/catalog/mod.rs | 15 + .../paimon/src/catalog/rest/rest_catalog.rs | 4 + crates/paimon/src/table/format_table_scan.rs | 20 +- crates/paimon/src/table/incremental_scan.rs | 10 +- crates/paimon/src/table/mod.rs | 54 +-- crates/paimon/src/table/query_auth.rs | 121 +++--- crates/paimon/src/table/rest_env.rs | 150 +++++-- crates/paimon/src/table/table_commit.rs | 55 ++- crates/paimon/src/table/table_write.rs | 16 + crates/paimon/tests/mock_server.rs | 5 +- crates/paimon/tests/rest_catalog_test.rs | 383 +++++++++++------- 27 files changed, 892 insertions(+), 283 deletions(-) diff --git a/crates/integrations/datafusion/src/system_tables/branches.rs b/crates/integrations/datafusion/src/system_tables/branches.rs index c925def71..54168ae6c 100644 --- a/crates/integrations/datafusion/src/system_tables/branches.rs +++ b/crates/integrations/datafusion/src/system_tables/branches.rs @@ -74,6 +74,7 @@ impl TableProvider for BranchesTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let table = self.table.clone(); let (names, create_times) = crate::runtime::await_with_runtime(async move { collect_branches(&table).await }) diff --git a/crates/integrations/datafusion/src/system_tables/consumers.rs b/crates/integrations/datafusion/src/system_tables/consumers.rs index 40c922cee..1bf9dbe2e 100644 --- a/crates/integrations/datafusion/src/system_tables/consumers.rs +++ b/crates/integrations/datafusion/src/system_tables/consumers.rs @@ -72,6 +72,7 @@ impl TableProvider for ConsumersTable { filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let manager = self.table.consumer_manager(); let requested_ids = requested_consumer_ids(filters); let consumers = crate::runtime::await_with_runtime(async move { diff --git a/crates/integrations/datafusion/src/system_tables/files.rs b/crates/integrations/datafusion/src/system_tables/files.rs index e9749007a..5668c06da 100644 --- a/crates/integrations/datafusion/src/system_tables/files.rs +++ b/crates/integrations/datafusion/src/system_tables/files.rs @@ -105,6 +105,7 @@ impl TableProvider for FilesTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let table = self.table.clone(); let rows = crate::runtime::await_with_runtime(async move { collect_file_rows(&table).await }) diff --git a/crates/integrations/datafusion/src/system_tables/manifests.rs b/crates/integrations/datafusion/src/system_tables/manifests.rs index 9380b316c..cbeca86ab 100644 --- a/crates/integrations/datafusion/src/system_tables/manifests.rs +++ b/crates/integrations/datafusion/src/system_tables/manifests.rs @@ -82,6 +82,7 @@ impl TableProvider for ManifestsTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let table = self.table.clone(); let metas = crate::runtime::await_with_runtime(async move { collect_manifests(&table).await }) diff --git a/crates/integrations/datafusion/src/system_tables/mod.rs b/crates/integrations/datafusion/src/system_tables/mod.rs index c81b8403d..3435c347b 100644 --- a/crates/integrations/datafusion/src/system_tables/mod.rs +++ b/crates/integrations/datafusion/src/system_tables/mod.rs @@ -124,6 +124,14 @@ fn wrap_to_system_table(name: &str, base_table: Table) -> Option DFResult<()> { + crate::runtime::await_with_runtime(table.ensure_read_authorized()) + .await + .map_err(to_datafusion_error) +} + pub(crate) fn provider_for_table( catalog: Arc, identifier: Identifier, diff --git a/crates/integrations/datafusion/src/system_tables/options.rs b/crates/integrations/datafusion/src/system_tables/options.rs index 04d85f87f..b78df0702 100644 --- a/crates/integrations/datafusion/src/system_tables/options.rs +++ b/crates/integrations/datafusion/src/system_tables/options.rs @@ -68,6 +68,7 @@ impl TableProvider for OptionsTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; // Java uses LinkedHashMap insertion order; HashMap has none — sort for stable output. let mut entries: Vec<(&String, &String)> = self.table.schema().options().iter().collect(); entries.sort_by(|a, b| a.0.cmp(b.0)); diff --git a/crates/integrations/datafusion/src/system_tables/partitions.rs b/crates/integrations/datafusion/src/system_tables/partitions.rs index 749bb2820..2052c1811 100644 --- a/crates/integrations/datafusion/src/system_tables/partitions.rs +++ b/crates/integrations/datafusion/src/system_tables/partitions.rs @@ -121,6 +121,7 @@ impl TableProvider for PartitionsTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let table = self.table.clone(); let partitions = if table.travel_snapshot().is_some() { crate::runtime::await_with_runtime(async move { diff --git a/crates/integrations/datafusion/src/system_tables/physical_files_size.rs b/crates/integrations/datafusion/src/system_tables/physical_files_size.rs index 01ef43c1b..5a6afbea7 100644 --- a/crates/integrations/datafusion/src/system_tables/physical_files_size.rs +++ b/crates/integrations/datafusion/src/system_tables/physical_files_size.rs @@ -75,6 +75,7 @@ impl TableProvider for PhysicalFilesSizeTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let table = self.table.clone(); let summary = crate::runtime::await_with_runtime(async move { let partition_depth = table.schema().partition_keys().len(); diff --git a/crates/integrations/datafusion/src/system_tables/referenced_files_size.rs b/crates/integrations/datafusion/src/system_tables/referenced_files_size.rs index 568663ca3..f1ff12aa2 100644 --- a/crates/integrations/datafusion/src/system_tables/referenced_files_size.rs +++ b/crates/integrations/datafusion/src/system_tables/referenced_files_size.rs @@ -76,6 +76,7 @@ impl TableProvider for ReferencedFilesSizeTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let table = self.table.clone(); let summaries = crate::runtime::await_with_runtime(async move { let schema = table.schema(); diff --git a/crates/integrations/datafusion/src/system_tables/schemas.rs b/crates/integrations/datafusion/src/system_tables/schemas.rs index 7575b3b02..171e67d9a 100644 --- a/crates/integrations/datafusion/src/system_tables/schemas.rs +++ b/crates/integrations/datafusion/src/system_tables/schemas.rs @@ -80,6 +80,7 @@ impl TableProvider for SchemasTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let table = self.table.clone(); let schemas = crate::runtime::await_with_runtime( diff --git a/crates/integrations/datafusion/src/system_tables/snapshots.rs b/crates/integrations/datafusion/src/system_tables/snapshots.rs index 040c51c38..987df8a82 100644 --- a/crates/integrations/datafusion/src/system_tables/snapshots.rs +++ b/crates/integrations/datafusion/src/system_tables/snapshots.rs @@ -86,6 +86,7 @@ impl TableProvider for SnapshotsTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let sm = self.table.snapshot_manager(); let snapshots = crate::runtime::await_with_runtime(async move { sm.list_all().await }) .await diff --git a/crates/integrations/datafusion/src/system_tables/table_indexes.rs b/crates/integrations/datafusion/src/system_tables/table_indexes.rs index cbd1c2c1a..184a288f3 100644 --- a/crates/integrations/datafusion/src/system_tables/table_indexes.rs +++ b/crates/integrations/datafusion/src/system_tables/table_indexes.rs @@ -104,6 +104,7 @@ impl TableProvider for TableIndexesTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let table = self.table.clone(); let entries = crate::runtime::await_with_runtime(async move { collect_index_entries(&table).await }) diff --git a/crates/integrations/datafusion/src/system_tables/tags.rs b/crates/integrations/datafusion/src/system_tables/tags.rs index 433d59f17..9e5de4a30 100644 --- a/crates/integrations/datafusion/src/system_tables/tags.rs +++ b/crates/integrations/datafusion/src/system_tables/tags.rs @@ -83,6 +83,7 @@ impl TableProvider for TagsTable { _filters: &[Expr], _limit: Option, ) -> DFResult> { + super::ensure_scan_authorized(&self.table).await?; let tm = self.table.tag_manager(); let tags = crate::runtime::await_with_runtime(async move { tm.list_all_with_metadata().await }) diff --git a/crates/paimon-rest-server/src/lib.rs b/crates/paimon-rest-server/src/lib.rs index 403b42203..3c10ca7ed 100644 --- a/crates/paimon-rest-server/src/lib.rs +++ b/crates/paimon-rest-server/src/lib.rs @@ -437,10 +437,11 @@ async fn get_table(path: RestPath, Extension(state): Extension>) - } }; + // FileSystemCatalog has no UUID concept; the full name is a stable id that + // satisfies the client's RESTEnv requirement. + let uuid = identifier.full_name(); let response = GetTableResponse::new( - // FileSystemCatalog has no UUID concept; the full name is a stable id - // that satisfies the client's RESTEnv requirement. - Some(identifier.full_name()), + Some(uuid), Some(table), Some(location), Some(false), diff --git a/crates/paimon-rest-server/tests/e2e.rs b/crates/paimon-rest-server/tests/e2e.rs index db27bb3eb..42cc45da3 100644 --- a/crates/paimon-rest-server/tests/e2e.rs +++ b/crates/paimon-rest-server/tests/e2e.rs @@ -525,3 +525,173 @@ async fn altering_the_declared_type_is_rejected() { .await .expect("still readable"); } + +#[tokio::test] +async fn test_branch_scan_against_the_real_server() { + let ctx = setup().await; + ctx.catalog + .create_database("db", true, HashMap::new()) + .await + .unwrap(); + let identifier = Identifier::new("db", "t"); + ctx.catalog + .create_table(&identifier, append_only_schema(), false) + .await + .unwrap(); + let base = ctx.catalog.get_table(&identifier).await.unwrap(); + + // A branch schema on disk, so `copy_with_branch` and the server both see it. + let branch_schema = paimon::spec::TableSchema::new(0, &append_only_schema()); + let schema_path = base.schema_manager().with_branch("dev").schema_path(0); + let schema_dir = schema_path.rsplit_once('/').map(|(d, _)| d).unwrap(); + base.file_io().mkdirs(schema_dir).await.unwrap(); + base.file_io() + .new_output(&schema_path) + .unwrap() + .write(serde_json::to_vec(&branch_schema).unwrap().into()) + .await + .unwrap(); + + // The branch reports the base table's uuid, so an ordinary branch scan + // through the copied handle still plans. + base.copy_with_branch("dev") + .await + .unwrap() + .new_read_builder() + .new_scan() + .plan() + .await + .expect("an ordinary branch read must plan against the real server"); + + // A decorated name is answered by the server for the live check only; + // the catalog never builds a handle from one. + assert!(ctx + .catalog + .get_table(&Identifier::new("db", "t$branch_dev")) + .await + .is_err()); +} + +#[tokio::test] +async fn test_a_commit_addressed_to_a_branch_is_refused() { + let ctx = setup().await; + ctx.catalog + .create_database("db", true, HashMap::new()) + .await + .unwrap(); + let identifier = Identifier::new("db", "t"); + ctx.catalog + .create_table(&identifier, append_only_schema(), false) + .await + .unwrap(); + let base = ctx.catalog.get_table(&identifier).await.unwrap(); + let schema_path = base.schema_manager().with_branch("dev").schema_path(0); + let schema_dir = schema_path.rsplit_once('/').map(|(d, _)| d).unwrap(); + base.file_io().mkdirs(schema_dir).await.unwrap(); + base.file_io() + .new_output(&schema_path) + .unwrap() + .write( + serde_json::to_vec(&paimon::spec::TableSchema::new(0, &append_only_schema())) + .unwrap() + .into(), + ) + .await + .unwrap(); + + // Straight at the endpoint, past the client's own branch-write refusal: + // the server used to resolve the branch and then commit to main. + let snapshot = paimon::spec::Snapshot::builder() + .version(3) + .id(1) + .schema_id(0) + .base_manifest_list("manifest-list-0".to_string()) + .delta_manifest_list("manifest-list-1".to_string()) + .commit_user("e2e".to_string()) + .commit_identifier(1) + .commit_kind(paimon::spec::CommitKind::APPEND) + .time_millis(0) + .build(); + let outcome = base + .rest_env() + .unwrap() + .api() + .commit_snapshot( + &Identifier::new("db", "t$branch_dev"), + "db.t", + &snapshot, + &[], + ) + .await; + assert!( + outcome.is_err(), + "a commit addressed to a branch must be refused" + ); + assert!( + base.snapshot_manager() + .get_latest_snapshot_id() + .await + .unwrap() + .is_none(), + "and main must be untouched" + ); +} + +#[tokio::test] +async fn test_load_table_refuses_a_decorated_object_table() { + let ctx = setup().await; + ctx.catalog + .create_database("db", true, HashMap::new()) + .await + .unwrap(); + let identifier = Identifier::new("db", "objects"); + let schema = paimon::spec::Schema::builder() + .column( + "ignored", + paimon::spec::DataType::Int(paimon::spec::IntType::new()), + ) + .option("type", "object-table") + .build() + .unwrap(); + ctx.catalog + .create_table(&identifier, schema, false) + .await + .unwrap(); + // With a branch schema on disk the server resolves the name, so only the + // client's own refusal keeps `load_table`'s object-table early return from + // handing back the base relation. + let loaded = ctx.catalog.load_table(&identifier).await.unwrap(); + let paimon::catalog::LoadedTable::Object(object) = loaded else { + panic!("expected an object table"); + }; + let manager = + paimon::table::SchemaManager::new(object.file_io().clone(), object.location().to_string()) + .with_branch("dev"); + let schema_path = manager.schema_path(0); + let schema_dir = schema_path.rsplit_once('/').map(|(d, _)| d).unwrap(); + object.file_io().mkdirs(schema_dir).await.unwrap(); + let (_, stored) = paimon::catalog::FileSystemCatalog::new({ + let mut o = Options::new(); + o.set( + CatalogOptions::WAREHOUSE, + ctx._warehouse.path().to_str().unwrap(), + ); + o + }) + .unwrap() + .fetch_table_schema(&identifier) + .await + .unwrap(); + object + .file_io() + .new_output(&schema_path) + .unwrap() + .write(serde_json::to_vec(&stored).unwrap().into()) + .await + .unwrap(); + assert!(ctx + .catalog + .load_table(&Identifier::new("db", "objects$branch_dev")) + .await + .is_err()); +} diff --git a/crates/paimon/src/catalog/filesystem.rs b/crates/paimon/src/catalog/filesystem.rs index 743f00286..949f6c89a 100644 --- a/crates/paimon/src/catalog/filesystem.rs +++ b/crates/paimon/src/catalog/filesystem.rs @@ -22,7 +22,9 @@ use std::collections::HashMap; use crate::api::GetTagResponse; -use crate::catalog::{Catalog, Database, Identifier, DB_LOCATION_PROP, DB_SUFFIX}; +use crate::catalog::{ + Catalog, Database, Identifier, DB_LOCATION_PROP, DB_SUFFIX, DEFAULT_MAIN_BRANCH, +}; use crate::common::{CatalogOptions, Options}; use crate::error::{ConfigInvalidSnafu, Error, Result}; use crate::io::cache::{create_local_cache, LocalCache}; @@ -190,26 +192,42 @@ impl FileSystemCatalog { Ok(dirs) } - /// Fetch the stored path and schema of an existing table, bypassing the - /// engine-type guard in [`Self::build_table`]: routing and catalog servers - /// need the declared type before deciding anything. pub async fn fetch_table_schema( &self, identifier: &Identifier, ) -> Result<(String, TableSchema)> { identifier.validate()?; + // Every load goes through here, so a system-table suffix is refused once, + // before any type-specific early return could hand back the base table. + if let Some(system) = identifier.system_table_name()? { + return Err(Error::Unsupported { + message: format!( + "'{}' names the system table '{system}', which this catalog does not serve", + identifier.full_name() + ), + }); + } - let table_path = self.table_path(identifier); + // `db.t$branch_x` names the base table's branch, as Java resolves it: + // the path is the table's, the schema the branch's latest. + let base = Identifier::new(identifier.database(), &identifier.table_name()?); + let table_path = self.table_path(&base); - if !self.table_exists(identifier).await? { + if !self.table_exists(&base).await? { return Err(Error::TableNotExist { full_name: identifier.full_name(), }); } - let schema = self - .load_latest_table_schema(&table_path) + let manager = SchemaManager::new(self.file_io.clone(), table_path.clone()); + let manager = match identifier.branch_name()? { + Some(branch) if branch != DEFAULT_MAIN_BRANCH => manager.with_branch(&branch), + _ => manager, + }; + let schema = manager + .latest() .await? + .map(|arc| (*arc).clone()) .ok_or_else(|| Error::TableNotExist { full_name: identifier.full_name(), })?; @@ -372,11 +390,13 @@ impl Catalog for FileSystemCatalog { } async fn get_table(&self, identifier: &Identifier) -> Result { + identifier.reject_decorated()?; let (table_path, schema) = self.fetch_table_schema(identifier).await?; self.build_table(identifier, table_path, schema) } async fn load_table(&self, identifier: &Identifier) -> Result { + identifier.reject_decorated()?; let (table_path, schema) = self.fetch_table_schema(identifier).await?; let options = CoreOptions::new(schema.options()); let declared = options.table_type()?; @@ -433,6 +453,7 @@ impl Catalog for FileSystemCatalog { ignore_if_exists: bool, ) -> Result<()> { identifier.validate()?; + identifier.reject_decorated()?; // Never persist a type nothing can load. let declared = CoreOptions::new(creation.options()).table_type()?; @@ -467,6 +488,7 @@ impl Catalog for FileSystemCatalog { async fn drop_table(&self, identifier: &Identifier, ignore_if_not_exists: bool) -> Result<()> { identifier.validate()?; + identifier.reject_decorated()?; let table_path = self.table_path(identifier); @@ -494,6 +516,8 @@ impl Catalog for FileSystemCatalog { ) -> Result<()> { from.validate()?; to.validate()?; + from.reject_decorated()?; + to.reject_decorated()?; let from_path = self.table_path(from); let to_path = self.table_path(to); @@ -527,6 +551,7 @@ impl Catalog for FileSystemCatalog { ignore_if_not_exists: bool, ) -> Result<()> { identifier.validate()?; + identifier.reject_decorated()?; let table_path = self.table_path(identifier); if !self.table_exists(identifier).await? { @@ -1400,6 +1425,83 @@ mod tests { ); } + #[tokio::test] + async fn test_fetch_table_schema_resolves_a_branch_name() { + let (_temp_dir, catalog) = create_test_catalog(); + catalog + .create_database("db1", false, HashMap::new()) + .await + .unwrap(); + let base = Identifier::new("db1", "t"); + catalog + .create_table( + &base, + Schema::builder() + .column("id", DataType::Int(IntType::new())) + .build() + .unwrap(), + false, + ) + .await + .unwrap(); + let (table_path, _) = catalog.fetch_table_schema(&base).await.unwrap(); + + // A branch schema on disk, with one column more than the base. + let branch_schema = TableSchema::new( + 0, + &Schema::builder() + .column("id", DataType::Int(IntType::new())) + .column("extra", DataType::Int(IntType::new())) + .build() + .unwrap(), + ); + let manager = + SchemaManager::new(catalog.file_io.clone(), table_path.clone()).with_branch("dev"); + let schema_path = manager.schema_path(0); + let schema_dir = schema_path + .rsplit_once('/') + .map(|(d, _)| d.to_string()) + .unwrap(); + catalog.file_io.mkdirs(&schema_dir).await.unwrap(); + catalog + .file_io + .new_output(&schema_path) + .unwrap() + .write(serde_json::to_vec(&branch_schema).unwrap().into()) + .await + .unwrap(); + + let (path, schema) = catalog + .fetch_table_schema(&Identifier::new("db1", "t$branch_dev")) + .await + .expect("a branch name resolves to the base table's branch"); + assert_eq!(path, table_path, "the path is the table's"); + assert_eq!(schema.fields().len(), 2, "the schema is the branch's"); + + // `main` named explicitly resolves at the table root, not a branch dir. + let (main_path, main_schema) = catalog + .fetch_table_schema(&Identifier::new("db1", "t$branch_main")) + .await + .unwrap(); + assert_eq!(main_path, table_path); + assert_eq!( + main_schema.fields().len(), + 1, + "the base schema, not a branch's" + ); + + // Only the server's lookup resolves these; no handle is built from one. + for name in ["t$branch_dev", "t$branch_main", "t$files"] { + assert!( + catalog + .get_table(&Identifier::new("db1", name)) + .await + .is_err(), + "{name}" + ); + } + } + #[tokio::test] async fn test_create_table_rejects_an_unknown_type() { let (_temp_dir, catalog) = create_test_catalog(); @@ -1463,6 +1565,33 @@ mod tests { Some(&expected_path.to_string()) ); + // With a branch schema present, only `load_table`'s own refusal stops the + // object-table early return from handing back the base relation. + let branch_schema_path = + SchemaManager::new(catalog.file_io.clone(), expected_path.to_string()) + .with_branch("dev") + .schema_path(0); + let branch_dir = branch_schema_path.rsplit_once('/').map(|(d, _)| d).unwrap(); + catalog.file_io.mkdirs(branch_dir).await.unwrap(); + catalog + .file_io + .new_output(&branch_schema_path) + .unwrap() + .write(serde_json::to_vec(&stored).unwrap().into()) + .await + .unwrap(); + // The object-table early return must not hand back the base relation + // for a name with a system suffix. + for name in ["objects$does_not_exist", "objects$branch_dev"] { + assert!( + catalog + .load_table(&Identifier::new("db1", name)) + .await + .is_err(), + "{name}: a decorated object-table name is refused, not silently stripped" + ); + } + let loaded = catalog.load_table(&identifier).await.unwrap(); let LoadedTable::Object(table) = loaded else { panic!("expected a native object table, got {loaded:?}"); diff --git a/crates/paimon/src/catalog/mod.rs b/crates/paimon/src/catalog/mod.rs index 0c66f8bef..a990b0d0d 100644 --- a/crates/paimon/src/catalog/mod.rs +++ b/crates/paimon/src/catalog/mod.rs @@ -168,6 +168,21 @@ impl Identifier { pub fn system_table_name(&self) -> Result> { Ok(self.parsed_object_name()?.system_table) } + + /// A `$branch_x` or `$files` name addresses a view of the table rather than + /// the table: no handle is built from one, and no mutation acts on one. + pub(crate) fn reject_decorated(&self) -> Result<()> { + let parsed = self.parsed_object_name()?; + if parsed.branch.is_some() || parsed.system_table.is_some() { + return Err(Error::Unsupported { + message: format!( + "'{}' is a decorated name; load the table and use `copy_with_branch`", + self.full_name() + ), + }); + } + Ok(()) + } } /// Parse a Paimon object name into table, optional branch, and optional system table. diff --git a/crates/paimon/src/catalog/rest/rest_catalog.rs b/crates/paimon/src/catalog/rest/rest_catalog.rs index fb4f47de1..8888e2b66 100644 --- a/crates/paimon/src/catalog/rest/rest_catalog.rs +++ b/crates/paimon/src/catalog/rest/rest_catalog.rs @@ -235,6 +235,7 @@ impl Catalog for RESTCatalog { // ======================= table methods =============================== async fn get_table(&self, identifier: &Identifier) -> Result
{ + identifier.reject_decorated()?; RESTEnv::load_table( identifier, self.api.clone(), @@ -246,6 +247,9 @@ impl Catalog for RESTCatalog { } async fn load_table(&self, identifier: &Identifier) -> Result { + // Before type dispatch: the object- and external-table returns never + // reach `build_table`'s own refusal. + identifier.reject_decorated()?; let response = RESTEnv::fetch_table_response(identifier, &self.api).await?; if let Some(schema) = response.schema.as_ref() { let options = crate::spec::CoreOptions::new(schema.options()); diff --git a/crates/paimon/src/table/format_table_scan.rs b/crates/paimon/src/table/format_table_scan.rs index e53b8a365..6203a4c59 100644 --- a/crates/paimon/src/table/format_table_scan.rs +++ b/crates/paimon/src/table/format_table_scan.rs @@ -64,30 +64,22 @@ impl<'a> FormatTableScan<'a> { } pub(crate) async fn plan(&self) -> crate::Result { - self.ensure_query_auth_allowed().await?; + self.table + .ensure_read_authorized_live("a format table") + .await?; self.plan_inner(None).await } pub(crate) async fn plan_with_trace(&self) -> crate::Result<(Plan, ScanTrace)> { - self.ensure_query_auth_allowed().await?; + self.table + .ensure_read_authorized_live("a format table") + .await?; let mut trace = ScanTrace::default(); let plan = self.plan_inner(Some(&mut trace)).await?; trace.planned_data_file_bytes = plan.planned_data_file_bytes(); Ok((plan, trace)) } - /// Refused outright. Asks the server: the option can be set after a load. - async fn ensure_query_auth_allowed(&self) -> crate::Result<()> { - let core_options = CoreOptions::new(self.table.schema().options()); - core_options.ensure_type_paimon_served(&self.table.identifier().full_name())?; - if self.table.server_query_auth_enabled().await? { - return Err(super::query_auth::unsupported( - "a format table cannot apply a row filter or column masking", - )); - } - Ok(()) - } - async fn plan_inner(&self, trace: Option<&mut ScanTrace>) -> crate::Result { if self.row_ranges.is_some() { return Err(crate::Error::Unsupported { diff --git a/crates/paimon/src/table/incremental_scan.rs b/crates/paimon/src/table/incremental_scan.rs index e9ac52b81..9cdbcf8b7 100644 --- a/crates/paimon/src/table/incremental_scan.rs +++ b/crates/paimon/src/table/incremental_scan.rs @@ -256,13 +256,9 @@ impl<'a> IncrementalScan<'a> { } pub async fn plan(&self) -> crate::Result { - let core_options = crate::spec::CoreOptions::new(self.table.schema().options()); - core_options.ensure_type_paimon_served(&self.table.identifier().full_name())?; - if self.table.server_query_auth_enabled().await? { - return Err(super::query_auth::unsupported( - "an incremental read cannot apply a row filter or column masking", - )); - } + self.table + .ensure_read_authorized_live("an incremental read") + .await?; let mode = self.resolve_mode(); self.validate_snapshot_range(mode).await?; if self.start_exclusive == self.end_inclusive { diff --git a/crates/paimon/src/table/mod.rs b/crates/paimon/src/table/mod.rs index ea7a3de52..64b2edc42 100644 --- a/crates/paimon/src/table/mod.rs +++ b/crates/paimon/src/table/mod.rs @@ -359,13 +359,21 @@ impl Table { } /// The live counterpart of [`CoreOptions::ensure_read_authorized`], which - /// reads the schema this handle was loaded with. - pub(crate) async fn ensure_read_authorized_live(&self, path: &str) -> Result<()> { - let local = CoreOptions::new(self.schema.options()); - local.ensure_type_paimon_served(&self.identifier.full_name())?; + /// reads the schema this handle was loaded with. For a read that plans + /// nothing — DataFusion's system tables — since the option can be set + /// after a load. + pub async fn ensure_read_authorized(&self) -> Result<()> { + self.ensure_read_authorized_live("a read without a plan") + .await + } + + /// As [`Self::ensure_read_authorized`], naming the operation that asks. + pub(crate) async fn ensure_read_authorized_live(&self, operation: &str) -> Result<()> { + CoreOptions::new(self.schema.options()) + .ensure_type_paimon_served(&self.identifier.full_name())?; if self.server_query_auth_enabled().await? { return Err(query_auth::unsupported(&format!( - "{path} reads index files directly and cannot apply a row filter or column masking" + "{operation} cannot apply a row filter or column masking" ))); } Ok(()) @@ -378,34 +386,34 @@ impl Table { let Some(rest_env) = &self.rest_env else { return Ok(local); }; - // Only ever strengthens: the name can be re-created over this handle's - // files, so the answer may be about a different table. + // Only ever strengthens. if local { return Ok(true); } - match rest_env.current_table().await?.schema.as_ref() { - Some(schema) => Ok(CoreOptions::new(schema.options()).query_auth_enabled()), - None => Ok(true), - } + rest_env.query_auth_enabled_live(&self.branch).await + } + + /// Whether this handle reads a schema other than the one the server rules + /// on: a time-travel selector (`copy_with_options` adds one without the + /// flag), a travelled or branch view, or a `$branch_x` / `$files` name + /// whose managers read the base table's own files. + pub(crate) fn reads_another_schema(&self) -> Result { + let travels = CoreOptions::new(self.schema.options()) + .try_time_travel_selector()? + .is_some(); + let decorated = self.identifier.branch_name()?.is_some() + || self.identifier.system_table_name()?.is_some(); + Ok(travels || self.time_traveled || self.branch_reference || decorated) } /// Whether this user may read this table; `None` when it is not - /// `query-auth.enabled`. `server_query_auth` is the caller's, so planning - /// asks the server once. + /// `query-auth.enabled`. `server_query_auth` is the caller's own lookup. pub(crate) async fn authorize_read( &self, server_query_auth: bool, ) -> Result>> { let local = CoreOptions::new(self.schema.options()); - // Ask the selector too: `copy_with_options` adds one without the flag. - let travels = local.try_time_travel_selector()?.is_some(); - // A `$branch_x` or `$files` handle authorizes against the decorated - // name while its managers read the base table's own files. - let decorated = self.identifier.branch_name()?.is_some() - || self.identifier.system_table_name()?.is_some(); - if (travels || self.time_traveled || self.branch_reference || decorated) - && local.query_auth_enabled() - { + if self.reads_another_schema()? && local.query_auth_enabled() { return Err(query_auth::unsupported( "a time-travelled or branch read authorizes against the table's current schema, \ which is not the one it reads", @@ -427,7 +435,7 @@ impl Table { if !server_query_auth { return Ok(None); } - if travels || self.time_traveled || self.branch_reference || decorated { + if self.reads_another_schema()? { return Err(query_auth::unsupported( "a time-travelled or branch read authorizes against the table's current schema, \ which is not the one it reads", diff --git a/crates/paimon/src/table/query_auth.rs b/crates/paimon/src/table/query_auth.rs index 14fafa186..bdcfa34a2 100644 --- a/crates/paimon/src/table/query_auth.rs +++ b/crates/paimon/src/table/query_auth.rs @@ -40,11 +40,10 @@ impl QueryAuthGrant { self.response.is_unrestricted() } - /// Travelled and branch views read a schema the server did not rule on. - /// Everything else follows from the session, which only the catalog mints. + /// A view of another schema is not the one the server ruled on. Everything + /// else follows from the session, which only the catalog mints. pub(crate) fn matches_table(&self, table: &super::Table) -> bool { - !table.is_time_traveled() - && !table.is_branch_reference() + !table.reads_another_schema().unwrap_or(true) && table.query_auth_session() == Some(self.session) } } @@ -87,7 +86,7 @@ pub(crate) async fn reject_unauthorized_stats( !current.fields().iter().any(|c| { c.id() == f.id() && c.name() == f.name() - && shape(c.data_type()) == shape(f.data_type()) + && contains(c.data_type(), f.data_type()) }) }) { return refuse(gone.name()); @@ -97,33 +96,28 @@ pub(crate) async fn reject_unauthorized_stats( Ok(()) } -/// The physical shape, descriptions stripped: `DataField` equality includes them, -/// so a comment-only edit would otherwise read as an unauthorized column. -fn shape(ty: &crate::spec::DataType) -> crate::spec::DataType { - use crate::spec::{ArrayType, DataType, MapType, MultisetType, RowType}; - match ty { - DataType::Row(row) => DataType::Row(RowType::new( - row.fields() +/// Whether `narrow` reads nothing `wide` does not have: nested children are +/// matched by name and must be contained in turn, so a projection of a `ROW` +/// passes and an extra child does not. Descriptions are not columns and are +/// ignored. +fn contains(wide: &crate::spec::DataType, narrow: &crate::spec::DataType) -> bool { + use crate::spec::DataType; + match (wide, narrow) { + (DataType::Row(w), DataType::Row(n)) => n.fields().iter().all(|nf| { + w.fields() .iter() - .map(|f| { - crate::spec::DataField::new(f.id(), f.name().to_string(), shape(f.data_type())) - }) - .collect(), - )), - DataType::Array(a) => DataType::Array(ArrayType::with_nullable( - ty.is_nullable(), - shape(a.element_type()), - )), - DataType::Multiset(m) => DataType::Multiset(MultisetType::with_nullable( - ty.is_nullable(), - shape(m.element_type()), - )), - DataType::Map(m) => DataType::Map(MapType::with_nullable( - ty.is_nullable(), - shape(m.key_type()), - shape(m.value_type()), - )), - other => other.clone(), + .any(|wf| wf.name() == nf.name() && contains(wf.data_type(), nf.data_type())) + }), + (DataType::Array(w), DataType::Array(n)) => contains(w.element_type(), n.element_type()), + (DataType::Multiset(w), DataType::Multiset(n)) => { + contains(w.element_type(), n.element_type()) + } + (DataType::Map(w), DataType::Map(n)) => { + contains(w.key_type(), n.key_type()) && contains(w.value_type(), n.value_type()) + } + // A `variant_get` pushdown reads a `VARIANT` column as a `ROW` of paths. + (DataType::Variant(_), DataType::Row(_)) => true, + (w, n) => w == n, } } @@ -167,7 +161,7 @@ pub(crate) fn reject_noncanonical_fields( let canonical = schema_fields.iter().any(|f| { f.id() == field.id() && f.name() == field.name() - && shape(f.data_type()) == shape(field.data_type()) + && contains(f.data_type(), field.data_type()) }); if !canonical { return Err(unsupported(&format!( @@ -239,6 +233,14 @@ mod tests { !grant.matches_table(&travelled), "an older schema is not the one the server ruled on" ); + let selected = table.copy_with_options(std::collections::HashMap::from([( + "scan.snapshot-id".to_string(), + "1".to_string(), + )])); + assert!( + !grant.matches_table(&selected), + "a selector travels without setting the flag" + ); let assembled = crate::table::Table::new( table.file_io().clone(), @@ -392,26 +394,53 @@ mod tests { } #[test] - fn test_a_comment_only_change_is_not_a_different_column() { + fn test_containment_ignores_comments_and_allows_narrowing() { use crate::spec::{DataField, DataType, IntType, RowType}; - let child = |desc: Option<&str>| { - let f = DataField::new(1, "a".to_string(), DataType::Int(IntType::new())); + let int = || DataType::Int(IntType::new()); + let child = |name: &str, desc: Option<&str>| { + let f = DataField::new(1, name.to_string(), int()); match desc { Some(d) => f.with_description(Some(d.to_string())), None => f, } }; - let row = |desc| DataType::Row(RowType::new(vec![child(desc)])); - assert_ne!( - row(None), - row(Some("why")), - "equality includes descriptions" - ); - assert_eq!( - super::shape(&row(None)), - super::shape(&row(Some("why"))), - "but a comment is not a column the server did not authorize" - ); + let row = |children: Vec| DataType::Row(RowType::new(children)); + let wide = row(vec![child("a", None), child("b", None)]); + + // A comment is not a column. + assert!(super::contains( + &wide, + &row(vec![child("a", Some("why")), child("b", None)]) + )); + // Projecting a subset of the children reads nothing extra. + assert!(super::contains(&wide, &row(vec![child("a", None)]))); + // An extra child would. + assert!(!super::contains( + &wide, + &row(vec![ + child("a", None), + child("b", None), + child("hidden", None) + ]) + )); + // And so would a child under another name. + assert!(!super::contains(&wide, &row(vec![child("c", None)]))); + } + + #[test] + fn test_a_variant_extraction_is_the_one_shape_change_allowed() { + use crate::spec::{DataField, DataType, IntType, RowType, VariantType}; + let int = || DataType::Int(IntType::new()); + let row = || DataType::Row(RowType::new(vec![DataField::new(0, "p".into(), int())])); + let schema = vec![ + DataField::new(1, "v".into(), DataType::Variant(VariantType::new())), + DataField::new(2, "n".into(), int()), + ]; + let read = |id, name: &str, ty| vec![DataField::new(id, name.into(), ty)]; + + assert!(super::reject_noncanonical_fields(&read(1, "v", row()), &schema).is_ok()); + assert!(super::reject_noncanonical_fields(&read(1, "v", int()), &schema).is_err()); + assert!(super::reject_noncanonical_fields(&read(2, "n", row()), &schema).is_err()); } #[test] diff --git a/crates/paimon/src/table/rest_env.rs b/crates/paimon/src/table/rest_env.rs index ef83530b1..355cd979f 100644 --- a/crates/paimon/src/table/rest_env.rs +++ b/crates/paimon/src/table/rest_env.rs @@ -93,67 +93,97 @@ impl RESTEnv { self.current_table_checked(schema_id).await?; let response = self .api - .auth_table_query(&self.branch_identifier(branch), select) + .auth_table_query(&self.branch_identifier(branch)?, select) .await?; self.current_table_checked(schema_id).await?; Ok(response) } - /// Asserts nothing about identity: an ordinary table must not inherit a - /// freshness restriction. - pub(crate) async fn current_table(&self) -> Result { - self.api.get_table(&self.identifier).await + /// Asked of the branch this handle reads. A `false` is trusted only from the + /// uuid this handle was loaded with — a replacement's says nothing about + /// these files. + pub(crate) async fn query_auth_enabled_live(&self, branch: &str) -> Result { + let identifier = self.branch_identifier(branch)?; + let response = self.api.get_table(&identifier).await?; + let Some(schema) = response.schema.as_ref() else { + return Ok(true); + }; + if crate::spec::CoreOptions::new(schema.options()).query_auth_enabled() { + return Ok(true); + } + // A branch answers for its own schema only. Whether the server reports + // the base table's id for `t$branch_x` is its own business, so the + // identity check below is for the name this handle was loaded with. + if identifier != self.identifier { + return Ok(false); + } + match response.id.as_deref() { + Some(uuid) if uuid == self.uuid => Ok(false), + Some(uuid) => Err(crate::Error::DataInvalid { + message: format!( + "table '{}' now resolves to uuid {uuid}, not the {} this handle was loaded \ + with; re-load the table before reading it", + identifier.full_name(), + self.uuid + ), + source: None, + }), + None => Ok(true), + } } /// Refused unless the name still resolves to the loaded table — a missing - /// identity too, which checks nothing. + /// identity too, which checks nothing. Asserts nothing on its own: an + /// ordinary table must not inherit a freshness restriction. pub(crate) async fn current_table_checked(&self, schema_id: i64) -> Result { - let response = self.current_table().await?; + let response = self.api.get_table(&self.identifier).await?; let name = self.identifier.full_name(); - let drifted = |what: &str, from: String, to: String| crate::Error::DataInvalid { - message: format!( - "table '{name}' now resolves to {what} {to}, not the {from} this handle was \ - loaded with; re-load the table before reading it" - ), - source: None, + let same = |what: &str, loaded: String, now: Option| match now { + Some(now) if now == loaded => Ok(()), + now => Err(crate::Error::DataInvalid { + message: format!( + "table '{name}' now resolves to {what} {}, not the {loaded} this handle was \ + loaded with; re-load the table before reading it", + now.as_deref().unwrap_or("nothing the server reports") + ), + source: None, + }), }; - match response.id.as_deref() { - Some(uuid) if uuid == self.uuid => {} - Some(uuid) => return Err(drifted("uuid", self.uuid.clone(), uuid.to_string())), - None => { - return Err(drifted( - "uuid", - self.uuid.clone(), - "nothing the server reports".to_string(), - )) - } - } - match response.schema_id { - Some(id) if id == schema_id => Ok(response), - Some(id) => Err(drifted("schema", schema_id.to_string(), id.to_string())), - None => Err(drifted( - "schema", - schema_id.to_string(), - "nothing the server reports".to_string(), - )), - } + same("uuid", self.uuid.clone(), response.id.clone())?; + same( + "schema", + schema_id.to_string(), + response.schema_id.map(|id| id.to_string()), + )?; + Ok(response) } /// `db.table$branch_`, as Java names a branch. Only the auth call uses it. - fn branch_identifier(&self, branch: &str) -> Identifier { + /// Built from the base table name: a handle loaded as `db.t$branch_x` + /// already carries the decoration, and must not double it. + fn branch_identifier(&self, branch: &str) -> Result { + // The object-name encoding cannot carry a `$`: `t$branch_a$b` parses as + // branch `a` plus system table `b`, for Java clients as much as here. + if branch.contains(crate::catalog::SYSTEM_TABLE_SPLITTER) { + return Err(Error::Unsupported { + message: format!( + "branch '{branch}' cannot be addressed over REST: its name contains '{}'", + crate::catalog::SYSTEM_TABLE_SPLITTER + ), + }); + } + let base = self.identifier.table_name()?; if branch == crate::catalog::DEFAULT_MAIN_BRANCH { - return self.identifier.clone(); + return Ok(Identifier::new(self.identifier.database(), base)); } - Identifier::new( + Ok(Identifier::new( self.identifier.database(), format!( - "{}{}{}{}", - self.identifier.object(), + "{base}{}{}{branch}", crate::catalog::SYSTEM_TABLE_SPLITTER, - crate::catalog::SYSTEM_BRANCH_PREFIX, - branch + crate::catalog::SYSTEM_BRANCH_PREFIX ), - ) + )) } /// Get the table identifier. @@ -203,8 +233,6 @@ impl RESTEnv { .map_err(|e| map_rest_error_for_table(e, identifier)) } - /// Build a Table from an already-fetched response, so routing can - /// inspect the declared type first. pub(crate) async fn build_table( identifier: &Identifier, response: crate::api::GetTableResponse, @@ -213,6 +241,7 @@ impl RESTEnv { data_token_enabled: bool, local_cache: Option>, ) -> Result
{ + identifier.reject_decorated()?; let schema = response.schema.ok_or_else(|| Error::DataInvalid { message: format!("Table {} response missing schema", identifier.full_name()), source: None, @@ -477,4 +506,39 @@ mod tests { assert!(rest_env.has_local_cache()); assert!(rest_env.clone().has_local_cache()); } + + #[tokio::test] + async fn test_branch_identifier_is_built_from_the_base_name() { + let mut options = Options::new(); + options.set(CatalogOptions::URI, "http://localhost:1"); + options.set(CatalogOptions::TOKEN_PROVIDER, "bear"); + options.set(CatalogOptions::TOKEN, "test-token"); + let api = Arc::new(RESTApi::new(options.clone(), false).await.unwrap()); + let env = |object: &str| { + RESTEnv::new( + Identifier::new("db", object), + "uuid".to_string(), + api.clone(), + options.clone(), + false, + None, + ) + }; + // Loaded as the branch itself: must not become `t$branch_dev$branch_dev`. + let decorated = env("t$branch_dev").branch_identifier("dev").unwrap(); + assert_eq!(decorated.object(), "t$branch_dev"); + assert_eq!( + env("t").branch_identifier("dev").unwrap().object(), + "t$branch_dev" + ); + assert_eq!( + env("t$branch_dev") + .branch_identifier("main") + .unwrap() + .object(), + "t" + ); + // The encoding has no room for a `$` inside the branch name. + assert!(env("t").branch_identifier("release$one").is_err()); + } } diff --git a/crates/paimon/src/table/table_commit.rs b/crates/paimon/src/table/table_commit.rs index 8a3735d6c..421a88505 100644 --- a/crates/paimon/src/table/table_commit.rs +++ b/crates/paimon/src/table/table_commit.rs @@ -205,7 +205,12 @@ impl TableCommit { filter_committed: bool, ) -> Result<()> { // A commit validates against the existing snapshot. - self.table.ensure_read_authorized_live("a commit").await?; + // Refused before anything is submitted, so the prepared files — index + // shards included — are safe to remove rather than leave orphaned. + if let Err(error) = self.table.ensure_read_authorized_live("a commit").await { + let _ = self.abort(&commit_messages).await; + return Err(error); + } self.table.ensure_not_branch_reference_for_write()?; validate_fixed_bucket_commit_mode(&commit_messages, false)?; validate_bucket_ownership(&commit_messages)?; @@ -337,7 +342,12 @@ impl TableCommit { filter_committed: bool, ) -> Result<()> { // A commit validates against the existing snapshot. - self.table.ensure_read_authorized_live("a commit").await?; + // Refused before anything is submitted, so the prepared files — index + // shards included — are safe to remove rather than leave orphaned. + if let Err(error) = self.table.ensure_read_authorized_live("a commit").await { + let _ = self.abort(&commit_messages).await; + return Err(error); + } self.table.ensure_not_branch_reference_for_write()?; validate_fixed_bucket_commit_mode(&commit_messages, true)?; validate_bucket_ownership(&commit_messages)?; @@ -5528,6 +5538,47 @@ mod tests { ); } + #[tokio::test] + async fn test_a_refused_commit_removes_the_prepared_index_files() { + let file_io = test_file_io(); + let table_path = "memory:/test_refused_commit_index_cleanup"; + setup_dirs(&file_io, table_path).await; + let table = test_table_with_options( + &file_io, + table_path, + HashMap::from([("query-auth.enabled".to_string(), "true".to_string())]), + ); + let commit = TableCommit::new(table, "test-user".to_string()); + + let index_path = format!("{table_path}/index/bucket-index"); + file_io + .mkdirs(&format!("{table_path}/index/")) + .await + .unwrap(); + file_io + .new_output(&index_path) + .unwrap() + .write(bytes::Bytes::from_static(b"index")) + .await + .unwrap(); + let mut message = CommitMessage::new(vec![], 0, vec![]); + message.new_index_files = vec![IndexFileMeta { + index_type: "HASH".to_string(), + file_name: "bucket-index".to_string(), + file_size: 5, + row_count: 1, + deletion_vectors_ranges: None, + external_path: None, + global_index_meta: None, + }]; + + assert!(commit.commit(vec![message]).await.is_err()); + assert!( + !file_io.exists(&index_path).await.unwrap(), + "a commit refused before submission must not leave its index files behind" + ); + } + #[tokio::test] async fn test_abort_deletes_index_files_from_the_data_file_directory() { // With `index-file-in-data-file-dir`, a new index file is written beside the diff --git a/crates/paimon/src/table/table_write.rs b/crates/paimon/src/table/table_write.rs index 1a0c39938..9a4afaaa4 100644 --- a/crates/paimon/src/table/table_write.rs +++ b/crates/paimon/src/table/table_write.rs @@ -148,6 +148,9 @@ pub struct TableWrite { has_dedicated_vector_fields: bool, row_kind_generator: Option, row_kind_filter: Option, + /// The first write or commit asks the server; `new` is sync and can only + /// read the schema cached on the handle. + live_checked: bool, } impl TableWrite { @@ -408,6 +411,7 @@ impl TableWrite { has_dedicated_vector_fields, row_kind_generator, row_kind_filter, + live_checked: false, }) } @@ -483,8 +487,19 @@ impl TableWrite { self } + /// Before the first lazy read: a PK write scans the latest snapshot, and a + /// dynamic-bucket write loads the hash index. + async fn ensure_live_authorized(&mut self) -> Result<()> { + if !self.live_checked { + self.table.ensure_read_authorized_live("a write").await?; + self.live_checked = true; + } + Ok(()) + } + /// Write an Arrow RecordBatch. Rows are routed to the correct partition and bucket. pub async fn write_arrow_batch(&mut self, batch: &RecordBatch) -> Result<()> { + self.ensure_live_authorized().await?; let Some(batch) = self.normalize_write_batch(batch)? else { return Ok(()); }; @@ -820,6 +835,7 @@ impl TableWrite { /// Close all writers and collect CommitMessages for use with TableCommit. /// Writers are cleared after this call, allowing the TableWrite to be reused. pub async fn prepare_commit(&mut self) -> Result> { + self.ensure_live_authorized().await?; let writers: Vec<(PartitionBucketKey, FileWriter)> = self.partition_writers.drain().collect(); diff --git a/crates/paimon/tests/mock_server.rs b/crates/paimon/tests/mock_server.rs index 781ea12c9..72ca0feee 100644 --- a/crates/paimon/tests/mock_server.rs +++ b/crates/paimon/tests/mock_server.rs @@ -1696,10 +1696,13 @@ impl RESTServer { }); let key = format!("{database}.{table}"); + // A `t$branch_x` registration reports an id of its own: whether a real + // server shares the base table's is not something the client may assume. + let uuid = table.to_string(); s.tables.insert( key, GetTableResponse::new( - Some(table.to_string()), + Some(uuid), Some(table.to_string()), Some(path.to_string()), Some(true), diff --git a/crates/paimon/tests/rest_catalog_test.rs b/crates/paimon/tests/rest_catalog_test.rs index 6aced8b15..4e9dd9964 100644 --- a/crates/paimon/tests/rest_catalog_test.rs +++ b/crates/paimon/tests/rest_catalog_test.rs @@ -2757,33 +2757,6 @@ async fn test_query_auth_user_granted_all_business_columns_can_read() { .expect("a user granted every column must be authorized"); } -#[tokio::test] -async fn test_query_auth_enabled_after_a_handle_was_loaded_is_still_enforced() { - let ctx = setup_catalog(vec!["default"]).await; - let tmp = tempfile::tempdir().unwrap(); - let path = format!("file://{}", tmp.path().display()); - ctx.server - .add_table_with_schema("default", "later", schema_of(&["id"], &[]), &path); - let table = ctx - .catalog - .get_table(&Identifier::new("default", "later")) - .await - .unwrap(); - - ctx.server - .set_table_schema_id("default", "later", schema_of(&["id"], GUARDED), 0); - ctx.server - .set_auth_response("default", "later", restricted()); - - assert_refused( - plan_err( - &table, - "a handle loaded before the option was set must still be authorized", - ) - .await, - ); -} - #[tokio::test] async fn test_query_auth_is_not_weakened_by_a_table_recreated_under_the_same_name() { let g = guarded("guarded", &["id"]).await; @@ -2803,27 +2776,26 @@ async fn test_query_auth_is_not_weakened_by_a_table_recreated_under_the_same_nam } #[tokio::test] -async fn test_query_auth_refuses_a_read_type_with_an_extra_nested_field() { +async fn test_query_auth_allows_a_nested_projection_but_not_an_extra_nested_field() { let ctx = setup_catalog(vec!["default"]).await; let tmp = tempfile::tempdir().unwrap(); let path = format!("file://{}", tmp.path().display()); - let nested = |extra: bool| { - let mut children = vec![paimon::spec::DataField::new( - 1, - "a".to_string(), - DataType::Int(IntType::new()), - )]; - if extra { - children.push(paimon::spec::DataField::new( - 2, - "hidden".to_string(), - DataType::Int(IntType::new()), - )); - } + let nested = |names: &[&str]| { + let children = names + .iter() + .enumerate() + .map(|(i, name)| { + paimon::spec::DataField::new( + i as i32 + 1, + name.to_string(), + DataType::Int(IntType::new()), + ) + }) + .collect(); DataType::Row(paimon::spec::RowType::new(children)) }; let served = Schema::builder() - .column("info", nested(false)) + .column("info", nested(&["a", "b"])) .option("query-auth.enabled", "true") .build() .unwrap(); @@ -2836,16 +2808,18 @@ async fn test_query_auth_refuses_a_read_type_with_an_extra_nested_field() { .await .unwrap(); let plan = table.new_read_builder().new_scan().plan().await.unwrap(); + let read_with = |info: DataType| { + let field = + paimon::spec::DataField::new(table.schema().fields()[0].id(), "info".to_string(), info); + let mut builder = table.new_read_builder(); + builder.with_read_type(vec![field]); + builder.new_read().unwrap().to_arrow(plan.splits()) + }; - // Same field id and name as the authorized column, one nested child more. - let forged = paimon::spec::DataField::new( - table.schema().fields()[0].id(), - "info".to_string(), - nested(true), - ); - let mut builder = table.new_read_builder(); - builder.with_read_type(vec![forged]); - let Err(err) = builder.new_read().unwrap().to_arrow(plan.splits()) else { + // Reading a subset of the authorized children is a projection. + assert!(read_with(nested(&["a"])).is_ok()); + // Reading one the server never ruled on is not. + let Err(err) = read_with(nested(&["a", "b", "hidden"])) else { panic!("a nested child the server never ruled on must be refused") }; assert_refused(err); @@ -2859,108 +2833,163 @@ async fn test_query_auth_refuses_a_decorated_handle() { for name in ["guarded$branch_dev", "guarded$files"] { ctx.server .add_table_with_schema("default", name, schema_of(&["id"], GUARDED), &path); - let table = ctx - .catalog - .get_table(&Identifier::new("default", name)) - .await - .unwrap(); - assert_refused( - plan_err( - &table, - "the decorated endpoint rules on files this handle does not read", - ) - .await, + } + // No handle is built from a decorated name; the branch is reached through + // `copy_with_branch`, and the live check asks the server about it there. + for name in ["guarded$branch_dev", "guarded$files"] { + assert!( + ctx.catalog + .get_table(&Identifier::new("default", name)) + .await + .is_err(), + "{name}" ); } } #[tokio::test] -async fn test_query_auth_enabled_after_a_load_still_refuses_metadata_and_writes() { +async fn test_a_disabled_answer_from_a_replacement_table_is_not_trusted() { let ctx = setup_catalog(vec!["default"]).await; let tmp = tempfile::tempdir().unwrap(); let path = format!("file://{}", tmp.path().display()); ctx.server - .add_table_with_schema("default", "meta", schema_of(&["id"], &[]), &path); + .add_table_with_schema("default", "replaced", schema_of(&["id"], &[]), &path); let table = ctx .catalog - .get_table(&Identifier::new("default", "meta")) + .get_table(&Identifier::new("default", "replaced")) .await .unwrap(); + + // A gets restricted auth, then the name is re-created as B with auth off: + // B's `false` says nothing about A's files this handle still points at. ctx.server - .set_table_schema_id("default", "meta", schema_of(&["id"], GUARDED), 0); + .set_auth_response("default", "replaced", restricted()); ctx.server - .set_auth_response("default", "meta", restricted()); + .set_table_uuid("default", "replaced", "uuid-of-b"); - assert_refused( - table - .partition_stats() - .await - .expect_err("partition stats expose partition values, row counts and sizes"), - ); - assert_refused( + assert_drifted( table - .new_global_index_drop_builder() + .new_vector_search_builder() .execute() .await - .expect_err("dropping an index is not something a restricted user may do"), + .expect_err("a false from another uuid must not authorize this handle"), + "now resolves to uuid", ); } #[tokio::test] -async fn test_query_auth_enabled_after_a_load_still_refuses_searches() { +async fn test_an_ordinary_branch_read_still_plans() { let ctx = setup_catalog(vec!["default"]).await; let tmp = tempfile::tempdir().unwrap(); let path = format!("file://{}", tmp.path().display()); ctx.server - .add_table_with_schema("default", "searched", schema_of(&["id"], &[]), &path); - let table = ctx + .add_table_with_schema("default", "plainbr", schema_of(&["id"], &[]), &path); + ctx.server.add_table_with_schema( + "default", + "plainbr$branch_dev", + schema_of(&["id"], &[]), + &path, + ); + let base = ctx .catalog - .get_table(&Identifier::new("default", "searched")) + .get_table(&Identifier::new("default", "plainbr")) + .await + .unwrap(); + let branch_schema = paimon::spec::TableSchema::new(0, &schema_of(&["id"], &[])); + base.file_io() + .new_output(&base.schema_manager().with_branch("dev").schema_path(0)) + .unwrap() + .write(serde_json::to_vec(&branch_schema).unwrap().into()) .await .unwrap(); + + base.copy_with_branch("dev") + .await + .unwrap() + .new_read_builder() + .new_scan() + .plan() + .await + .expect("asking the branch must not break an ordinary branch read"); +} + +#[tokio::test] +async fn test_query_auth_enabled_on_a_branch_is_seen_by_a_branch_handle() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + // The base table stays ordinary; only the branch gets restricted auth. ctx.server - .set_table_schema_id("default", "searched", schema_of(&["id"], GUARDED), 0); + .add_table_with_schema("default", "br", schema_of(&["id"], &[]), &path); + ctx.server.add_table_with_schema( + "default", + "br$branch_dev", + schema_of(&["id"], GUARDED), + &path, + ); ctx.server - .set_auth_response("default", "searched", restricted()); + .set_auth_response("default", "br$branch_dev", restricted()); + + let base = ctx + .catalog + .get_table(&Identifier::new("default", "br")) + .await + .unwrap(); + // The branch schema on disk predates the option, so the branch handle + // caches `false` too. + let branch_schema = paimon::spec::TableSchema::new(0, &schema_of(&["id"], &[])); + base.file_io() + .new_output(&base.schema_manager().with_branch("dev").schema_path(0)) + .unwrap() + .write(serde_json::to_vec(&branch_schema).unwrap().into()) + .await + .unwrap(); + let branch = base.copy_with_branch("dev").await.unwrap(); assert_refused( - table - .new_vector_search_builder() - .execute() - .await - .expect_err("a vector search reads index files directly"), - ); - #[cfg(feature = "fulltext")] - assert_refused( - table - .new_full_text_search_builder() - .execute() - .await - .expect_err("a full-text search reads index files directly"), - ); - assert_refused( - table - .new_hybrid_search_builder() - .execute() - .await - .expect_err("a hybrid search reads index files directly"), - ); - assert_refused( - table - .new_batch_vector_search_builder() - .execute() - .await - .expect_err("the batch path is reachable without the outer builder"), - ); - assert_refused( - table - .new_lumina_index_build_builder() - .execute() + branch + .new_read_builder() + .new_scan() + .plan() .await - .expect_err("building an index scans the table's rows"), + .expect_err("the live state must be the branch's, not the base table's"), ); } +#[tokio::test] +async fn test_a_branch_reporting_its_own_uuid_still_reads() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); + // Neither is query-auth. The server answers `t$branch_dev` with an id of + // its own, which a client must not read as "the table was replaced". + ctx.server + .add_table_with_schema("default", "own", schema_of(&["id"], &[]), &path); + ctx.server + .add_table_with_schema("default", "own$branch_dev", schema_of(&["id"], &[]), &path); + + let base = ctx + .catalog + .get_table(&Identifier::new("default", "own")) + .await + .unwrap(); + let branch_schema = paimon::spec::TableSchema::new(0, &schema_of(&["id"], &[])); + base.file_io() + .new_output(&base.schema_manager().with_branch("dev").schema_path(0)) + .unwrap() + .write(serde_json::to_vec(&branch_schema).unwrap().into()) + .await + .unwrap(); + let branch = base.copy_with_branch("dev").await.unwrap(); + + branch + .new_read_builder() + .new_scan() + .plan() + .await + .expect("a branch id of the server's own choosing is not a replaced table"); +} + #[tokio::test] async fn test_a_search_entry_asks_the_server_once() { let ctx = setup_catalog(vec!["default"]).await; @@ -3035,29 +3064,120 @@ async fn test_planning_an_ordinary_rest_table_asks_the_server_once() { } #[tokio::test] -async fn test_query_auth_refuses_scan_all_files_and_format_tables() { +async fn test_query_auth_enabled_after_a_load_is_seen_by_every_entry() { let ctx = setup_catalog(vec!["default"]).await; let tmp = tempfile::tempdir().unwrap(); let path = format!("file://{}", tmp.path().display()); ctx.server - .add_table_with_schema("default", "metadata", schema_of(&["id"], &[]), &path); + .add_table_with_schema("default", "later", schema_of(&["id"], &[]), &path); let table = ctx .catalog - .get_table(&Identifier::new("default", "metadata")) + .get_table(&Identifier::new("default", "later")) .await .unwrap(); + // Built before the flip: `new_write` is sync and sees only the cached schema. + let mut writer = table.new_write_builder().new_write().unwrap(); + + ctx.server + .set_table_schema_id("default", "later", schema_of(&["id"], GUARDED), 0); ctx.server - .set_table_schema_id("default", "metadata", schema_of(&["id"], GUARDED), 0); + .set_auth_response("default", "later", restricted()); - let err = table - .new_read_builder() - .new_scan() - .with_scan_all_files() - .plan() - .await - .expect_err("file metadata is not something the auth endpoint can rule on"); - assert_refused(err); + assert_refused(plan_err(&table, "a scan must ask the server, not the cached flag").await); + assert_refused( + table + .ensure_read_authorized() + .await + .expect_err("a read without a plan must ask the server too"), + ); + assert_refused( + table + .new_read_builder() + .new_scan() + .with_scan_all_files() + .plan() + .await + .expect_err("file metadata is not something the auth endpoint can rule on"), + ); + assert_refused( + table + .new_read_builder() + .new_incremental_scan(paimon::table::IncrementalScanMode::Delta, 0, 1) + .plan() + .await + .expect_err("an incremental read cannot apply the server's rules"), + ); + assert_refused( + table + .new_vector_search_builder() + .execute() + .await + .expect_err("a vector search reads index files directly"), + ); + #[cfg(feature = "fulltext")] + assert_refused( + table + .new_full_text_search_builder() + .execute() + .await + .expect_err("a full-text search reads index files directly"), + ); + assert_refused( + table + .new_hybrid_search_builder() + .execute() + .await + .expect_err("a hybrid search reads index files directly"), + ); + assert_refused( + table + .new_batch_vector_search_builder() + .execute() + .await + .expect_err("the batch path is reachable without the outer builder"), + ); + assert_refused( + table + .new_lumina_index_build_builder() + .execute() + .await + .expect_err("building an index scans the table's rows"), + ); + assert_refused( + table + .partition_stats() + .await + .expect_err("partition stats expose partition values, row counts and sizes"), + ); + assert_refused( + table + .new_global_index_drop_builder() + .execute() + .await + .expect_err("dropping an index is not something a restricted user may do"), + ); + let batch = RecordBatch::try_new( + Arc::new(ArrowSchema::new(vec![ArrowField::new( + "id", + ArrowDataType::Int32, + false, + )])), + vec![Arc::new(Int32Array::from(vec![1]))], + ) + .unwrap(); + assert_refused( + writer + .write_arrow_batch(&batch) + .await + .expect_err("the first write scans the snapshot before any commit"), + ); +} +#[tokio::test] +async fn test_query_auth_enabled_after_a_load_is_seen_by_a_format_table() { + let ctx = setup_catalog(vec!["default"]).await; + let tmp = tempfile::tempdir().unwrap(); + let path = format!("file://{}", tmp.path().display()); let format = &[("type", "format-table"), ("file.format", "parquet")]; ctx.server .add_table_with_schema("default", "fmt", schema_of(&["id"], format), &path); @@ -3072,15 +3192,6 @@ async fn test_query_auth_refuses_scan_all_files_and_format_tables() { .set_table_schema_id("default", "fmt", schema_of(&["id"], &guarded_format), 0); assert_refused(plan_err(&fmt, "a format table cannot apply the server's rules").await); - - assert_refused( - table - .new_read_builder() - .new_incremental_scan(paimon::table::IncrementalScanMode::Delta, 0, 1) - .plan() - .await - .expect_err("an incremental read cannot apply the server's rules"), - ); } #[tokio::test] From a437d69c0b448e11a38f67a9d77dbd07f2c789f4 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Sat, 12 Sep 2026 03:53:06 -0400 Subject: [PATCH 5/7] test(auth): gate the file:// branch-schema tests off Windows --- crates/paimon/tests/rest_catalog_test.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/paimon/tests/rest_catalog_test.rs b/crates/paimon/tests/rest_catalog_test.rs index 4e9dd9964..260322e79 100644 --- a/crates/paimon/tests/rest_catalog_test.rs +++ b/crates/paimon/tests/rest_catalog_test.rs @@ -2877,6 +2877,9 @@ async fn test_a_disabled_answer_from_a_replacement_table_is_not_trusted() { ); } +// Writes a branch schema under the `file://` tempdir, which `FileIO` cannot +// derive on Windows (see #397). +#[cfg(not(windows))] #[tokio::test] async fn test_an_ordinary_branch_read_still_plans() { let ctx = setup_catalog(vec!["default"]).await; @@ -2913,6 +2916,9 @@ async fn test_an_ordinary_branch_read_still_plans() { .expect("asking the branch must not break an ordinary branch read"); } +// Writes a branch schema under the `file://` tempdir, which `FileIO` cannot +// derive on Windows (see #397). +#[cfg(not(windows))] #[tokio::test] async fn test_query_auth_enabled_on_a_branch_is_seen_by_a_branch_handle() { let ctx = setup_catalog(vec!["default"]).await; @@ -2956,6 +2962,9 @@ async fn test_query_auth_enabled_on_a_branch_is_seen_by_a_branch_handle() { ); } +// Writes a branch schema under the `file://` tempdir, which `FileIO` cannot +// derive on Windows (see #397). +#[cfg(not(windows))] #[tokio::test] async fn test_a_branch_reporting_its_own_uuid_still_reads() { let ctx = setup_catalog(vec!["default"]).await; From aa137a4c4960671182079836010831d905e076e8 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Mon, 14 Sep 2026 09:26:15 -0400 Subject: [PATCH 6/7] feat(auth): refuse engine-planned vector splits that carry the query-auth marker --- crates/paimon/src/table/pk_vector_scan.rs | 29 +++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/crates/paimon/src/table/pk_vector_scan.rs b/crates/paimon/src/table/pk_vector_scan.rs index 9b845af7d..b8f7a5c69 100644 --- a/crates/paimon/src/table/pk_vector_scan.rs +++ b/crates/paimon/src/table/pk_vector_scan.rs @@ -439,6 +439,12 @@ fn plan_from_bucket_splits( "bucket-split planning requires at least one bucket split", )); } + // Sync, so the split's marker stands in for asking the server, as in `to_arrow`. + if splits.iter().any(|s| s.data_split().query_auth_required()) { + return Err(crate::table::query_auth::unsupported( + "an engine-planned vector split of such a table carries no authorization", + )); + } let mut snapshot_id: Option = None; let mut seen_buckets: HashSet = HashSet::new(); @@ -1322,6 +1328,29 @@ mod tests { .unwrap_or_default() } + #[test] + fn a_marked_engine_split_is_refused() { + let data_split = DataSplitBuilder::new() + .with_snapshot(11) + .with_partition(BinaryRow::new(0)) + .with_bucket(0) + .with_bucket_path("bucket-0".to_string()) + .with_total_buckets(1) + .with_data_files(vec![dfm("d0", 5, 5, Some(1))]) + .build() + .unwrap() + .planned(None); + let split = BucketVectorSearchSplit::new_for_test(data_split, vec![], Default::default()); + let Err(err) = plan_from_bucket_splits("ivf-pq", 2, None, "/tbl", false, vec![split]) + else { + panic!("a marked split must not plan") + }; + assert!( + matches!(err, crate::Error::Unsupported { ref message } if message.contains("query-auth.enabled")), + "{err:?}" + ); + } + #[test] fn plans_the_java_golden_bucket_split() { let split = BucketVectorSearchSplit::deserialize(BUCKET_SPLIT_GOLDEN).unwrap(); From 2cc1bd2d48703ce62a3233b68f27388c606485a2 Mon Sep 17 00:00:00 2001 From: Li Jiajia Date: Mon, 14 Sep 2026 10:57:13 -0400 Subject: [PATCH 7/7] fix(auth): match nested fields by id and keep a refused retry from deleting committed files --- crates/paimon/src/table/query_auth.rs | 18 ++++++--- crates/paimon/src/table/table_commit.rs | 49 +++++++++++++------------ 2 files changed, 37 insertions(+), 30 deletions(-) diff --git a/crates/paimon/src/table/query_auth.rs b/crates/paimon/src/table/query_auth.rs index bdcfa34a2..1903e60e9 100644 --- a/crates/paimon/src/table/query_auth.rs +++ b/crates/paimon/src/table/query_auth.rs @@ -97,16 +97,18 @@ pub(crate) async fn reject_unauthorized_stats( } /// Whether `narrow` reads nothing `wide` does not have: nested children are -/// matched by name and must be contained in turn, so a projection of a `ROW` -/// passes and an extra child does not. Descriptions are not columns and are -/// ignored. +/// matched by id and name and must be contained in turn, so a projection of a +/// `ROW` passes while an extra child, or one re-added under a new id, does +/// not. Descriptions are not columns and are ignored. fn contains(wide: &crate::spec::DataType, narrow: &crate::spec::DataType) -> bool { use crate::spec::DataType; match (wide, narrow) { (DataType::Row(w), DataType::Row(n)) => n.fields().iter().all(|nf| { - w.fields() - .iter() - .any(|wf| wf.name() == nf.name() && contains(wf.data_type(), nf.data_type())) + w.fields().iter().any(|wf| { + wf.id() == nf.id() + && wf.name() == nf.name() + && contains(wf.data_type(), nf.data_type()) + }) }), (DataType::Array(w), DataType::Array(n)) => contains(w.element_type(), n.element_type()), (DataType::Multiset(w), DataType::Multiset(n)) => { @@ -425,6 +427,10 @@ mod tests { )); // And so would a child under another name. assert!(!super::contains(&wide, &row(vec![child("c", None)]))); + // Or the same name and type re-added under a new id: older files still + // resolve the old id. + let readded = DataField::new(9, "a".to_string(), int()); + assert!(!super::contains(&wide, &row(vec![readded]))); } #[test] diff --git a/crates/paimon/src/table/table_commit.rs b/crates/paimon/src/table/table_commit.rs index 421a88505..189a0c654 100644 --- a/crates/paimon/src/table/table_commit.rs +++ b/crates/paimon/src/table/table_commit.rs @@ -205,12 +205,9 @@ impl TableCommit { filter_committed: bool, ) -> Result<()> { // A commit validates against the existing snapshot. - // Refused before anything is submitted, so the prepared files — index - // shards included — are safe to remove rather than leave orphaned. - if let Err(error) = self.table.ensure_read_authorized_live("a commit").await { - let _ = self.abort(&commit_messages).await; - return Err(error); - } + // A refusal here must not clean up: a retry with an identifier that + // already committed names files a snapshot references. + self.table.ensure_read_authorized_live("a commit").await?; self.table.ensure_not_branch_reference_for_write()?; validate_fixed_bucket_commit_mode(&commit_messages, false)?; validate_bucket_ownership(&commit_messages)?; @@ -342,12 +339,9 @@ impl TableCommit { filter_committed: bool, ) -> Result<()> { // A commit validates against the existing snapshot. - // Refused before anything is submitted, so the prepared files — index - // shards included — are safe to remove rather than leave orphaned. - if let Err(error) = self.table.ensure_read_authorized_live("a commit").await { - let _ = self.abort(&commit_messages).await; - return Err(error); - } + // A refusal here must not clean up: a retry with an identifier that + // already committed names files a snapshot references. + self.table.ensure_read_authorized_live("a commit").await?; self.table.ensure_not_branch_reference_for_write()?; validate_fixed_bucket_commit_mode(&commit_messages, true)?; validate_bucket_ownership(&commit_messages)?; @@ -5539,17 +5533,10 @@ mod tests { } #[tokio::test] - async fn test_a_refused_commit_removes_the_prepared_index_files() { + async fn test_a_refused_retry_keeps_the_files_its_identifier_committed() { let file_io = test_file_io(); - let table_path = "memory:/test_refused_commit_index_cleanup"; + let table_path = "memory:/test_refused_retry_keeps_committed_files"; setup_dirs(&file_io, table_path).await; - let table = test_table_with_options( - &file_io, - table_path, - HashMap::from([("query-auth.enabled".to_string(), "true".to_string())]), - ); - let commit = TableCommit::new(table, "test-user".to_string()); - let index_path = format!("{table_path}/index/bucket-index"); file_io .mkdirs(&format!("{table_path}/index/")) @@ -5571,11 +5558,25 @@ mod tests { external_path: None, global_index_meta: None, }]; + setup_commit(&file_io, table_path) + .commit_with_identifier(vec![message.clone()], 7) + .await + .unwrap(); - assert!(commit.commit(vec![message]).await.is_err()); + // The option arrives between the commit and its retry. + let guarded = test_table_with_options( + &file_io, + table_path, + HashMap::from([("query-auth.enabled".to_string(), "true".to_string())]), + ); + let retry = TableCommit::new(guarded, "test-user".to_string()); + assert!(retry + .filter_and_commit_with_identifier(vec![message], 7) + .await + .is_err()); assert!( - !file_io.exists(&index_path).await.unwrap(), - "a commit refused before submission must not leave its index files behind" + file_io.exists(&index_path).await.unwrap(), + "a refused retry must not delete files the first commit's snapshot references" ); }