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

device tracker - tomato https support #11566

Merged
merged 13 commits into from
Jan 24, 2018
Merged

device tracker - tomato https support #11566

merged 13 commits into from
Jan 24, 2018

Conversation

GregoryDosh
Copy link
Contributor

@GregoryDosh GregoryDosh commented Jan 10, 2018

Description:

Adding https support to the tomato device tracker. Still need to update documentation in other repo.

Related issue (if applicable): fixes #

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4389

Example entry for configuration.yaml (if applicable):

Checklist:

  • The code change is tested and works locally.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@homeassistant
Copy link
Contributor

Hi @GregoryDosh,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@tschmidty69
Copy link
Contributor

Nice, was wondering why the routers didn't have https support. It would be ideal to have tests for the configuration at least. If you are inspired the unifi test looks like a good base to start from: https://github.com/home-assistant/home-assistant/blob/dev/tests/components/device_tracker/test_unifi.py

@GregoryDosh
Copy link
Contributor Author

I was also rather surprised by the lack of https support. I can look into adding some tests based on unifi tests.

"-42,5500,1000,7043,0]];\n"
"dhcpd_lease = [ ['chromecast','172.10.10.5','F4:F5:D8:AA:AA:AA',"
"'0 days, 16:17:08'],['wemo','172.10.10.6','58:EF:68:00:00:00',"
"'0 days, 12:09:08']];", 200)

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

"-42,5500,1000,7043,0],['eth1','58:EF:68:00:00:00',"
"-42,5500,1000,7043,0]];\n"
"dhcpd_lease = [ ['chromecast','172.10.10.5','F4:F5:D8:AA:AA:AA',"
"'0 days, 16:17:08'],['wemo','172.10.10.6','58:EF:68:00:00:00',"

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

