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

Added option to validate incoming URL query parameters #1122

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
27 changes: 27 additions & 0 deletions optimade/server/routers/utils.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# pylint: disable=import-outside-toplevel,too-many-locals
import re
from warnings import warn
import urllib.parse
from datetime import datetime
from typing import Any, Dict, List, Set, Union
Expand All @@ -22,6 +23,8 @@
from optimade.server.exceptions import BadRequest, InternalServerError
from optimade.server.query_params import EntryListingQueryParams, SingleEntryQueryParams
from optimade.utils import mongo_id_for_database, get_providers, PROVIDER_LIST_URLS
from optimade.server.mappers import BaseResourceMapper
from optimade.server.warnings import UnknownProviderQueryParameter

__all__ = (
"BASE_URL_PREFIXES",
Expand Down Expand Up @@ -235,6 +238,7 @@ def get_entries(
"""Generalized /{entry} endpoint getter"""
from optimade.server.routers import ENTRY_COLLECTIONS

check_params(request, params)
(
results,
data_returned,
Expand Down Expand Up @@ -284,6 +288,7 @@ def get_single_entry(
) -> EntryResponseOne:
from optimade.server.routers import ENTRY_COLLECTIONS

check_params(request, params)
params.filter = f'id="{entry_id}"'
(
results,
Expand Down Expand Up @@ -319,3 +324,25 @@ def get_single_entry(
),
included=included,
)


def check_params(
request: Request, params: Union[EntryListingQueryParams, SingleEntryQueryParams]
):
for param in request.query_params.keys():
if not hasattr(params, param):
split_param = param.split("_")
if param.startswith("_") and len(split_param) > 2:
Copy link
Member

Choose a reason for hiding this comment

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

Personally I would suggest looping over all the parameters and accumulating all the errors and warnings before emitting them.

It would probably be fine to just raise one error for all unrecognised fields, e.g.

"The query parameter(s) ['fitler', '_exmpl_test'] are not recognised by this entrypoint."

Something like:

for param in request.query_params.keys():
    errors = []
    warnings = []
    if not hasattr(params, param):
        split_param = params.split("_")
        if param.startswith("_") and len(split_param) > 2:
            prefix = split_param[1]
            if prefix in BaseResourceMapper.SUPPORTED_PREFIXES:
                errors.append(param)
            elif prefix not in BaseResourceMapper.KNOWN_PROVIDER_PREFIXES:
                warnings.append(param)
        else:
            errors.append(param)

    if warnings:
         warn(f"The query parameter(s) '{warnings}' are unrecognised and have been ignored.")
 
    if errors:
         raise BadRequest(f"The query parameter(s) '{errors}' are not recognised by this endpoint.")

if split_param[1] not in BaseResourceMapper.KNOWN_PROVIDER_PREFIXES:
warn(
f"The Query parameter '{param}' has an unknown provider prefix: '{split_param[1]}'. This query parameter has been ignored.",
UnknownProviderQueryParameter,
)
elif split_param[1] in BaseResourceMapper.SUPPORTED_PREFIXES:
raise BadRequest(
detail=f"The query parameter '{param}' has a prefix that is supported by this server, yet the parameter is not known."
)
else:
raise BadRequest(
detail=f"The query parameter '{param}' is not known by this entry point."
)
Copy link
Member

Choose a reason for hiding this comment

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

I would unify the error messages for the second and third cases (I'm not sure telling the client that the prefix is supported is that much more useful). This should allow the code to be simplified quite a bit (especially wrt. my other comment).

Copy link
Member

Choose a reason for hiding this comment

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

Only issue I can foresee is if an implementation is already using a custom query parameter without registering it in the query param model. Maybe this param check should be toggle-able and off by default (for now)?

7 changes: 7 additions & 0 deletions optimade/server/warnings.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,10 @@ class UnknownProviderProperty(OptimadeWarning):
recognised by this implementation.

"""


class UnknownProviderQueryParameter(OptimadeWarning):
"""A provider-specific query parameter has been requested in the query who's prefix is not
JPBergsma marked this conversation as resolved.
Show resolved Hide resolved
recognised by this implementation.

"""
46 changes: 46 additions & 0 deletions tests/server/query_params/test_filter.py
Original file line number Diff line number Diff line change
Expand Up @@ -597,3 +597,49 @@ def test_filter_on_unknown_fields(check_response, check_error_response):
request = "/structures?filter=_optimade_random_field = 1"
expected_ids = []
check_response(request, expected_ids=expected_ids)


def test_wrong_query_param(check_error_response):
request = "/structures?_exmpl_filter=nelements=2"
check_error_response(
request,
expected_status=400,
expected_title="Bad Request",
expected_detail="The query parameter '_exmpl_filter' has a prefix that is supported by this server, yet the parameter is not known.",
)

request = "/structures?filer=nelements=2"
check_error_response(
request,
expected_status=400,
expected_title="Bad Request",
expected_detail="The query parameter 'filer' is not known by this entry point.",
)

request = "/structures/mpf_3819?filter=nelements=2"
check_error_response(
request,
expected_status=400,
expected_title="Bad Request",
expected_detail="The query parameter 'filter' is not known by this entry point.",
)


def test_handling_prefixed_query_param(check_response):
request = "/structures?_odbx_filter=nelements=2&filter=elements LENGTH >= 9"
expected_ids = ["mpf_3819"]
check_response(request, expected_ids)

request = (
"/structures?_unknown_filter=elements HAS 'Si'&filter=elements LENGTH >= 9"
)
expected_ids = ["mpf_3819"]
expected_warnings = [
{
"title": "UnknownProviderQueryParameter",
"detail": "The Query parameter '_unknown_filter' has an unknown provider prefix: 'unknown'. This query parameter has been ignored.",
}
]
check_response(
request, expected_ids=expected_ids, expected_warnings=expected_warnings
)