feat(BackendApiService): improved logon to backend - #2718
Conversation
…on based on fleet telemetry issues
…ter handling them
feat(CloudConnection): can show error message on redirect
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements OAuth2 authentication with PKCE for the cloud connection feature, including new API endpoints for authorization and token exchange. The review feedback highlights several areas for improvement: redundant using directives, the need for localization of hardcoded strings in both the UI and snackbar notifications, the importance of robust URL cleanup in the OAuth flow, missing ConfigureAwait(false) calls in asynchronous database operations, and the inconsistency caused by mixing System.Text.Json and Newtonsoft.Json libraries.
| @inject NavigationManager NavigationManager | ||
| @inject ITextLocalizationService TextLocalizer | ||
| @inject IOAuthNotificationService OAuthNotificationService | ||
| @using TeslaSolarCharger.Client.Services.Contracts |
There was a problem hiding this comment.
This @using directive is redundant as it is already declared on line 4.
| <MudText Typo="Typo.body1" Class="mb-4">To connect to the cloud, please log in with your Solar4Car.com account. You will be redirected to the secure login page.</MudText> | ||
|
|
||
| <RightAlignedButtonComponent OnButtonClicked="Submit" | ||
| ButtonText='@T(TranslationKeys.CloudConnectionLoginButton)' | ||
| ButtonText='Log in with Solar4Car.com' |
There was a problem hiding this comment.
These strings are hardcoded. To maintain consistency with the rest of the application and support localization, please use the TranslationKeys and the T() helper method.
<MudText Typo="Typo.body1" Class="mb-4">@T(TranslationKeys.CloudConnectionLoginDescription)</MudText>
<RightAlignedButtonComponent OnButtonClicked="Submit"
ButtonText='@T(TranslationKeys.CloudConnectionLoginButton)'
References
- Ensure consistent logic is used across similar methods for robustness and to avoid inconsistent behavior.
| if (query.TryGetValue("code", out var code) && query.TryGetValue("state", out var state)) | ||
| { | ||
| var baseUrl = navigationManager.BaseUri + "cloudconnection"; | ||
| var result = await httpClientHelper.SendPostRequestAsync<object>($"/api/BackendApi/ExchangeToken?code={Uri.EscapeDataString(code.ToString())}&state={Uri.EscapeDataString(state.ToString())}&baseUrl={Uri.EscapeDataString(baseUrl)}", null).ConfigureAwait(false); | ||
| if (result.HasError) | ||
| { | ||
| snackbar.Add("Failed to complete cloud connection: " + result.ErrorMessage, Severity.Error); | ||
| } | ||
| else | ||
| { | ||
| snackbar.Add("Cloud connection completed successfully.", Severity.Success); | ||
| // Redirect to self without query parameters to clean up the URL | ||
| navigationManager.NavigateTo("cloudconnection"); | ||
| } | ||
| } |
There was a problem hiding this comment.
The query parameters code and state should be cleared from the URL even if the token exchange fails. Currently, if an error occurs, the parameters remain in the URL, which could lead to repeated failed attempts if the page is refreshed. Additionally, consider using the existing handled logic to unify URL cleanup.
References
- Ensure consistent logic is used across similar methods for robustness and to avoid inconsistent behavior.
| snackbar.Add("Failed to complete cloud connection: " + result.ErrorMessage, Severity.Error); | ||
| } | ||
| else | ||
| { | ||
| snackbar.Add("Cloud connection completed successfully.", Severity.Success); |
There was a problem hiding this comment.
These snackbar messages are hardcoded. Please use the localization service to ensure they can be translated.
References
- Ensure consistent logic is used across similar methods for robustness and to avoid inconsistent behavior.
|
|
||
| memoryCache.Set($"pkce_{state}", codeVerifier, TimeSpan.FromMinutes(10)); | ||
|
|
||
| var authorizeUrl = $"{configurationWrapper.BackendApiBaseUrl().Replace("/api/", "")}/connect/authorize?client_id={installationId}&redirect_uri={Uri.EscapeDataString(baseUrl)}&response_type=code&state={state}&code_challenge={codeChallenge}&code_challenge_method=S256&installation_id={installationId}"; |
There was a problem hiding this comment.
Using .Replace("/api/", "") on the base URL is fragile and assumes a specific URL structure. It would be safer to have a dedicated configuration setting for the Identity/Authority base URL, or use a more robust method to derive the base domain if they are guaranteed to be hosted together.
References
- Ensure consistent logic is used across similar methods for robustness and to avoid inconsistent behavior.
|
|
||
| var newToken = result.Data ?? throw new InvalidOperationException("Could not parse token"); | ||
|
|
||
| var token = await teslaSolarChargerContext.BackendTokens.SingleOrDefaultAsync(); |
There was a problem hiding this comment.
Missing .ConfigureAwait(false). To maintain consistency with the rest of the service and avoid potential synchronization context issues, please add it here.
var token = await teslaSolarChargerContext.BackendTokens.SingleOrDefaultAsync().ConfigureAwait(false);References
- Ensure consistent logic is used across similar methods for robustness and to avoid inconsistent behavior.
| } | ||
| else | ||
| { | ||
| var problemDetails = await response.Content.ReadFromJsonAsync<ProblemDetails>(); |
There was a problem hiding this comment.
This line uses System.Net.Http.Json (which relies on System.Text.Json), while the rest of the method (and file) uses Newtonsoft.Json (line 211). Mixing JSON libraries can lead to inconsistent behavior regarding property naming policies (e.g., camelCase vs. PascalCase). It is recommended to stick to one library for consistency.
References
- Ensure consistent logic is used across similar methods for robustness and to avoid inconsistent behavior.
59d2467 to
c83c0c1
Compare
1 participant
No description provided.