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

Fix form validation with equal and in operator for adding expression with group to filter #2750

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
46 changes: 24 additions & 22 deletions python/nav/web/alertprofiles/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -650,29 +650,31 @@ def clean(self) -> Dict[str, Any]:
value = validated_data["value"]

if match_field.data_type == MatchField.IP:
if operator_type == Operator.IN:
ip_list = value.split()
else:
ip_list = [value]
validated_ip_addresses = []
for ip in ip_list:
if not is_valid_ip(ip=ip, strict=True) and not is_valid_cidr(cidr=ip):
self.add_error(
field="value",
error=forms.ValidationError(("Invalid IP address: %s" % ip)),
)
else:
validated_ip_addresses.append(str(ip))
# Bring ip address back into original format to be processed below
value = " ".join(validated_ip_addresses)
validated_data["value"] = self._clean_ip_addresses(
operator_type=operator_type, value=value
)
return validated_data

if operator_type == Operator.IN:
"""If input was a multiple choice list we have to join each option in one
string, where each option is separated by a | (pipe).
If input was a IP adress we should replace space with | (pipe)."""
if match_field.data_type == MatchField.IP:
validated_data["value"] = value.replace(' ', '|')
else:
validated_data["value"] = "|".join(value)
validated_data["value"] = "|".join(value)
elif operator_type == Operator.EQUALS:
validated_data["value"] = value[0]
Comment on lines +659 to +661
Copy link
Contributor

Choose a reason for hiding this comment

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

You could do early returns. If you plan one for every operator_type this order is good, since EQUALS and IN are the most common (exists for the most match_fields).

Copy link
Contributor

Choose a reason for hiding this comment

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

If there are other cases of match_fields behaving oddly you can split out handling of those too, like for ip-addresses. Isolate 'em!

Copy link
Contributor

Choose a reason for hiding this comment

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

The odd ones are "ip_address", "severity" and "sysname". "sysname" can do partial string matching, "severity" does numerical comparison. They're both candidates for their own methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does look to me that the actual cleaning for "sysname" and "severity" will look identical to EQUALS. If you do an early return after IN you could remove the "if" for EQUALS and just do value = value[0]. "severity" might need casting to int but that should be it!


return validated_data

def _clean_ip_addresses(self, operator_type, value):
if operator_type == Operator.IN:
ip_list = value.split()
else:
ip_list = [value]
validated_ip_addresses = []
for ip in ip_list:
if not is_valid_ip(ip=ip, strict=True) and not is_valid_cidr(cidr=ip):
self.add_error(
field="value",
error=forms.ValidationError(("Invalid IP address: %s" % ip)),
)
else:
validated_ip_addresses.append(str(ip))

return "|".join(validated_ip_addresses)
32 changes: 30 additions & 2 deletions tests/integration/web/alertprofiles_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -405,10 +405,12 @@ def test_alertprofiles_add_expression_with_multiple_non_valid_ip_addresses_shoul
).exists()
assert f"Invalid IP address: {invalid_ip}" in smart_str(response.content)

def test_alertprofiles_add_expression_with_multiple_alert_types_should_succeed(
def test_alertprofiles_add_expression_with_in_condition_should_succeed(
self, client, dummy_filter
):
"""Tests that an expression with multiple alert types can be added"""
"""Tests that an expression with an in condition can be added, alert type is
just an example
"""
string_match_field = MatchField.objects.get(name="Alert type")
url = reverse("alertprofiles-filters-saveexpression")
data = {
Expand All @@ -429,6 +431,32 @@ def test_alertprofiles_add_expression_with_multiple_alert_types_should_succeed(
response.content
)

def test_alertprofiles_add_expression_with_equals_condition_should_succeed(
self, client, dummy_filter
):
"""Tests that an expression with an equals condition can be added, group is
just an example
"""
group_match_field = MatchField.objects.get(name="Group")
url = reverse("alertprofiles-filters-saveexpression")
data = {
"filter": dummy_filter.pk,
"match_field": group_match_field.pk,
"operator": Operator.EQUALS,
"value": "AD",
}
response = client.post(url, data=data, follow=True)
assert response.status_code == 200
assert Expression.objects.filter(
filter=dummy_filter,
match_field=group_match_field,
operator=Operator.EQUALS,
value=data["value"],
).exists()
assert f"Added expression to filter {dummy_filter}" in smart_str(
response.content
)


class TestsPermissions:
def test_set_accountgroup_permissions_should_not_crash(self, db, client):
Expand Down
Loading