Polestar: change to oauth2#28604
Conversation
|
@andig based on your comment in #28466 I made a first draft to change the Polestar Vehicle implementation to oauth2. As I am really not an expert in this, I made it as a draft for now, collecting some feedback. The login works for me but will do some further testing as well. Looking forward to your feedback which is hopefully not too bad 😉 |
|
Not sure I fully understand the implementation. Regarding login as refresh fallback: this is only needed if refresh tokens expire. Only seeing this with very unfriendly IDPs... If not, the initial login could happen once and refresh be done as oauth2 standard without need for |
We are talking about Polestar here 🤣 But I guess it makes sense to change it. Still trying to fully understand how this works... |
|
Nice- good to merge if this works for you :) |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
login, the error fromcookiejar.New(nil)is currently ignored (v.Client.Jar, _ = ...); if jar creation fails you may want to surface or at least log this instead of silently proceeding with a nil/old jar. - Consider making
OAuth2Configunexported (e.g.oauth2Config) or documenting its intended external use, since a mutable global*oauth2.Configexported from the package can be modified by callers in ways that affect authentication behavior unexpectedly. - In
NewIdentityandloginyou usecontext.Background()to build the OAuth2 context; if you expect callers to control cancellation/timeouts, consider threading a context intoNewIdentityinstead of hard-codingBackground.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `login`, the error from `cookiejar.New(nil)` is currently ignored (`v.Client.Jar, _ = ...`); if jar creation fails you may want to surface or at least log this instead of silently proceeding with a nil/old jar.
- Consider making `OAuth2Config` unexported (e.g. `oauth2Config`) or documenting its intended external use, since a mutable global `*oauth2.Config` exported from the package can be modified by callers in ways that affect authentication behavior unexpectedly.
- In `NewIdentity` and `login` you use `context.Background()` to build the OAuth2 context; if you expect callers to control cancellation/timeouts, consider threading a context into `NewIdentity` instead of hard-coding `Background`.
## Individual Comments
### Comment 1
<location path="vehicle/polestar/identity.go" line_range="58" />
<code_context>
func (v *Identity) login() (*oauth2.Token, error) {
- cv := oauth2.GenerateVerifier()
+ v.Client.Jar, _ = cookiejar.New(nil)
- data := url.Values{
</code_context>
<issue_to_address>
**issue (bug_risk):** Check and handle the error from cookiejar.New instead of ignoring it.
Ignoring the `cookiejar.New` error can hide why login or cookie handling fails if jar creation ever breaks (e.g., due to resource limits or future changes). Please either return the error from `login` or log it and avoid proceeding as if a jar was successfully created.
</issue_to_address>
### Comment 2
<location path="vehicle/polestar/identity.go" line_range="53" />
<code_context>
- v.TokenSource = oauth.RefreshTokenSource(token, v.refreshToken)
-
- return v, nil
+ ctx := context.WithValue(context.Background(), oauth2.HTTPClient, v.Client)
+ return oauth2.ReuseTokenSource(token, OAuth2Config.TokenSource(ctx, token)), nil
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align context handling for initial token source with the timeout pattern used in login.
In `NewIdentity`, `OAuth2Config.TokenSource` is initialized with `context.Background()` and no timeout, while `login` wraps the HTTP client context with `WithTimeout`. To avoid long-running refresh calls and keep behavior consistent, use `request.Timeout` (as in `login`) for this context as well, or centralize context creation so all token requests share the same timeout policy.
Suggested implementation:
```golang
ctx, cancel := context.WithTimeout(context.Background(), request.Timeout)
defer cancel()
ctx = context.WithValue(ctx, oauth2.HTTPClient, v.Client)
return oauth2.ReuseTokenSource(token, OAuth2Config.TokenSource(ctx, token)), nil
```
1. Ensure `context` is imported at the top of `vehicle/polestar/identity.go`:
- Add `"context"` to the import block if it is not already present.
2. This assumes `request.Timeout` is already defined and used by `login`; if `login` uses a helper like `request.New` or a different timeout constant, align this call to match exactly that pattern (e.g., same timeout constant or helper function).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| func (v *Identity) login() (*oauth2.Token, error) { | ||
| cv := oauth2.GenerateVerifier() | ||
| v.Client.Jar, _ = cookiejar.New(nil) |
There was a problem hiding this comment.
issue (bug_risk): Check and handle the error from cookiejar.New instead of ignoring it.
Ignoring the cookiejar.New error can hide why login or cookie handling fails if jar creation ever breaks (e.g., due to resource limits or future changes). Please either return the error from login or log it and avoid proceeding as if a jar was successfully created.
| v.TokenSource = oauth.RefreshTokenSource(token, v.refreshToken) | ||
|
|
||
| return v, nil | ||
| ctx := context.WithValue(context.Background(), oauth2.HTTPClient, v.Client) |
There was a problem hiding this comment.
suggestion (bug_risk): Align context handling for initial token source with the timeout pattern used in login.
In NewIdentity, OAuth2Config.TokenSource is initialized with context.Background() and no timeout, while login wraps the HTTP client context with WithTimeout. To avoid long-running refresh calls and keep behavior consistent, use request.Timeout (as in login) for this context as well, or centralize context creation so all token requests share the same timeout policy.
Suggested implementation:
ctx, cancel := context.WithTimeout(context.Background(), request.Timeout)
defer cancel()
ctx = context.WithValue(ctx, oauth2.HTTPClient, v.Client)
return oauth2.ReuseTokenSource(token, OAuth2Config.TokenSource(ctx, token)), nil- Ensure
contextis imported at the top ofvehicle/polestar/identity.go:- Add
"context"to the import block if it is not already present.
- Add
- This assumes
request.Timeoutis already defined and used bylogin; ifloginuses a helper likerequest.Newor a different timeout constant, align this call to match exactly that pattern (e.g., same timeout constant or helper function).
|
Hi @andig, hi @loebse Have installed the actual version v0.303.2 just an hour ago an still have the problem. Regards Schrotti |
|
You either have to use the nightly build or wait for v0.303.3 |
|
Thanks for the fast info. Will wait. |
|
Just an update. Updated to v0.304.0 and the Polestar connection works well again 👍 Thanks. |
|
Flowers 💐 for @loebse ! |
|
Updated to v0.304.0 also mein Polestar PS2 wird weiterhin nicht erkannt, |
|
Na dann... |
|
Hi @noottzz Das wird bei mir in der Konfiguration angezeigt und aktualisiert auch wenn das Fahrzeug gar nicht am Ladekabel angeschlossen ist. |
Die Polestar API erlaubt es nach wie vor nicht, zu erkennen wann das Auto angeschlossen ist aber nicht lädt. |
|
Nein, ich habe das Problem "Request failed with status code 400.cannot create vehicle type 'template': cannot create vehicle type 'polestar': login failed: missing authorization code. in/evcc/api/config/test/vehicle/merge/3" |
|
Na dann mach bitte ein neues Issue. Hiermit das das nichts zu tun. |
Based on the comment in #28466 I changed the Polestar Vehicle implementation to oauth2