search

kbr / fritzconnection

Python-Tool to communicate with the AVM Fritz!Box by the TR-064 protocol and the AHA-HTTP-Interface
MIT License
303 stars 59 forks source link

Add FritzAuthorizationError #170

Closed mib1185 closed 1 year ago

mib1185 commented 1 year ago

Actually, it is not differentiated between possible reasons for a connection issue (eq. unreachable or un-authorized). This causes issues in handling connection issues (see home-assistant/core/issues/71822)

With this a http-401 will raise a FritzSecurityError, which ensures that the we can distinguish between wrong credentials or just not reachable device.

cc @chemelli74

prior this change

./test.py 
FRITZ!Box 7530 AX at http://192.168.1.1
FRITZ!OS: 7.31
Traceback (most recent call last):
  File "/home/mib85/git/fritzconnection/fritzconnection/core/soaper.py", line 146, in raise_fritzconnection_error
    root = etree.fromstring(response.content)
  File "/usr/lib/python3.10/xml/etree/ElementTree.py", line 1349, in XML
    parser.feed(text)
xml.etree.ElementTree.ParseError: mismatched tag: line 1, column 156

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/mib85/git/fritzconnection/./test.py", line 14, in <module>
    main()
  File "/home/mib85/git/fritzconnection/./test.py", line 10, in main
    info = fritz_status.get_device_info()
  File "/home/mib85/git/fritzconnection/fritzconnection/lib/fritzstatus.py", line 337, in get_device_info
    return ArgumentNamespace(self.fc.call_action("DeviceInfo1", "GetInfo"))
  File "/home/mib85/git/fritzconnection/fritzconnection/core/fritzconnection.py", line 397, in call_action
    return self.soaper.execute(service, action_name, arguments)
  File "/home/mib85/git/fritzconnection/fritzconnection/core/soaper.py", line 265, in execute
    return handle_response(response)
  File "/home/mib85/git/fritzconnection/fritzconnection/core/soaper.py", line 247, in handle_response
    raise_fritzconnection_error(response)
  File "/home/mib85/git/fritzconnection/fritzconnection/core/soaper.py", line 156, in raise_fritzconnection_error
    raise FritzConnectionException(msg)
fritzconnection.core.exceptions.FritzConnectionException: Unable to perform operation. 401 Unauthorized (ERR_NONE)401 UnauthorizedERR_NONEWebserver Sun, 25 Sep 2022 14:58:07 GMT

with this change

./test.py 
FRITZ!Box 7530 AX at http://192.168.1.1
FRITZ!OS: 7.31
Traceback (most recent call last):
  File "/home/mib85/git/fritzconnection/./test.py", line 14, in <module>
    main()
  File "/home/mib85/git/fritzconnection/./test.py", line 10, in main
    info = fritz_status.get_device_info()
  File "/home/mib85/git/fritzconnection/fritzconnection/lib/fritzstatus.py", line 337, in get_device_info
    return ArgumentNamespace(self.fc.call_action("DeviceInfo1", "GetInfo"))
  File "/home/mib85/git/fritzconnection/fritzconnection/core/fritzconnection.py", line 397, in call_action
    return self.soaper.execute(service, action_name, arguments)
  File "/home/mib85/git/fritzconnection/fritzconnection/core/soaper.py", line 265, in execute
    return handle_response(response)
  File "/home/mib85/git/fritzconnection/fritzconnection/core/soaper.py", line 247, in handle_response
    raise_fritzconnection_error(response)
  File "/home/mib85/git/fritzconnection/fritzconnection/core/soaper.py", line 143, in raise_fritzconnection_error
    raise FritzSecurityError(response.text)
fritzconnection.core.exceptions.FritzSecurityError: <HTML><HEAD><TITLE>401 Unauthorized (ERR_NONE)</TITLE></HEAD><BODY><H1>401 Unauthorized</H1><BR>ERR_NONE<HR><B>Webserver</B> Sun, 25 Sep 2022 14:57:28 GMT</BODY></HTML>
kbr commented 1 year ago

A FritzSecurityError for an authorization error would be confusing. Also that the error message is html instead of plain text is a surprising result.

Beside this I want to keep the exception types as small as possible. Some background:

There are roughly two kinds of exceptions: the triple of FritzConnectionException, FritzServiceError, FritzActionError and the group of all other exceptions. The latter gets used for errors reported from the router according to error codes defined by AVM and should not get interpreted or used otherwise.

The former are indicating that the user tries to call a service or an action not matching the known API or that something else has happened. This "something else" is covered by the generic FritzConnectionException with a detailed message about the incident.

For example, if the device or network is unreachable or a timeout happened, the message starts with "Unable to get a connection" followed by more detailed information. Or in case of a permission issue the message starts with "Unable to perform operation" again followed by more information.

This way the error message provides all necessary information for applications to implement a specific error handling.

kbr commented 1 year ago

On the other hand, a FritzAuthorizationError could be useful beside the generic FritzConnectionException, to make it more clear for the user what happened – even if I don't want to introduce more exception types.

mib1185 commented 1 year ago

with your agreement, i would like to add the FritzAuthorizationError (which raises i case of any "Unable to perform operation" case)

kbr commented 1 year ago

Please feel free to do it, but it should not cover any case. Just a html-response with a 401 status should raise this exception. So it could roughly be something like this:

STATUS_UNAUTHORIZED = 401

...

msg = f"Unable to perform operation. {detail}"
if response.status_code == STATUS_UNAUTHORIZED:
    raise FritzAuthorizationError(msg)
raise FritzConnectionException(msg)
mib1185 commented 1 year ago

the FritzAuthorizationError is added and tests were extended to check that only http/401 will raise it. Further some small cleanup of unesed imports in soaper test module

chemelli74 commented 1 year ago

Hi Klaus, can you consider to release a patch version to include this change ?

Thank you in advance,

Simone

kbr commented 1 year ago

Hi Simone, sorry for the delay. Patch version is difficult, because everything is merged to master. Something I should change. Hope to make a new release over the holidays.

chemelli74 commented 1 year ago

Hi Klaus, can you be so kind are lease 1.10.4 ?

Simone

kbr commented 1 year ago

Hi Simone, unfortunately holiday season turns out to be busy season. Hope to care about the next release soon.

kbr commented 1 year ago

Hi Simone, took a long time, but got it -> 1.11.0