Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
validators.py: avoid spurious error messages after schema validation …
…fails

If validation against the colander schema for the request fails, then
request.validated is left empty - this will cause subsequent validation
to fail for reasons that have nothing to do with what was submitted and
returning the error messages to the user can be quite confusing.

For example: if a new update is submitted with empty notes, not only does
the user see 'Notes : Required', they also see 'Builds : You may not specify
an empty list of builds.' and 'Builds : ACL validation mechanism was unable
to determine ACLs.' - which are entirely spurious.

In all validators that are run after the colander schema validator, check
to see if a) request.validated is empty b) request.errors is non-empty,
and in this case skip further validation. This is done as a decorator to
have to avoid having dozens of separate code paths for validation-after-schema-
failure that would need to be tested/covered.

Signed-off-by: Owen W. Taylor <otaylor@fishsoup.net>
  • Loading branch information
owtaylor authored and bowlofeggs committed Jun 18, 2018
1 parent 6354367 commit 5241205
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 9 deletions.
41 changes: 38 additions & 3 deletions bodhi/server/validators.py
Expand Up @@ -19,6 +19,7 @@
"""A collection of validators for Bodhi requests."""

from datetime import datetime, timedelta
from functools import wraps

from pyramid.exceptions import HTTPNotFound, HTTPBadRequest
from pyramid.httpexceptions import HTTPFound, HTTPNotImplemented
Expand Down Expand Up @@ -66,6 +67,20 @@
""".replace('\n', ' ')


def postschema_validator(f):
"""Modify a validator function, so that it is skipped if schema validation already failed."""
@wraps(f)
def validator(request, **kwargs):
# The check on request.errors is to make sure we don't bypass other checks without
# failing the request
if len(request.validated) == 0 and len(request.errors) > 0:
return

f(request, **kwargs)

return validator


# This one is a colander validator which is different from the cornice
# validators defined elsehwere.
def validate_csrf_token(node, value):
Expand Down Expand Up @@ -173,6 +188,7 @@ def cache_nvrs(request, build):
request.buildinfo[build]['nvr'] = kbinfo['name'], kbinfo['version'], kbinfo['release']


@postschema_validator
def validate_nvrs(request, **kwargs):
"""
Ensure the the given builds reference valid Build objects.
Expand All @@ -196,6 +212,7 @@ def validate_nvrs(request, **kwargs):
return


@postschema_validator
def validate_builds(request, **kwargs):
"""
Ensure that the builds parameter is valid for the request.
Expand Down Expand Up @@ -245,6 +262,7 @@ def validate_builds(request, **kwargs):
return


@postschema_validator
def validate_build_tags(request, **kwargs):
"""
Ensure that all of the referenced builds are tagged as candidates.
Expand Down Expand Up @@ -295,6 +313,7 @@ def validate_build_tags(request, **kwargs):
build, valid_tags))


@postschema_validator
def validate_tags(request, **kwargs):
"""
Ensure that the referenced tags are valid Koji tags.
Expand All @@ -320,6 +339,7 @@ def validate_tags(request, **kwargs):
'Invalid tag: %s' % tag_name)


@postschema_validator
def validate_acls(request, **kwargs):
"""
Ensure the user has commit privs to these builds or is an admin.
Expand Down Expand Up @@ -487,6 +507,7 @@ def validate_acls(request, **kwargs):
"access to {}".format(user.name, package.name))


@postschema_validator
def validate_uniqueness(request, **kwargs):
"""
Check for multiple builds from the same package and same release.
Expand Down Expand Up @@ -528,6 +549,7 @@ def validate_uniqueness(request, **kwargs):
return


@postschema_validator
def validate_enums(request, **kwargs):
"""
Convert from strings to our enumerated types.
Expand All @@ -550,6 +572,7 @@ def validate_enums(request, **kwargs):
request.validated[param] = enum.from_string(value)


@postschema_validator
def validate_packages(request, **kwargs):
"""
Make sure referenced packages exist.
Expand Down Expand Up @@ -581,6 +604,7 @@ def validate_packages(request, **kwargs):
request.validated["packages"] = validated_packages


@postschema_validator
def validate_updates(request, **kwargs):
"""
Make sure referenced updates exist.
Expand Down Expand Up @@ -616,6 +640,7 @@ def validate_updates(request, **kwargs):
request.validated["updates"] = validated_updates


@postschema_validator
def validate_groups(request, **kwargs):
"""
Make sure the referenced groups exist.
Expand Down Expand Up @@ -648,6 +673,7 @@ def validate_groups(request, **kwargs):
request.validated["groups"] = validated_groups


@postschema_validator
def validate_release(request, **kwargs):
"""
Make sure the referenced release exists.
Expand All @@ -672,6 +698,7 @@ def validate_release(request, **kwargs):
"Invalid release specified: {}".format(releasename))


@postschema_validator
def validate_releases(request, **kwargs):
"""
Make sure referenced releases exist.
Expand Down Expand Up @@ -707,6 +734,7 @@ def validate_releases(request, **kwargs):
request.validated["releases"] = validated_releases


@postschema_validator
def validate_bugs(request, **kwargs):
"""
Ensure that the list of bugs are all valid integers.
Expand All @@ -724,6 +752,7 @@ def validate_bugs(request, **kwargs):
"Invalid bug ID specified: {}".format(bugs))


