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

feat(adapter): take away error handling from adapters #1871

Merged
merged 13 commits into from
May 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions config/babel.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// We aim to have the same support as Next.js
// https://nextjs.org/docs/getting-started#system-requirements
// https://nextjs.org/docs/basic-features/supported-browsers-features

module.exports = {
presets: [["@babel/preset-env", { targets: { node: "10.13" } }]],
plugins: [
"@babel/plugin-proposal-class-properties",
"@babel/plugin-transform-runtime",
],
comments: false,
overrides: [
{
test: ["../src/client/**"],
presets: [["@babel/preset-env", { targets: { ie: "11" } }]],
},
{
test: ["../src/server/pages/**"],
presets: ["preact"],
},
],
}
15 changes: 0 additions & 15 deletions config/babel.config.json

This file was deleted.

395 changes: 386 additions & 9 deletions package-lock.json

Large diffs are not rendered by default.

6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@
},
"scripts": {
"build": "npm run build:js && npm run build:css",
"build:js": "node ./config/build.js && babel --config-file ./config/babel.config.json src --out-dir dist",
"build:js": "node ./config/build.js && babel --config-file ./config/babel.config.js src --out-dir dist",
"build:css": "postcss --config config/postcss.config.js src/**/*.css --base src --dir dist && node config/wrap-css.js",
"dev:setup": "npm run build:css && cd app && npm i",
"dev": "cd app && npm run dev",
"watch": "npm run watch:js | npm run watch:css",
"watch:js": "babel --config-file ./config/babel.config.json --watch src --out-dir dist",
"watch:js": "babel --config-file ./config/babel.config.js --watch src --out-dir dist",
"watch:css": "postcss --config config/postcss.config.js --watch src/**/*.css --base src --dir dist",
"test": "echo \"Write some tests...\"; npm run test:types",
"test:types": "dtslint types",
Expand All @@ -61,6 +61,7 @@
],
"license": "ISC",
"dependencies": {
"@babel/runtime": "^7.14.0",
"@next-auth/prisma-legacy-adapter": "canary",
"@next-auth/typeorm-legacy-adapter": "canary",
"crypto-js": "^4.0.0",
Expand Down Expand Up @@ -91,6 +92,7 @@
"@babel/cli": "^7.8.4",
"@babel/core": "^7.9.6",
"@babel/plugin-proposal-class-properties": "^7.13.0",
"@babel/plugin-transform-runtime": "^7.13.15",
"@babel/preset-env": "^7.9.6",
"@prisma/client": "^2.16.1",
"@semantic-release/commit-analyzer": "^8.0.1",
Expand Down
36 changes: 36 additions & 0 deletions src/adapters/error-handler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { UnknownError } from "../lib/errors"

/**
* Handles adapter induced errors.
* @param {import("types/adapters").AdapterInstance} adapter
* @param {import("types").LoggerInstance} logger
* @return {import("types/adapters").AdapterInstance}
*/
export default function adapterErrorHandler(adapter, logger) {
return Object.keys(adapter).reduce((acc, method) => {
const name = capitalize(method)
const code = upperSnake(name, adapter.displayName)

const adapterMethod = adapter[method]
acc[method] = async (...args) => {
try {
logger.debug(code, ...args)
return await adapterMethod(...args)
Copy link
Member Author

Choose a reason for hiding this comment

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

should log out args with debug, and an adapter prefix(?)

@kripod, currently logger.debug is rewritten in adapters to include a prefix for code. Do you think we could return a name property next to getAdapter for this purpose?

any thoughts?

Copy link
Contributor

@kripod kripod Apr 30, 2021

Choose a reason for hiding this comment

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

We may specify an attribute like { displayName: string } as the minimum required config param when instantiating adapters. By doing so, we would also solve the typing issue of Adapter(requiredParam, …).

Copy link
Member Author

Choose a reason for hiding this comment

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

After my recent commit, it will be possible for adapters to return a displayName property in getAdapter. Unfortunately we don't have the adapter configuration when we invoke the .getAdapter() method.

} catch (error) {
logger.error(`${code}_ERROR`, error)
const e = new UnknownError(error)
e.name = `${name}Error`
throw e
}
}
return acc
}, {})
}

function capitalize(s) {
return `${s[0].toUpperCase()}${s.slice(1)}`
}

function upperSnake(s, prefix = "ADAPTER") {
return `${prefix}_${s.replace(/([A-Z])/g, "_$1")}`.toUpperCase()
}
75 changes: 50 additions & 25 deletions src/server/lib/callback-handler.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { AccountNotLinkedError } from '../../lib/errors'
import dispatchEvent from '../lib/dispatch-event'
import { AccountNotLinkedError } from "../../lib/errors"
import dispatchEvent from "../lib/dispatch-event"
import adapterErrorHandler from "../../adapters/error-handler"

/**
* This function handles the complex flow of signing users in, and either creating,
Expand All @@ -12,20 +13,29 @@ import dispatchEvent from '../lib/dispatch-event'
* All verification (e.g. OAuth flows or email address verificaiton flows) are
* done prior to this handler being called to avoid additonal complexity in this
* handler.
* @param {import("types").Session} sessionToken
* @param {import("types").Profile} profile
* @param {import("types").Account} account
* @param {import("types/internals").AppOptions} options
*/
export default async function callbackHandler (sessionToken, profile, providerAccount, options) {
export default async function callbackHandler(
sessionToken,
profile,
providerAccount,
options
) {
// Input validation
if (!profile) throw new Error('Missing profile')
if (!providerAccount?.id || !providerAccount.type) throw new Error('Missing or invalid provider account')
if (!['email', 'oauth'].includes(providerAccount.type)) throw new Error('Provider not supported')
if (!profile) throw new Error("Missing profile")
if (!providerAccount?.id || !providerAccount.type)
throw new Error("Missing or invalid provider account")
if (!["email", "oauth"].includes(providerAccount.type))
throw new Error("Provider not supported")

const {
adapter,
jwt,
events,
session: {
jwt: useJwtSession
}
session: { jwt: useJwtSession },
} = options

// If no adapter is configured then we don't have a database and cannot
Expand All @@ -34,7 +44,7 @@ export default async function callbackHandler (sessionToken, profile, providerAc
return {
user: profile,
account: providerAccount,
session: {}
session: {},
}
}

Expand All @@ -47,8 +57,8 @@ export default async function callbackHandler (sessionToken, profile, providerAc
linkAccount,
createSession,
getSession,
deleteSession
} = await adapter.getAdapter(options)
deleteSession,
} = adapterErrorHandler(await adapter.getAdapter(options), options.logger)

let session = null
let user = null
Expand All @@ -74,9 +84,11 @@ export default async function callbackHandler (sessionToken, profile, providerAc
}
}

if (providerAccount.type === 'email') {
if (providerAccount.type === "email") {
// If signing in with an email, check if an account with the same email address exists already
const userByEmail = profile.email ? await getUserByEmail(profile.email) : null
const userByEmail = profile.email
? await getUserByEmail(profile.email)
: null
if (userByEmail) {
// If they are not already signed in as the same user, this flow will
// sign them out of the current session and sign them in as the new user
Expand Down Expand Up @@ -107,11 +119,14 @@ export default async function callbackHandler (sessionToken, profile, providerAc
return {
session,
user,
isNewUser
isNewUser,
}
} else if (providerAccount.type === 'oauth') {
} else if (providerAccount.type === "oauth") {
// If signing in with oauth account, check to see if the account exists already
const userByProviderAccountId = await getUserByProviderAccountId(providerAccount.provider, providerAccount.id)
const userByProviderAccountId = await getUserByProviderAccountId(
providerAccount.provider,
providerAccount.id
)
if (userByProviderAccountId) {
if (isSignedIn) {
// If the user is already signed in with this account, we don't need to do anything
Expand All @@ -122,7 +137,7 @@ export default async function callbackHandler (sessionToken, profile, providerAc
return {
session,
user,
isNewUser
isNewUser,
}
}
// If the user is currently signed in, but the new account they are signing in
Expand All @@ -132,11 +147,13 @@ export default async function callbackHandler (sessionToken, profile, providerAc
}
// If there is no active session, but the account being signed in with is already
// associated with a valid user then create session to sign the user in.
session = useJwtSession ? {} : await createSession(userByProviderAccountId)
session = useJwtSession
? {}
: await createSession(userByProviderAccountId)
return {
session,
user: userByProviderAccountId,
isNewUser
isNewUser,
}
} else {
if (isSignedIn) {
Expand All @@ -151,13 +168,16 @@ export default async function callbackHandler (sessionToken, profile, providerAc
providerAccount.accessToken,
providerAccount.accessTokenExpires
)
await dispatchEvent(events.linkAccount, { user, providerAccount: providerAccount })
await dispatchEvent(events.linkAccount, {
user,
providerAccount: providerAccount,
})

// As they are already signed in, we don't need to do anything after linking them
return {
session,
user,
isNewUser
isNewUser,
}
}

Expand All @@ -178,7 +198,9 @@ export default async function callbackHandler (sessionToken, profile, providerAc
//
// OAuth providers should require email address verification to prevent this, but in
// practice that is not always the case; this helps protect against that.
const userByEmail = profile.email ? await getUserByEmail(profile.email) : null
const userByEmail = profile.email
? await getUserByEmail(profile.email)
: null
if (userByEmail) {
// We end up here when we don't have an account with the same [provider].id *BUT*
// we do already have an account with the same email address as the one in the
Expand Down Expand Up @@ -207,14 +229,17 @@ export default async function callbackHandler (sessionToken, profile, providerAc
providerAccount.accessToken,
providerAccount.accessTokenExpires
)
await dispatchEvent(events.linkAccount, { user, providerAccount: providerAccount })
await dispatchEvent(events.linkAccount, {
user,
providerAccount: providerAccount,
})

session = useJwtSession ? {} : await createSession(user)
isNewUser = true
return {
session,
user,
isNewUser
isNewUser,
}
}
}
Expand Down
36 changes: 29 additions & 7 deletions src/server/lib/signin/email.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,44 @@
import { randomBytes } from 'crypto'
import { randomBytes } from "crypto"
import adapterErrorHandler from "../../../adapters/error-handler"

export default async function email (email, provider, options) {
/**
*
* @param {string} email
* @param {import("types/providers").EmailConfig} provider
* @param {import("types/internals").AppOptions} options
* @returns
*/
export default async function email(email, provider, options) {
try {
const { baseUrl, basePath, adapter } = options
const { baseUrl, basePath, adapter, logger } = options

const { createVerificationRequest } = await adapter.getAdapter(options)
const { createVerificationRequest } = adapterErrorHandler(
await adapter.getAdapter(options),
logger
)

// Prefer provider specific secret, but use default secret if none specified
const secret = provider.secret || options.secret

// Generate token
const token = await provider.generateVerificationToken?.() ?? randomBytes(32).toString('hex')
const token =
(await provider.generateVerificationToken?.()) ??
randomBytes(32).toString("hex")

// Send email with link containing token (the unhashed version)
const url = `${baseUrl}${basePath}/callback/${encodeURIComponent(provider.id)}?email=${encodeURIComponent(email)}&token=${encodeURIComponent(token)}`
const url = `${baseUrl}${basePath}/callback/${encodeURIComponent(
provider.id
)}?email=${encodeURIComponent(email)}&token=${encodeURIComponent(token)}`

// @TODO Create invite (send secret so can be hashed)
await createVerificationRequest(email, url, token, secret, provider, options)
await createVerificationRequest(
email,
url,
token,
secret,
provider,
options
)

// Return promise
return Promise.resolve()
Expand Down
Loading