-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fleet API support #36
Conversation
@brianflex @briangru @ramonsmits I'd appreciate your review if you have the time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only quickly reviewed the lib changes. I'll review but more importantly test from the EU (Netherlands).
library/Scopes.cs
Outdated
public static string EnergyDeviceData = "energy_device_data"; | ||
public static string EnergyCommands = "energy_cmds"; | ||
|
||
public static string GetScopeString(params string[] scopes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a public API I guess it would be better that it would be based on an IEnumerable<string>
but that is my personal preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that still work with params
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't but it would allow to just pass for example a HashSet or List.
public static string GetScopeString(params string[] scopes) | ||
{ | ||
var sb = new StringBuilder(); | ||
sb.Append("openid offline_access "); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these also scopes? Why not add this as public static strings too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my reading of the docs, these are always needed, so I thought it was best if users don't need to specify them every time.
@@ -33,15 +33,29 @@ public class TeslaAuthHelper | |||
{ | |||
const string TESLA_CLIENT_ID = "81527cff06843c8634fdc09e8ac0abefb46ac849f38fe1e431c2ef2106796384"; | |||
const string TESLA_CLIENT_SECRET = "c7257eb71a564034f9419ee651c7d0e5f7aa6bfbd18bafb5c5c033b093bb2fa3"; | |||
const string TESLA_REDIRECT_URI = "https://auth.tesla.com/void/callback\""; | |||
const string TESLA_SCOPES = "openid email offline_access"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here also scopes that should be used from Scopes
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the legacy Owner API scopes.
string code = HttpUtility.ParseQueryString(location.Query).Get("code"); | ||
return code; | ||
} | ||
|
||
|
||
async Task<Tokens> ExchangeCodeForBearerTokenAsync(string code, HttpClient client, CancellationToken cancellationToken) | ||
{ | ||
var body = new JObject |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use System.Text.Json.Nodes ? Could potentially remove the dependency on Newtonsoft.Json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks will do
library/TeslaAuthHelper.cs
Outdated
}; | ||
{"redirect_uri", redirectUri}, | ||
{ "scope", scopes }, | ||
{ "audience", "https://fleet-api.prd.na.vn.cloud.tesla.com" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, but isn't this url based on region? See https://developer.tesla.com/docs/fleet-api#overview
- North America, Asia-Pacific (excluding China): https://fleet-api.prd.na.vn.cloud.tesla.com/
- Europe, Middle East, Africa: https://fleet-api.prd.eu.vn.cloud.tesla.com/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch this was on my Todo list and I missed it. Although it would be good to test if this actually matters.
Thanks @ramonsmits - I addressed most of your feedback, and also improved comments and added a new console sample. |
Co-authored-by: Ramon Smits <ramon.smits@gmail.com>
Co-authored-by: Ramon Smits <ramon.smits@gmail.com>
@ramonsmits Thanks for the fast reviews. Have you been able to test it yet? |
I checked but it seems I need to create an account. I don't know if this
will affect my regular account as it is only free for a short time so
unfortunately not going to test to find out.
…On Tue, Oct 17, 2023, 10:28 Tom Hollander ***@***.***> wrote:
@ramonsmits <https://github.com/ramonsmits> Thanks for the fast reviews.
Have you been able to test it yet?
—
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABFLJRGTXCTJPI3BE24DI3X7Y6SRAVCNFSM6AAAAAA6B4FSZ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONRVHEZDMMBZGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Updated the library to support the Fleet API.
Removed the code that supported the pure API (browserless) authentication since it doesn't work anymore.
Updated WPF sample
Added new ASP.NET Core web sample
Updated README.
Note, I've tested this with my own apps registered for the Tesla Fleet API. While I can successfully obtain tokens, I'm yet to figure out the partner registration process so I haven't proven the tokens can be used. I'll get to that soon but I was keen to get others to try the API in the meantime.