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

[Bug?]: getAuthProviderHeader does not support getting headers from a plain Request object #11649

Open
1 task done
cdubz opened this issue Oct 3, 2024 · 6 comments
Open
1 task done
Labels
bug/needs-info More information is needed for reproduction

Comments

@cdubz
Copy link

cdubz commented Oct 3, 2024

What's not working?

The current definition of getAuthProviderHeader says it supports getting the auth provider header from a APIGatewayProxyEvent or Request:

export const getAuthProviderHeader = (
  event: APIGatewayProxyEvent | Request,
) => {

This works as expected for APIGatewayProxyEvent, but not for Request.

How do we reproduce the bug?

import { getAuthProviderHeader } from '@redwoodjs/api'

const testRequest = new Request('https://redwoodjs.com/')
testRequest.headers.append('auth-provider', 'test')
const authProvider = getAuthProviderHeader(request))

In the above example, authProvider should be test, but it is undefined.

What's your environment? (If it applies)

System:
    OS: Linux 6.10 Debian GNU/Linux 11 (bullseye) 11 (bullseye)
    Shell: 5.1.4 - /bin/bash
  Binaries:
    Node: 20.14.0 - /tmp/xfs-54a3a8c7/node
    Yarn: 4.4.0 - /tmp/xfs-54a3a8c7/yarn
  npmPackages:
    @redwoodjs/auth-dbauth-setup: 8.3.0 => 8.3.0 
    @redwoodjs/cli-data-migrate: 8.3.0 => 8.3.0 
    @redwoodjs/cli-storybook-vite: 8.3.0 => 8.3.0 
    @redwoodjs/core: 8.3.0 => 8.3.0 
    @redwoodjs/realtime: 8.3.0 => 8.3.0 
    @redwoodjs/studio: 12.0.0 => 12.0.0 
  redwood.toml:
    [api]
    port = 8911
    [browser]
    open = false
    [experimental.opentelemetry]
    enabled = false
    [notifications]
    versionUpdates = ["latest"]
    [studio.graphiql.authImpersonation]
    authProvider = "dbAuth"
    email = "${AUTH_IMPERSONATION_USER_EMAIL}"
    userId = "${AUTH_IMPERSONATION_USER_ID}"
    [web]
    title = "Local Public"
    port = 8910
    apiUrl = "${REDWOOD_API_URL}"
    sourceMap = true
    includeEnvironmentVariables = [
      "APP_ENV",
      "CLOUDFLARE_IMAGES_URL",
      "PBS_OAUTH_BASE_URL",
      "PBS_OAUTH_CLIENT_ID",
      "PUBLIC_API_URL",
      "SENTRY_DSN",
    ]

Are you interested in working on this?

  • I'm interested in working on this
@cdubz cdubz added the bug/needs-info More information is needed for reproduction label Oct 3, 2024
@dthyresson
Copy link
Contributor

dthyresson commented Oct 3, 2024

Hi @cdubz I dug into the code and it might be that underneath we're looking for a WHATWG Request vs http or Node Request. See:

import { Headers, Request as PonyfillRequest } from '@whatwg-node/fetch'

import { Headers, Request as PonyfillRequest } from '@whatwg-node/fetch'

It could be that when getting the event header here

