From 40aacf9279c00c4e3c27669fc38a0ca196a788a4 Mon Sep 17 00:00:00 2001 From: Manish Dhanwal <104636306+ManishDhanwal07@users.noreply.github.com> Date: Wed, 22 Mar 2023 12:53:49 +0100 Subject: [PATCH] feat(core): Make OAuth2 error handling consistent with success handling (#5555) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- packages/cli/package.json | 1 + packages/cli/src/Server.ts | 5 ++ .../src/credentials/oauth2Credential.api.ts | 48 +++++++----------- .../templates/oauth-error-callback.handlebars | 19 +++++++ pnpm-lock.yaml | 49 +++++++++++++++++-- 5 files changed, 87 insertions(+), 35 deletions(-) create mode 100644 packages/cli/templates/oauth-error-callback.handlebars diff --git a/packages/cli/package.json b/packages/cli/package.json index f4523f6f2e..5ac94cf153 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -141,6 +141,7 @@ "curlconverter": "^3.0.0", "dotenv": "^8.0.0", "express": "^4.18.2", + "express-handlebars": "^7.0.2", "express-async-errors": "^3.1.1", "express-openapi-validator": "^4.13.6", "express-prom-bundle": "^6.6.0", diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index 92a7632b2e..9fd680772d 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -19,6 +19,7 @@ import { createHmac } from 'crypto'; import { promisify } from 'util'; import cookieParser from 'cookie-parser'; import express from 'express'; +import { engine as expressHandlebars } from 'express-handlebars'; import type { ServeStaticOptions } from 'serve-static'; import type { FindManyOptions } from 'typeorm'; import { Not, In } from 'typeorm'; @@ -183,6 +184,10 @@ class Server extends AbstractServer { constructor() { super(); + this.app.engine('handlebars', expressHandlebars({ defaultLayout: false })); + this.app.set('view engine', 'handlebars'); + this.app.set('views', TEMPLATES_DIR); + this.loadNodesAndCredentials = Container.get(LoadNodesAndCredentials); this.credentialTypes = Container.get(CredentialTypes); this.nodeTypes = Container.get(NodeTypes); diff --git a/packages/cli/src/credentials/oauth2Credential.api.ts b/packages/cli/src/credentials/oauth2Credential.api.ts index 06a48b110b..a8fd84cef7 100644 --- a/packages/cli/src/credentials/oauth2Credential.api.ts +++ b/packages/cli/src/credentials/oauth2Credential.api.ts @@ -11,7 +11,6 @@ import type { WorkflowExecuteMode, INodeCredentialsDetails, ICredentialsEncrypted, - IDataObject, } from 'n8n-workflow'; import { LoggerProxy } from 'n8n-workflow'; import { resolve as pathResolve } from 'path'; @@ -112,7 +111,7 @@ oauth2CredentialController.get( ); const token = new Csrf(); - // Generate a CSRF prevention token and send it as a OAuth2 state stringma/ERR + // Generate a CSRF prevention token and send it as an OAuth2 state string const csrfSecret = token.secretSync(); const state = { token: token.create(csrfSecret), @@ -174,6 +173,9 @@ oauth2CredentialController.get( }), ); +const renderCallbackError = (res: express.Response, errorMessage: string) => + res.render('oauth-error-callback', { error: { message: errorMessage } }); + /** * GET /oauth2-credential/callback * @@ -188,12 +190,12 @@ oauth2CredentialController.get( const { code, state: stateEncoded } = req.query; if (!code || !stateEncoded) { - const errorResponse = new ResponseHelper.ServiceUnavailableError( + return renderCallbackError( + res, `Insufficient parameters for OAuth2 callback. Received following query parameters: ${JSON.stringify( req.query, )}`, ); - return ResponseHelper.sendErrorResponse(res, errorResponse); } let state; @@ -203,31 +205,21 @@ oauth2CredentialController.get( token: string; }; } catch (error) { - const errorResponse = new ResponseHelper.ServiceUnavailableError( - 'Invalid state format returned', - ); - return ResponseHelper.sendErrorResponse(res, errorResponse); + return renderCallbackError(res, 'Invalid state format returned'); } const credential = await getCredentialWithoutUser(state.cid); if (!credential) { - LoggerProxy.error('OAuth2 callback failed because of insufficient permissions', { + const errorMessage = 'OAuth2 callback failed because of insufficient permissions'; + LoggerProxy.error(errorMessage, { userId: req.user?.id, credentialId: state.cid, }); - const errorResponse = new ResponseHelper.NotFoundError( - RESPONSE_ERROR_MESSAGES.NO_CREDENTIAL, - ); - return ResponseHelper.sendErrorResponse(res, errorResponse); + return renderCallbackError(res, errorMessage); } - let encryptionKey: string; - try { - encryptionKey = await UserSettings.getEncryptionKey(); - } catch (error) { - throw new ResponseHelper.InternalServerError((error as IDataObject).message as string); - } + const encryptionKey = await UserSettings.getEncryptionKey(); const mode: WorkflowExecuteMode = 'internal'; const timezone = config.getEnv('generic.timezone'); @@ -251,14 +243,12 @@ oauth2CredentialController.get( decryptedDataOriginal.csrfSecret === undefined || !token.verify(decryptedDataOriginal.csrfSecret as string, state.token) ) { - LoggerProxy.debug('OAuth2 callback state is invalid', { + const errorMessage = 'The OAuth2 callback state is invalid!'; + LoggerProxy.debug(errorMessage, { userId: req.user?.id, credentialId: state.cid, }); - const errorResponse = new ResponseHelper.NotFoundError( - 'The OAuth2 callback state is invalid!', - ); - return ResponseHelper.sendErrorResponse(res, errorResponse); + return renderCallbackError(res, errorMessage); } let options = {}; @@ -298,12 +288,12 @@ oauth2CredentialController.get( } if (oauthToken === undefined) { - LoggerProxy.error('OAuth2 callback failed: unable to get access tokens', { + const errorMessage = 'Unable to get OAuth2 access tokens!'; + LoggerProxy.error(errorMessage, { userId: req.user?.id, credentialId: state.cid, }); - const errorResponse = new ResponseHelper.NotFoundError('Unable to get access tokens!'); - return ResponseHelper.sendErrorResponse(res, errorResponse); + return renderCallbackError(res, errorMessage); } if (decryptedDataOriginal.oauthTokenData) { @@ -336,9 +326,7 @@ oauth2CredentialController.get( return res.sendFile(pathResolve(TEMPLATES_DIR, 'oauth-callback.html')); } catch (error) { - // Error response - // eslint-disable-next-line @typescript-eslint/no-unsafe-argument - return ResponseHelper.sendErrorResponse(res, error); + return renderCallbackError(res, (error as Error).message); } }, ); diff --git a/packages/cli/templates/oauth-error-callback.handlebars b/packages/cli/templates/oauth-error-callback.handlebars new file mode 100644 index 0000000000..38af48fd82 --- /dev/null +++ b/packages/cli/templates/oauth-error-callback.handlebars @@ -0,0 +1,19 @@ + + + n8n - OAuth Callback + + + + {{#if error}} +

Error:

+ 
{{error.message}}
+ {{/if}} + Failed to connect. The window can be closed now. + + + diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 036b98883b..affaa1999f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -182,6 +182,7 @@ importers: dotenv: ^8.0.0 express: ^4.18.2 express-async-errors: ^3.1.1 + express-handlebars: ^7.0.2 express-openapi-validator: ^4.13.6 express-prom-bundle: ^6.6.0 fast-glob: ^3.2.5 @@ -283,6 +284,7 @@ importers: dotenv: 8.6.0 express: 4.18.2 express-async-errors: 3.1.1_express@4.18.2 + express-handlebars: 7.0.2 express-openapi-validator: 4.13.8 express-prom-bundle: 6.6.0_prom-client@13.2.0 fast-glob: 3.2.12 @@ -11095,6 +11097,15 @@ packages: express: 4.18.2 dev: false + /express-handlebars/7.0.2: + resolution: {integrity: sha512-W7QuI8i4jjxGNqZJebXzugHuJV32uGTMWqSzqTKsgBTPVsWR3oYR2b9VNQ53vmWbUH1G8r/9GIO3WCqa1Myf1w==} + engines: {node: '>=v16'} + dependencies: + glob: 9.3.1 + graceful-fs: 4.2.10 + handlebars: 4.7.7 + dev: false + /express-openapi-validator/4.13.8: resolution: {integrity: sha512-89/sdkq+BKBuIyykaMl/vR9grFc3WFUPTjFo0THHbu+5g+q8rA7fKeoMfz+h84yOQIBcztmJ5ZJdk5uhEls31A==} dependencies: @@ -12026,6 +12037,16 @@ packages: minimatch: 5.1.5 once: 1.4.0 + /glob/9.3.1: + resolution: {integrity: sha512-qERvJb7IGsnkx6YYmaaGvDpf77c951hICMdWaFXyH3PlVob8sbPJJyJX0kWkiCWyXUzoy9UOTNjGg0RbD8bYIw==} + engines: {node: '>=16 || 14 >=14.17'} + dependencies: + fs.realpath: 1.0.0 + minimatch: 7.4.2 + minipass: 4.2.5 + path-scurry: 1.6.2 + dev: false + /global-dirs/3.0.0: resolution: {integrity: sha512-v8ho2DS5RiCjftj1nD9NmnfaOzTdud7RRnVd9kFNOjqZbISlx5DQ+OrTkywgd0dIt7oFCvKetZSHoHcP3sDdiA==} engines: {node: '>=10'} @@ -14968,6 +14989,11 @@ packages: dependencies: yallist: 4.0.0 + /lru-cache/7.18.3: + resolution: {integrity: sha512-jumlc0BIUrS3qJGgIkWZsyfAM7NCWiBcCDhnd+3NNM5KbBmLTgHVfWBcg6W+rLUsIpzpERPsvwUP7CckAQSOoA==} + engines: {node: '>=12'} + dev: false + /lru-memoizer/2.1.4: resolution: {integrity: sha512-IXAq50s4qwrOBrXJklY+KhgZF+5y98PDaNo0gi/v2KQBFLyWr+JyFvijZXkGKjQj/h9c0OwoE+JZbwUXce76hQ==} dependencies: @@ -15312,6 +15338,13 @@ packages: dependencies: brace-expansion: 2.0.1 + /minimatch/7.4.2: + resolution: {integrity: sha512-xy4q7wou3vUoC9k1xGTXc+awNdGaGVHtFUaey8tiX4H1QRc04DZ/rmDFwNm2EBsuYEhAZ6SgMmYf3InGY6OauA==} + engines: {node: '>=10'} + dependencies: + brace-expansion: 2.0.1 + dev: false + /minimist/1.2.7: resolution: {integrity: sha512-bzfL1YUZsP41gmu/qjrEk0Q6i2ix/cVeAhbCbqH9u3zYutS1cLg00qhrD0M2MVdCcx4Sc0UpP2eBWo9rotpq6g==} @@ -15365,11 +15398,9 @@ packages: dependencies: yallist: 4.0.0 - /minipass/4.0.0: - resolution: {integrity: sha512-g2Uuh2jEKoht+zvO6vJqXmYpflPqzRBT+Th2h01DKh5z7wbY/AZ2gCQ78cP70YoHPyFdY30YBV5WxgLOEwOykw==} + /minipass/4.2.5: + resolution: {integrity: sha512-+yQl7SX3bIT83Lhb4BVorMAHVuqsskxRdlmO9kTpyukp8vsm2Sn/fUOV9xlnG8/a5JsypJzap21lz/y3FBMJ8Q==} engines: {node: '>=8'} - dependencies: - yallist: 4.0.0 /minizlib/2.1.2: resolution: {integrity: sha512-bAxsR8BVfj60DWXHE3u30oHzfl4G7khkSuPW+qvpd7jFRHm7dLxOjUk1EHACJ/hxLY8phGJ0YhYHZo7jil7Qdg==} @@ -16597,6 +16628,14 @@ packages: path-root-regex: 0.1.2 dev: true + /path-scurry/1.6.2: + resolution: {integrity: sha512-J6MQNh56h6eHFY3vsQ+Lq+zKPwn71POieutmVt2leU8W+zz8HVIdJyn3I3Zs6IKbIQtuKXirVjTBFNBcbFO44Q==} + engines: {node: '>=16 || 14 >=14.17'} + dependencies: + lru-cache: 7.18.3 + minipass: 4.2.5 + dev: false + /path-to-regexp/0.1.7: resolution: {integrity: sha512-5DFkuoqlv1uYQKxy8omFBeJPQcdoE07Kv2sferDCrAq1ohOU+MSDswDIbnx3YAM60qIOnYa53wBhXW0EbMonrQ==} @@ -19523,7 +19562,7 @@ packages: dependencies: chownr: 2.0.0 fs-minipass: 2.1.0 - minipass: 4.0.0 + minipass: 4.2.5 minizlib: 2.1.2 mkdirp: 1.0.4 yallist: 4.0.0