@postschema_validator
def validate_update(request, **kwargs):
"""
Make sure the requested update exists.
Expand Down Expand Up @@ -799,6 +828,7 @@ def validate_ignore_user(request, **kwargs):
return ensure_user_exists("ignore_user", request)


@postschema_validator
def validate_update_id(request, **kwargs):
"""
Ensure that a given update id exists.
Expand Down Expand Up @@ -836,6 +866,7 @@ def _conditionally_get_update(request):
return update


@postschema_validator
def validate_bug_feedback(request, **kwargs):
"""
Ensure that bug feedback references bugs associated with the given update.
Expand Down Expand Up @@ -876,6 +907,7 @@ def validate_bug_feedback(request, **kwargs):
request.validated["bug_feedback"] = validated


@postschema_validator
def validate_testcase_feedback(request, **kwargs):
"""
Ensure that the referenced test case exists and is associated with the referenced package.
Expand Down Expand Up @@ -957,6 +989,7 @@ def validate_comment_id(request, **kwargs):
request.errors.status = HTTPNotFound.code


@postschema_validator
def validate_override_builds(request, **kwargs):
"""
Ensure that the override builds are properly referenced.
Expand Down Expand Up @@ -1065,6 +1098,7 @@ def _validate_override_build(request, nvr, db):
return build


@postschema_validator
def validate_expiration_date(request, **kwargs):
"""
Ensure the expiration date is in the future.
Expand Down Expand Up @@ -1095,6 +1129,7 @@ def validate_expiration_date(request, **kwargs):
request.validated['expiration_date'] = expiration_date


@postschema_validator
def validate_captcha(request, **kwargs):
"""
Validate the captcha.
Expand Down Expand Up @@ -1148,6 +1183,7 @@ def validate_captcha(request, **kwargs):
del request.session['captcha']


@postschema_validator
def validate_stack(request, **kwargs):
"""
Make sure this singular stack exists.
Expand Down Expand Up @@ -1186,6 +1222,7 @@ def _get_valid_requirements(request, requirements):
yield testcase['name']


@postschema_validator
def validate_requirements(request, **kwargs):
"""
Validate the requirements parameter for the stack.
Expand All @@ -1212,6 +1249,7 @@ def validate_requirements(request, **kwargs):
return


@postschema_validator
def validate_request(request, **kwargs):
"""
Ensure that this update is newer than whatever is in the requested state.
Expand All @@ -1224,9 +1262,6 @@ def validate_request(request, **kwargs):
update = request.validated['update']
db = request.db

if 'request' not in request.validated:
# Invalid request. Let the colander error from our schemas.py bubble up.
return
if request.validated['request'] in (UpdateRequest.stable, UpdateRequest.batched):
target = UpdateStatus.stable
elif request.validated['request'] is UpdateRequest.testing:
Expand Down
34 changes: 28 additions & 6 deletions bodhi/tests/server/test_validators.py
Expand Up @@ -36,9 +36,7 @@ def test_invalid_token(self):
comment = {'update': update.title, 'text': 'invalid CSRF', 'karma': 0,
'csrf_token': 'wrong_token'}

# Surprisingly, using the wrong CSRF token gives a 404 code because it thinks the update is
# also not found.
r = self.app.post_json('/comments/', comment, status=404)
r = self.app.post_json('/comments/', comment, status=400)

expected_reponse = {
u'status': u'error',
Expand All @@ -47,9 +45,7 @@ def test_invalid_token(self):
(u'CSRF tokens do not match. This happens if you have the page open for a long '
u'time. Please reload the page and try to submit your data again. Make sure to '
u'save your input somewhere before reloading. '),
u'location': u'body', u'name': u'csrf_token'},
{u'description': u'Invalid update specified: None', u'location': u'url',
u'name': u'update'}]}
u'location': u'body', u'name': u'csrf_token'}]}
self.assertEqual(r.json, expected_reponse)

def test_valid_token(self):
Expand Down Expand Up @@ -337,6 +333,19 @@ def test_invalid(self):
'description': 'Invalid bug ids specified: invalid'}])
self.assertEqual(request.errors.status, exceptions.HTTPBadRequest.code)

def test_no_feedbacks(self):
"""Nothing to do if no feedback."""
request = mock.Mock()
request.db = self.db
request.errors = Errors()
request.validated = {'update': models.Update.query.first()}

validators.validate_bug_feedback(request)

self.assertEqual(
request.errors,
[])


class TestValidateCaptcha(BaseTestCase):
"""Test the validate_captcha() function."""
Expand Down Expand Up @@ -683,6 +692,19 @@ def test_invalid(self):
'description': 'Invalid testcase names specified: invalid'}])
self.assertEqual(request.errors.status, exceptions.HTTPBadRequest.code)

def test_no_feedbacks(self):
"""Nothing to do if no feedback."""
request = mock.Mock()
request.db = self.db
request.errors = Errors()
request.validated = {'update': models.Update.query.first()}

validators.validate_testcase_feedback(request)

self.assertEqual(
request.errors,
[])

def test_update_not_found(self):
"""It should 404 if the update is not found."""
request = mock.Mock()
Expand Down

0 comments on commit 5241205

Please sign in to comment.