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

refactor(authlify): get authlify token from header #326

Merged
merged 8 commits into from
Apr 4, 2022
Merged

Conversation

dwwoelfel
Copy link
Contributor

@dwwoelfel dwwoelfel commented Feb 16, 2022

Which problem is this pull request solving?

The graph token is now available via the x-nf-graph-token header.

This will work better across different frameworks and require less effort to support a new framework.

List other issues or pull requests related to this problem

https://github.com/netlify/proxy/pull/767

Describe the solution you've chosen

This is fully laid out in https://www.notion.so/netlify/Graph-token-location-876947e0084447b49df96ec11248dd03, but I'll briefly summarize below:

This gets the graph token from the new x-nf-graph-token header instead of the event when used in a function or ODB. When used in a build, we retrieve the token from the NETLIFY_GRAPH_TOKEN environment variable.

It also exposes a getNetlifyGraphToken function that users can use to get the token instead of relying on event.netlifyGraphToken, which isn't available in e.g. next.js

Checklist

Please add a x inside each checkbox:

  • I have read the contribution guidelines.
  • The status checks are successful (continuous integration). Those can be seen below.

@dwwoelfel
Copy link
Contributor Author

@ascorbic do people use @netlify/functions during their build steps?

We initially wanted to support getSecrets during build steps, but I don't think we've been emphasizing that and it causes problems using the same function for both the build and functions.

Is there a way for us to tell if anyone is using it during the build?

@ascorbic
Copy link
Contributor

@dwwoelfel No, it's not something people usually do, but there isn't any particular reason it would be a problem. That said, conceptually it seems a bit odd having the same function. Would it be needing env vars injected by us when it's used? Presumably that would mean it wouldn't locally except via the Netlify CLI: somebody running next build or gatsby build directly would not have access to those? I would suggest having a new function (perhaps getBuildSecrets), possibly in @netlify/build, that includes specific checks to see that it's running in a Netlify environment (i.e. in CI or via the Netlify CLI) and fails if not with a helpful error message. It can check for process.env.NETLIFY to see that.

Copy link
Contributor

@ascorbic ascorbic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far. Just a few points to make it more flexible mostly.

src/lib/onegraph_request.ts Outdated Show resolved Hide resolved
src/lib/onegraph_request.ts Outdated Show resolved Hide resolved
src/lib/secrets_helper.ts Outdated Show resolved Hide resolved
src/lib/secrets_helper.ts Outdated Show resolved Hide resolved
src/lib/secrets_helper.ts Outdated Show resolved Hide resolved
src/lib/secrets_helper.ts Outdated Show resolved Hide resolved
src/lib/secrets_helper.ts Outdated Show resolved Hide resolved
src/lib/secrets_helper.ts Outdated Show resolved Hide resolved
@dwwoelfel dwwoelfel added the type: chore work needed to keep the product and development running smoothly label Mar 29, 2022
src/lib/graph_token.ts Outdated Show resolved Hide resolved
export type HandlerEventWithOneGraph = HandlerEvent & OneGraphPayload
const logErrors = function (errors: GraphTokenResponseError[]) {
for (const error of errors) {
let errorMessage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we only changing the function name for the error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I thought of making a function to create the error message, but thought it was overkill.

@dwwoelfel dwwoelfel marked this pull request as ready for review March 29, 2022 22:15
@dwwoelfel dwwoelfel requested a review from ascorbic March 30, 2022 16:00
Copy link
Contributor

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kodiakhq kodiakhq bot merged commit 9019789 into main Apr 4, 2022
@kodiakhq kodiakhq bot deleted the dww/get-secrets branch April 4, 2022 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants