Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auth: REST + Webhook Auth operation context missing #8771

Closed
fkowal opened this issue Aug 10, 2022 · 11 comments
Closed

Auth: REST + Webhook Auth operation context missing #8771

fkowal opened this issue Aug 10, 2022 · 11 comments
Labels
k/enhancement New feature or improve an existing feature

Comments

@fkowal
Copy link

fkowal commented Aug 10, 2022

Is your proposal related to a problem?

Inconsistent behavior of webhook request for REST api. #7910
I am unable to pass queryParam from ENV variable (secret required by external system)

Solution

Consistent webhook that provides the context for REST & graphql queries

Given:

query sample_query($arg: String) {
   run_action_that_requires_a_query_param_secret { ... } 
}
and a REST endpoint /api/rest/endpoint/sample_query_path/:arg that executed the above query

HASURA_GRAPHQL_AUTH_HOOK=GET

Example request: `{hasuraUrl}/api/rest/endpoint/sample_query_path/argValue`
Auth request1: `{authEndpoint}?path=/api/rest/endpoint/sample_query_path/argValue&type=rest|graphql`
Auth request2: `{authEndpoint}?operationName=sample_query&[arg=argValue]&type=rest|graphql <- optional arg values

HASURA_GRAPHQL_AUTH_HOOK=POST

Auth request1: { headers, request: { operationName: sample_query, variables: { arg: "argValue" }} <- consitent with running the above sample query directly via `/v1/graphql`}

Auth request2: { headers, type: "rest", path: "/api/rest/endpoint/sample_query_path/:arg", variables: {arg: "argValue"}}}

Auth request3: { headers, request: { type: "rest", path: "/api/rest/endpoint/sample_query_path/:arg", variables: {arg: "argValue"}}}

When the operation name is available to the auth webhook
I would be able to pass the x-hasura-external-api-secret and use it via ${session_variables[...]} in the action
But only when operationName / path should have access to this

If the feature is approved, would you be willing to submit a PR?

No

@fkowal fkowal added the k/enhancement New feature or improve an existing feature label Aug 10, 2022
@hasura hasura deleted a comment from Blackbarbie8 Aug 25, 2022
@robertjdominguez robertjdominguez changed the title REST + Webhook Auth operation context missing Auth: REST + Webhook Auth operation context missing Aug 25, 2022
@robertjdominguez
Copy link
Contributor

Closing this in favor of #7910, @fkowal. If you'd prefer to keep this alive for conversation, let me know and I'll convert to a discussion 👍

@robertjdominguez robertjdominguez closed this as not planned Won't fix, can't repro, duplicate, stale Aug 25, 2022
@fkowal
Copy link
Author

fkowal commented Aug 26, 2022

i'll send a sample PR soon

i explored and changed the approach from described above

and it should enable support graphql : aliases and namespaced db / queries

{
  namespace {
      alias: table { ... }
      
}

highlight

resource=namespace#table:alias
and chaning the src/buildQuery/index.ts that split the resourcename and wrapping the resource in a proxy object that adds 2 fields {namespace,alias} to make the accessible to fieldsBuilder etc

@Blackbarbie8
Copy link

@axe-me
Copy link

axe-me commented Dec 7, 2023

If the request body can include the restful URL and params, it will help a lot for the auth server to validate the request from some third-party webhook callbacks. can we reopen this? Right now the body is just like :

{ headers: {...}, request: null }

ideally it looks like this:

{ headers: {...}, request: { path, params } }

@robertjdominguez
Copy link
Contributor

We've still #7910 open, @axe-me.

@axe-me
Copy link

axe-me commented Dec 7, 2023

@robertjdominguez thanks for replying. But I don't think these are the same issues. This one is more related to auth webhook side of things, which is more about missing the request information of payload sent from hasura engine for restified endpoint.

@axe-me
Copy link

axe-me commented Dec 7, 2023

I have no Haskell knowledge at all, but it seems we could modify this block of code to create and pass the extra request info:

AHPost handler -> do
(userInfo, authHeaders, handlerState, includeInternal, extraUserInfo) <- getInfo Nothing
(queryJSON, parsedReq) <-
runExcept (parseBody reqBody) `onLeft` \e -> do
logErrorAndResp (Just userInfo) requestId req (reqBody, Nothing) includeInternal origHeaders extraUserInfo (qErrModifier e)
res <- lift $ runHandler (_lsLogger appEnvLoggers) handlerState $ handler parsedReq
pure (res, userInfo, authHeaders, includeInternal, Just queryJSON, extraUserInfo)

@robertjdominguez
Copy link
Contributor

@robertjdominguez thanks for replying. But I don't think these are the same issues. This one is more related to auth webhook side of things, which is more about missing the request information of payload sent from hasura engine for restified endpoint.

Got it, @axe-me 👍

The team is heads-down on v3 at the moment, but I'll bring this up next week and see if someone has the bandwidth to take a look!

@axe-me
Copy link

axe-me commented Dec 8, 2023

thanks in advance! I put up a PR, but I'm nothing sure if this gonna work since I have zero knowledge about haskell before. #10049

@axe-me
Copy link

axe-me commented Dec 12, 2023

@robertjdominguez Hi Rob, just a kindly reminder here, do you think if you have any time to review my PR above this week? Don't want to be pushy, but this is kinda become a roadblock for our development.

@robertjdominguez
Copy link
Contributor

I totally get it @axe-me, and sorry to hear this is a blocker for you. I've reached out to the team and they're trying to prioritize the review with other work.

I'll update you as soon as I know more 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
k/enhancement New feature or improve an existing feature
Projects
None yet
Development

No branches or pull requests

5 participants
@fkowal @axe-me @robertjdominguez @Blackbarbie8 and others