Add support for UBC1303BA00 and other modems with JSON-formatted lists #24

jussihi · 2020-11-09T00:50:43Z

As it seems like there is some need for the JSON-formatted lists (#13 and #10), and the original devs seem to have abandoned these newer devices... and ofc the fact that I have a UBC1303BA00, I decided to extend the code a little for my needs for HomeAssistant :-)

I have only tested the functionality on my own modem, so I don't know if this change breaks something in the old. I haven't really coded in python so there might be some errors.

Things I did:

Introduce class DigestAuthAuthenticator which does proper Digest HTTP auth for the modem I'm using. Old authenticators were not able to auth with my modem.
Introduced a JSONList boolean param to the MODELS dict. The get_connected_devices_lan method uses this later on when parsing the devices in the JSON list.
Some changes to the code in _get and other parts, see the commit diff...

@mzdrale please merge and bump version so that I can enjoy this from upstream in my HomeAssistant! Cheers!

This commit adds support for UBC1303BA00 and also makes it easy to further expand the compatibility with other Ubee devices which list the LAN devices dynamically from a JSON list.

jussihi · 2020-11-09T01:17:02Z

Ok, fixed some quickly found errors. Mainly: the auto-detection does not seem to work with UBC1303BA00, since it does not have the RootDevice.xml file. But this is not an issue for myself. Maybe it should be written down to some readme or something though...

jussihi · 2020-11-10T22:03:26Z

The latest commit also removes all dictionary entities with "unknown" as their hostname. These seem to be some recently connected but at the moment disconnected devices.

pyubee/__init__.py

StevenLooman · 2020-11-12T20:41:48Z

pyubee/__init__.py

-            _LOGGER_TRAFFIC.debug('  Header: %s: %s', key, value)
-        _LOGGER_TRAFFIC.debug('  Data: %s', repr(response.text))
-
-        return response

    def _post(self, url, data, **headers):


Won't this need the alternate flow for the new Authenticator?

Afaik this is handled by the one-liner requests.get(...). I'll leave this open though, not 100% sure if some manual work is needed here.

StevenLooman · 2020-11-12T20:43:29Z

pyubee/__init__.py

@@ -136,6 +141,32 @@ def headers(self):

        return headers

+class DigestAuthAuthenticator(Authenticator):


This does not seem to do anything by itself, except holding a username/password. Later on, the flow is changed solely for this Authenticator. I am for using as much as possible from other libraries, but I am also for consistency in the software and flow. I see you "forgot" to change the _post() method (probably don't need it in your case.) When somebody else wants to add support for their router, s/he will have to figure out what is happening/why it is done this way.

One option is to make our own DigestAuthenticator, but with its own logic and not relying on requests. Sure, this defeats the re-use of the requests' HTTPDigestAuth, but simplifies the the software in other parts.

Or maybe the current Authenticators can be changed such that these can be passed to requests.get(auth=..)/requests.post(auth=..)

Or perhaps there is another option, one I haven't thought of.

You are right, I didn't need the POST method so I left it untouched. I can change that for sure.

I also would like the code flow to be consistent. However, reinventing the wheel here would just feel stupider. According to all the debug prints in the rest of the _get() method it seems like it is rather cumbersome. On top of that, I don't really see the point in not relying in requests; it is used later on in both _get() and _post() anyway. If I wrote the code from scratch, I would've maybe

used the ready Authenticator classes from requests instead of creating our own (and if our own is needed, create a child class of an requests authenticator)

saved that class as an object in the Ubee class, so that it can be dynamically used as for example inside the _get() as follows: response = requests.get(url, timeout=HTTP_REQUEST_TIMEOUT, auth=_self.authenticator(self.username, self.password)) where self.authenticator was a (derivative of) requests Authenticator class.

So yes, I would've preferred the Authenticators of pyubee to be compatible with requests. It would remove the reinvention of the wheel I was talking before. After the change, the whole GET or POST methods would be one-liners :)

I will take a closer look at the comments tomorrow. Thanks for the review. This is just my opinion on this one comment!

You are entirely right that it is better to use the tools requests provides, thank you for showing this. I guess this was added at some point and others just continued building on the things that were already there.

Though some Authenticators do sneak in their own GETs/POSTs, necessary for some models. Not sure how to handle this, but probably possible.

StevenLooman · 2020-11-12T20:49:53Z

Thank you for contributing. I've added some comments. Feel free to reply if anything is unclear or you have other/better ideas!

StevenLooman · 2020-11-16T20:33:08Z

You can use tox to run flake8/pylint/pydocstyle.

jussihi · 2020-11-17T11:34:20Z

I fixed some of the conventional problems in the latest commit, I left the problematic parts still unresolved. You may edit them and merge when it is properly done. I think that the Authenticator will be as is, since I don't have other Ubee model to test other Authenticator methods if I changed the code flow towards how I did with the DigestAuth.

StevenLooman · 2020-11-20T12:31:50Z

pyubee/__init__.py

+            # We are using digest auth:
+            response = requests.get(url, timeout=HTTP_REQUEST_TIMEOUT, auth=HTTPDigestAuth(self.username, self.password))
+            return response
+        """Use the rudimentary auth""" 


This is a doc-comment, usually found at the top of the method. Please put the old comment (""" Do a HTTP Get.""") at the top, and convert this to a normal comment, i.e.:

# Use the rudimentary auth

StevenLooman · 2020-11-20T12:32:10Z

pyubee/__init__.py

+            # We are using digest auth:
+            response = requests.post(url, data=data, timeout=HTTP_REQUEST_TIMEOUT, auth=HTTPDigestAuth(self.username, self.password))
+            return response
+        """Use the rudimentary auth""" 


Same as the method-comment for the _get() method.

StevenLooman · 2020-11-20T12:32:36Z

Just some minor things. Other than that it looks ok.

@mzdrale If this is resolved, can you release a new version?

mzdrale · 2020-11-20T16:36:19Z

Thank you @jussihi for contributing and @StevenLooman for reviewing this PR.

I'm not near computer right now. I will release a new version tomorrow, probably. In the meantime, @jussihi please resolve @StevenLooman's comments.

mzdrale · 2020-11-25T11:44:01Z

Hey @jussihi , please take a look at Steven's comments above. I'm waiting for you to resolve them before releasing a new version.

mzdrale · 2020-11-27T16:15:34Z

New version 0.11 released.
Thank you guys!

mzdrale · 2020-11-27T18:03:23Z

@jussihi if you want, you can update Home Assistant Ubee component and create PR.

jussihi · 2020-11-27T18:55:17Z

@mzdrale I'm unfamiliar with that process, so you can do it. Thanks.

mzdrale · 2020-11-27T21:41:19Z

Home Assistant PR #43708.

mzdrale · 2020-12-01T11:28:38Z

Unfortunately, Home Assistant guys reviewed code and decided not to merge my PR and to remove Ubee compoment from Home Assistant. See their comment.
We'll have to create custom Home Assistant integration for Ubee.
In the meantime you can update ubee component in your home assistant manually, to use pyubee 0.11 instead of 0.10.

jussihi · 2020-12-01T11:42:25Z

Time to move over to nmap device tracker :-)

mzdrale · 2020-12-01T12:22:16Z

Or to do what I did: Buy a router that supports OpenWRT or DDWRT, for example TP-Link Archer C7, install OpenWRT or DDWRT on it and configure Ubee router just to forward all traffic to your OpenWRT/DDWRT router. I guess you got Ubee router from your ISP, like I did and you can't replace it.

jussihi · 2020-12-01T12:39:07Z

@mzdrale that's what I might very well do.

Does your router allow you to block specific devices from accessing WAN, but they can still access/be accessed from LAN? At the moment, I have a Roomba vacuum but I don't want it to be able to access its cloud. Anyway, I should be able to access the roomba itself from the LAN (homeassistant). Is this setup possible with OpenWRT/DDWRT? At the moment I need to have separate device as the LAN firewall which routes all traffic in my LAN (and blocks WAN for Roomba). I would love to get rid of it. Sorry for offtopic :)

mzdrale · 2020-12-01T14:35:31Z

Absolutely, OpenWRT and DDWRT can do that. It's exactly what I'm doing with my IoT devices.

jussihi · 2020-12-11T19:45:24Z

@Hukuma1 , the component will not come back, as it needs web scraping in order to work. Also, your issue is completely off-topic from this subject we talked about earlier. You can always make a new issue, though. It is indeed possible to make it power cycle every now and then, the request website just needs to be configured to ubee. But yes, I would suggest you to make a new issue.

jussihi added 4 commits November 9, 2020 02:40

Add support for UBC1303BA00

4c0801a

This commit adds support for UBC1303BA00 and also makes it easy to further expand the compatibility with other Ubee devices which list the LAN devices dynamically from a JSON list.

Update supported devices in readme

2423b8d

Fix incorrect unauth and auth URLs for UBC1303BA00

584cdc1

Fix an error in auto-detection of the modem model.

c6a16f2

Remove recently connected 'Unknown'-named devices from the list.

d60ac9c

Remove debug print :-)

