PM-13424 - Show error when logging into an unofficial Bitwarden server #1072
Conversation
…during authentication
|
No New Or Fixed Issues Found |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1072 +/- ##
=======================================
Coverage 89.48% 89.49%
=======================================
Files 675 675
Lines 42544 42560 +16
=======================================
+ Hits 38072 38090 +18
+ Misses 4472 4470 -2 ☔ View full report in Codecov by Sentry. |
| @@ -1049,3 +1049,4 @@ | |||
| "NoAccountFoundPleaseLogInAgainIfYouContinueToSeeThisError" = "No account found. Please log in again if you continue to see this error."; | |||
| "ImportError" = "Import error"; | |||
| "NoLoginsWereImported" = "No logins were imported"; | |||
| "ThisIsNotARecognizedBitwardenServer" = "This is not a recognized Bitwarden server"; | |||
There was a problem hiding this comment.
🤔 Should this end with a period? It also doesn't seem to be the same as we have on the Android side.
There was a problem hiding this comment.
ah good catch! thanks!
| @@ -44,6 +44,21 @@ class AlertNetworkingTests: BitwardenTestCase { | |||
| XCTAssertNil(action.handler) | |||
| } | |||
|
|
|||
| /// `.networkResponseError` builds an alert to display a server error message for an unofficial Bitwarden server.. | |||
There was a problem hiding this comment.
⛏️
| /// `.networkResponseError` builds an alert to display a server error message for an unofficial Bitwarden server.. | |
| /// `.networkResponseError` builds an alert to display a server error message for an unofficial Bitwarden server. |
| /// - Parameter errorResponse: The error received from the network response. | ||
| /// | ||
| private func handleErrorResponse(_ errorResponse: Error) async { | ||
| let error = if await services.configService.getConfig()?.isOfficialBitwardenServer() == false { |
There was a problem hiding this comment.
🤔 My concern here is that we might eat errors not related to it being an unofficial server, presenting them as an unofficial server error. Or would we have handled other errors (such as "incorrect username/password" before this, rendering that concern moot?
There was a problem hiding this comment.
this is a good point... If the server property isn't nil, regardless of which error is given back from the server then we would display the new error, so your concern isn't moot. Im wondering if we should check if its a validationError(error:) before checking the server value?
There was a problem hiding this comment.
Does this make sense?
private func handleErrorResponse(_ errorResponse: Error) async {
if case let serverError as ServerError = errorResponse,
case .validationError = serverError {
coordinator.showAlert(.networkResponseError(serverError))
services.errorReporter.log(error: serverError)
return
}
// Check if the server is unofficial, otherwise use the original error
let error = await services.configService.getConfig()?.isOfficialBitwardenServer() == false
? ServerError.unofficialBitwardenServerError
: errorResponse
// Show the appropriate alert and log the error
coordinator.showAlert(.networkResponseError(error))
services.errorReporter.log(error: error)
}
There was a problem hiding this comment.
hmmm actually I think i need to think about this more... We probably only want to show the unofficial server error is we cant deserialize an existing error?
There was a problem hiding this comment.
This ended up feeling safe but lemme know if you find any issues with this logic
private func handleErrorResponse(_ errorResponse: Error) async {
let error = if case let serverError as ServerError = errorResponse {
serverError
} else {
await services.configService.getConfig()?.isOfficialBitwardenServer() == false
? ServerError.unofficialBitwardenServerError
: errorResponse
}
coordinator.showAlert(.networkResponseError(error))
services.errorReporter.log(error: error)
}
There was a problem hiding this comment.
The ticket itself indicates that it's only when we get an unknown error—so yeah, if we get a better error from the server, we should use that; this is to cover the situations we have where the older server version is sending something we can't properly parse in some way, or other things we haven't anticipated yet
BitwardenShared/UI/Platform/Application/Extensions/Alert+Networking.swift
Show resolved
Hide resolved
| // After setting a new region, feature flags need to be reloaded | ||
| Task { | ||
| await loadFeatureFlag() | ||
| } | ||
| await refreshConfig() |
There was a problem hiding this comment.
- On perform
.appearedIt won't callrefreshProfileStatebecause it's going to await untilrefreshConfigfinishes. - When setting a Self-host environment it won't close the Self-host settings view until
refreshConfigfinishes given that it's called in func didSaveEnvironment(urls: EnvironmentUrlData) async
I'd go back to wrap the config refresh call in a Task so we don't have these issues and if we really need to wait for the config for some thing, we'd need to use some other approach, but given this is only for feature flags I'm not that concerned that is not loaded correctly on low internet connection as we have fallback flows for emailVerificationFeatureFlag.
🎟️ Tracking
PM-13424
📔 Objective
Implementation Details
After conversation in Slack, we decided to use the presence of
ConfigResponse.Serverproperty to determine if we are communicating with an official Bitwarden server or not.ConfigResponse.Serveris null we assume it is an official Bitwarden serverConfigResponse.Serveris not null we assume it is an unofficial Bitwarden server.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes