Skip to content

Conversation

@corneyl
Copy link
Contributor

@corneyl corneyl commented Mar 13, 2021

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
Copy link
Owner

MikeBrink commented Mar 14, 2021

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
Copy link
Contributor Author

corneyl commented Mar 16, 2021

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
Copy link
Contributor Author

corneyl commented Mar 16, 2021

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
Copy link
Contributor Author

corneyl commented Mar 22, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

corneyl commented Mar 23, 2021

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

Old situation:

  • A PicnicAPI object is created with username/password.
  • These credentials are used to make a login request
  • The response contains a token (x-picnic-auth) which is saved in a variable
  • The saved token is used to authenticate subsequent requests

Situation in this PR:

  • A PicnicAPI object is created with username/password ór previous auth token (both is still possible).
  • If credentials are supplied:
    • these are used to make a login request
    • The response contains a token (x-picnic-auth) which is saved in a variable
  • If a token is supplied:
    • The token is saved in a variable
  • The saved token is used to authenticate subsequent requests
  • If any subsequent request (so to any endpoint) contains a changed x-picnic-auth token, the saved one is updated to the new one.

So in short:

  • Username/password can still be used using the same interface as before.
  • Support is added for authenticating using an auth token as well.
  • If Picnic replies with an updated auth token, this one is saved an used in subsequent requests (the old one might expire at one point).
  • A method is added to be able to retrieve the auth token for external storage, like e.g. in a Home Assistant config entry.

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
Copy link
Owner

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

@MikeBrink MikeBrink merged commit 18faa77 into MikeBrink:master Mar 25, 2021
@corneyl
Copy link
Contributor Author

corneyl commented Mar 27, 2021

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants