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(rest): implement query parameter validation (issue 1573) #2307

Closed
wants to merge 41 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
72594eb
feat(rest): validate query parameters against their schema
YaelGit Jan 30, 2019
0e04254
feat(rest): validate query parameters against their schema
YaelGit Jan 30, 2019
8e3248c
Merge branch 'master' into issue-1573
YaelGit Jan 30, 2019
a4c2086
chore: fix test cases to be compatible with [email protected]
raymondfeng Jan 30, 2019
066d525
build(testlab): move test files to `src/__tests__`
bajtos Jan 24, 2019
4c6b176
docs: fix instructions for verifying TypeScript setup
bajtos Jan 25, 2019
f946d48
docs(testlab): describe test fixture directories
bajtos Jan 29, 2019
0121c10
fix: remove unused juggler import
nabdelgadir Jan 31, 2019
181e1f1
chore(context): add more unit tests for binding filters
raymondfeng Jan 31, 2019
0710055
chore: upgrade dependencies for build
raymondfeng Jan 31, 2019
91a37dc
build: move test files to `src/__tests__` in example apps
bajtos Jan 31, 2019
a624b95
docs(cli): use a custom repository base class
gczobel-f5 Jan 31, 2019
0e92b88
docs(cli): use a custom repository base class
gczobel-f5 Feb 3, 2019
edbbe88
feat(cli): use a custom repository base class
gczobel-f5 Jan 9, 2019
4695e3a
chore: add required config for greenkeeper integration
raymondfeng Jan 31, 2019
857868e
chore: update dependencies proposed by greenkeeper
raymondfeng Jan 31, 2019
bfe8c27
chore: whitelist greenkeeper branches
raymondfeng Jan 31, 2019
a8a409c
chore: add Greenkeeper badge
raymondfeng Jan 31, 2019
84c6a88
chore: add dominique to maintainers
nabdelgadir Feb 4, 2019
1a6ac91
build: move test files to `src/__tests__` in packages
bajtos Feb 4, 2019
89340b0
docs: how to update target model in hasmany relation
ludohenin Jan 26, 2019
d3a3bea
feat(cli): scaffold test files to `src/__tests__`
bajtos Feb 5, 2019
51cba45
fix(benchmark): set TypeScript's rootDir to src
bajtos Feb 5, 2019
a3da024
feat(build): use `dist/__tests__` in code examples and tests
bajtos Feb 5, 2019
a54fbf1
fix(example-todo): "npm run migration" script path
bajtos Feb 5, 2019
bf26cc3
fix(example-todo-list): "npm run migration" script path
bajtos Feb 5, 2019
75731f9
fix(docs): update test paths to `src/__tests__`
bajtos Feb 5, 2019
5042698
fix(rest): sanitize json for JSON.parse()
raymondfeng Feb 6, 2019
b5f12be
chore: update @types/debug to version 4.1.0
raymondfeng Feb 7, 2019
6ef5d85
chore: exclude "good first issue" from stalebot
bajtos Feb 8, 2019
65ee865
fix: update to the most recent lodash version
jannyHou Feb 7, 2019
596a143
chore: publish release
dhmlau Feb 8, 2019
95e919e
feat(rest): validate query parameters against their schema
YaelGit Jan 30, 2019
112847b
feat(rest): validate query parameters against their schema
YaelGit Jan 30, 2019
f14bd93
Merge branch 'issue-1573' of github.com:YaelGit/loopback-next into is…
YaelGit Feb 11, 2019
7707fa5
fix(docs): broken link in Crafting in LB
dhmlau Feb 10, 2019
bf22e5c
feat(rest): validate query parameters against their schema
YaelGit Jan 30, 2019
131ccd9
feat(rest): validate query parameters against their schema
YaelGit Jan 30, 2019
1fe7f9c
feat(rest): validate query parameters against their schema
YaelGit Jan 30, 2019
14d7909
feat(rest): validate query parameters against their schema
YaelGit Jan 30, 2019
c2c1c07
Merge branch 'issue-1573' of github.com:YaelGit/loopback-next into is…
YaelGit Feb 12, 2019
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
40 changes: 37 additions & 3 deletions packages/rest/src/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {RestHttpErrors} from './rest-http-error';
import {ResolvedRoute} from './router';
import {OperationArgs, PathParameterValues, Request} from './types';
import {validateRequestBody} from './validation/request-body.validator';
import {validateRequestQuery} from './validation/request-query.validator';
const debug = debugFactory('loopback:rest:parser');

