From c382fa158dc84b329328606f663efe574f102a7d Mon Sep 17 00:00:00 2001 From: Tobie Morgan Hitchcock Date: Tue, 17 Sep 2024 15:43:30 +0100 Subject: [PATCH] Improvements to behaviour (#4800) --- core/src/doc/check.rs | 136 +++++++++------------------------------ core/src/doc/document.rs | 126 ++++++++++++++++++++++++++++++++++-- core/src/doc/pluck.rs | 120 ++++++++++++++++++++++++++++++---- sdk/tests/delete.rs | 46 ++++++++++++- sdk/tests/field.rs | 2 +- 5 files changed, 304 insertions(+), 126 deletions(-) diff --git a/core/src/doc/check.rs b/core/src/doc/check.rs index 4cb35773..15eed29b 100644 --- a/core/src/doc/check.rs +++ b/core/src/doc/check.rs @@ -124,27 +124,11 @@ impl Document { // The id is a match, so don't error v if rid.id.is(&v) => (), // The in field does not match - v => match v.convert_to_record() { - // This is a value which matches the id - Ok(v) if v.eq(&rid) => (), - // The value is a record but doesn't match - Ok(v) => { - return Err(Error::IdMismatch { - value: v.to_string(), - }) - } - // The in field does not match at all - Err(Error::ConvertTo { - from, - .. - }) => { - return Err(Error::IdMismatch { - value: from.to_string(), - }) - } - // Return any other error - Err(e) => return Err(e), - }, + v => { + return Err(Error::IdMismatch { + value: v.to_string(), + }) + } } } } @@ -161,27 +145,11 @@ impl Document { // The in is a match, so don't error v if l.id.is(&v) => (), // The in field does not match - v => match v.convert_to_record() { - // This is a value which matches the id - Ok(v) if v.eq(l) => (), - // The value is a record but doesn't match - Ok(v) => { - return Err(Error::InMismatch { - value: v.to_string(), - }) - } - // The in field does not match at all - Err(Error::ConvertTo { - from, - .. - }) => { - return Err(Error::InMismatch { - value: from.to_string(), - }) - } - // Return any other error - Err(e) => return Err(e), - }, + v => { + return Err(Error::InMismatch { + value: v.to_string(), + }) + } } } // Check that the 'out' field matches @@ -192,27 +160,11 @@ impl Document { // The out is a match, so don't error v if r.id.is(&v) => (), // The in field does not match - v => match v.convert_to_record() { - // This is a value which matches the id - Ok(v) if v.eq(r) => (), - // The value is a record but doesn't match - Ok(v) => { - return Err(Error::OutMismatch { - value: v.to_string(), - }) - } - // The in field does not match at all - Err(Error::ConvertTo { - from, - .. - }) => { - return Err(Error::OutMismatch { - value: from.to_string(), - }) - } - // Return any other error - Err(e) => return Err(e), - }, + v => { + return Err(Error::OutMismatch { + value: v.to_string(), + }) + } } } } @@ -225,27 +177,11 @@ impl Document { // The in is a match, so don't error v if l.id.is(&v) => (), // The in field does not match - v => match v.convert_to_record() { - // This is a value which matches the id - Ok(v) if v.eq(l) => (), - // The value is a record but doesn't match - Ok(v) => { - return Err(Error::InMismatch { - value: v.to_string(), - }) - } - // The in field does not match at all - Err(Error::ConvertTo { - from, - .. - }) => { - return Err(Error::InMismatch { - value: from.to_string(), - }) - } - // Return any other error - Err(e) => return Err(e), - }, + v => { + return Err(Error::InMismatch { + value: v.to_string(), + }) + } } // Check that the 'out' field matches match data.pick(&*OUT).compute(stk, ctx, opt, Some(&self.current)).await? { @@ -254,27 +190,11 @@ impl Document { // The out is a match, so don't error v if l.id.is(&v) => (), // The out field does not match - v => match v.convert_to_record() { - // This is a value which matches the id - Ok(v) if v.eq(l) => (), - // The value is a record but doesn't match - Ok(v) => { - return Err(Error::OutMismatch { - value: v.to_string(), - }) - } - // The out field does not match at all - Err(Error::ConvertTo { - from, - .. - }) => { - return Err(Error::OutMismatch { - value: from.to_string(), - }) - } - // Return any other error - Err(e) => return Err(e), - }, + v => { + return Err(Error::OutMismatch { + value: v.to_string(), + }) + } } } } @@ -287,7 +207,7 @@ impl Document { /// a table, or from an index can be filtered out /// before being included within the query output. pub async fn check_where_condition( - &self, + &mut self, stk: &mut Stk, ctx: &Context, opt: &Options, @@ -295,8 +215,10 @@ impl Document { ) -> Result<(), Error> { // Check where condition if let Some(cond) = stm.conds() { + // Process the current permitted + self.process_permitted_current(stk, ctx, opt).await?; // Check if the expression is truthy - if !cond.compute(stk, ctx, opt, Some(&self.current)).await?.is_truthy() { + if !cond.compute(stk, ctx, opt, Some(&self.current_permitted)).await?.is_truthy() { // Ignore this document return Err(Error::Ignore); } diff --git a/core/src/doc/document.rs b/core/src/doc/document.rs index 6b1c4478..dac5cdbc 100644 --- a/core/src/doc/document.rs +++ b/core/src/doc/document.rs @@ -1,10 +1,12 @@ use crate::ctx::Context; +use crate::ctx::MutableContext; use crate::dbs::Options; use crate::dbs::Workable; use crate::err::Error; use crate::iam::Action; use crate::iam::ResourceKind; use crate::idx::planner::iterators::IteratorRecord; +use crate::sql::permission::Permission; use crate::sql::statements::define::DefineEventStatement; use crate::sql::statements::define::DefineFieldStatement; use crate::sql::statements::define::DefineIndexStatement; @@ -13,6 +15,7 @@ use crate::sql::statements::live::LiveStatement; use crate::sql::thing::Thing; use crate::sql::value::Value; use crate::sql::Base; +use reblessive::tree::Stk; use std::fmt::{Debug, Formatter}; use std::mem; use std::ops::Deref; @@ -23,10 +26,12 @@ pub(crate) struct Document { pub(super) extras: Workable, pub(super) initial: CursorDoc, pub(super) current: CursorDoc, + pub(super) initial_permitted: CursorDoc, + pub(super) current_permitted: CursorDoc, } #[non_exhaustive] -#[cfg_attr(debug_assertions, derive(Debug))] +#[derive(Clone, Debug)] pub struct CursorDoc { pub(crate) rid: Option>, pub(crate) ir: Option>, @@ -34,8 +39,7 @@ pub struct CursorDoc { } #[non_exhaustive] -#[cfg_attr(debug_assertions, derive(Debug))] -#[derive(Clone)] +#[derive(Clone, Debug)] pub(crate) struct CursorValue { mutable: Value, read_only: Option>, @@ -165,7 +169,9 @@ impl Document { id: id.clone(), extras, current: CursorDoc::new(id.clone(), ir.clone(), val.clone()), - initial: CursorDoc::new(id, ir, val), + initial: CursorDoc::new(id.clone(), ir.clone(), val.clone()), + current_permitted: CursorDoc::new(id.clone(), ir.clone(), val.clone()), + initial_permitted: CursorDoc::new(id.clone(), ir.clone(), val.clone()), } } @@ -180,6 +186,118 @@ impl Document { pub(crate) fn initial_doc(&self) -> &Value { &self.initial.doc } + + pub(crate) async fn process_permitted_current( + &mut self, + stk: &mut Stk, + ctx: &Context, + opt: &Options, + ) -> Result<(), Error> { + // Clone the fully permitted documents + self.current_permitted = self.current.clone(); + // Check if this record exists + if self.id.is_some() { + // Should we run permissions checks? + if opt.check_perms(Action::View)? { + // Loop through all field statements + for fd in self.fd(ctx, opt).await?.iter() { + // + let mut out = self + .current + .doc + .as_ref() + .compute(stk, ctx, opt, Some(&self.current)) + .await?; + // Loop over each field in document + for k in out.each(&fd.name).iter() { + // Process the field permissions + match &fd.permissions.select { + Permission::Full => (), + Permission::None => out.del(stk, ctx, opt, k).await?, + Permission::Specific(e) => { + // Disable permissions + let opt = &opt.new_with_perms(false); + // Get the current value + let val = Arc::new(self.current.doc.as_ref().pick(k)); + // Configure the context + let mut ctx = MutableContext::new(ctx); + ctx.add_value("value", val); + let ctx = ctx.freeze(); + // Process the PERMISSION clause + if !e + .compute(stk, &ctx, opt, Some(&self.current)) + .await? + .is_truthy() + { + out.del(stk, &ctx, opt, k).await? + } + } + } + } + // Update the permitted document + self.current_permitted.doc = out.into(); + } + } + } + // Carry on + Ok(()) + } + + pub(crate) async fn process_permitted_initial( + &mut self, + stk: &mut Stk, + ctx: &Context, + opt: &Options, + ) -> Result<(), Error> { + // Clone the fully permitted documents + self.initial_permitted = self.initial.clone(); + // Check if this record exists + if self.id.is_some() { + // Should we run permissions checks? + if opt.check_perms(Action::View)? { + // Loop through all field statements + for fd in self.fd(ctx, opt).await?.iter() { + // + let mut out = self + .initial + .doc + .as_ref() + .compute(stk, ctx, opt, Some(&self.initial)) + .await?; + // Loop over each field in document + for k in out.each(&fd.name).iter() { + // Process the field permissions + match &fd.permissions.select { + Permission::Full => (), + Permission::None => out.del(stk, ctx, opt, k).await?, + Permission::Specific(e) => { + // Disable permissions + let opt = &opt.new_with_perms(false); + // Get the current value + let val = Arc::new(self.initial.doc.as_ref().pick(k)); + // Configure the context + let mut ctx = MutableContext::new(ctx); + ctx.add_value("value", val); + let ctx = ctx.freeze(); + // Process the PERMISSION clause + if !e + .compute(stk, &ctx, opt, Some(&self.initial)) + .await? + .is_truthy() + { + out.del(stk, &ctx, opt, k).await? + } + } + } + } + // Update the permitted document + self.initial_permitted.doc = out.into(); + } + } + } + // Carry on + Ok(()) + } } impl Document { diff --git a/core/src/doc/pluck.rs b/core/src/doc/pluck.rs index 95bc60db..88f9b469 100644 --- a/core/src/doc/pluck.rs +++ b/core/src/doc/pluck.rs @@ -25,36 +25,91 @@ impl Document { ) -> Result { // Ensure futures are run let opt = &opt.new_with_futures(true); + // Check if this record exists + if self.id.is_some() { + // Should we run permissions checks? + if opt.check_perms(Action::View)? { + // Get the table for this document + let table = self.tb(ctx, opt).await?; + // Get the permissions for this table + let perms = &table.permissions.select; + // Process the table permissions + match perms { + Permission::None => return Err(Error::Ignore), + Permission::Full => (), + Permission::Specific(e) => { + // Disable permissions + let opt = &opt.new_with_perms(false); + // Process the PERMISSION clause + if !e + .compute( + stk, + ctx, + opt, + Some(match stm.is_delete() { + true => &self.initial, + false => &self.current, + }), + ) + .await? + .is_truthy() + { + return Err(Error::Ignore); + } + } + } + } + } // Process the desired output let mut out = match stm.output() { Some(v) => match v { Output::None => Err(Error::Ignore), Output::Null => Ok(Value::Null), Output::Diff => { + // Process the current permitted + self.process_permitted_current(stk, ctx, opt).await?; + // Process the initial permitted + self.process_permitted_initial(stk, ctx, opt).await?; // Output a DIFF of any changes applied to the document Ok(self - .initial + .initial_permitted .doc .as_ref() - .diff(self.current.doc.as_ref(), Idiom::default()) + .diff(self.current_permitted.doc.as_ref(), Idiom::default()) .into()) } Output::After => { + // Process the current permitted + self.process_permitted_current(stk, ctx, opt).await?; // Output the full document after all changes were applied - self.current.doc.as_ref().compute(stk, ctx, opt, Some(&self.current)).await + self.current_permitted + .doc + .as_ref() + .compute(stk, ctx, opt, Some(&self.current_permitted)) + .await } Output::Before => { + // Process the initial permitted + self.process_permitted_initial(stk, ctx, opt).await?; // Output the full document before any changes were applied - self.initial.doc.as_ref().compute(stk, ctx, opt, Some(&self.initial)).await + self.initial_permitted + .doc + .as_ref() + .compute(stk, ctx, opt, Some(&self.initial_permitted)) + .await } Output::Fields(v) => { + // Process the current permitted + self.process_permitted_current(stk, ctx, opt).await?; + // Process the initial permitted + self.process_permitted_initial(stk, ctx, opt).await?; // Configure the context let mut ctx = MutableContext::new(ctx); - ctx.add_value("after", self.current.doc.as_arc()); - ctx.add_value("before", self.initial.doc.as_arc()); + ctx.add_value("after", self.current_permitted.doc.as_arc()); + ctx.add_value("before", self.initial_permitted.doc.as_arc()); let ctx = ctx.freeze(); // Output the specified fields - v.compute(stk, &ctx, opt, Some(&self.current), false).await + v.compute(stk, &ctx, opt, Some(&self.current_permitted), false).await } }, None => match stm { @@ -68,22 +123,61 @@ impl Document { _ => s.expr.compute(stk, ctx, opt, Some(&self.current), false).await, }, Statement::Select(s) => { - s.expr.compute(stk, ctx, opt, Some(&self.current), s.group.is_some()).await + // Process the current permitted + self.process_permitted_current(stk, ctx, opt).await?; + s.expr + .compute(stk, ctx, opt, Some(&self.current_permitted), s.group.is_some()) + .await } Statement::Create(_) => { - self.current.doc.as_ref().compute(stk, ctx, opt, Some(&self.current)).await + // Process the current permitted + self.process_permitted_current(stk, ctx, opt).await?; + // Process the document output + self.current_permitted + .doc + .as_ref() + .compute(stk, ctx, opt, Some(&self.current_permitted)) + .await } Statement::Upsert(_) => { - self.current.doc.as_ref().compute(stk, ctx, opt, Some(&self.current)).await + // Process the current permitted + self.process_permitted_current(stk, ctx, opt).await?; + // Process the document output + self.current_permitted + .doc + .as_ref() + .compute(stk, ctx, opt, Some(&self.current_permitted)) + .await } Statement::Update(_) => { - self.current.doc.as_ref().compute(stk, ctx, opt, Some(&self.current)).await + // Process the current permitted + self.process_permitted_current(stk, ctx, opt).await?; + // Process the document output + self.current_permitted + .doc + .as_ref() + .compute(stk, ctx, opt, Some(&self.current_permitted)) + .await } Statement::Relate(_) => { - self.current.doc.as_ref().compute(stk, ctx, opt, Some(&self.current)).await + // Process the current permitted + self.process_permitted_current(stk, ctx, opt).await?; + // Process the document output + self.current_permitted + .doc + .as_ref() + .compute(stk, ctx, opt, Some(&self.current_permitted)) + .await } Statement::Insert(_) => { - self.current.doc.as_ref().compute(stk, ctx, opt, Some(&self.current)).await + // Process the current permitted + self.process_permitted_current(stk, ctx, opt).await?; + // Process the document output + self.current_permitted + .doc + .as_ref() + .compute(stk, ctx, opt, Some(&self.current_permitted)) + .await } _ => Err(Error::Ignore), }, diff --git a/sdk/tests/delete.rs b/sdk/tests/delete.rs index b9d46e88..0f4e64e6 100644 --- a/sdk/tests/delete.rs +++ b/sdk/tests/delete.rs @@ -429,7 +429,7 @@ async fn delete_filtered_live_notification() -> Result<(), Error> { } #[tokio::test] -async fn delete_with_permissions() -> Result<(), Error> { +async fn delete_with_permissions_no_select() -> Result<(), Error> { let sql = " DEFINE TABLE friends_with PERMISSIONS FOR delete WHERE in = $auth; CREATE user:john, user:mary; @@ -462,6 +462,50 @@ async fn delete_with_permissions() -> Result<(), Error> { assert_eq!(res.len(), 2); // let tmp = res.remove(0).result?; + let val = Value::parse("[]"); + assert_eq!(tmp, val); + // + let tmp = res.remove(0).result?; + let val = Value::parse("[]"); + assert_eq!(tmp, val); + // + Ok(()) +} + +#[tokio::test] +async fn delete_with_permissions_with_select() -> Result<(), Error> { + let sql = " + DEFINE TABLE friends_with PERMISSIONS FOR select, delete WHERE in = $auth; + CREATE user:john, user:mary; + RELATE user:john->friends_with:1->user:mary; + RELATE user:mary->friends_with:2->user:john; + "; + let dbs = new_ds().await?.with_auth_enabled(true); + let ses = Session::owner().with_ns("test").with_db("test"); + let res = &mut dbs.execute(sql, &ses, None).await?; + assert_eq!(res.len(), 4); + // + let tmp = res.remove(0).result; + assert!(tmp.is_ok()); + // + let tmp = res.remove(0).result; + assert!(tmp.is_ok()); + // + let tmp = res.remove(0).result; + assert!(tmp.is_ok()); + // + let tmp = res.remove(0).result; + assert!(tmp.is_ok()); + // + let sql = " + DELETE friends_with:1 RETURN BEFORE; + DELETE friends_with:2 RETURN BEFORE; + "; + let ses = Session::for_record("test", "test", "test", Thing::from(("user", "john")).into()); + let res = &mut dbs.execute(sql, &ses, None).await?; + assert_eq!(res.len(), 2); + // + let tmp = res.remove(0).result?; let val = Value::parse( "[ { diff --git a/sdk/tests/field.rs b/sdk/tests/field.rs index a6d9a11f..7a5d631b 100644 --- a/sdk/tests/field.rs +++ b/sdk/tests/field.rs @@ -844,7 +844,7 @@ async fn field_definition_edge_permissions() -> Result<(), Error> { DEFINE TABLE user SCHEMAFULL; DEFINE TABLE business SCHEMAFULL; DEFINE FIELD owner ON TABLE business TYPE record; - DEFINE TABLE contact TYPE RELATION SCHEMAFULL PERMISSIONS FOR create WHERE in.owner.id = $auth.id; + DEFINE TABLE contact TYPE RELATION SCHEMAFULL PERMISSIONS FOR select, create WHERE in.owner.id = $auth.id; INSERT INTO user (id, name) VALUES (user:one, 'John'), (user:two, 'Lucy'); INSERT INTO business (id, owner) VALUES (business:one, user:one), (business:two, user:two); ";