chore: inject the Authlify Token in `netlify dev` #4167

anmonteiro · 2022-02-02T02:00:38Z

🎉 Thanks for submitting a pull request! 🎉

Summary

Reported by @sgrove
mirrors what was done in https://github.com/netlify/proxy/pull/719 for local dev:
- in builds, inject the ONEGRAPH_AUTHLIFY_TOKEN variable
- in function invocations, adds authlifyToken to the event

For us to review and ship your PR efficiently, please perform the following steps:

Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
Read the contribution guidelines 📖. This ensures your code follows our style guide and
passes our tests.
Update or add tests (if any source code was changed or added) 🧪
Update or add documentation (if features were changed or added) 📝
Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

github-actions · 2022-02-02T02:01:28Z

📊 Benchmark results

Comparing with fc1c991

Package size: 362 MB

⬆️ 0.14% increase vs. fc1c991

^  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  360 MB  362 MB 
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T

Legend

T-30 (fc1c991): 361 MB
T-29 (a7c6aab): 361 MB
T-28 (c2f4f1d): 361 MB
T-27 (cc2c6dd): 361 MB
T-26 (3d40fe1): 361 MB
T-25 (1fcb0e3): 361 MB
T-24 (e763556): 361 MB
T-23 (be0bb11): 361 MB
T-22 (d6d99d3): 361 MB
T-21 (d5d421d): 361 MB
T-20 (432774f): 360 MB
T-19 (93b64cf): 360 MB
T-18 (cd7af03): 360 MB
T-17 (c8281ec): 360 MB
T-16 (54040d8): 360 MB
T-15 (1981515): 360 MB
T-14 (434cdb7): 360 MB
T-13 (c89d81a): 360 MB
T-12 (0550e60): 360 MB
T-11 (f1c4d10): 360 MB
T-10 (50b3483): 360 MB
T-9 (df33dd0): 360 MB
T-8 (a41323f): 360 MB
T-7 (b2b0faa): 360 MB
T-6 (7d65669): 360 MB
T-5 (1c8968d): 360 MB
T-4 (40b1cf9): 360 MB
T-3 (39eb031): 360 MB
T-2 (8720854): 360 MB
T-1 (7bb0322): 360 MB
T (current commit): 362 MB

anmonteiro · 2022-02-02T02:02:39Z

src/commands/dev/dev.js