export const isFetchApiRequest = (
perhaps the Request is of a different type?

Worth trying?

If so, we should fix that type to be clearer.

@cdubz
Copy link
Author

cdubz commented Oct 3, 2024

Ah, yeah I didn't notice that.

We're integrating the useResponseCache plugin for the GraphQL handler and that is giving us a Node Request at the point where we want to set up a session ID: https://github.com/dotansimha/graphql-yoga/blob/9efdc3831a517890dbb00c783d6da86c6a6d9b4e/packages/plugins/response-cache/src/index.ts#L28

We're having to get the auth provider here because we are using a custom provider alongside dbAuth.

It does work for us with getEventHeader but only if we manually put in Auth-Provider because the AUTH_PROVIDER_HEADER const is all lowercase and getEventHeader only supports either of the provided casing or all lowercase

import { getEventHeader } from '@redwoodjs/api'
const authProvider = getEventHeader(request, 'Auth-Provider')

Re: isFetchApiRequest -- we don't get to it because getAuthProviderHeader doesn't find the header.

I was wondering if it would make sense to adjust getEventHeader to support any casing for headers (e.g., make Auth-Provider, auth-provider, AUTH-PROVIDER all match) and then just use that in getAuthProviderHeader instead of the Object.keys handling? Maybe I'm missing some other issue there.

@dthyresson
Copy link
Contributor

dthyresson commented Oct 3, 2024

We're integrating the useResponseCache plugin for the GraphQL handler

Interesting -- the reason we use the WHATWG request is in fact because that's what the Guild uses in all GraphQL Yoga.

This https://github.com/ardatan/whatwg-node is managed by Arda from The Guild -- so I'd have though them to be compatible.

I'll ask.

I do also see: https://github.com/dotansimha/graphql-yoga/blob/9efdc3831a517890dbb00c783d6da86c6a6d9b4e/packages/plugins/response-cache/src/index.ts#L134

        const operationId = request.headers.get('If-None-Match');

Maybe it this case since it is custom auth, it makes sense just to get the raw request headers and authorize?

That said when I look at:

export const getAuthProviderHeader = (
  event: APIGatewayProxyEvent | Request,
) => {
  const authProviderKey = Object.keys(event?.headers ?? {}).find(
    (key) => key.toLowerCase() === AUTH_PROVIDER_HEADER,
  )
  if (authProviderKey) {
    return getEventHeader(event, authProviderKey)
  }
  return undefined
}

and your

const authProvider = getEventHeader(request, 'Auth-Provider')

Seems like should work.

Have you tried:

const testRequest = new Request('https://redwoodjs.com/', {
  headers: {
    'auth-provider': 'test',
  },
});

or

testRequest.headers.set('auth-provider', 'test');

But append should mutate the headers in place so kind of scratching my head why.

I still think just getting headers on own for this custom auth may make sense -- at least add a workaround? until we can identify the cause and fix? Maybe?

@cdubz
Copy link
Author

cdubz commented Oct 7, 2024

To be clear -- getAuthProviderHeader should indeed work. The issue is just that it doesn't work with a plain NodeJS request object.

The alternative approach uses getEventHeader which looks like this:

export const getEventHeader = (
  event: APIGatewayProxyEvent | Request,
  headerName: string,
) => {
  if (isFetchApiRequest(event)) {
    return event.headers.get(headerName)
  }

  return event.headers[headerName] || event.headers[headerName.toLowerCase()]
}

It is only checking for headerName exactly as provided or all lower case. So

const authProvider = getEventHeader(request, 'Auth-Provider')

works fine, but

const authProvider = getEventHeader(request, AUTH_PROVIDER_HEADER)

does not work (if the header is anything other than exactly auth-provider).

It's a minor thing really, but it might be nice to adjust getEventHeader to do something more like getAuthProviderHeader:

export const getEventHeader = (
  event: APIGatewayProxyEvent | Request,
  headerName: string,
) => {
  if (isFetchApiRequest(event)) {
    return event.headers.get(headerName)
  }

  const headerKey = Object.keys(event?.headers ?? {}).find(
    (key) => key.toLowerCase() === headerName.toLowerCase(),
  )

  return headerKey ? event.headers[headerKey] : undefined
}

And from there getAuthProviderHeader could even be adjusted to just do this:

export const getAuthProviderHeader = (
  event: APIGatewayProxyEvent | Request,
) => getEventHeader(event, AUTH_PROVIDER_HEADER)

This would make things work regardless of the casing of the passed in headerName (I think, didn't actually test 😁).

I still think just getting headers on own for this custom auth may make sense -- at least add a workaround? until we can identify the cause and fix? Maybe?

Yes, absolutely. That is what I'm doing and it's really fine. Just trying to think if we can improve these helpful utilities.

@dthyresson
Copy link
Contributor

Just trying to think if we can improve these helpful utilities.

Agreed! Any many thanks for the feedback and suggestions.

I like this change and makes sense --- and tons cleaner.

I'll share with team!

image

@cdubz
Copy link
Author

cdubz commented Oct 7, 2024

Sounds good! Note I just made a small edit to fix my example code. I was returning the key name not it's value 🤦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/needs-info More information is needed for reproduction
Projects
None yet
Development

No branches or pull requests

2 participants