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.

Sign up for GitHub

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

Add a mac_address config validation helper #18571

Closed
wants to merge 2 commits into from
Closed

Add a mac_address config validation helper #18571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4a35bc
Add a mac_address config validation helper
ahayworth Nov 18, 2018
036623a
linter: fix whitespace
ahayworth Nov 19, 2018
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
54 changes: 54 additions & 0 deletions homeassistant/helpers/config_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,60 @@ def validator(config):
return validator


def is_mac_address(value=None, separators=None, allow_lowercase=True,
Copy link
Member

Choose a reason for hiding this comment

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

This method seems overkill and is not in line with our other config validation. What we do instead is that we pick a format and normalize everything to be that specific format. A method to format mac addresses is already available here and could be moved to config_validation.

Also this method does weird things by accepting value (unused) and raising Invalid (but that is raised outside of the scope of actual validating config)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balloob I think you're right - I reflected on it this morning and it does indeed feel like overkill. I'll close this and reconfigure the PR that uses it to use other validators. I don't think normalizing MAC addresses in the whole codebase is something I could do soon, though. 😄

FWIW - The reason value was accepted was because it seems to be passed implicitly by voluptuous when you call one of these helpers. Without it (leaving separators=None as the first parameter), you'd end up with weird behavior. For example, calling cv.is_mac_address without arguments would end up calling cv.is_mac_address(separator="whatever you wanted to validate"), and your regular expression would be ^[0-9a-zA-Z]{2}[whatever you wanted to validate]{1}[0-9a-zA-Z{2}[whatever you wanted to validate]{1} .... It was odd.

In any case, thank you for the feedback! 😄

allow_uppercase=True, chunk=2):
"""Validate that a value is a MAC address."""
if not allow_lowercase and not allow_uppercase:
raise vol.Invalid("Must not disallow upper and lowercase")

if separators is None:
separators = ['', '.', ':', '-']
elif not isinstance(separators, list):
raise vol.Invalid('separators must be a list')

if 12 % chunk != 0:
raise vol.Invalid("Invalid chunk size for MAC address")

characters = ["0-9"]
if allow_lowercase:
characters.append("a-z")
if allow_uppercase:
characters.append("A-Z")

repeat = int(12 / chunk)
character_class = "[%s]{%s}" % ("".join(characters), chunk)
format_list = []

for s in separators:
if s == '':
format_list.append(s.join([character_class] * repeat))
else:
sep = "[%s]{1}" % re.escape(s)
format_list.append(sep.join([character_class] * repeat))

regex_list = []
for f in format_list:
regex_list.append(re.compile("^%s$" % f))

def validator(value: Any) -> str:
"""Validate that the value is a MAC address."""
if not isinstance(value, str):
raise vol.Invalid('not a string value: {}'.format(value))

matches = False
for r in regex_list:
if r.match(value):
matches = True
break

if not matches:
err = "value {} is not a permitted MAC address".format(value)
raise vol.Invalid(err)

return value
return validator


# Validator helpers

def key_dependency(key, dependency):
Expand Down
71 changes: 71 additions & 0 deletions tests/helpers/test_config_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,3 +584,74 @@ def test_is_regex():

valid_re = ".*"
schema(valid_re)


def test_is_mac_address():
"""Test the mac_address validator."""
with pytest.raises(vol.Invalid):
schema = vol.Schema(cv.is_mac_address(separators=True))

with pytest.raises(vol.Invalid):
schema = vol.Schema(cv.is_mac_address(chunk=11))

with pytest.raises(vol.Invalid):
schema = vol.Schema(cv.is_mac_address(allow_lowercase=False,
allow_uppercase=False))

with pytest.raises(vol.Invalid):
schema = vol.Schema(cv.is_mac_address(allow_uppercase=False))
schema('0123456789AB')

with pytest.raises(vol.Invalid):
schema = vol.Schema(cv.is_mac_address(allow_lowercase=False))
schema('0123456789ab')

with pytest.raises(vol.Invalid):
schema = vol.Schema(cv.is_mac_address(separators=[':']))
schema('01-23-45-67-89-ab')

with pytest.raises(vol.Invalid):
schema = vol.Schema(cv.is_mac_address(separators=[':']))
schema('not a mac!')

test_2_chunk = [
'01:23:45:67:89:AB',
'01:23:45:67:89:ab',
'01.23.45.67.89.AB',
'01.23.45.67.89.ab',
'01-23-45-67-89-AB',
'01-23-45-67-89-ab',
'0123456789AB',
'0123456789ab',
]
schema = vol.Schema(cv.is_mac_address)
for mac in test_2_chunk:
schema(mac)

test_4_chunk = [
'0123:4567:89AB',
'0123:4567:89ab',
'0123.4567.89AB',
'0123.4567.89ab',
'0123-4567-89AB',
'0123-4567-89ab',
'0123456789AB',
'0123456789ab',
]
schema = vol.Schema(cv.is_mac_address(chunk=4))
for mac in test_4_chunk:
schema(mac)

test_6_chunk = [
'012345:6789AB',
'012345:6789ab',
'012345.6789AB',
'012345.6789ab',
'012345-6789AB',
'012345-6789ab',
'0123456789AB',
'0123456789ab',
]
schema = vol.Schema(cv.is_mac_address(chunk=6))
for mac in test_6_chunk:
schema(mac)