+      authlifyJWT == null
+        ? {}
+        : {
+            ONEGRAPH_AUTHLIFY_TOKEN: {


follows the format expected by injectEnvVariables. Not sure if there's a helper to create this?

anmonteiro · 2022-02-02T02:03:31Z

src/lib/functions/server.js

@@ -105,6 +106,11 @@ const createHandler = function ({ functionsRegistry }) {
      rawQuery,
    }

+    if (config.authlify && config.authlify.authlifyTokenId != null) {


follows https://github.com/netlify/proxy/pull/719 by injecting the token whenever there's an authlify token ID, and not only when --graph is passed.

sgrove

The only concern I have is that we're exposing authlifyToken to end users when it should probably be netlifyGraphToken. Let's say that's fine for the beta period, and then migrate users off of it (well) before GA.

anmonteiro · 2022-02-03T18:53:34Z

Tests seem flakey and unrelated to my changes -- they fail in different OSes in different runs, for different tests 😢.

I'm planning on merging this later today if there are no objections. I'll follow up with a fix if indeed there are broken things.

EDIT: trying to get them to pass after #4164

erezrokah

Hi @sgrove and @anmonteiro.

I added 2 comments

erezrokah · 2022-02-03T19:00:04Z

src/commands/dev/dev.js

@@ -253,7 +254,29 @@ const dev = async (options, command) => {
    )
  }

-  await injectEnvVariables({ env: command.netlify.cachedConfig.env, site })
+  const startNetlifyGraphWatcher = Boolean(options.graph)
+  const netlifyToken = await command.authenticate()


This will open up a browser and authenticate if users are not logged in.
This is not the expected behavior for the CLI as ntl dev should not require it.

We can put this under the graph condition for now, but we should probably have better error reporting.
Users can also pass --offline to the dev command which should prevent any requests from being sent.

we'll need the netlify bearer token for the user. What's an alternative way of getting it?

That's the way to get it. We should only do it when running with graph I believe

There’s also the API Authentication use case. I can do it only in the presence of an authlify token ID, then. Appreciate the review!

erezrokah · 2022-02-03T19:06:06Z

src/commands/dev/dev.js

+      authlifyJWT == null
+        ? {}
+        : {
+            ONEGRAPH_AUTHLIFY_TOKEN: {


Where do we want ONEGRAPH_AUTHLIFY_TOKEN injected to?

The PR description says builds and functions.
This injects it into functions and dev command, but not builds

It's injected in builds via Bitballoon.

Are there local builds where I should inject the variable too?

The CLI has a build command

cli/src/commands/build/build.js

Line 24 in e763556

command.setAnalyticsPayload({ dry: options.dry })

The CLI gets all the configuration, including environment variables from netlify/config, so I wonder if it will be easier to implement this PR in that package.
See the code to retrieve site information in https://github.com/netlify/build/blob/dc76f4b27a9fe92395c7d20346ce1655f8340d8c/packages/config/src/api/site_info.js

I feel like this is more specific to the CLI than the generic config package.

@erezrokah are you on board if we merge this with the current functionality to unblock Mathias's upcoming talk and make the build improvements later?

…s on

erezrokah

Hi @anmonteiro, I'm good with unblocking.

I think it will great to follow up with a test case for this, do you mind opening an issue for it?

Do you mind opening another issue for supporting this in ntl build? If we inject the auth token in our build system, will should inject it to ntl build too.

erezrokah · 2022-02-04T12:09:34Z

This is released in https://github.com/netlify/cli/releases/tag/v8.15.5

anmonteiro · 2022-02-04T18:52:19Z

Thanks, @erezrokah! I opened #4182 and #4181 per your suggestions.

chore: inject the Authlify Token in netlify dev

b974325

github-actions bot added the type: chore label Feb 2, 2022

anmonteiro requested a review from sgrove February 2, 2022 02:01

anmonteiro commented Feb 2, 2022

View reviewed changes

anmonteiro added 2 commits February 1, 2022 19:23

fix: check if config is not undefined too

Verified

This commit was signed with the committer’s verified signature.

caendesilva Caen De Silva

GPG key ID: F1E5FB7C768BCE52

Verified
Learn about vigilant mode

adbac1c

fix: put the claims in the netlify namespace

Verified

This commit was signed with the committer’s verified signature.

caendesilva Caen De Silva

GPG key ID: F1E5FB7C768BCE52

Verified
Learn about vigilant mode

f778c78

sgrove previously approved these changes Feb 2, 2022

View reviewed changes

Merge branch 'main' into anmonteiro/authlify-token-in-dev

Verified

This commit was signed with the committer’s verified signature.

caendesilva Caen De Silva

GPG key ID: F1E5FB7C768BCE52

Verified
Learn about vigilant mode

1c8e7a7

anmonteiro added the automerge label Feb 3, 2022

erezrokah suggested changes Feb 3, 2022

View reviewed changes

anmonteiro removed the automerge label Feb 3, 2022

fix: only call command.authenticate() if API Auth or NetlifyGraph i…

Verified

This commit was signed with the committer’s verified signature.

caendesilva Caen De Silva

GPG key ID: F1E5FB7C768BCE52

Verified
Learn about vigilant mode

f4a5397

…s on

anmonteiro dismissed sgrove’s stale review via f4a5397 February 3, 2022 21:28

erezrokah enabled auto-merge (squash) February 4, 2022 10:32

erezrokah disabled auto-merge February 4, 2022 10:36

erezrokah enabled auto-merge (squash) February 4, 2022 10:36

erezrokah approved these changes Feb 4, 2022

View reviewed changes

Merge branch 'main' into anmonteiro/authlify-token-in-dev

85c4abd

erezrokah merged commit 4ffba32 into main Feb 4, 2022

erezrokah deleted the anmonteiro/authlify-token-in-dev branch February 4, 2022 10:51

token-generator-app bot mentioned this pull request Feb 4, 2022

chore(main): release 8.15.5 #4175

Merged

This was referenced Feb 4, 2022

Add tests for netlify graph + API Authentication #4181

Closed

Inject ONEGRAPH_AUTHLIFY_TOKEN when running ntl build #4182

Closed

token-generator-app bot mentioned this pull request May 26, 2022

chore(main): release 10.3.3 #4629

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: inject the Authlify Token in `netlify dev` #4167

chore: inject the Authlify Token in `netlify dev` #4167

anmonteiro commented Feb 2, 2022

github-actions bot commented Feb 2, 2022 •

edited

Loading

anmonteiro Feb 2, 2022

anmonteiro Feb 2, 2022

sgrove left a comment •

edited

Loading

anmonteiro commented Feb 3, 2022 •

edited

Loading

erezrokah left a comment

erezrokah Feb 3, 2022 •

edited

Loading

anmonteiro Feb 3, 2022

erezrokah Feb 3, 2022

anmonteiro Feb 3, 2022

erezrokah Feb 3, 2022

anmonteiro Feb 3, 2022

erezrokah Feb 3, 2022

anmonteiro Feb 3, 2022

erezrokah left a comment

erezrokah commented Feb 4, 2022

anmonteiro commented Feb 4, 2022

chore: inject the Authlify Token in netlify dev #4167

chore: inject the Authlify Token in netlify dev #4167

Conversation

anmonteiro commented Feb 2, 2022

Summary

github-actions bot commented Feb 2, 2022 • edited Loading

📊 Benchmark results

Package size: 362 MB

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgrove left a comment • edited Loading

Choose a reason for hiding this comment

anmonteiro commented Feb 3, 2022 • edited Loading

erezrokah left a comment

Choose a reason for hiding this comment

erezrokah Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erezrokah left a comment

Choose a reason for hiding this comment

erezrokah commented Feb 4, 2022

anmonteiro commented Feb 4, 2022

chore: inject the Authlify Token in `netlify dev` #4167

chore: inject the Authlify Token in `netlify dev` #4167

github-actions bot commented Feb 2, 2022 •

edited

Loading

sgrove left a comment •

edited

Loading

anmonteiro commented Feb 3, 2022 •

edited

Loading

erezrokah Feb 3, 2022 •

edited

Loading