65b8088

StevenLooman reviewed Nov 12, 2020

View reviewed changes

Conventions fixed for review

671563b

StevenLooman reviewed Nov 20, 2020

View reviewed changes

Update __init__.py

51172aa

mzdrale merged commit 38b0dcc into mzdrale:master Nov 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for UBC1303BA00 and other modems with JSON-formatted lists #24

Add support for UBC1303BA00 and other modems with JSON-formatted lists #24

jussihi commented Nov 9, 2020 •

edited

Loading

jussihi commented Nov 9, 2020

jussihi commented Nov 10, 2020

StevenLooman Nov 12, 2020

jussihi Nov 17, 2020

StevenLooman Nov 12, 2020

jussihi Nov 12, 2020

StevenLooman Nov 16, 2020

StevenLooman commented Nov 12, 2020

StevenLooman commented Nov 16, 2020

jussihi commented Nov 17, 2020 •

edited

Loading

StevenLooman Nov 20, 2020

StevenLooman Nov 20, 2020

StevenLooman commented Nov 20, 2020

mzdrale commented Nov 20, 2020 •

edited

Loading

mzdrale commented Nov 25, 2020 •

edited

Loading

mzdrale commented Nov 27, 2020

mzdrale commented Nov 27, 2020

jussihi commented Nov 27, 2020

