MikeBrink / python-picnic-api

Unofficial Python wrapper for the Picnic API.
Apache License 2.0
53 stars 17 forks source link

Add support for authentication using auth-token #11

Closed corneyl closed 3 years ago

corneyl commented 3 years ago

In a review comment of the PR to add the Picnic integration to HA (home-assistant/core#47507) it was requested to not save the username/password, but the auth token which is used to authenticate each request.

This PR adds support for using the auth token as authentication means. I've also added some tests to test the new behaviour.

Thanks again for reviewing!

MikeBrink commented 3 years ago

Hi,

First of all, thank you for your second contribution! I do understand the need for logging in using a token. However, this token expires, is this handled by your integration?

corneyl commented 3 years ago

The expiration of the token results in a PicnicAuthError being thrown like with any auth error, which can then indeed be handled by the integration. I've not yet implemented this, but will add this indeed.

Actually, now I'm testing with it a bit more in depth, the auth token can change already in the response of a request. So it will auto-renew if we use this token. In other words, when using an auth token in the request, the response can contain a new auth token. I'll update the PR so the token of the session is updated and it's possible to easily extract it.

corneyl commented 3 years ago

The change became a bit bigger because of this, sorry... 😃 Also I've made the responsibilities of the client and session classes a bit more strict, but the PicnicAPI interface is still fully backwards compatible. Let me know what you think!

corneyl commented 3 years ago

Hi Mike, I know you're busy with you thesis, but can you quickly check if you have any remarks on this PR? Thanks!

MikeBrink commented 3 years ago

Hi corneyl,

I wanted to do some longer running tests, but I didn’t have the time yet. I understand the need to not store the user credentials, but it might cause some issues for users whom do not regularly send requests to picnic. I wanted to test how often these requests should be (I think monthly would be an absolute minimum, what do you think?). I also see your needs for me to push this quickly and I would love to do so. Do you have an idea of how often these requests should be? And is it okay to make it optional to store usercredentials?

Love to hear your thoughts

corneyl commented 3 years ago

I'll explain the changes a bit more, including their impact.

Old situation:

Situation in this PR:

So in short:

I hope this made it a bit more clear! Btw, I tried with a token that's a few weeks old and it still worked, but an updated token was send in the response. This new key also worked for authentication, and if used, the key in the response was the same. So it's just a way to update the auth tokens so now and then. I expect the app to work the same, for safety reasons it probably also doesn't save the credentials of the user, but only the auth token.

Thanks!

MikeBrink commented 3 years ago

Okay thanks for the clarification! I’ll upload the new version tomorrow to pypi!

corneyl commented 3 years ago

Thanks a lot!