return MockSessionResponse("wldev = [ ['eth1','F4:F5:D8:AA:AA:AA',"
"-42,5500,1000,7043,0],['eth1','58:EF:68:00:00:00',"
"-42,5500,1000,7043,0]];\n"
"dhcpd_lease = [ ['chromecast','172.10.10.5','F4:F5:D8:AA:AA:AA',"

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

elif "gimmie_good_data" in args[0].body:
return MockSessionResponse("wldev = [ ['eth1','F4:F5:D8:AA:AA:AA',"
"-42,5500,1000,7043,0],['eth1','58:EF:68:00:00:00',"
"-42,5500,1000,7043,0]];\n"

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent

return MockSessionResponse('This shouldn\'t (wldev = be here.;', 200)
elif "gimmie_good_data" in args[0].body:
return MockSessionResponse("wldev = [ ['eth1','F4:F5:D8:AA:AA:AA',"
"-42,5500,1000,7043,0],['eth1','58:EF:68:00:00:00',"

Choose a reason for hiding this comment

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

continuation line under-indented for visual indent


CONF_HTTP_ID = 'http_id'

_LOGGER = logging.getLogger(__name__)
_LOGGER = logging.getLogger("{}.{}".format(__name__, "Tomato"))
Copy link
Member

Choose a reason for hiding this comment

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

No need to change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I was just pulling the logger on L51 to match here. I can revert the change and still pull L51 out.

@pvizeli
Copy link
Member

pvizeli commented Jan 17, 2018

What is the reason to use https with disable verify? In this case you can also use http

@GregoryDosh
Copy link
Contributor Author

GregoryDosh commented Jan 18, 2018

@pvizeli Not sure I follow. You're saying that if using an address of https://my-router:443 with verify=False that I should just be able to use http://my-router:443 and skip the verify piece?

If that's the case then I don't get that same behavior with my router. My router isn't serving up port 80 and if I don't specify https the requests package gives an error.

Here are the permutations I ran:

import itertools
import requests
import sys
import urllib3

host = "my-router"

scenario_types = {
    "scheme": ["http", "https"],
    "ports": [80, 443],
    "ssl": [True, False],
    "verify_ssl": [True, False, "router.pem"]
}

for scheme, port, ssl, verify_ssl in \
        itertools.product(*scenario_types.values()):
    address = "{}://{}:{}".format(scheme, host, port)
    print("Schema {:<5}, Host {}, Port {:>3}, SSL {:<5}, Verify SSL {:<10}, Address {:<20} ".
          format(scheme, host, port, str(ssl), str(verify_ssl), address), end="")
    try:
        if ssl:
            response = requests.get(address,
                                    timeout=3,
                                    verify=verify_ssl)
        else:
            response = requests.get(address, timeout=3)
        print(response.status_code)
    except:
        print(sys.exc_info()[0])

Gives me the following output (note that a status code of 401 is actually good).

Schema http , Host my-router, Port  80, SSL True , Verify SSL True      , Address http://my-router:80   <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port  80, SSL True , Verify SSL False     , Address http://my-router:80   <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port  80, SSL True , Verify SSL router.pem, Address http://my-router:80   <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port  80, SSL False, Verify SSL True      , Address http://my-router:80   <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port  80, SSL False, Verify SSL False     , Address http://my-router:80   <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port  80, SSL False, Verify SSL router.pem, Address http://my-router:80   <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port 443, SSL True , Verify SSL True      , Address http://my-router:443  <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port 443, SSL True , Verify SSL False     , Address http://my-router:443  <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port 443, SSL True , Verify SSL router.pem, Address http://my-router:443  <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port 443, SSL False, Verify SSL True      , Address http://my-router:443  <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port 443, SSL False, Verify SSL False     , Address http://my-router:443  <class 'requests.exceptions.ConnectionError'>
Schema http , Host my-router, Port 443, SSL False, Verify SSL router.pem, Address http://my-router:443  <class 'requests.exceptions.ConnectionError'>
Schema https, Host my-router, Port  80, SSL True , Verify SSL True      , Address https://my-router:80  <class 'requests.exceptions.ConnectionError'>
Schema https, Host my-router, Port  80, SSL True , Verify SSL False     , Address https://my-router:80  <class 'requests.exceptions.ConnectionError'>
Schema https, Host my-router, Port  80, SSL True , Verify SSL router.pem, Address https://my-router:80  <class 'requests.exceptions.ConnectionError'>
Schema https, Host my-router, Port  80, SSL False, Verify SSL True      , Address https://my-router:80  <class 'requests.exceptions.ConnectionError'>
Schema https, Host my-router, Port  80, SSL False, Verify SSL False     , Address https://my-router:80  <class 'requests.exceptions.ConnectionError'>
Schema https, Host my-router, Port  80, SSL False, Verify SSL router.pem, Address https://my-router:80  <class 'requests.exceptions.ConnectionError'>
Schema https, Host my-router, Port 443, SSL True , Verify SSL True      , Address https://my-router:443 <class 'requests.exceptions.SSLError'>
Schema https, Host my-router, Port 443, SSL True , Verify SSL False     , Address https://my-router:443 401
Schema https, Host my-router, Port 443, SSL True , Verify SSL router.pem, Address https://my-router:443 401
Schema https, Host my-router, Port 443, SSL False, Verify SSL True      , Address https://my-router:443 <class 'requests.exceptions.SSLError'>
Schema https, Host my-router, Port 443, SSL False, Verify SSL False     , Address https://my-router:443 <class 'requests.exceptions.SSLError'>
Schema https, Host my-router, Port 443, SSL False, Verify SSL router.pem, Address https://my-router:443 <class 'requests.exceptions.SSLError'>


self.req = requests.Request(
'POST', 'http://{}/update.cgi'.format(host),
'POST', 'http{}://{}:{}/update.cgi'.format(
"s" if self.ssl else "", host, port
Copy link
Contributor

Choose a reason for hiding this comment

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

If ssl is true, port still defaults to 80. Is that what you were expecting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I hadn't even really thought much about that to be honest. I'll throw a small update in for that so that it'll default to 80 or 443 depending on if there is/isn't ssl enabled.

Copy link
Contributor

@sdague sdague left a comment

Choose a reason for hiding this comment

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

All seems like good adds to me. Thanks for this.

@sdague sdague merged commit d65ac74 into home-assistant:dev Jan 24, 2018
@balloob balloob mentioned this pull request Jan 26, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants