add v2 compatibility layer #40
Conversation
|
Add to the PR description:
|
There was a problem hiding this comment.
Looks pretty good IMO.
There's a fair bit being moved around so it would be great to get regression tests in place in future to help with this.
I think the behaviour has changed WRT expected auth token not being set, but I'm ok with the change if you are.
After you've tested on a v3 project and v2 project I think I can approve.
| #[arg(long, env = "ENABLE_V2_COMPATIBILITY")] | ||
| enable_v2_compatibility: bool, |
| let expected_auth_header: Option<HeaderValue> = | ||
| serve_command | ||
| .service_token_secret | ||
| .and_then(|service_token_secret| { | ||
| let expected_bearer = format!("Bearer {}", service_token_secret); | ||
| HeaderValue::from_str(&expected_bearer).ok() | ||
| }); | ||
|
|
||
| let router = create_router::<C>(server_state) | ||
| .layer( | ||
| TraceLayer::new_for_http() | ||
| .make_span_with(DefaultMakeSpan::default().level(Level::INFO)), | ||
| ) | ||
| .layer(ValidateRequestHeaderLayer::custom( | ||
| move |request: &mut Request<Body>| { | ||
| // Validate the request | ||
| let auth_header = request.headers().get("Authorization").cloned(); | ||
| let router = create_router::<C>( | ||
| server_state.clone(), | ||
| serve_command.service_token_secret.clone(), | ||
| ); | ||
|
|
||
| // NOTE: The comparison should probably be more permissive to allow for whitespace, etc. | ||
| if auth_header == expected_auth_header { | ||
| return Ok(()); | ||
| } | ||
| Err(( | ||
| StatusCode::UNAUTHORIZED, | ||
| Json(ErrorResponse { | ||
| message: "Internal error".into(), | ||
| details: serde_json::Value::Object(serde_json::Map::from_iter([( | ||
| "cause".into(), | ||
| serde_json::Value::String("Bearer token does not match.".to_string()), | ||
| )])), | ||
| }), | ||
| ) | ||
| .into_response()) | ||
| }, | ||
| )); | ||
| let router = if serve_command.enable_v2_compatibility { | ||
| let v2_router = create_v2_router(server_state, serve_command.service_token_secret.clone()); | ||
| Router::new().merge(router).nest("/v2", v2_router) | ||
| } else { | ||
| router | ||
| }; | ||
|
|
There was a problem hiding this comment.
It would be so good if we had some kind of regression tests. Cest la vie.
There was a problem hiding this comment.
I'll do some old fashion "try it a few times" regression testing. Agreed I'd love to have more confidence this didn't break anything
| if let Some(expected_token_value) = service_token_secret.clone() { | ||
| if let Some(config_header) = | ||
| request.headers().get("x-hasura-dataconnector-config") | ||
| { | ||
| if let Ok(config) = | ||
| serde_json::from_slice::<SourceConfig>(config_header.as_bytes()) | ||
| { | ||
| if let Some(provided_token_value) = config.service_token_secret { | ||
| if provided_token_value == expected_token_value { | ||
| // if token set, config header must be present and include token | ||
| return Ok(()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| // if token not set, always authorize | ||
| return Ok(()); |
There was a problem hiding this comment.
Correct me if I'm wrong but I think this is a change in behaviour:
We should authorise only if:
- Neither the header or expected token are set
- or, Both the header and expected token match
and reject in all other cases.
This implementation authorises if:
- Expected token is not set, but header is set.
Should we allow this? Maybe but it is a change in behavoiur.
There was a problem hiding this comment.
Stricly speaking these are new endpoints so not a behavior change. But this is different from the v3 implementation.
I'll change it to match so we don't have to think about subtle behavior differences in the future.
|
Did some testing with v3 engine, can confirm auth (still) works for v3 and we can query data just fine. Can also query data from v2 engine. |
add
--enable_v2_compatibilityflag and env var. When enabled, a GDC server will be available under the/v2route.Functionality is currently limited to mutations.
Some functionality is not supported, such as Array fields.
also had to conditionally remove sigterm handler so it would compile on windows