Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[oauth] Token expiration is calculated from local time #3065

Closed
jlaur opened this issue Aug 16, 2022 · 6 comments · Fixed by #3066
Closed

[oauth] Token expiration is calculated from local time #3065

jlaur opened this issue Aug 16, 2022 · 6 comments · Fixed by #3066
Labels
bug An unexpected problem or unintended behavior of the Core PR pending

Comments

@jlaur
Copy link
Contributor

jlaur commented Aug 16, 2022

When looking at openhab/openhab-addons#13274 I noticed a flaw in AccessTokenResponse: It uses LocalDateTime for calculating token expiration. This means that the token creation time is dependent on system time-zone for being represented as an instant on the time-line.

This has the potential to cause problems if system time-zone is changed or if the LocalDateTime provided for isExpired is generated differently than how OAuthConnector generates it.

It would be less error-prone to use Instant for setCreatedOn, getCreatedOn and isExpired.

I would propose to change this in five steps to avoid breaking addons:

  1. In AccessTokenResponse change createdOn internally to Instant. Add overload for isExpired taking Instant and deprecate current method. Deprecate getCreatedOn and add new getCreatedOnAsInstant (temporary, for migration). Change setCreatedOn to take Instant and adapt OAuthConnector accordingly (currently no addons use this method).
  2. Adapt addons to use getCreatedOnAsInstant and new isExpired taking Instant.
  3. Change implementation of getCreatedOn to the same as getCreatedOnAsInstant and deprecate the latter. Remove isExpired taking LocalDateTime.
  4. Rename usages of getCreatedOnAsInstant to getCreatedOn in addons.
  5. Remove getCreatedOnAsInstant.

See also #2898.

@jlaur
Copy link
Contributor Author

jlaur commented Aug 16, 2022

I am preparing a draft pull request with first step of this proposal.

@J-N-K
Copy link
Member

J-N-K commented Aug 16, 2022

Do you know how many add-ons use this? I think we can do it on one step.

@jlaur
Copy link
Contributor Author

jlaur commented Aug 16, 2022

@J-N-K - it would be simpler, yes, although temporarily breaking. I could prepare PR for addons so it would be ready. Addons affected:

  • getCreatedOn: homeconnect
  • isExpired: automower, ecobee, homeconnect

@J-N-K
Copy link
Member

J-N-K commented Aug 16, 2022

Since this is not user facing, I would consider it acceptable. I can review here and if the PR here and in addons are approved I can merge first. That‘ll at most break one snapshot if addons is not merged the same day.

@jlaur
Copy link
Contributor Author

jlaur commented Aug 16, 2022

@J-N-K - I have now adapted the PR to change the API at once, and also created a PR towards addons (mentioned above).

@wborn wborn added bug An unexpected problem or unintended behavior of the Core PR pending labels Aug 20, 2022
@openhab-bot
Copy link
Collaborator

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/automower-husqvarna/137401/9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core PR pending
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants