N4S4 / synology-api

A Python wrapper around Synology API
MIT License
368 stars 138 forks source link

Improve login handling #106

Closed eddybl closed 1 year ago

eddybl commented 1 year ago

I am just starting with the API and tried to connect using the core module.

For me, base_api_core.Core() failed on line 9:

self.session.login('Core')

Relevant part of the traceback:

<...>synology_api\base_api_core.py in __init__(self, ip_address, port, username, password, secure, cert_verify, dsm_version, debug, otp_code)
      7         self.session = syn.Authentication(ip_address, port, username, password, secure, cert_verify, dsm_version, debug, otp_code)
      8 
----> 9         self.session.login('Core')
     10         self.session.get_api_list('Core')
     11         self.session.get_api_list()

<...>synology_api\auth.py in login(self, application)
     40         else:
     41             session_request = requests.get(self._base_url + login_api, param, verify=self._verify)
---> 42             self._sid = session_request.json()['data']['sid']
     43             self._session_expire = False
     44             if self._debug is True:

KeyError: 'data'

The error is misleading, since auth.login() has no error checking on the requests.get() result, but what happens is that session_request .json() has this content:

{'error': {'code': 402}, 'success': False}

And finally, 402 seems to be permission denied.

My first suggestion would be to improve the auth.login()in a similar fashion to auth.logou() and check for

if response.json()['success'] is True:

But there is also a PR which tries to improve the situation: https://github.com/N4S4/synology-api/pull/100 I like the addition of the error codes, but since the original author was not responding I wonder if it makes sense to make a similar PR which resolves your requests?

eddybl commented 1 year ago

I looked into the behavior of the Authentication class a bit more and I think there are some more general issues:

  1. Failed login attempts might only result in an unrelated error message (see above)
  2. The returns for login() etc are not used anywhere, since the returns are never used. This should probably be prints instead (for debugging)
  3. most other methods other then login() seem to use a fixed version:, although they should probably all use version: self._version
  4. A failed login does not result in any exception, so the code continues and may result in the debug output You are now logged in!
N4S4 commented 1 year ago

It surely needs to improve, I was working on error handling function as PR #100 suggest but I wasn't much into it. Ill try to work on it and take your suggestion time permitting

eddybl commented 1 year ago

I will try to work on a pull request. Once I started looking into it it got just bigger then I originally thought, so it might take a couple of days to find enough time ;)

N4S4 commented 1 year ago

lol me too, however I'll do my best to improve this part

N4S4 commented 1 year ago

ok so based on PR #100 and very little changes I managed to let error codes work and implemented in all modules, I still need to add it for logout process. I can upload to a new branck and if you want to test too would be great

check it out at error codes branch

eddybl commented 1 year ago

Ok, I will try to have a look this evening!

eddybl commented 1 year ago

I added a PR on the error_code branch: #108

N4S4 commented 1 year ago

closed with PR #109