yilsem/aiturk-mobil
1
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

fix(#1283): respect plugin ownership of MCP servers (#1296)

Browse Source
This commit is contained in:
m57 authored and GitHub committed 2026-09-26 21:44:41 +03:00
1 parent ee487e0087
commit c34de55dd9
6 files changed
+341 -25
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

app/src/androidTest/java/com/m57/hermescontrol/ui/mcp/McpOwnershipCardTest.kt
+77
View File
@@ -0,0 +1,77 @@
package com.m57.hermescontrol.ui.mcp
import androidx.compose.ui.test.assertIsEnabled
import androidx.compose.ui.test.assertIsNotEnabled
import androidx.compose.ui.test.isToggleable
import androidx.compose.ui.test.junit4.createComposeRule
import androidx.compose.ui.test.onNodeWithContentDescription
import androidx.compose.ui.test.onNodeWithText
import androidx.compose.ui.test.performClick
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.filters.MediumTest
import com.m57.hermescontrol.data.model.McpServer
import com.m57.hermescontrol.theme.HermesControlTheme
import com.m57.hermescontrol.theme.LocalSpacing
import com.m57.hermescontrol.ui.mcp.components.ServerCard
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
@RunWith(AndroidJUnit4::class)
@MediumTest
class McpOwnershipCardTest {
@get:Rule val composeRule = createComposeRule()
@Test
fun pluginRowExplainsOwnershipAndDisablesMutations() {
showServer("plugin", "security")
composeRule.onNodeWithText("From plugin security · Read-only", useUnmergedTree = true).assertExists()
composeRule.onNode(isToggleable()).assertIsNotEnabled()
composeRule.onNodeWithContentDescription("Delete").assertIsNotEnabled()
composeRule.onNodeWithText("Authorize").assertIsNotEnabled()
composeRule.onNodeWithText("Test").assertIsEnabled()
composeRule.onNodeWithText("Env vars").performClick()
composeRule.onNodeWithContentDescription("Remove").assertIsNotEnabled()
composeRule.onNodeWithText("Add env var").assertIsNotEnabled()
composeRule.onNodeWithText("KEY", useUnmergedTree = true).assertExists()
}
@Test
fun configRowKeepsMutationsEnabled() {
showServer("config", null)
composeRule.onNode(isToggleable()).assertIsEnabled()
composeRule.onNodeWithContentDescription("Delete").assertIsEnabled()
composeRule.onNodeWithText("Authorize").assertIsEnabled()
composeRule.onNodeWithText("Env vars").performClick()
composeRule.onNodeWithContentDescription("Remove").assertIsEnabled()
composeRule.onNodeWithText("Add env var").assertIsEnabled()
}
private fun showServer(
source: String,
plugin: String?,
) {
val server =
McpServer(
name = "scanner",
enabled = true,
source = source,
plugin = plugin,
auth = "oauth",
env = mapOf("KEY" to "test-value"),
)
val viewModel = McpServersViewModel()
composeRule.setContent {
HermesControlTheme {
ServerCard(
server = server,
state = McpServersUiState(servers = listOf(server)),
viewModel = viewModel,
spacing = LocalSpacing.current,
onClick = {},
onOpenBrowser = {},
)
}
}
}
}
app/src/main/java/com/m57/hermescontrol/data/model/McpServer.kt
+6 -1
View File
@@ -20,7 +20,12 @@ data class McpServer(
val error: String? = null,
val auth: String? = null,
val tools: List<String>? = null,
)
val source: String = "config",
val plugin: String? = null,
) {
// #1283: plugin-provided servers are managed by their owning plugin.
val isPluginOwned: Boolean get() = source == "plugin"
}
@Serializable
data class McpServerTestResponse(
app/src/main/java/com/m57/hermescontrol/ui/mcp/McpServersViewModel.kt
+38 -20
View File
@@ -12,11 +12,13 @@ import com.m57.hermescontrol.data.model.McpServerTestResponse
import com.m57.hermescontrol.data.model.McpServerToggleRequest
import com.m57.hermescontrol.data.model.McpServersResponse
import com.m57.hermescontrol.data.remote.ApiClient
import com.m57.hermescontrol.data.remote.NetworkError
import com.m57.hermescontrol.data.remote.NetworkResult
import com.m57.hermescontrol.data.remote.safeApiCall
import com.m57.hermescontrol.ui.common.ToastHost
import com.m57.hermescontrol.ui.common.safeLaunchLoad
import com.m57.hermescontrol.ui.common.safeLaunchSwrLoad
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.async
import kotlinx.coroutines.awaitAll
@@ -66,8 +68,9 @@ data class McpServersUiState(
val activeOAuthFlow: McpOAuthFlowResponse? = null,
)
class McpServersViewModel :
ViewModel(),
class McpServersViewModel(
private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO,
) : ViewModel(),
ToastHost {
private val _uiState = MutableStateFlow(McpServersUiState())
val uiState: StateFlow<McpServersUiState> = _uiState.asStateFlow()
@@ -131,6 +134,7 @@ class McpServersViewModel :
// ── Server toggle ────────────────────────────────────────
fun toggleServer(server: McpServer) {
if (server.isPluginOwned || isPluginOwned(server.name)) return
val originalEnabled = server.enabled
val targetEnabled = !originalEnabled
@@ -145,7 +149,7 @@ class McpServersViewModel :
viewModelScope.launch {
val result =
withContext(Dispatchers.IO) {
withContext(ioDispatcher) {
safeApiCall {
ApiClient.hermesApi.toggleMcpServer(
server.name,
@@ -163,7 +167,7 @@ class McpServersViewModel :
}
is NetworkResult.Failure -> {
revertToggle(server.name, originalEnabled, "Failed to toggle server: ${result.error.message}")
revertToggle(server.name, originalEnabled, mutationError(result.error, "Failed to toggle server"))
}
}
}
@@ -178,7 +182,7 @@ class McpServersViewModel :
)
}
val result =
withContext(Dispatchers.IO) {
withContext(ioDispatcher) {
safeApiCall { ApiClient.hermesApi.testMcpServer(name) }
}
when (result) {
@@ -234,7 +238,7 @@ class McpServersViewModel :
val testJobs =
enabledServers.map { server ->
async(Dispatchers.IO) {
async(ioDispatcher) {
val res = safeApiCall { ApiClient.hermesApi.testMcpServer(server.name) }
server.name to res
}
@@ -271,9 +275,10 @@ class McpServersViewModel :
}
fun deleteServer(name: String) {
if (isPluginOwned(name)) return
viewModelScope.launch {
val result =
withContext(Dispatchers.IO) {
withContext(ioDispatcher) {
safeApiCall { ApiClient.hermesApi.deleteMcpServer(name) }
}
when (result) {
@@ -283,7 +288,7 @@ class McpServersViewModel :
}
is NetworkResult.Failure -> {
_uiState.update { it.copy(toastMessage = "Failed to delete server: ${result.error.message}") }
_uiState.update { it.copy(toastMessage = mutationError(result.error, "Failed to delete server")) }
}
}
}
@@ -311,7 +316,7 @@ class McpServersViewModel :
viewModelScope.launch {
val importJobs =
requests.map { req ->
async(Dispatchers.IO) {
async(ioDispatcher) {
val result = safeApiCall { ApiClient.hermesApi.addMcpServer(req) }
req to result
}
@@ -387,7 +392,7 @@ class McpServersViewModel :
viewModelScope.launch {
val request = AddServerRequestBuilder.build(state)
val result =
withContext(Dispatchers.IO) {
withContext(ioDispatcher) {
safeApiCall { ApiClient.hermesApi.addMcpServer(request) }
}
when (result) {
@@ -412,7 +417,7 @@ class McpServersViewModel :
_uiState.update {
it.copy(
addingServer = false,
toastMessage = "Failed to add server: ${result.error.message}",
toastMessage = mutationError(result.error, "Failed to add server"),
)
}
}
@@ -423,6 +428,7 @@ class McpServersViewModel :
// ── Env var editing ──────────────────────────────────────
fun startEditingEnv(server: McpServer) {
if (server.isPluginOwned || isPluginOwned(server.name)) return
_uiState.update { it.copy(editingEnvFor = server.name, envKeyInput = "", envValueInput = "") }
}
@@ -439,6 +445,7 @@ class McpServersViewModel :
}
fun addEnvVar(serverName: String) {
if (isPluginOwned(serverName)) return
val state = _uiState.value
val key = state.envKeyInput.trim()
val value = state.envValueInput.trim()
@@ -450,7 +457,7 @@ class McpServersViewModel :
val existingEnv = state.servers.find { it.name == serverName }?.env ?: emptyMap()
val updatedEnv = existingEnv + (key to value)
val result =
withContext(Dispatchers.IO) {
withContext(ioDispatcher) {
safeApiCall { ApiClient.hermesApi.updateMcpServer(serverName, mapOf("env" to updatedEnv)) }
}
when (result) {
@@ -466,7 +473,7 @@ class McpServersViewModel :
}
is NetworkResult.Failure -> {
_uiState.update { it.copy(toastMessage = "Failed to add env var: ${result.error.message}") }
_uiState.update { it.copy(toastMessage = mutationError(result.error, "Failed to add env var")) }
}
}
}
@@ -476,12 +483,13 @@ class McpServersViewModel :
serverName: String,
key: String,
) {
if (isPluginOwned(serverName)) return
viewModelScope.launch {
val state = _uiState.value
val existingEnv = state.servers.find { it.name == serverName }?.env ?: emptyMap()
val updatedEnv = existingEnv - key
val result =
withContext(Dispatchers.IO) {
withContext(ioDispatcher) {
safeApiCall { ApiClient.hermesApi.updateMcpServer(serverName, mapOf("env" to updatedEnv)) }
}
when (result) {
@@ -491,7 +499,7 @@ class McpServersViewModel :
}
is NetworkResult.Failure -> {
_uiState.update { it.copy(toastMessage = "Failed to remove env var: ${result.error.message}") }
_uiState.update { it.copy(toastMessage = mutationError(result.error, "Failed to remove env var")) }
}
}
}
@@ -503,7 +511,7 @@ class McpServersViewModel :
_uiState.update { it.copy(catalogLoading = true, catalogError = null) }
viewModelScope.launch {
val result =
withContext(Dispatchers.IO) {
withContext(ioDispatcher) {
safeApiCall { ApiClient.hermesApi.getMcpCatalog() }
}
when (result) {
@@ -542,7 +550,7 @@ class McpServersViewModel :
env = state.catalogInstallEnv.ifEmpty { null },
)
val result =
withContext(Dispatchers.IO) {
withContext(ioDispatcher) {
safeApiCall { ApiClient.hermesApi.installMcpCatalogEntry(request) }
}
when (result) {
@@ -584,11 +592,12 @@ class McpServersViewModel :
server: McpServer,
onOpenBrowser: (String) -> Unit,
) {
if (server.isPluginOwned || isPluginOwned(server.name)) return
oauthPollJob?.cancel()
_uiState.update { it.copy(toastMessage = "Starting OAuth authorization…") }
viewModelScope.launch {
val result =
withContext(Dispatchers.IO) {
withContext(ioDispatcher) {
safeApiCall { ApiClient.hermesApi.authMcpServer(server.name) }
}
when (result) {
@@ -608,7 +617,7 @@ class McpServersViewModel :
}
is NetworkResult.Failure -> {
_uiState.update { it.copy(toastMessage = "Failed to start OAuth: ${result.error.message}") }
_uiState.update { it.copy(toastMessage = mutationError(result.error, "Failed to start OAuth")) }
}
}
}
@@ -623,7 +632,7 @@ class McpServersViewModel :
while (polling) {
kotlinx.coroutines.delay(2000)
val result =
withContext(Dispatchers.IO) {
withContext(ioDispatcher) {
safeApiCall { ApiClient.hermesApi.getMcpOAuthFlowStatus(flowId) }
}
when (result) {
@@ -688,6 +697,15 @@ class McpServersViewModel :
// ── Helpers ──────────────────────────────────────────────
// #1283: re-check current ownership so stale row callbacks cannot mutate a plugin server.
private fun isPluginOwned(name: String): Boolean =
_uiState.value.servers.any { it.name == name && it.isPluginOwned }
private fun mutationError(
error: NetworkError,
prefix: String,
): String = if (error is NetworkError.Http && error.code == 409) error.message else "$prefix: ${error.message}"
private fun revertToggle(
name: String,
originalEnabled: Boolean,
app/src/main/java/com/m57/hermescontrol/ui/mcp/components/ServerCard.kt
+25 -4
View File
@@ -139,6 +139,16 @@ fun ServerCard(
color = MaterialTheme.colorScheme.onSurfaceVariant,
)
if (server.isPluginOwned) {
Text(
text =
server.plugin?.let { stringResource(R.string.mcp_servers_from_plugin, it) }
?: stringResource(R.string.mcp_servers_plugin_managed),
style = MaterialTheme.typography.bodySmall,
color = MaterialTheme.colorScheme.onSurfaceVariant,
)
}
// Tool count & schema token overhead badge (issue #1029)
val toolInfos = testResult?.tools
val toolCount = toolInfos?.size ?: server.tools?.size
@@ -173,6 +183,7 @@ fun ServerCard(
}
Switch(
checked = server.enabled,
enabled = !server.isPluginOwned,
onCheckedChange = { viewModel.toggleServer(server) },
)
}
@@ -218,6 +229,7 @@ fun ServerCard(
if (server.auth == "oauth") {
FilledTonalButton(
onClick = { viewModel.startMcpOAuthFlow(server, onOpenBrowser) },
enabled = !server.isPluginOwned,
modifier = Modifier.weight(1.2f),
contentPadding = PaddingValues(horizontal = 12.dp, vertical = 10.dp),
) {
@@ -245,7 +257,10 @@ fun ServerCard(
Text(stringResource(R.string.mcp_servers_action_test), maxLines = 1)
}
}
IconButton(onClick = { viewModel.deleteServer(server.name) }) {
IconButton(
onClick = { viewModel.deleteServer(server.name) },
enabled = !server.isPluginOwned,
) {
Icon(Icons.Filled.Delete, contentDescription = stringResource(R.string.action_delete))
}
}
@@ -275,7 +290,7 @@ fun EnvVarSection(
spacing: Spacing,
modifier: Modifier = Modifier,
) {
val isEditing = state.editingEnvFor == server.name
val isEditing = !server.isPluginOwned && state.editingEnvFor == server.name
val env = server.env ?: emptyMap()
Column(modifier = modifier) {
@@ -300,7 +315,10 @@ fun EnvVarSection(
color = MaterialTheme.colorScheme.onSurfaceVariant,
)
}
IconButton(onClick = { viewModel.removeEnvVar(server.name, key) }) {
IconButton(
onClick = { viewModel.removeEnvVar(server.name, key) },
enabled = !server.isPluginOwned,
) {
Icon(Icons.Filled.Close, contentDescription = "Remove", modifier = Modifier.size(16.dp))
}
}
@@ -342,7 +360,10 @@ fun EnvVarSection(
}
}
} else {
TextButton(onClick = { viewModel.startEditingEnv(server) }) {
TextButton(
onClick = { viewModel.startEditingEnv(server) },
enabled = !server.isPluginOwned,
) {
Icon(Icons.Filled.Add, contentDescription = null, modifier = Modifier.size(14.dp))
Spacer(modifier = Modifier.width(spacing.xs))
Text(stringResource(R.string.mcp_servers_env_add))
app/src/main/res/values/strings.xml
+2
View File
@@ -1119,6 +1119,8 @@
<!-- McpServers Screen -->
<string name="mcp_servers_label_transport">Transport: %1$s</string>
<string name="mcp_servers_from_plugin">From plugin %1$s · Read-only</string>
<string name="mcp_servers_plugin_managed">Plugin-managed · Read-only</string>
<string name="mcp_servers_label_command">Command:</string>
<string name="mcp_servers_action_test">Test</string>
<string name="mcp_servers_action_test_all">Test All</string>
app/src/test/java/com/m57/hermescontrol/ui/mcp/McpOwnershipTest.kt
+193
View File
@@ -0,0 +1,193 @@
package com.m57.hermescontrol.ui.mcp
import com.m57.hermescontrol.data.local.AuthManager
import com.m57.hermescontrol.data.model.McpServer
import com.m57.hermescontrol.data.model.McpServerTestResponse
import com.m57.hermescontrol.data.model.McpServersResponse
import com.m57.hermescontrol.data.remote.ApiClient
import com.m57.hermescontrol.data.remote.HermesApiService
import com.m57.hermescontrol.data.remote.OkHttpProvider
import io.mockk.coEvery
import io.mockk.coVerify
import io.mockk.every
import io.mockk.mockk
import io.mockk.mockkObject
import io.mockk.unmockkAll
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.StandardTestDispatcher
import kotlinx.coroutines.test.resetMain
import kotlinx.coroutines.test.setMain
import okhttp3.ResponseBody.Companion.toResponseBody
import org.junit.After
import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Assert.assertNull
import org.junit.Assert.assertTrue
import org.junit.Before
import org.junit.Test
import retrofit2.Response
@OptIn(ExperimentalCoroutinesApi::class)
class McpOwnershipTest {
private val dispatcher = StandardTestDispatcher()
private lateinit var api: HermesApiService
private val owned = McpServer(name = "scanner", enabled = true, source = "plugin", plugin = "security")
private val conflict = "Server 'scanner' is provided by plugin 'security' and cannot be modified"
@Before
fun setUp() {
Dispatchers.setMain(dispatcher)
api = mockk()
mockkObject(ApiClient, AuthManager)
every { ApiClient.hermesApi } returns api
every { AuthManager.currentDataScope() } throws IllegalStateException("No test scope")
}
@After
fun tearDown() {
Dispatchers.resetMain()
unmockkAll()
}
@Test
fun `ownership decodes and legacy servers remain editable`() {
val json = OkHttpProvider.json
val plugin =
json.decodeFromString<McpServer>(
"""{"name":"scanner","enabled":true,"source":"plugin","plugin":"security"}""",
)
assertEquals(owned, plugin)
assertTrue(plugin.isPluginOwned)
assertTrue(plugin.copy(plugin = null).isPluginOwned)
val legacy = json.decodeFromString<McpServer>("""{"name":"local","enabled":true}""")
assertFalse(legacy.isPluginOwned)
assertNull(legacy.plugin)
val config =
json.decodeFromString<McpServer>(
"""{"name":"local","enabled":true,"source":"config","plugin":null}""",
)
assertFalse(config.isPluginOwned)
}
@Test
fun `plugin mutations are blocked even for stale callbacks but testing remains available`() {
val vm = loaded(owned)
val stale = owned.copy(source = "config", plugin = null)
vm.toggleServer(stale)
vm.deleteServer(owned.name)
vm.startEditingEnv(stale)
vm.updateEnvKey("KEY")
vm.updateEnvValue("value")
vm.addEnvVar(owned.name)
vm.removeEnvVar(owned.name, "KEY")
vm.startMcpOAuthFlow(stale) { error("Must not open browser") }
dispatcher.scheduler.advanceUntilIdle()
assertTrue(
vm.uiState.value.servers
.single()
.enabled,
)
assertNull(vm.uiState.value.editingEnvFor)
coVerify(exactly = 0) { api.toggleMcpServer(any(), any()) }
coVerify(exactly = 0) { api.deleteMcpServer(any()) }
coVerify(exactly = 0) { api.updateMcpServer(any(), any()) }
coVerify(exactly = 0) { api.authMcpServer(any()) }
coEvery { api.testMcpServer(owned.name) } returns Response.success(McpServerTestResponse(ok = true))
vm.testServer(owned.name)
dispatcher.scheduler.advanceUntilIdle()
assertTrue(
vm.uiState.value.serverTestResults
.getValue(owned.name)
.ok,
)
}
@Test
fun `config server toggle and environment editing remain available`() {
val config = owned.copy(source = "config", plugin = null)
val vm = loaded(config)
coEvery { api.toggleMcpServer(config.name, any()) } returns Response.success(Unit)
vm.toggleServer(config)
dispatcher.scheduler.advanceUntilIdle()
assertFalse(
vm.uiState.value.servers
.single()
.enabled,
)
vm.startEditingEnv(config)
assertEquals(config.name, vm.uiState.value.editingEnvFor)
coEvery { api.updateMcpServer(config.name, any()) } returns Response.success(config)
vm.updateEnvKey("KEY")
vm.updateEnvValue("value")
vm.addEnvVar(config.name)
dispatcher.scheduler.advanceUntilIdle()
coVerify { api.updateMcpServer(config.name, mapOf("env" to mapOf("KEY" to "value"))) }
vm.removeEnvVar(config.name, "KEY")
dispatcher.scheduler.advanceUntilIdle()
coVerify { api.updateMcpServer(config.name, mapOf("env" to emptyMap<String, String>())) }
coEvery { api.deleteMcpServer(config.name) } returns Response.success(Unit)
vm.deleteServer(config.name)
dispatcher.scheduler.advanceUntilIdle()
coVerify(exactly = 1) { api.deleteMcpServer(config.name) }
}
@Test
fun `stale ownership conflicts surface detail verbatim and revert toggle`() {
val config = owned.copy(source = "config", plugin = null)
val vm = loaded(config)
coEvery { api.toggleMcpServer(any(), any()) } returns conflictResponse()
vm.toggleServer(config)
dispatcher.scheduler.advanceUntilIdle()
assertTrue(
vm.uiState.value.servers
.single()
.enabled,
)
assertEquals(conflict, vm.uiState.value.toastMessage)
coEvery { api.deleteMcpServer(any()) } returns conflictResponse()
vm.deleteServer(config.name)
dispatcher.scheduler.advanceUntilIdle()
assertEquals(conflict, vm.uiState.value.toastMessage)
coEvery { api.updateMcpServer(any(), any()) } coAnswers { conflictResponse() }
vm.updateEnvKey("KEY")
vm.addEnvVar(config.name)
dispatcher.scheduler.advanceUntilIdle()
assertEquals(conflict, vm.uiState.value.toastMessage)
vm.clearToast()
vm.removeEnvVar(config.name, "KEY")
dispatcher.scheduler.advanceUntilIdle()
assertEquals(conflict, vm.uiState.value.toastMessage)
coEvery { api.authMcpServer(any()) } returns conflictResponse()
vm.clearToast()
vm.startMcpOAuthFlow(config) { error("Must not open browser") }
dispatcher.scheduler.advanceUntilIdle()
assertEquals(conflict, vm.uiState.value.toastMessage)
}
@Test
fun `add collision surfaces plugin conflict verbatim`() {
coEvery { api.addMcpServer(any()) } returns conflictResponse()
val vm = McpServersViewModel(ioDispatcher = dispatcher)
vm.toggleAddForm()
vm.updateAddServerName(owned.name)
vm.updateAddServerUrl("https://example.com/mcp")
vm.submitAddServer()
dispatcher.scheduler.advanceUntilIdle()
assertEquals(conflict, vm.uiState.value.toastMessage)
assertFalse(vm.uiState.value.addingServer)
assertTrue(vm.uiState.value.showAddForm)
assertEquals(owned.name, vm.uiState.value.addServerName)
}
private fun loaded(server: McpServer): McpServersViewModel {
coEvery { api.getMcpServers() } returns Response.success(McpServersResponse(listOf(server)))
return McpServersViewModel(ioDispatcher = dispatcher).also {
it.loadServers()
dispatcher.scheduler.advanceUntilIdle()
assertEquals(listOf(server), it.uiState.value.servers)
}
}
private fun <T> conflictResponse(): Response<T> = Response.error(409, """{"detail":"$conflict"}""".toResponseBody())
}