/**
Expand All @@ -38,11 +39,18 @@ export async function parseOperationArgs(
operationSpec,
request,
);

const query = await requestBodyParser.loadRequestBodyIfNeeded(
operationSpec,
request,
);

Copy link
Contributor

Choose a reason for hiding this comment

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

This is suspicious. Why do we parse the request body again into query?

return buildOperationArguments(
operationSpec,
request,
pathParams,
body,
query,
route.schemas,
);
}
Expand All @@ -52,6 +60,7 @@ function buildOperationArguments(
request: Request,
pathParams: PathParameterValues,
body: RequestBody,
query: RequestBody,
globalSchemas: SchemasObject,
): OperationArgs {
let requestBodyIndex: number = -1;
Expand All @@ -67,6 +76,12 @@ function buildOperationArguments(

const paramArgs: OperationArgs = [];

let isQuery = false;
let paramName = '';
let paramSchema = {};
let queryValue = {};
let schemasValue = {};

for (const paramSpec of operationSpec.parameters || []) {
if (isReferenceObject(paramSpec)) {
// TODO(bajtos) implement $ref parameters
Expand All @@ -77,11 +92,30 @@ function buildOperationArguments(
const rawValue = getParamFromRequest(spec, request, pathParams);
const coercedValue = coerceParameter(rawValue, spec);
paramArgs.push(coercedValue);
}

debug('Validating request body - value %j', body);
validateRequestBody(body, operationSpec.requestBody, globalSchemas);
if (spec.in === 'query' && paramSpec.schema != null) {
isQuery = true;
paramName = paramSpec.name;
paramSchema = paramSpec.schema || [];
// tslint:disable-next-line:no-any
(<any>queryValue)[paramName] = coercedValue;
// tslint:disable-next-line:no-any
(<any>schemasValue)[paramName] = paramSchema;
}
}

//if query parameters from URL - send to query validation
if (isQuery) {
query.value = queryValue;
globalSchemas = {properties: schemasValue};
query.schema = globalSchemas;
validateRequestQuery(query, operationSpec.requestBody, globalSchemas);
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, you are building an object with all parameters from the query string, and then validate this object in one pass. I have two reservations about this approach:

  • It seems overly complicated to me.
  • There are more parameter sources besides query and body, we need to validate parameters from all sources (e.g. path and header).

Have you considered to call ajv validator repeatedly, once for each parameter?

Copy link
Author

Choose a reason for hiding this comment

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

Hello @bajtos ,
sorry for the delay - yes i do plan to close the issue soon and in regards to your comments:

  1. I will do the rebase as requested.
  2. Regarding the validation of all parameters all together - i did it in this way due to following same validation standard done for body parameters.
  3. Regrading the request for handling other parameter types, i think a new issue should be opened for other parameter types as the initial issue was for query only.

}
//if body parameters - send to body validation
else {
debug('Validating request body - value %j', body);
validateRequestBody(body, operationSpec.requestBody, globalSchemas);
}
if (requestBodyIndex > -1) paramArgs.splice(requestBodyIndex, 0, body.value);
return paramArgs;
}
Expand Down
11 changes: 11 additions & 0 deletions packages/rest/src/rest-http-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,17 @@ export namespace RestHttpErrors {
},
);
}
export const INVALID_REQUEST_QUERY_MESSAGE =
'The request query is invalid. See error object details property for more info.';

export function invalidRequestQuery(): HttpErrors.HttpError {
return Object.assign(
new HttpErrors.UnprocessableEntity(INVALID_REQUEST_QUERY_MESSAGE),
{
code: 'VALIDATION_FAILED',
},
);
}

/**
* An invalid request body error contains a `details` property as the machine-readable error.
Expand Down
150 changes: 150 additions & 0 deletions packages/rest/src/validation/request-query.validator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
import {
RequestBodyObject,
SchemaObject,
ReferenceObject,
SchemasObject,
} from '@loopback/openapi-v3-types';
import * as AJV from 'ajv';
import * as debugModule from 'debug';
import * as util from 'util';
import {HttpErrors, RestHttpErrors, RequestBody} from '..';
import * as _ from 'lodash';

const toJsonSchema = require('openapi-schema-to-json-schema');
const debug = debugModule('loopback:rest:validation');

export type RequestQueryValidationOptions = AJV.Options;

/**
* Check whether the request query is valid according to the provided OpenAPI schema.
* The JSON schema is generated from the OpenAPI schema which is typically defined
* by `@requestQuery()`.
* The validation leverages AJS schema validator.
* @param query The request query parsed from an HTTP request.
* @param requestQuerySpec The OpenAPI requestQuery specification defined in `@requestQuery()`.
* @param globalSchemas The referenced schemas generated from `OpenAPISpec.components.schemas`.
*/
export function validateRequestQuery(
query: RequestBody,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to reuse RequestBody for query, we should rename the type as something like ValueWithSchema.

Copy link
Author

Choose a reason for hiding this comment

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

@raymondfeng - do you mean we should duplicate the type requestBody in types.ts and then use this new type (ValueWithSchema) in request-query.validator.ts?
should we also duplicate the BodyParser, BodyParserFunction? and then duplicate the code in body-parser.ts?

Copy link
Contributor

Choose a reason for hiding this comment

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

No duplication. What I meant is to refactor RequestBody to ValueWithSchema into its own file and use it for both body and query validation.

requestQuerySpec?: RequestBodyObject,
globalSchemas: SchemasObject = {},
options: RequestQueryValidationOptions = {},
) {
const required = requestQuerySpec && requestQuerySpec.required;

if (required && query.value == undefined) {
const err = Object.assign(
new HttpErrors.BadRequest('Request query is required'),
{
code: 'MISSING_REQUIRED_PARAMETER',
parameterName: 'request query',
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to see which query param.

Copy link
Author

Choose a reason for hiding this comment

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

is this sufficient?
query_validation_errors

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.

},
);
throw err;
}

const schema = query.schema;
/* istanbul ignore if */
if (debug.enabled) {
debug('Request query schema: %j', util.inspect(schema, {depth: null}));
}
if (!schema) return;

options = Object.assign({coerceTypes: query.coercionRequired}, options);
validateValueAgainstSchema(query.value, schema, globalSchemas, options);
}

