search

bogosj / tesla

Provides a wrapper around the API to easily query and command a Telsa car.
Other
23 stars 18 forks source link

Migrate oauth2 functionality to the oauth2 library #11

Closed michaelharo closed 3 years ago

andig commented 3 years ago

@michaelharo you're basically rebuilding the uhthomas library but without the MFA part? Why not just use the OAuth config from there?

andig commented 3 years ago

Sorry, I was wrong.

My idea was/is to make the authorisation stuff from https://github.com/uhthomas/tesla/blob/master/cmd/login/main.go as reusable as possible, including access to the oauth config. Currently that's not the case, discussion in https://github.com/uhthomas/tesla_exporter/issues/7#issuecomment-777197505. So instead of merging here I'd propose to work with uhthomas to provide a clean interface.

michaelharo commented 3 years ago

I think the login stuff should exist, but not be part of the NewClient interface as we each have different ways we want to provide login functionality, but once we have an oauth2 access/refresh token I assume everything else is common.

andig commented 3 years ago

I would like the ability to pass in an HTTP client that is configured for logging (instrumenting its roundtripper). Oauth does that by using the context, so the oauth client could handle authentication, token renewal and logging. The remaining pieces would be documentation how to create that client (using uhthomas) and improving the api of uhthomas.

I think the login stuff should exist

In terms of being duplicated from uhthomas? For what purpose?

michaelharo commented 3 years ago

As long as the context passed as part of NewClient has the http client associated in it then I think that meets your requirement.

michaelharo commented 3 years ago

@andig Do you have any additional concerns about this pull request?

andig commented 3 years ago

LGTM! Should we mention in the README that injecting a client can be done via the context as its not obvious?

andig commented 3 years ago

Fix #8