feat: monitor whether API Authentication has been enabled #4235
Conversation
anmonteiro
added
the
type: feature
📊 Benchmark resultsComparing with cb5f302 Package size: 438 MB⬇️ 0.00% decrease vs. cb5f302 Legend
|
anmonteiro
changed the title
| // XXX(anmonteiro): this name is deprecated. Delete after 3/31/2022 | ||
| const jwt = generateNetlifyGraphJWT(config.netlifyGraphConfig) | ||
| event.authlifyToken = jwt | ||
| event.netlifyGraphToken = jwt |
There was a problem hiding this comment.
We should get the changes ported to nf-server and proxy soon as well
There was a problem hiding this comment.
Similarly, anywhere we inject ONEGRAPH_AUTHLIFY_TOKEN in the env we should also inject NETLIFY_GRAPH_TOKEN so we're future-proof for the public launch.
But that can be a follow-on PR, best if it isn't done here.
|
@anmonteiro Looks like some relevant tests might have been affected. |
src/lib/functions/server.js
Outdated
| // performance optimization, load express on demand | ||
| // eslint-disable-next-line node/global-require | ||
| const express = require('express') | ||
| // eslint-disable-next-line node/global-require | ||
| const expressLogging = require('express-logging') | ||
| const app = express() | ||
| const functionHandler = createHandler({ config, functionsRegistry }) | ||
| const functionHandler = await createHandler(options) |
There was a problem hiding this comment.
createHandler is not async so this await seems redundant? unless we should be awaiting startPollingForAPIAuthentication here https://github.com/netlify/cli/pull/4235/files#diff-de106b743be68ae2e9146d09931f894e258783a68db916a6d0e39e3908b28c84R90
There was a problem hiding this comment.
This might be an artifact from a previous version of the code. Let me see if it can be safely removd.
| delete config.authlify | ||
| } | ||
|
|
||
| setTimeout(helper, frequency) |
There was a problem hiding this comment.
Have we considered using https://github.com/sindresorhus/p-wait-for#p-wait-for to abstract the polling part?
There was a problem hiding this comment.
As an aside, isn't it better to reduce the number of dependencies, especially for code that's so small like this and does what we need? It'll help cut down on renovate PRs, ongoing security issues, etc.
There was a problem hiding this comment.
We already have it as a dependency so package size will remain the same
Line 283 in 7251872
Whether we should inline dependencies or not has been discussed before, and you can see a good explanation on the reasons we've chosen to prefer well maintained, well tested, small, single purpose packages here.
I don't believe inlining the code will reduce security issues. Using a well maintained package ensures any security issue is reported and known to us. I don't believe that would be the case when inlining.
Happy to discuss this again in an issue though if you'd like to open one.
There was a problem hiding this comment.
This sounds like a good improvement to make as a follow-up PR. I'm opening an issue to change our pollers in Graph to use p-wait-for.
…tlify/cli into anmonteiro/monitor-api-authentication
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #4226
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)