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

Don't allow group names with leading or trailing whitespace #8834

Merged
merged 1 commit into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions h/schemas/api/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,22 @@ def validate(self, data):
:rtype: dict

"""

appstruct = super().validate(data)
appstruct = self._whitelisted_fields_only(appstruct)
self._validate_name(appstruct)
self._validate_groupid(appstruct)

return appstruct

def _validate_name(self, appstruct):
name = appstruct.get("name")

if name and name.strip() != name:
raise ValidationError(
"Group names can't have leading or trailing whitespace."
)

def _validate_groupid(self, appstruct):
"""
Validate the ``groupid`` to make sure it adheres to authority restrictions.
Expand Down
28 changes: 28 additions & 0 deletions tests/unit/h/schemas/api/group_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,34 @@ def test_it_raises_if_name_too_long(self, schema):
with pytest.raises(ValidationError, match="name:.*is too long"):
schema.validate({"name": "o" * (GROUP_NAME_MAX_LENGTH + 1)})

def test_it_raises_if_name_isnt_a_string(self, schema):
Copy link
Member

@robertknight robertknight Jul 31, 2024

Choose a reason for hiding this comment

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

There are tests for the sad path here, but what about the happy path? If I copy some text with leading/trailing whitespace and paste it into the name field, the group name should be trimmed, assuming it is neither too short nor too long.

name = 42

with pytest.raises(
ValidationError, match=f"name: {name} is not of type 'string'"
):
schema.validate({"name": name})
Comment on lines +43 to +49
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When calling string methods like .trim() in the validation() method, it'd be easy to introduce a crash when someone calls the API with a non-string name. Added this test to cover that.


@pytest.mark.parametrize(
"name",
[
f" {'o' * GROUP_NAME_MIN_LENGTH}",
f"{'o' * GROUP_NAME_MIN_LENGTH} ",
" " * GROUP_NAME_MIN_LENGTH,
],
)
def test_it_raises_if_name_has_leading_or_trailing_whitespace(self, schema, name):
with pytest.raises(
ValidationError,
match="Group names can't have leading or trailing whitespace.",
):
schema.validate({"name": name})

def test_it_validates_with_no_data(self, schema):
appstruct = schema.validate({})

assert appstruct == {}
Comment on lines +66 to +69
Copy link
Contributor Author

@seanh seanh Jul 30, 2024

Choose a reason for hiding this comment

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

GroupAPISchema is the base schema that CreateGroupAPISchema and UpdateGroupAPISchema inherit from. CreateGroupAPISchema makes name required but UpdateGroupAPISchema doesn't. So in both GroupAPISchema and UpdateGroupAPISchema an empty dict is valid data. When adding manual code about name to validate() it would be easy to write code that crashes if there's no name in the given data, so adding this test to cover that case.


def test_it_validates_with_valid_name(self, schema):
appstruct = schema.validate({"name": "Perfectly Fine"})

Expand Down
Loading