-
-
Notifications
You must be signed in to change notification settings - Fork 956
Verify Thread dataset state after export and auto-promote when server has none #6818
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -295,19 +295,53 @@ class ThreadManagerImpl @Inject constructor( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .addOnFailureListener { cont.resumeWithException(it) } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| override suspend fun sendThreadDatasetExportResult(result: ActivityResult, serverId: Int): String? { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (result.resultCode == Activity.RESULT_OK && coreSupportsThread(serverId)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val threadNetworkCredentials = ThreadNetworkCredentials.fromIntentSenderResultData(result.data!!) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val added = serverManager.webSocketRepository( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| serverId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ).addThreadDataset(threadNetworkCredentials.activeOperationalDataset) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (added) return threadNetworkCredentials.networkName | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (e: Exception) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Timber.e(e, "Error while executing server new Thread credentials request") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @OptIn(ExperimentalStdlibApi::class) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| override suspend fun sendThreadDatasetExportResult( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you are up for the task we could start unit testing this function. Claude is pretty good at this exercise on Android. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| result: ActivityResult, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| serverId: Int, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): ThreadManager.ExportResult { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (result.resultCode != Activity.RESULT_OK || !coreSupportsThread(serverId)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ThreadManager.ExportResult.NotSent | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val credentials = ThreadNetworkCredentials.fromIntentSenderResultData(result.data!!) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can avoid crashing here by checking if data is null and returning NotSent. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val webSocket = serverManager.webSocketRepository(serverId) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val added = try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| webSocket.addThreadDataset(credentials.activeOperationalDataset) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (e: Exception) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a coroutine, we have a bad design in the app by catching all Exception but we can't change it now. But because of that we have to reThrow |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Timber.e(e, "Error while executing server new Thread credentials request") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ThreadManager.ExportResult.Failed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!added) return ThreadManager.ExportResult.Failed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // thread/add_dataset_tlv never promotes a dataset to preferred on the server side. To | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // surface an honest result we re-query and compare extended PAN IDs. If the server has | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // no preferred dataset at all (e.g. no border router announcing one), promote the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // just-exported dataset; we never override an existing server preference. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val deviceExtPan = credentials.extendedPanId.toHexString(HexFormat.UpperCase) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Since you ignore the case bellow we don't care |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val serverPrefersExported = try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole logic could be extracted into a private function. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val datasets = webSocket.getThreadDatasets() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val preferred = datasets?.firstOrNull { it.preferred } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| when { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| preferred == null -> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val ours = datasets?.firstOrNull { it.extendedPanId.equals(deviceExtPan, ignoreCase = true) } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (ours != null && webSocket.setThreadPreferredDataset(ours.datasetId)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably need to check if core has this endpoint no? Or the min version we use for thread already contains this endpoint? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Timber.d("Thread: promoted exported dataset to preferred on the server") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+322
to
+331
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| preferred.extendedPanId.equals(deviceExtPan, ignoreCase = true) -> true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else -> false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+321
to
+335
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The when using instead of a if is sometimes less readable IMO. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (e: Exception) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here we need to rethrow |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Timber.w(e, "Unable to verify Thread preferred dataset after export") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could also be setting the prefered dataset since you potentially makes 2 call. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ThreadManager.ExportResult.Sent( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| networkName = credentials.networkName, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| serverPrefersExported = serverPrefersExported, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private suspend fun deleteOrphanedThreadCredentials(context: Context, serverId: Int) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,19 +133,39 @@ class DeveloperSettingsPresenterImpl @Inject constructor( | |
| ) { | ||
| mainScope.launch { | ||
| try { | ||
| val submitted = threadManager.sendThreadDatasetExportResult(result, serverId) | ||
| if (submitted != null) { | ||
| if (isDeviceOnly) { | ||
| view.onThreadDebugResult(context.getString(commonR.string.thread_debug_result_exported), true) | ||
| } else { | ||
| // If we got permission while both had a dataset, the device prefers a different network | ||
| val out = "${context.getString( | ||
| commonR.string.thread_debug_result_mismatch, | ||
| )} ${context.getString(commonR.string.thread_debug_result_mismatch_detail, submitted)}" | ||
| view.onThreadDebugResult(out, null) | ||
| when (val sent = threadManager.sendThreadDatasetExportResult(result, serverId)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like it can be simplified maybe something like this when (val sent = threadManager.sendThreadDatasetExportResult(result, serverId)) {
is ThreadManager.ExportResult.Sent -> when {
isDeviceOnly -> view.onThreadDebugResult(
context.getString(commonR.string.thread_debug_result_exported),
true,
)
sent.serverPrefersExported == true -> view.onThreadDebugResult(
context.getString(commonR.string.thread_debug_result_match),
true,
)
else -> {
val headlineRes = if (sent.serverPrefersExported == false) {
commonR.string.thread_debug_result_exported_not_preferred
} else {
commonR.string.thread_debug_result_mismatch
}
val detail = context.getString(
commonR.string.thread_debug_result_mismatch_detail,
sent.networkName,
)
view.onThreadDebugResult("${context.getString(headlineRes)} $detail", null)
}
} |
||
| is ThreadManager.ExportResult.Sent -> { | ||
| if (isDeviceOnly) { | ||
| view.onThreadDebugResult( | ||
| context.getString(commonR.string.thread_debug_result_exported), | ||
| true, | ||
| ) | ||
| } else if (sent.serverPrefersExported == true) { | ||
| view.onThreadDebugResult(context.getString(commonR.string.thread_debug_result_match), true) | ||
| } else if (sent.serverPrefersExported == false) { | ||
| // The dataset was added on the server but a different one is still | ||
| // preferred (typically a server-managed border router announces it). | ||
| val out = "${context.getString( | ||
| commonR.string.thread_debug_result_exported_not_preferred, | ||
| )} ${context.getString( | ||
| commonR.string.thread_debug_result_mismatch_detail, | ||
| sent.networkName, | ||
| )}" | ||
| view.onThreadDebugResult(out, null) | ||
| } else { | ||
| val out = "${context.getString( | ||
| commonR.string.thread_debug_result_mismatch, | ||
| )} ${context.getString( | ||
| commonR.string.thread_debug_result_mismatch_detail, | ||
| sent.networkName, | ||
| )}" | ||
| view.onThreadDebugResult(out, null) | ||
|
Comment on lines
+156
to
+162
|
||
| } | ||
| } | ||
| } else { | ||
| view.onThreadDebugResult(context.getString(commonR.string.thread_debug_result_error), false) | ||
| ThreadManager.ExportResult.NotSent, | ||
| ThreadManager.ExportResult.Failed, | ||
| -> | ||
| view.onThreadDebugResult(context.getString(commonR.string.thread_debug_result_error), false) | ||
| } | ||
| } catch (e: Exception) { | ||
| view.onThreadDebugResult(context.getString(commonR.string.thread_debug_result_error), false) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -603,12 +603,16 @@ class WebViewPresenterImpl @Inject constructor( | |||||
| mainScope.launch { | ||||||
| val sent = threadUseCase.sendThreadDatasetExportResult(result, serverId) | ||||||
| Timber.d( | ||||||
| "Thread ${if (!sent.isNullOrBlank()) "sent credential for $sent" else "did not send credential"}", | ||||||
| "Thread ${if (sent is ThreadManager.ExportResult.Sent) { | ||||||
| "sent credential for ${sent.networkName} (server prefers exported: ${sent.serverPrefersExported})" | ||||||
| } else { | ||||||
| "did not send credential" | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| }}", | ||||||
| ) | ||||||
| if (sent.isNullOrBlank()) { | ||||||
| mutableMatterThreadStep.tryEmit(MatterThreadStep.THREAD_NONE) | ||||||
| } else { | ||||||
| if (sent is ThreadManager.ExportResult.Sent) { | ||||||
| mutableMatterThreadStep.tryEmit(MatterThreadStep.THREAD_SENT) | ||||||
| } else { | ||||||
| mutableMatterThreadStep.tryEmit(MatterThreadStep.THREAD_NONE) | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -420,6 +420,16 @@ class WebSocketRepositoryImpl internal constructor( | |
| return response?.success == true | ||
| } | ||
|
|
||
| override suspend fun setThreadPreferredDataset(datasetId: String): Boolean { | ||
| val response = webSocketCore.sendMessage( | ||
| mapOf( | ||
| "type" to "thread/set_preferred_dataset", | ||
| "dataset_id" to datasetId, | ||
|
Comment on lines
+423
to
+427
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed |
||
| ), | ||
| ) | ||
| return response?.success == true | ||
| } | ||
|
|
||
| /** | ||
| * Update server entry in [serverManager] with information from a [CurrentUserResponse] like user | ||
| * name and admin status. | ||
|
|
||
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 needed anymore (you can probably also remove it in the rest of the file)