From 4e0979c1c343b66207b71782a44899057ac7b2b0 Mon Sep 17 00:00:00 2001 From: Panagiotis Karatakis Date: Sat, 23 Nov 2024 01:56:29 +0000 Subject: [PATCH] feat(jit): plan ahead auth requirements when required (#3139) Co-authored-by: Tushar Mathur --- src/core/jit/exec_const.rs | 32 ++++--- src/core/jit/model.rs | 11 ++- src/core/jit/request.rs | 3 +- src/core/jit/transform/auth_planer.rs | 86 +++++++++++++++++++ src/core/jit/transform/input_resolver.rs | 1 + src/core/jit/transform/mod.rs | 2 + src/core/rest/partial_request.rs | 2 +- tests/core/snapshots/auth-basic.md_1.snap | 9 +- tests/core/snapshots/auth-basic.md_2.snap | 9 +- tests/core/snapshots/auth-basic.md_4.snap | 9 +- tests/core/snapshots/auth-basic.md_6.snap | 9 +- tests/core/snapshots/auth-jwt.md_1.snap | 9 +- tests/core/snapshots/auth-jwt.md_3.snap | 9 +- tests/core/snapshots/auth-jwt.md_5.snap | 9 +- tests/core/snapshots/auth_order.md_0.snap | 19 ++++ .../core/snapshots/auth_order.md_client.snap | 11 +++ .../core/snapshots/auth_order.md_merged.snap | 11 +++ .../graphql-conformance-001.md_5.snap | 9 +- .../graphql-conformance-015.md_10.snap | 9 +- .../graphql-conformance-015.md_9.snap | 9 +- .../graphql-conformance-http-001.md_5.snap | 9 +- .../graphql-conformance-http-015.md_10.snap | 9 +- .../graphql-conformance-http-015.md_9.snap | 9 +- tests/execution/auth_order.md | 37 ++++++++ .../jit_spec__tests__executor_variables.snap | 8 +- ...jit_spec__tests__operation_plan_cache.snap | 8 +- 26 files changed, 228 insertions(+), 120 deletions(-) create mode 100644 src/core/jit/transform/auth_planer.rs create mode 100644 tests/core/snapshots/auth_order.md_0.snap create mode 100644 tests/core/snapshots/auth_order.md_client.snap create mode 100644 tests/core/snapshots/auth_order.md_merged.snap create mode 100644 tests/execution/auth_order.md diff --git a/src/core/jit/exec_const.rs b/src/core/jit/exec_const.rs index 108dc820af..1606f05631 100644 --- a/src/core/jit/exec_const.rs +++ b/src/core/jit/exec_const.rs @@ -6,14 +6,12 @@ use tailcall_valid::Validator; use super::context::Context; use super::exec::{Executor, IRExecutor}; -use super::{ - transform, AnyResponse, BuildError, Error, OperationPlan, Pos, Positioned, Request, Response, - Result, -}; +use super::graphql_error::GraphQLError; +use super::{transform, AnyResponse, BuildError, Error, OperationPlan, Request, Response, Result}; use crate::core::app_context::AppContext; use crate::core::http::RequestContext; use crate::core::ir::model::IR; -use crate::core::ir::{self, EvalContext}; +use crate::core::ir::{self, EmptyResolverContext, EvalContext}; use crate::core::jit::synth::Synth; use crate::core::jit::transform::InputResolver; use crate::core::json::{JsonLike, JsonLikeList}; @@ -42,6 +40,20 @@ impl ConstValueExecutor { req_ctx: &RequestContext, request: Request, ) -> AnyResponse> { + // Run all the IRs in the before chain + if let Some(ir) = &self.plan.before { + let mut eval_context = EvalContext::new(req_ctx, &EmptyResolverContext {}); + match ir.eval(&mut eval_context).await { + Ok(_) => (), + Err(err) => { + let resp: Response = Response::default(); + return resp + .with_errors(vec![GraphQLError::new(err.to_string(), None)]) + .into(); + } + } + } + let is_introspection_query = req_ctx.server.get_enable_introspection() && self.plan.is_introspection_query; let variables = &request.variables; @@ -54,7 +66,7 @@ impl ConstValueExecutor { let resp: Response = Response::default(); // this shouldn't actually ever happen return resp - .with_errors(vec![Positioned::new(Error::Unknown, Pos::default())]) + .with_errors(vec![GraphQLError::new(Error::Unknown.to_string(), None)]) .into(); }; @@ -69,9 +81,9 @@ impl ConstValueExecutor { Err(err) => { let resp: Response = Response::default(); return resp - .with_errors(vec![Positioned::new( - BuildError::from(err).into(), - Pos::default(), + .with_errors(vec![GraphQLError::new( + BuildError::from(err).to_string(), + None, )]) .into(); } @@ -128,7 +140,7 @@ impl<'a> ConstValueExec<'a> { } } -impl<'ctx> IRExecutor for ConstValueExec<'ctx> { +impl IRExecutor for ConstValueExec<'_> { type Input = ConstValue; type Output = ConstValue; type Error = Error; diff --git a/src/core/jit/model.rs b/src/core/jit/model.rs index f0b30ce21c..6e1793f551 100644 --- a/src/core/jit/model.rs +++ b/src/core/jit/model.rs @@ -290,6 +290,9 @@ pub struct OperationPlan { pub is_protected: bool, pub min_cache_ttl: Option, pub selection: Vec>, + + /// An IR that should be executed before the operation starts executing + pub before: Option, } impl std::fmt::Debug for OperationPlan { @@ -312,15 +315,16 @@ impl OperationPlan { } Ok(OperationPlan { + selection, root_name: self.root_name, operation_type: self.operation_type, - selection, index: self.index, is_introspection_query: self.is_introspection_query, is_dedupe: self.is_dedupe, is_const: self.is_const, - min_cache_ttl: self.min_cache_ttl, is_protected: self.is_protected, + min_cache_ttl: self.min_cache_ttl, + before: self.before, }) } } @@ -345,8 +349,9 @@ impl OperationPlan { is_introspection_query, is_dedupe: false, is_const: false, - min_cache_ttl: None, is_protected: false, + min_cache_ttl: None, + before: Default::default(), } } diff --git a/src/core/jit/request.rs b/src/core/jit/request.rs index 459ccf7162..41c3dd1997 100644 --- a/src/core/jit/request.rs +++ b/src/core/jit/request.rs @@ -46,8 +46,9 @@ impl Request { let plan = builder.build(self.operation_name.as_deref())?; transform::CheckConst::new() - .pipe(transform::CheckDedupe::new()) .pipe(transform::CheckProtected::new()) + .pipe(transform::AuthPlaner::new().when(blueprint.server.auth.is_some())) + .pipe(transform::CheckDedupe::new()) .pipe(transform::CheckCache::new()) .transform(plan) .to_result() diff --git a/src/core/jit/transform/auth_planer.rs b/src/core/jit/transform/auth_planer.rs new file mode 100644 index 0000000000..585409d22f --- /dev/null +++ b/src/core/jit/transform/auth_planer.rs @@ -0,0 +1,86 @@ +use std::convert::Infallible; + +use tailcall_valid::Valid; + +use crate::core::blueprint::DynamicValue; +use crate::core::ir::model::IR; +use crate::core::jit::{Field, OperationPlan}; +use crate::core::Transform; + +pub struct AuthPlaner { + marker: std::marker::PhantomData, +} + +impl AuthPlaner { + pub fn new() -> Self { + Self { marker: std::marker::PhantomData } + } +} + +impl Transform for AuthPlaner { + type Value = OperationPlan; + type Error = Infallible; + + fn transform(&self, mut plan: Self::Value) -> Valid { + let mut before = Vec::new(); + + plan.selection = plan + .selection + .into_iter() + .map(|field| drop_protect_field(&mut before, field)) + .collect(); + let ir = before.into_iter(); + + plan.before = match plan.before { + Some(before) => Some(ir.fold(before, |a, b| a.pipe(b))), + None => ir.reduce(|a, b| a.pipe(b)), + }; + Valid::succeed(plan) + } +} + +/// Used to recursively update the field ands its selections to remove +/// IR::Protected +fn drop_protect_field(before: &mut Vec, mut field: Field) -> Field { + if let Some(mut ir) = field.ir { + let is_protected = drop_protect_ir(&mut ir); + + field.selection = field + .selection + .into_iter() + .map(|field| drop_protect_field(before, field)) + .collect(); + + if is_protected { + let ir = IR::Protect(Box::new(IR::Dynamic(DynamicValue::Value( + Default::default(), + )))); + before.push(ir); + } + + field.ir = Some(ir); + } + field +} + +/// This function modifies an IR chain by detecting and removing any +/// instances of IR::Protect from the chain. Returns `true` when it modifies the +/// IR. +pub fn drop_protect_ir(ir: &mut IR) -> bool { + match ir { + IR::Dynamic(_) => false, + IR::IO(_) => false, + IR::Cache(_) => false, + IR::Path(ir, _) => drop_protect_ir(ir), + IR::ContextPath(_) => false, + IR::Protect(inner_ir) => { + *ir = *inner_ir.clone(); + true + } + IR::Map(_) => false, + IR::Pipe(ir1, ir2) => drop_protect_ir(ir1) || drop_protect_ir(ir2), + IR::Discriminate(_, ir) => drop_protect_ir(ir), + IR::Entity(_) => false, + IR::Service(_) => false, + } +} diff --git a/src/core/jit/transform/input_resolver.rs b/src/core/jit/transform/input_resolver.rs index b83d524238..920bcfdae1 100644 --- a/src/core/jit/transform/input_resolver.rs +++ b/src/core/jit/transform/input_resolver.rs @@ -78,6 +78,7 @@ where is_protected: self.plan.is_protected, min_cache_ttl: self.plan.min_cache_ttl, selection, + before: self.plan.before, }) } diff --git a/src/core/jit/transform/mod.rs b/src/core/jit/transform/mod.rs index 986e5c695c..0ff7878991 100644 --- a/src/core/jit/transform/mod.rs +++ b/src/core/jit/transform/mod.rs @@ -1,3 +1,4 @@ +mod auth_planer; mod check_cache; mod check_const; mod check_dedupe; @@ -5,6 +6,7 @@ mod check_protected; mod input_resolver; mod skip; +pub use auth_planer::*; pub use check_cache::*; pub use check_const::*; pub use check_dedupe::*; diff --git a/src/core/rest/partial_request.rs b/src/core/rest/partial_request.rs index dab44c810c..cfd8053483 100644 --- a/src/core/rest/partial_request.rs +++ b/src/core/rest/partial_request.rs @@ -15,7 +15,7 @@ pub struct PartialRequest<'a> { pub path: &'a Path, } -impl<'a> PartialRequest<'a> { +impl PartialRequest<'_> { pub async fn into_request(self, request: Request) -> Result { let mut variables = self.variables; if let Some(key) = self.body { diff --git a/tests/core/snapshots/auth-basic.md_1.snap b/tests/core/snapshots/auth-basic.md_1.snap index 5fd8164e7c..63ca3f7b5a 100644 --- a/tests/core/snapshots/auth-basic.md_1.snap +++ b/tests/core/snapshots/auth-basic.md_1.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Authentication Failure: Missing Authorization Header", - "locations": [ - { - "line": 2, - "column": 3 - } - ] + "message": "Authentication Failure: Missing Authorization Header" } ] } diff --git a/tests/core/snapshots/auth-basic.md_2.snap b/tests/core/snapshots/auth-basic.md_2.snap index 1147bdbb16..c11bd23cd6 100644 --- a/tests/core/snapshots/auth-basic.md_2.snap +++ b/tests/core/snapshots/auth-basic.md_2.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Authentication Failure: Invalid Authorization Header", - "locations": [ - { - "line": 2, - "column": 3 - } - ] + "message": "Authentication Failure: Invalid Authorization Header" } ] } diff --git a/tests/core/snapshots/auth-basic.md_4.snap b/tests/core/snapshots/auth-basic.md_4.snap index 5fd8164e7c..63ca3f7b5a 100644 --- a/tests/core/snapshots/auth-basic.md_4.snap +++ b/tests/core/snapshots/auth-basic.md_4.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Authentication Failure: Missing Authorization Header", - "locations": [ - { - "line": 2, - "column": 3 - } - ] + "message": "Authentication Failure: Missing Authorization Header" } ] } diff --git a/tests/core/snapshots/auth-basic.md_6.snap b/tests/core/snapshots/auth-basic.md_6.snap index 1147bdbb16..c11bd23cd6 100644 --- a/tests/core/snapshots/auth-basic.md_6.snap +++ b/tests/core/snapshots/auth-basic.md_6.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Authentication Failure: Invalid Authorization Header", - "locations": [ - { - "line": 2, - "column": 3 - } - ] + "message": "Authentication Failure: Invalid Authorization Header" } ] } diff --git a/tests/core/snapshots/auth-jwt.md_1.snap b/tests/core/snapshots/auth-jwt.md_1.snap index 5fd8164e7c..63ca3f7b5a 100644 --- a/tests/core/snapshots/auth-jwt.md_1.snap +++ b/tests/core/snapshots/auth-jwt.md_1.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Authentication Failure: Missing Authorization Header", - "locations": [ - { - "line": 2, - "column": 3 - } - ] + "message": "Authentication Failure: Missing Authorization Header" } ] } diff --git a/tests/core/snapshots/auth-jwt.md_3.snap b/tests/core/snapshots/auth-jwt.md_3.snap index 5fd8164e7c..63ca3f7b5a 100644 --- a/tests/core/snapshots/auth-jwt.md_3.snap +++ b/tests/core/snapshots/auth-jwt.md_3.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Authentication Failure: Missing Authorization Header", - "locations": [ - { - "line": 2, - "column": 3 - } - ] + "message": "Authentication Failure: Missing Authorization Header" } ] } diff --git a/tests/core/snapshots/auth-jwt.md_5.snap b/tests/core/snapshots/auth-jwt.md_5.snap index 1147bdbb16..c11bd23cd6 100644 --- a/tests/core/snapshots/auth-jwt.md_5.snap +++ b/tests/core/snapshots/auth-jwt.md_5.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Authentication Failure: Invalid Authorization Header", - "locations": [ - { - "line": 2, - "column": 3 - } - ] + "message": "Authentication Failure: Invalid Authorization Header" } ] } diff --git a/tests/core/snapshots/auth_order.md_0.snap b/tests/core/snapshots/auth_order.md_0.snap new file mode 100644 index 0000000000..63ca3f7b5a --- /dev/null +++ b/tests/core/snapshots/auth_order.md_0.snap @@ -0,0 +1,19 @@ +--- +source: tests/core/spec.rs +expression: response +snapshot_kind: text +--- +{ + "status": 200, + "headers": { + "content-type": "application/json" + }, + "body": { + "data": null, + "errors": [ + { + "message": "Authentication Failure: Missing Authorization Header" + } + ] + } +} diff --git a/tests/core/snapshots/auth_order.md_client.snap b/tests/core/snapshots/auth_order.md_client.snap new file mode 100644 index 0000000000..66a444f416 --- /dev/null +++ b/tests/core/snapshots/auth_order.md_client.snap @@ -0,0 +1,11 @@ +--- +source: tests/core/spec.rs +expression: formatted +--- +type Query { + data: String +} + +schema { + query: Query +} diff --git a/tests/core/snapshots/auth_order.md_merged.snap b/tests/core/snapshots/auth_order.md_merged.snap new file mode 100644 index 0000000000..e53d975500 --- /dev/null +++ b/tests/core/snapshots/auth_order.md_merged.snap @@ -0,0 +1,11 @@ +--- +source: tests/core/spec.rs +expression: formatter +--- +schema @server @upstream @link(id: "htpasswd", src: ".htpasswd", type: Htpasswd) { + query: Query +} + +type Query { + data: String @http(url: "http://upstream/data") @protected +} diff --git a/tests/core/snapshots/graphql-conformance-001.md_5.snap b/tests/core/snapshots/graphql-conformance-001.md_5.snap index 028246ac51..0f61cb1cd5 100644 --- a/tests/core/snapshots/graphql-conformance-001.md_5.snap +++ b/tests/core/snapshots/graphql-conformance-001.md_5.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Build error: ResolveInputError: Argument `id` for field `user` is required", - "locations": [ - { - "line": 0, - "column": 0 - } - ] + "message": "ResolveInputError: Argument `id` for field `user` is required" } ] } diff --git a/tests/core/snapshots/graphql-conformance-015.md_10.snap b/tests/core/snapshots/graphql-conformance-015.md_10.snap index 40dcdfd94b..e71a8d0f9e 100644 --- a/tests/core/snapshots/graphql-conformance-015.md_10.snap +++ b/tests/core/snapshots/graphql-conformance-015.md_10.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Build error: ResolveInputError: Argument `size` for field `profilePic` is required", - "locations": [ - { - "line": 0, - "column": 0 - } - ] + "message": "ResolveInputError: Argument `size` for field `profilePic` is required" } ] } diff --git a/tests/core/snapshots/graphql-conformance-015.md_9.snap b/tests/core/snapshots/graphql-conformance-015.md_9.snap index 6d14fcb37a..ef6d6b9399 100644 --- a/tests/core/snapshots/graphql-conformance-015.md_9.snap +++ b/tests/core/snapshots/graphql-conformance-015.md_9.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Build error: ResolveInputError: Argument `height` for field `featuredVideoPreview.video` is required", - "locations": [ - { - "line": 0, - "column": 0 - } - ] + "message": "ResolveInputError: Argument `height` for field `featuredVideoPreview.video` is required" } ] } diff --git a/tests/core/snapshots/graphql-conformance-http-001.md_5.snap b/tests/core/snapshots/graphql-conformance-http-001.md_5.snap index 028246ac51..0f61cb1cd5 100644 --- a/tests/core/snapshots/graphql-conformance-http-001.md_5.snap +++ b/tests/core/snapshots/graphql-conformance-http-001.md_5.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Build error: ResolveInputError: Argument `id` for field `user` is required", - "locations": [ - { - "line": 0, - "column": 0 - } - ] + "message": "ResolveInputError: Argument `id` for field `user` is required" } ] } diff --git a/tests/core/snapshots/graphql-conformance-http-015.md_10.snap b/tests/core/snapshots/graphql-conformance-http-015.md_10.snap index 40dcdfd94b..e71a8d0f9e 100644 --- a/tests/core/snapshots/graphql-conformance-http-015.md_10.snap +++ b/tests/core/snapshots/graphql-conformance-http-015.md_10.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Build error: ResolveInputError: Argument `size` for field `profilePic` is required", - "locations": [ - { - "line": 0, - "column": 0 - } - ] + "message": "ResolveInputError: Argument `size` for field `profilePic` is required" } ] } diff --git a/tests/core/snapshots/graphql-conformance-http-015.md_9.snap b/tests/core/snapshots/graphql-conformance-http-015.md_9.snap index 6d14fcb37a..ef6d6b9399 100644 --- a/tests/core/snapshots/graphql-conformance-http-015.md_9.snap +++ b/tests/core/snapshots/graphql-conformance-http-015.md_9.snap @@ -1,6 +1,7 @@ --- source: tests/core/spec.rs expression: response +snapshot_kind: text --- { "status": 200, @@ -11,13 +12,7 @@ expression: response "data": null, "errors": [ { - "message": "Build error: ResolveInputError: Argument `height` for field `featuredVideoPreview.video` is required", - "locations": [ - { - "line": 0, - "column": 0 - } - ] + "message": "ResolveInputError: Argument `height` for field `featuredVideoPreview.video` is required" } ] } diff --git a/tests/execution/auth_order.md b/tests/execution/auth_order.md new file mode 100644 index 0000000000..6959c2b95a --- /dev/null +++ b/tests/execution/auth_order.md @@ -0,0 +1,37 @@ +# auth order + +```graphql @config +schema @server @upstream @link(id: "htpasswd", src: ".htpasswd", type: Htpasswd) { + query: Query +} + +type Query { + data: String @http(url: "http://upstream/data") @protected +} +``` + +```text @file:.htpasswd +testuser1:$apr1$e3dp9qh2$fFIfHU9bilvVZBl8TxKzL/ +testuser2:$2y$10$wJ/mZDURcAOBIrswCAKFsO0Nk7BpHmWl/XuhF7lNm3gBAFH3ofsuu +testuser3:{SHA}Y2fEjdGT1W6nsLqtJbGUVeUp9e4= +``` + +```yml @mock +- request: + method: GET + url: http://upstream/data + expectedHits: 0 + response: + status: 500 + body: false +``` + +```yml @test +- method: POST + url: http://localhost:8000/graphql + body: + query: | + query { + data + } +``` diff --git a/tests/snapshots/jit_spec__tests__executor_variables.snap b/tests/snapshots/jit_spec__tests__executor_variables.snap index 5bcbfbb38a..f66f2cb67e 100644 --- a/tests/snapshots/jit_spec__tests__executor_variables.snap +++ b/tests/snapshots/jit_spec__tests__executor_variables.snap @@ -7,13 +7,7 @@ snapshot_kind: text "data": null, "errors": [ { - "message": "Build error: ResolveInputError: Variable `id` is not defined", - "locations": [ - { - "line": 0, - "column": 0 - } - ] + "message": "ResolveInputError: Variable `id` is not defined" } ] } diff --git a/tests/snapshots/jit_spec__tests__operation_plan_cache.snap b/tests/snapshots/jit_spec__tests__operation_plan_cache.snap index 5bcbfbb38a..f66f2cb67e 100644 --- a/tests/snapshots/jit_spec__tests__operation_plan_cache.snap +++ b/tests/snapshots/jit_spec__tests__operation_plan_cache.snap @@ -7,13 +7,7 @@ snapshot_kind: text "data": null, "errors": [ { - "message": "Build error: ResolveInputError: Variable `id` is not defined", - "locations": [ - { - "line": 0, - "column": 0 - } - ] + "message": "ResolveInputError: Variable `id` is not defined" } ] }