Skip to content

test: e2e test website accountPayment loggedIn, includes magic.link testMode support in test-e2e job only#1754

Merged
gobengo merged 73 commits intomainfrom
e2e-authn
Aug 24, 2022
Merged

test: e2e test website accountPayment loggedIn, includes magic.link testMode support in test-e2e job only#1754
gobengo merged 73 commits intomainfrom
e2e-authn

Conversation

@gobengo
Copy link
Copy Markdown
Contributor

@gobengo gobengo commented Aug 16, 2022

Motivation

  • e2e test the logged in flow for /account/payment ui
  • we hadn't previously found a way to e2e test auth
  • playwright.dev's html reporter is awesome and lets you see console, network, screenshots of the whole e2e test! preview

What

  • add playwright-report (html) to ci artifacts. this is useful to debug failed e2e test runs (with screenshots, console, network recorded!)
  • add ability to opt-in to using magic.link testMode (not on by default, on in ci test-e2e)
  • test-e2e run has api running and db with loaded schema
  • test logged-in case of accountPayment

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 16, 2022

@gobengo gobengo changed the title e2e test accountPayment loggedIn test(website): e2e test accountPayment loggedIn Aug 16, 2022
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 16, 2022

package-lock.json changes

Click to toggle table visibility
Name Status Previous Current
@cspotcode/source-map-support ADDED - 0.8.1
@jridgewell/trace-mapping UPDATED 0.3.4 0.3.9
@tsconfig/node10 ADDED - 1.0.9
@tsconfig/node12 ADDED - 1.0.11
@tsconfig/node14 ADDED - 1.0.3
@tsconfig/node16 ADDED - 1.0.3
@typescript-eslint/parser ADDED - 5.33.1
@typescript-eslint/scope-manager ADDED - 5.33.1
@typescript-eslint/types ADDED - 5.33.1
@typescript-eslint/typescript-estree ADDED - 5.33.1
@typescript-eslint/visitor-keys ADDED - 5.33.1
acorn-walk ADDED - 8.2.0
arg ADDED - 4.1.3
create-require ADDED - 1.1.1
make-error ADDED - 1.3.6
ts-node ADDED - 10.9.1
tsutils ADDED - 3.21.0
v8-compile-cache-lib ADDED - 3.0.1
yn ADDED - 3.1.1

};

/**
* @param {Object} [props]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was required for next build to build locally for me with tsc, and makes sense to clarify that the argument destructuring wont work if props === undefined

@@ -1,5 +1,6 @@
const API = /** @type {string} **/ (process.env.NEXT_PUBLIC_API);
const MAGIC_TOKEN = /** @type {string} **/ (process.env.NEXT_PUBLIC_MAGIC);
const MAGIC_TESTMODE_ENABLED = Boolean(process.env.NEXT_PUBLIC_MAGIC_TESTMODE_ENABLED);
Copy link
Copy Markdown
Contributor Author

@gobengo gobengo Aug 23, 2022

Choose a reason for hiding this comment

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

I think that we're relying on miniflare/wrangler to replace this NEXT_PUBLIC_MAGIC_TESTMODE_ENABLED (and NEXT_PUBLIC_MAGIC) is one reason you have to explicitly tell miniflare about your env vars either through the env file or through the --binding KEY=VALUE flags I added when the website ci workflow invokes npm -w '@web3-storage/api' start. I think maybe miniflare devs would be hesitant to automatically pass everything through from miniflare env without an allowlist because it could lead to a developers secrets getting into the build without explicit configuration that thats what they'd want.

@gobengo
Copy link
Copy Markdown
Contributor Author

gobengo commented Aug 23, 2022

the main new thing I'd point out, which I think made the code clearer, was to reify the fact that that there are two distinct kinds of of 'magic.link bypasses'

  1. one we use only in unit tests, which allows a special allowlisted bearer token to work, but only when env.DANGEROUSLY_BYPASS_MAGIC_AUTH is truthy
  2. one we use only in e2e tests, which enables detection of any incoming Bearer tokens that are JWTs that look like a magic.link test mode token (it looks in jwt sub and aud), and doesnt bother to validate those tokens + ensures those tokens can be resolved to appropriate metadata for the rest of our 'user login' stuff to work, but only when env.NEXT_PUBLIC_MAGIC_TESTMODE_ENABLED (which also gets used on the frontend)
    • this bypass, when enabled, is also used in a controller used by the loginOrRegister route that handles magic.link tokens (because now those tokens can be testMode tokens but only when enabled via env var aka only in test-e2e ci)

@gobengo gobengo changed the title test: add magic.link testMode in ci, use it to e2e test website accountPayment loggedIn test: e2e test website accountPayment loggedIn, includes magic.link testMode support in test-e2e job only Aug 23, 2022
Copy link
Copy Markdown
Contributor

@yusefnapora yusefnapora left a comment

Choose a reason for hiding this comment

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

I don't see anything that looks dangerous here (at least, nothing that's not lampshaded with "dangerouslyDoTheThing" naming 👍 ), and I like the explicit naming for the different types of bypass.

I'm not super crazy about the env file rewriter script... but I'm guessing that we need it in order to allow miniflare to use the bindings set in CI? That seems like a reasonable justification, IMO.

It would also be rad to add a note to the dev docs / README about the bypass modes - @gobengo maybe you could grab that as a follow up when you're back from dweb camp? I don't think we need to block on it, but it seems worth a mention. I left a couple other non-blocking suggestions if you have time to grab.

Comment thread packages/api/src/magic.link.js Outdated
@@ -1,3 +1,5 @@
import { magicTestModeIsEnabledFromEnv, maybeJsonParseable } from './utils/env.js'

export const createMagicTestTokenBypass = (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can remove the export here and just export the concrete value magicLinkBypassForUnitTestingWithTestToken, that seems a bit cleaner. But if you need to export this for testing, that's okay by me...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment thread packages/website/scripts/envfile.ts
@gobengo
Copy link
Copy Markdown
Contributor Author

gobengo commented Aug 24, 2022

status: I believe checks are green except for GITHUB_TOKEN rate limits

@gobengo gobengo merged commit 6d6f92c into main Aug 24, 2022
@gobengo gobengo deleted the e2e-authn branch August 24, 2022 01:38
Comment on lines +28 to +30
assert.equal(fn({
NEXT_PUBLIC_MAGIC_TESTMODE_ENABLED: 'not-json-parseable'
}), true)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@gobengo having anything truth that isn't 'false' or '0' mean true seems like a foot gun. Please can we have exactly 1 valid way to enable test mode:

NEXT_PUBLIC_MAGIC_TESTMODE_ENABLED === 'true'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants