Hyundai EU - Add support for EVs#25
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| - | - | Generic High Entropy Secret | 3e3ceca | Sources/BetterBlueKit/API/HyundaiEurope/HyundaiEuropeAPIClient.swift | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Hey, thanks for sending this over! I'll review it later today. |
|
@schmidtwmark if there's any way to test a build with this in it, I can give it a try and give feedback. |
|
You can use bbcli to test the API directly. There may be some changes needed to bbcli to support these fixes |
|
It is only a first draft where mainly the information fetch is working at least for my mj25 ioniq5. Thats the current status tested with CCS2 endpoints:
Did i forget something? Feel free to merge it already to provide the first draft also to others that may own an older model car. |
|
@Nachtlatscher so to get the refresh token, do I just inspect the network traffic of the stock Bluelink app and intercept the refresh token? |
You need to do the login workaround of homeassitant project / phyton api to get the token. Maybe there is also some information about the process. |
There was a problem hiding this comment.
Looks like a good start! Some general feedback:
I like the idea of having ResponseKey / ResponseKeyPath, but I think it could be much cleaner. Instead of having getKeyForPath(string, bool), you should get the correct ResponseKey -> String map once at the top of the response based on whether it needs CCS2 or not. Then, you can just do keyPathMap[.chargeTime] or whatever to get the full path.
The response keys should be moved into the API/HyundaiEurope directory at minimum, and named appropriately. They are not relevant to other brands/regions
My understanding here is that this requires the user to use a separate Python script to fetch their refresh token, which is then fed into the password field. Is there a reason that cannot be integrated into bbcli / the BetterBlue app directly? As the person who will have to respond to emails about this, I'm going to get a lot of confused and angry people if they have to run a Python script.
| public func convert(_ temperature: Double, to targetUnits: Units) -> Double { | ||
| let convertedTemperature: Double | ||
|
|
||
| convertedTemperature = switch (self, targetUnits) { |
There was a problem hiding this comment.
Let's reuse code here -- if you're going to add a convert function, then the format function should call it directly
| public var regId: String, vin: String, model: String | ||
| public var accountId: UUID, fuelType: FuelType | ||
| public var generation: Int, odometer: Distance, vehicleKey: String? | ||
| public var ccs2Supported: Bool |
There was a problem hiding this comment.
I think we need a better approach for handling region / brand specific options. Can we add an enum like:
case hyundaiEurope(ccs2Supported: bool)
}```
Then I could make a separate PR for moving vehicleKey into a `case kiaUS` enum value
There was a problem hiding this comment.
i currently rework that to a version you hopefully more happy with
| public let accessToken: String | ||
| public let refreshToken: String | ||
| public let expiresAt: Date | ||
| public var deviceId: String |
There was a problem hiding this comment.
There's already a notion of DeviceID in the APIClientConfiguration. Why do we need it here as well?
There was a problem hiding this comment.
Yes it is but Hyundai/Kia Europe requires to register the device against the API.
And the configuration value cant be updated.
I dont know if this registration has some kind of lifetime.
Currently i see two szenarios:
- give the possibility to update the deviceId in configuration
- on Account init i see the configuration and the client is created.
Maybe it would be possible to call a "registerDevice()" with the created Api that saves the deviceId in "Account"
-> each time a client is created the config would use this stable id then.
-> config parameter still has to be editable because you first need to create the API to call the endpoint or return nil if its not implemented for that Api?
I tryed solution 2 but its not working
If i put in the function to ApiProtocolClient and write a default to prevent implementation for each Brand
it allways calls the default instead of EU Impl.
I don´t know why...
If you can provide the information how it should work or an other idea i would be happy.
There was a problem hiding this comment.
Update i got it working now.
Maybe look again at current commit and tell me what you think about it
| func commandPathAndBody(for command: VehicleCommand) -> (String, [String: Any]) { | ||
| func commandPathAndBody(for command: VehicleCommand, ccs2: Bool = true, deviceId: String) | ||
| -> (String, [String: Any]) { | ||
| switch command { |
There was a problem hiding this comment.
This would be easier to read if you split this into two functions -- one for ccs2, one for legacy. Or, switch on (command, ccs2)
schmidtwmark
left a comment
There was a problem hiding this comment.
Thanks for the improvements, looking better!
| self.expiresAt = expiresAt | ||
| } | ||
|
|
||
| public mutating func setDeviceID(_ deviceId: String) -> Self { |
There was a problem hiding this comment.
no it is not i forgot to delete it.
| } | ||
| } | ||
|
|
||
| public enum VehicleMarketOptions: Codable, Equatable, Sendable { |
There was a problem hiding this comment.
Something I hadn't thought about when I suggested this: this is going to end up stored in SwiftData, which is notoriously bad at handling data structures more complicated than arrays. I think this is the right approach, and having Codable should make it work, but in my experience this could lead to to some weird crashes on startup. Just make sure to test the full app thoroughly by fully killing and and reopening it -- if you see crashes when it tries to read this value, then there's a problem.
There was a problem hiding this comment.
no crashes so far but i will do some more testing for sure.
| } | ||
|
|
||
| // MARK: - default so set deviceId in configuration | ||
| public func registerDevice() async throws -> String? { |
There was a problem hiding this comment.
It looks like this is is still creating a new deviceID, which should be handled by the Account in BetterBlue?
There was a problem hiding this comment.
I did not find a better solution.
EU Hyundai and Kia need that deviceId requested by api call -> done corresponding method in HyundaiEuropeApi
Account does following:
- Create an APIClient with an configuration (no deviceId)
- this Client is used then to call "registerDevice() -> String?"
->if an Api does not define this method then the UUID is returned and set in currend used configuration
(thats more or less what you did in Account Line 122 for commented for kia but done for all)
If you look into ApiClient.swift there is a default implementation too (Line:144).
But i cant do the configuration overwrite there.
And the FakeAPIClient is the reason why there is still a default in Line 144 of ApiClient.
The FakeAPIClient is not a SubClass of APIBaseClient
What do you think?
Is there a better way?
| try await fetchVehicleStatus(for: vehicle, authToken: authToken, cached: true) | ||
| } | ||
|
|
||
| public func registerDevice() async throws -> String? { |
There was a problem hiding this comment.
It's unclear what this is meant to be doing
There was a problem hiding this comment.
see on the other comment.
Its just needed because FakeAPIClient
+ login as with username and password to generate access and refresh token + refresh token is used for login with priority after first login + parsing of CCNC and non CCNC car data + get a command token for command execution + add functions to get values directly from deeper json path
|
@schmidtwmark sry for the force pushes i had to resolve conflicts with your lates changes and changed commit informations to my private Information instead of business one. With this state of the branch i got rid of only using a refresh token.
Each additional Login (Access Token expired) will use the Refresh Token to get a new Access Token. I think thats a good start for an initial merge. |
schmidtwmark
left a comment
There was a problem hiding this comment.
This is looking pretty good!
One other thing to add -- BetterBlueKit includes bbcli, a CLI swift program that lets you login and fetch status. It's really useful for testing without having to run the entire BetterBlue app. It can also be used to test a captured JSON response without pinging the server again.
Can you add an (optional) refreshToken parameter to bbcli and the plumbing to read it correctly?
| return current | ||
| } | ||
|
|
||
| private func getAnyFromJson(from data: [String: Any], key keyString: String?) -> Any? { |
There was a problem hiding this comment.
These functions could probably be moved into a generic API parsing swift file -- would be nice to reuse this keystring setup for other APIs!
+ if refresh token runs into error and password is given try to get a new refresh token + error if login fails because of credentials
schmidtwmark
left a comment
There was a problem hiding this comment.
This is looking good! I’m going to merge this (and the BetterBlue changes) as is later tonight. I’ll need to test and make sure it doesn’t break existing stuff, but then I’ll roll it out later this week.
Nice i will look at the encrypted password flow commented to the issue. So that will maybe be the next step then to switch from unencrypted to encrypted one for next pull request. |
Let us know so we can test |
Changes Login to use refresh token.
Implementation to fetch data for EV Cars
Fix: #8