From eac88278daee726cfdb0a874a58e7b8ce33a68fc Mon Sep 17 00:00:00 2001 From: Dan Callaghan Date: Apr 26 2018 22:52:50 +0000 Subject: [PATCH 1/2] use reqparse properly for 'since' parameter The response for a 400 Bad Request is supposed to be a JSON dict keyed on the invalid parameter, not a plain JSON string message. If we use a proper type= for the reqparse argument, and let that function raise ValueError, we will get the proper validation behaviour for "free". --- diff --git a/tests/test_api_v10.py b/tests/test_api_v10.py index 357858c..9ab0541 100644 --- a/tests/test_api_v10.py +++ b/tests/test_api_v10.py @@ -376,13 +376,24 @@ def test_filtering_waivers_by_since(client, session): def test_filtering_waivers_by_malformed_since(client, session): - create_waiver(session, subject={'subject.test1': 'subject1'}, - testcase='testcase1', username='foo', product_version='foo-1') - wrong_since = 123 - r = client.get('/api/v1.0/waivers/?since=%s' % wrong_since) + r = client.get('/api/v1.0/waivers/?since=123') res_data = json.loads(r.get_data(as_text=True)) assert r.status_code == 400 - assert res_data['message'] == "'since' parameter not in ISO8601 format" + assert res_data['message']['since'] == \ + "time data '123' does not match format '%Y-%m-%dT%H:%M:%S.%f'" + + r = client.get('/api/v1.0/waivers/?since=%s,badend' % datetime.datetime.utcnow().isoformat()) + res_data = json.loads(r.get_data(as_text=True)) + assert r.status_code == 400 + assert res_data['message']['since'] == \ + "time data 'badend' does not match format '%Y-%m-%dT%H:%M:%S.%f'" + + r = client.get('/api/v1.0/waivers/?since=%s,too,many,commas' + % datetime.datetime.utcnow().isoformat()) + res_data = json.loads(r.get_data(as_text=True)) + assert r.status_code == 400 + assert res_data['message']['since'] == \ + "time data 'too,many,commas' does not match format '%Y-%m-%dT%H:%M:%S.%f'" def test_filtering_waivers_by_proxied_by(client, session): diff --git a/waiverdb/api_v1.py b/waiverdb/api_v1.py index cedef60..07c737c 100644 --- a/waiverdb/api_v1.py +++ b/waiverdb/api_v1.py @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0+ import json +import datetime import requests from flask import Blueprint, request, current_app @@ -10,7 +11,7 @@ from sqlalchemy.sql.expression import func, cast from waiverdb import __version__ from waiverdb.models import db, Waiver -from waiverdb.utils import reqparse_since, json_collection, jsonp +from waiverdb.utils import json_collection, jsonp from waiverdb.fields import waiver_fields import waiverdb.auth @@ -57,6 +58,33 @@ def _validate_results_filter(results): " subject and testcase")) +def reqparse_since(since): + """ + Parses the 'since' query parameter, which is expected to be either a + single ISO8601 timestamp representing the start of a time period:: + + 2017-02-13T23:37:58.193281 + + or a comma-separated pair of timestamps representing the start and end of + a range:: + + 2017-02-13T23:37:58.193281,2017-02-16T23:37:58.193281 + + Returns a tuple (start, end) of datetime.datetime instances. + """ + start = None + end = None + if ',' in since: + start, end = since.split(',', 1) + else: + start = since + if start: + start = datetime.datetime.strptime(start, "%Y-%m-%dT%H:%M:%S.%f") + if end: + end = datetime.datetime.strptime(end, "%Y-%m-%dT%H:%M:%S.%f") + return start, end + + # RP contains request parsers (reqparse.RequestParser). # Parsers are added in each 'resource section' for better readability RP = {} @@ -77,7 +105,7 @@ RP['get_waivers'].add_argument('username', location='args') RP['get_waivers'].add_argument('include_obsolete', type=bool, default=False, location='args') # XXX This matches the since query parameter in resultsdb but I think it would # be good to use two parameters(since and until). -RP['get_waivers'].add_argument('since', location='args') +RP['get_waivers'].add_argument('since', type=reqparse_since, location='args') RP['get_waivers'].add_argument('page', default=1, type=int, location='args') RP['get_waivers'].add_argument('limit', default=10, type=int, location='args') RP['get_waivers'].add_argument('proxied_by', location='args') @@ -148,12 +176,7 @@ class WaiversResource(Resource): if args['proxied_by']: query = query.filter(Waiver.proxied_by == args['proxied_by']) if args['since']: - try: - since_start, since_end = reqparse_since(args['since']) - except ValueError: - raise BadRequest("'since' parameter not in ISO8601 format") - except TypeError: - raise BadRequest("'since' parameter not in ISO8601 format") + since_start, since_end = args['since'] if since_start: query = query.filter(Waiver.timestamp >= since_start) if since_end: diff --git a/waiverdb/utils.py b/waiverdb/utils.py index b760503..724e50f 100644 --- a/waiverdb/utils.py +++ b/waiverdb/utils.py @@ -1,6 +1,5 @@ # SPDX-License-Identifier: GPL-2.0+ -import datetime import functools import stomp from flask import request, url_for, jsonify, current_app @@ -10,24 +9,6 @@ from werkzeug.exceptions import NotFound, HTTPException from contextlib import contextmanager -def reqparse_since(since): - """ - This parses the since(i.e. 2017-02-13T23:37:58.193281, 2017-02-16T23:37:58.193281) - query parameter and returns a tuple. - """ - start = None - end = None - if ',' in since: - start, end = since.split(',') - else: - start = since - if start: - start = datetime.datetime.strptime(start, "%Y-%m-%dT%H:%M:%S.%f") - if end: - end = datetime.datetime.strptime(end, "%Y-%m-%dT%H:%M:%S.%f") - return start, end - - def json_collection(query, page=1, limit=10): """ Helper function for Flask request handlers which want to return From 1948158b785ec337816cbf648434a51cef909513 Mon Sep 17 00:00:00 2001 From: Dan Callaghan Date: Apr 26 2018 22:53:03 +0000 Subject: [PATCH 2/2] tweak wording of GET /api/v1.0/waivers/ parameters --- diff --git a/waiverdb/api_v1.py b/waiverdb/api_v1.py index 07c737c..c464ad9 100644 --- a/waiverdb/api_v1.py +++ b/waiverdb/api_v1.py @@ -149,10 +149,12 @@ class WaiversResource(Resource): :query int limit: Limit the number of items returned. :query dict subject: Only include waivers for the given subject. :query string testcase: Only include waivers for the given test case name. - :query string product_version: Filter the waivers by product version. - :query string username: Filter the waivers by username. - :query string proxied_by: Filter the waivers by the users who are - allowed to create waivers on behalf of other users. + :query string product_version: Only include waivers for the given + product version. + :query string username: Only include waivers which were submitted by + the given user. + :query string proxied_by: Only include waivers which were proxied on + behalf of someone else by the given user. :query string since: An ISO 8601 formatted datetime (e.g. 2017-03-16T13:40:05+00:00) to filter results by. Optionally provide a second ISO 8601 datetime separated by a comma to retrieve a range (e.g. 2017-03-16T13:40:05+00:00,