From dc0055885543d195b3bc7278a541a22a66bc26b5 Mon Sep 17 00:00:00 2001 From: Douglas Muraoka Date: Tue, 2 Jul 2019 17:49:49 -0300 Subject: [PATCH 1/5] GraphQL: Improve session token error message Fixes the session token related error messages during GraphQL operations. If any authentication error were thrown, it was not correctly handled by the GraphQL express middleware, and ended responding the request with a JSON parsing error. --- src/GraphQL/ParseGraphQLServer.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/GraphQL/ParseGraphQLServer.js b/src/GraphQL/ParseGraphQLServer.js index 5cb4c1c747..82bd27d76a 100644 --- a/src/GraphQL/ParseGraphQLServer.js +++ b/src/GraphQL/ParseGraphQLServer.js @@ -9,6 +9,8 @@ import { handleParseHeaders } from '../middlewares'; import requiredParameter from '../requiredParameter'; import defaultLogger from '../logger'; import { ParseGraphQLSchema } from './ParseGraphQLSchema'; +import { ApolloError } from 'apollo-client'; +import Parse from 'parse/node'; class ParseGraphQLServer { constructor(parseServer, config) { @@ -55,6 +57,17 @@ class ParseGraphQLServer { app.use(this.config.graphQLPath, corsMiddleware()); app.use(this.config.graphQLPath, bodyParser.json()); app.use(this.config.graphQLPath, handleParseHeaders); + app.use(this.config.graphQLPath, (err, req, res, next) => { + if (err && err instanceof Parse.Error) { + res.json( + new ApolloError({ + networkError: err, + }) + ); + } else { + next(); + } + }); app.use( this.config.graphQLPath, graphqlExpress(async req => await this._getGraphQLOptions(req)) From 0c4593ca303cf53eeede12a26724cb5f0d5c3c02 Mon Sep 17 00:00:00 2001 From: Douglas Muraoka Date: Wed, 3 Jul 2019 13:33:53 -0300 Subject: [PATCH 2/5] Refactor handleError usage --- src/GraphQL/ParseGraphQLSchema.js | 9 ++------- src/GraphQL/ParseGraphQLServer.js | 11 +++-------- src/GraphQL/ParseGraphQLUtils.js | 14 ++++++++++++++ 3 files changed, 19 insertions(+), 15 deletions(-) create mode 100644 src/GraphQL/ParseGraphQLUtils.js diff --git a/src/GraphQL/ParseGraphQLSchema.js b/src/GraphQL/ParseGraphQLSchema.js index 7664eef525..023ab40b99 100644 --- a/src/GraphQL/ParseGraphQLSchema.js +++ b/src/GraphQL/ParseGraphQLSchema.js @@ -1,6 +1,5 @@ import Parse from 'parse/node'; import { GraphQLSchema, GraphQLObjectType } from 'graphql'; -import { ApolloError } from 'apollo-server-core'; import requiredParameter from '../requiredParameter'; import * as defaultGraphQLTypes from './loaders/defaultGraphQLTypes'; import * as parseClassTypes from './loaders/parseClassTypes'; @@ -8,6 +7,7 @@ import * as parseClassQueries from './loaders/parseClassQueries'; import * as parseClassMutations from './loaders/parseClassMutations'; import * as defaultGraphQLQueries from './loaders/defaultGraphQLQueries'; import * as defaultGraphQLMutations from './loaders/defaultGraphQLMutations'; +import { toGraphQLError } from './ParseGraphQLUtils'; class ParseGraphQLSchema { constructor(databaseController, log) { @@ -100,17 +100,12 @@ class ParseGraphQLSchema { } handleError(error) { - let code, message; if (error instanceof Parse.Error) { this.log.error('Parse error: ', error); - code = error.code; - message = error.message; } else { this.log.error('Uncaught internal server error.', error, error.stack); - code = Parse.Error.INTERNAL_SERVER_ERROR; - message = 'Internal server error.'; } - throw new ApolloError(message, code); + throw toGraphQLError(error); } } diff --git a/src/GraphQL/ParseGraphQLServer.js b/src/GraphQL/ParseGraphQLServer.js index 82bd27d76a..b8b13f215f 100644 --- a/src/GraphQL/ParseGraphQLServer.js +++ b/src/GraphQL/ParseGraphQLServer.js @@ -9,8 +9,7 @@ import { handleParseHeaders } from '../middlewares'; import requiredParameter from '../requiredParameter'; import defaultLogger from '../logger'; import { ParseGraphQLSchema } from './ParseGraphQLSchema'; -import { ApolloError } from 'apollo-client'; -import Parse from 'parse/node'; +import { toGraphQLError } from './ParseGraphQLUtils'; class ParseGraphQLServer { constructor(parseServer, config) { @@ -58,12 +57,8 @@ class ParseGraphQLServer { app.use(this.config.graphQLPath, bodyParser.json()); app.use(this.config.graphQLPath, handleParseHeaders); app.use(this.config.graphQLPath, (err, req, res, next) => { - if (err && err instanceof Parse.Error) { - res.json( - new ApolloError({ - networkError: err, - }) - ); + if (err) { + res.json(toGraphQLError(err)); } else { next(); } diff --git a/src/GraphQL/ParseGraphQLUtils.js b/src/GraphQL/ParseGraphQLUtils.js new file mode 100644 index 0000000000..912686d759 --- /dev/null +++ b/src/GraphQL/ParseGraphQLUtils.js @@ -0,0 +1,14 @@ +import Parse from 'parse/node'; +import { ApolloError } from 'apollo-server-core'; + +export function toGraphQLError (error) { + let code, message; + if (error instanceof Parse.Error) { + code = error.code; + message = error.message; + } else { + code = Parse.Error.INTERNAL_SERVER_ERROR; + message = 'Internal server error'; + } + return new ApolloError(message, code); +} From 64c5060e686f8d3559e220b25154c44fd09c1f55 Mon Sep 17 00:00:00 2001 From: Douglas Muraoka Date: Wed, 10 Jul 2019 17:23:39 -0300 Subject: [PATCH 3/5] Use handleParseErrors middleware to handle invalid session token error --- spec/ParseGraphQLServer.spec.js | 13 +++++++++---- src/GraphQL/ParseGraphQLServer.js | 11 ++--------- src/middlewares.js | 3 +++ 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/spec/ParseGraphQLServer.spec.js b/spec/ParseGraphQLServer.spec.js index 74fc3b6f76..7d8496cb7d 100644 --- a/spec/ParseGraphQLServer.spec.js +++ b/spec/ParseGraphQLServer.spec.js @@ -3198,8 +3198,8 @@ describe('ParseGraphQLServer', () => { }); expect(logOut.data.users.logOut).toBeTruthy(); - await expectAsync( - apolloClient.query({ + try { + await apolloClient.query({ query: gql` query GetCurrentUser { users { @@ -3214,8 +3214,13 @@ describe('ParseGraphQLServer', () => { 'X-Parse-Session-Token': sessionToken, }, }, - }) - ).toBeRejected(); + }); + } catch (err) { + expect(err.networkError.result).toEqual({ + code: 209, + error: 'Invalid session token', + }); + } }); }); diff --git a/src/GraphQL/ParseGraphQLServer.js b/src/GraphQL/ParseGraphQLServer.js index b8b13f215f..d9ac8f419a 100644 --- a/src/GraphQL/ParseGraphQLServer.js +++ b/src/GraphQL/ParseGraphQLServer.js @@ -5,11 +5,10 @@ import { graphqlExpress } from 'apollo-server-express/dist/expressApollo'; import { renderPlaygroundPage } from '@apollographql/graphql-playground-html'; import { execute, subscribe } from 'graphql'; import { SubscriptionServer } from 'subscriptions-transport-ws'; -import { handleParseHeaders } from '../middlewares'; +import { handleParseErrors, handleParseHeaders } from '../middlewares'; import requiredParameter from '../requiredParameter'; import defaultLogger from '../logger'; import { ParseGraphQLSchema } from './ParseGraphQLSchema'; -import { toGraphQLError } from './ParseGraphQLUtils'; class ParseGraphQLServer { constructor(parseServer, config) { @@ -56,13 +55,7 @@ class ParseGraphQLServer { app.use(this.config.graphQLPath, corsMiddleware()); app.use(this.config.graphQLPath, bodyParser.json()); app.use(this.config.graphQLPath, handleParseHeaders); - app.use(this.config.graphQLPath, (err, req, res, next) => { - if (err) { - res.json(toGraphQLError(err)); - } else { - next(); - } - }); + app.use(this.config.graphQLPath, handleParseErrors); app.use( this.config.graphQLPath, graphqlExpress(async req => await this._getGraphQLOptions(req)) diff --git a/src/middlewares.js b/src/middlewares.js index be831b09a7..b4b5223327 100644 --- a/src/middlewares.js +++ b/src/middlewares.js @@ -319,6 +319,9 @@ export function handleParseErrors(err, req, res, next) { case Parse.Error.OBJECT_NOT_FOUND: httpStatus = 404; break; + case Parse.Error.INVALID_SESSION_TOKEN: + httpStatus = 401; + break; default: httpStatus = 400; } From ff2adf81492d24ec2f99b53b36a674800161bc99 Mon Sep 17 00:00:00 2001 From: Douglas Muraoka Date: Fri, 12 Jul 2019 14:32:28 -0300 Subject: [PATCH 4/5] fix: Status code 400 when session token is invalid --- spec/ParseGraphQLServer.spec.js | 102 +++++++++++++++++++++++++++++++- src/middlewares.js | 5 +- 2 files changed, 102 insertions(+), 5 deletions(-) diff --git a/spec/ParseGraphQLServer.spec.js b/spec/ParseGraphQLServer.spec.js index 7d8496cb7d..7d0414b438 100644 --- a/spec/ParseGraphQLServer.spec.js +++ b/spec/ParseGraphQLServer.spec.js @@ -3216,7 +3216,107 @@ describe('ParseGraphQLServer', () => { }, }); } catch (err) { - expect(err.networkError.result).toEqual({ + const { statusCode, result } = err.networkError; + expect(statusCode).toBe(400); + expect(result).toEqual({ + code: 209, + error: 'Invalid session token', + }); + } + }); + }); + + describe('Session Token', () => { + it('should fail due to invalid session token', async () => { + try { + await apolloClient.query({ + query: gql` + query GetCurrentUser { + users { + me { + username + } + } + } + `, + context: { + headers: { + 'X-Parse-Session-Token': 'foo', + }, + }, + }); + } catch (err) { + const { statusCode, result } = err.networkError; + expect(statusCode).toBe(400); + expect(result).toEqual({ + code: 209, + error: 'Invalid session token', + }); + } + }); + + it('should fail due to empty session token', async () => { + try { + await apolloClient.query({ + query: gql` + query GetCurrentUser { + users { + me { + username + } + } + } + `, + context: { + headers: { + 'X-Parse-Session-Token': '', + }, + }, + }); + } catch (err) { + const { statusCode, result } = err.networkError; + expect(statusCode).toBe(400); + expect(result).toEqual({ + code: 209, + error: 'Invalid session token', + }); + } + }); + + it('should find a user and fail due to empty session token', async () => { + const car = new Parse.Object('Car'); + await car.save(); + + await parseGraphQLServer.parseGraphQLSchema.databaseController.schemaCache.clear(); + + try { + await apolloClient.query({ + query: gql` + query GetCurrentUser { + users { + me { + username + } + } + objects { + findCar { + results { + objectId + } + } + } + } + `, + context: { + headers: { + 'X-Parse-Session-Token': '', + }, + }, + }); + } catch (err) { + const { statusCode, result } = err.networkError; + expect(statusCode).toBe(400); + expect(result).toEqual({ code: 209, error: 'Invalid session token', }); diff --git a/src/middlewares.js b/src/middlewares.js index b4b5223327..1aa80f9e6b 100644 --- a/src/middlewares.js +++ b/src/middlewares.js @@ -171,7 +171,7 @@ export function handleParseHeaders(req, res, next) { delete info.sessionToken; } - if (!info.sessionToken) { + if (info.sessionToken === undefined) { req.auth = new auth.Auth({ config: req.config, installationId: info.installationId, @@ -319,9 +319,6 @@ export function handleParseErrors(err, req, res, next) { case Parse.Error.OBJECT_NOT_FOUND: httpStatus = 404; break; - case Parse.Error.INVALID_SESSION_TOKEN: - httpStatus = 401; - break; default: httpStatus = 400; } From 2f57df5e8a87fcd3b4b62c52451e278a424eb7bf Mon Sep 17 00:00:00 2001 From: Douglas Muraoka Date: Fri, 12 Jul 2019 17:33:08 -0300 Subject: [PATCH 5/5] fix: Undo handleParseErrors middleware change --- spec/ParseGraphQLServer.spec.js | 22 +++++++++---------- src/GraphQL/ParseGraphQLSchema.js | 2 +- ...seGraphQLUtils.js => parseGraphQLUtils.js} | 2 +- src/middlewares.js | 2 +- 4 files changed, 13 insertions(+), 15 deletions(-) rename src/GraphQL/{ParseGraphQLUtils.js => parseGraphQLUtils.js} (89%) diff --git a/spec/ParseGraphQLServer.spec.js b/spec/ParseGraphQLServer.spec.js index 7d0414b438..dd061ea894 100644 --- a/spec/ParseGraphQLServer.spec.js +++ b/spec/ParseGraphQLServer.spec.js @@ -3215,6 +3215,7 @@ describe('ParseGraphQLServer', () => { }, }, }); + fail('should not retrieve current user due to session token'); } catch (err) { const { statusCode, result } = err.networkError; expect(statusCode).toBe(400); @@ -3245,6 +3246,7 @@ describe('ParseGraphQLServer', () => { }, }, }); + fail('should not retrieve current user due to session token'); } catch (err) { const { statusCode, result } = err.networkError; expect(statusCode).toBe(400); @@ -3273,13 +3275,11 @@ describe('ParseGraphQLServer', () => { }, }, }); + fail('should not retrieve current user due to session token'); } catch (err) { - const { statusCode, result } = err.networkError; - expect(statusCode).toBe(400); - expect(result).toEqual({ - code: 209, - error: 'Invalid session token', - }); + const { graphQLErrors } = err; + expect(graphQLErrors.length).toBe(1); + expect(graphQLErrors[0].message).toBe('Invalid session token'); } }); @@ -3313,13 +3313,11 @@ describe('ParseGraphQLServer', () => { }, }, }); + fail('should not retrieve current user due to session token'); } catch (err) { - const { statusCode, result } = err.networkError; - expect(statusCode).toBe(400); - expect(result).toEqual({ - code: 209, - error: 'Invalid session token', - }); + const { graphQLErrors } = err; + expect(graphQLErrors.length).toBe(1); + expect(graphQLErrors[0].message).toBe('Invalid session token'); } }); }); diff --git a/src/GraphQL/ParseGraphQLSchema.js b/src/GraphQL/ParseGraphQLSchema.js index 023ab40b99..e298273bb2 100644 --- a/src/GraphQL/ParseGraphQLSchema.js +++ b/src/GraphQL/ParseGraphQLSchema.js @@ -7,7 +7,7 @@ import * as parseClassQueries from './loaders/parseClassQueries'; import * as parseClassMutations from './loaders/parseClassMutations'; import * as defaultGraphQLQueries from './loaders/defaultGraphQLQueries'; import * as defaultGraphQLMutations from './loaders/defaultGraphQLMutations'; -import { toGraphQLError } from './ParseGraphQLUtils'; +import { toGraphQLError } from './parseGraphQLUtils'; class ParseGraphQLSchema { constructor(databaseController, log) { diff --git a/src/GraphQL/ParseGraphQLUtils.js b/src/GraphQL/parseGraphQLUtils.js similarity index 89% rename from src/GraphQL/ParseGraphQLUtils.js rename to src/GraphQL/parseGraphQLUtils.js index 912686d759..79f7192e53 100644 --- a/src/GraphQL/ParseGraphQLUtils.js +++ b/src/GraphQL/parseGraphQLUtils.js @@ -1,7 +1,7 @@ import Parse from 'parse/node'; import { ApolloError } from 'apollo-server-core'; -export function toGraphQLError (error) { +export function toGraphQLError(error) { let code, message; if (error instanceof Parse.Error) { code = error.code; diff --git a/src/middlewares.js b/src/middlewares.js index 1aa80f9e6b..be831b09a7 100644 --- a/src/middlewares.js +++ b/src/middlewares.js @@ -171,7 +171,7 @@ export function handleParseHeaders(req, res, next) { delete info.sessionToken; } - if (info.sessionToken === undefined) { + if (!info.sessionToken) { req.auth = new auth.Auth({ config: req.config, installationId: info.installationId,