mzdrale commented Nov 27, 2020

mzdrale commented Dec 1, 2020 •

edited

Loading

jussihi commented Dec 1, 2020

mzdrale commented Dec 1, 2020 •

edited

Loading

jussihi commented Dec 1, 2020 •

edited

Loading

mzdrale commented Dec 1, 2020

jussihi commented Dec 11, 2020

		@@ -136,6 +141,32 @@ def headers(self):

		return headers

		class DigestAuthAuthenticator(Authenticator):

Add support for UBC1303BA00 and other modems with JSON-formatted lists #24

Add support for UBC1303BA00 and other modems with JSON-formatted lists #24

Conversation

jussihi commented Nov 9, 2020 • edited Loading

jussihi commented Nov 9, 2020

jussihi commented Nov 10, 2020

StevenLooman Nov 12, 2020

Choose a reason for hiding this comment

jussihi Nov 17, 2020

Choose a reason for hiding this comment

StevenLooman Nov 12, 2020

Choose a reason for hiding this comment

jussihi Nov 12, 2020

Choose a reason for hiding this comment

StevenLooman Nov 16, 2020

Choose a reason for hiding this comment

StevenLooman commented Nov 12, 2020

StevenLooman commented Nov 16, 2020

jussihi commented Nov 17, 2020 • edited Loading

StevenLooman Nov 20, 2020

Choose a reason for hiding this comment

StevenLooman Nov 20, 2020

Choose a reason for hiding this comment

StevenLooman commented Nov 20, 2020

mzdrale commented Nov 20, 2020 • edited Loading

mzdrale commented Nov 25, 2020 • edited Loading

mzdrale commented Nov 27, 2020

mzdrale commented Nov 27, 2020

jussihi commented Nov 27, 2020

mzdrale commented Nov 27, 2020

mzdrale commented Dec 1, 2020 • edited Loading

jussihi commented Dec 1, 2020

mzdrale commented Dec 1, 2020 • edited Loading

jussihi commented Dec 1, 2020 • edited Loading

mzdrale commented Dec 1, 2020

jussihi commented Dec 11, 2020

jussihi commented Nov 9, 2020 •

edited

Loading

jussihi commented Nov 17, 2020 •

edited

Loading

mzdrale commented Nov 20, 2020 •

edited

Loading

mzdrale commented Nov 25, 2020 •

edited

Loading

mzdrale commented Dec 1, 2020 •

edited

Loading

mzdrale commented Dec 1, 2020 •

edited

Loading

jussihi commented Dec 1, 2020 •

edited

Loading