nfarina / homebridge-tesla

Tesla plugin for homebridge: https://github.com/nfarina/homebridge
154 stars 38 forks source link

Add API token support in config #13

Closed aveach closed 5 years ago

aveach commented 5 years ago
This commit adds support for a user to specify an authToken in their
config instead of providing a username and password; since leaving
a password in plain text is typically undesirable.

Be warned that using the authToken or providing a username and password,
in this scenario, is almost identical as far as security is concerned.
Anyone with this token will have full control of the vehicle.

* Resolves #7 Add API token support
nfarina commented 5 years ago

Thanks for contributing this!

Did you test this by supplying an auth token without user/pass? I'd be surprised if it worked as intended, since It doesn't look like the config value is actually read. I'd expect to see something like this.authToken = config["authToken"]; in the constructor.

Also, I don't think the getAuthToken() method even needs to be changed at all - it will simply pick up the this.authToken value and use it as-is without fetching anything else.

Finally, could you add a bit to the README explaining how to use this (for example, how to go about getting the auth token string itself for placing in your config)?

aveach commented 5 years ago

Doh... helps to actually read the config value in the constructor. 🤦‍♂️ So should go without saying, that I did not test it. Fairly new to Homebridge... what would be the best way to test this on my end?

Yeah you're right. Since you call:

const { username, password, authToken } = this;

      // Return cached value if we have one.
      if (authToken) return authToken;

we'll technically just return since the authToken will be "cached"

Sure thing. Can add to the README 👍

nfarina commented 5 years ago

Looks great, thanks for rebasing! OK to merge?

aveach commented 5 years ago

Didn’t get a test setup. Mind giving it a shot before merge?

nfarina commented 5 years ago

Haven't had a chance and I'm headed out of town for a week! I'm 99.9% sure it'll work though, so gonna merge now. Cheers!

aveach commented 5 years ago

@nfarina just noticed a bug haha. Docs show autoToken instead of authToken. Can you make a quick fix?

nfarina commented 5 years ago

Fixed and published!