/**
* Convert an OpenAPI schema to the corresponding JSON schema.
* @param openapiSchema The OpenAPI schema to convert.
*/
function convertToJsonSchema(openapiSchema: SchemaObject) {
const jsonSchema = toJsonSchema(openapiSchema);
delete jsonSchema['$schema'];
/* istanbul ignore if */
if (debug.enabled) {
debug(
'Converted OpenAPI schema to JSON schema: %s',
util.inspect(jsonSchema, {depth: null}),
);
}
return jsonSchema;
}

/**
* Validate the request query data against JSON schema.
* @param query The request query data.
* @param schema The JSON schema used to perform the validation.
* @param globalSchemas Schema references.
*/

const compiledSchemaCache = new WeakMap();

function validateValueAgainstSchema(
// tslint:disable-next-line:no-any
query: any,
schema: SchemaObject | ReferenceObject,
globalSchemas?: SchemasObject,
options?: RequestQueryValidationOptions,
) {
let validate;

if (compiledSchemaCache.has(schema)) {
validate = compiledSchemaCache.get(schema);
} else {
validate = createValidator(schema, globalSchemas, options);
compiledSchemaCache.set(schema, validate);
}

if (validate(query)) {
debug('Request query passed AJV validation.');
return;
}

const validationErrors = validate.errors;

/* istanbul ignore if */
if (debug.enabled) {
debug(
'Invalid request query: %s. Errors: %s',
util.inspect(query, {depth: null}),
util.inspect(validationErrors),
);
}

const error = RestHttpErrors.invalidRequestQuery();
error.details = _.map(validationErrors, e => {
return {
path: e.dataPath,
code: e.keyword,
message: e.message,
info: e.params,
};
});
throw error;
}

function createValidator(
schema: SchemaObject,
globalSchemas?: SchemasObject,
options?: RequestQueryValidationOptions,
): Function {
const jsonSchema = convertToJsonSchema(schema);

const schemaWithRef = Object.assign({components: {}}, jsonSchema);
schemaWithRef.components = {
schemas: globalSchemas,
};

const ajv = new AJV(
Object.assign(
{},
{
allErrors: true,
},
options,
),
);

return ajv.compile(schemaWithRef);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There seem to be quite a bit duplicate code as request-body-validator. Can we refactor the code for better reuse?

Copy link
Author

Choose a reason for hiding this comment

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

@raymondfeng -Thanks for the review and comments.
I will try to reply: I was actually looking to differentiate the 2 in order to allow/take into consideration future changes and needs-do you think this is un-needed? will the only difference be - the extraction of the query params and the structure to be sent to same validation function to leverage the AJV ? we need not forget, we also need different error messages. Please share your thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would create a utility function that validates ValueWithSchema using AJV.

2 changes: 1 addition & 1 deletion packages/rest/test/unit/coercion/paramObject.unit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe('coerce object param - optional', function() {
test(OPTIONAL_ANY_OBJECT, {key: 'value'}, {key: 'value'});
test(OPTIONAL_ANY_OBJECT, undefined, undefined);
test(OPTIONAL_ANY_OBJECT, '', undefined);
test(OPTIONAL_ANY_OBJECT, 'null', null);
test(OPTIONAL_ANY_OBJECT, {key: 'null'}, {key: 'null'});
});

context('nested values are not coerced', () => {
Expand Down
Loading