* fix(network): scope management REST calls with ?profile= (fixes #528) Mobile fired all management REST calls with NO profile scope, so every profile read/wrote the backend's implicit default profile — multi-profile setups silently corrupted config/skills/toolsets/mcp/model/env. Add ProfileScopeInterceptor (OkHttp) that appends ?profile=<activeProfile> to the same endpoint families the desktop/web dashboard scopes (web/src/lib/api.ts PROFILE_SCOPED_PREFIXES), respecting: - no active profile -> pass through (legacy default behavior) - explicit ?profile= on the URL wins over the global scope - only scoped families rewritten; ops/pairing/cron/profiles stay global Wired into ApiClient.buildService(). 5 MockWebServer tests cover scoped append, explicit-param precedence, no-profile passthrough, non-scoped skip, and skills scoping. Mirrors desktop/web contract derived from audit of /opt/hermes-agent. * fix(ci): stub getSelectedProfileId in ApiClientTest + harden prefix match (fixes PR #540) - ApiClientTest: stub AuthManager.getSelectedProfileId() to null. The new ProfileScopeInterceptor (registered in ApiClient.buildService) calls it, and the unstubbed real method threw IllegalStateException at AuthManager.kt:56, breaking all 4 auth-interceptor tests. - ProfileScopeInterceptor: replace startsWith(prefix) with whole-segment match (path == prefix || path.startsWith(prefix/)) so lookalike paths like /api/statusXYZ are NOT wrongly scoped (Sourcery review, PR #540). - ProfileScopeInterceptorTest: add lookalikePath_notScoped + scopedSubPath_isScoped to lock the segment-boundary fix.
This commit is contained in:
4 files changed
+212
No files matched your search
@@ -117,6 +117,7 @@ object ApiClient {
|
|||||||
.base
|
.base
|
||||||
.newBuilder()
|
.newBuilder()
|
||||||
.addInterceptor(authInterceptor)
|
.addInterceptor(authInterceptor)
|
||||||
|
.addInterceptor(ProfileScopeInterceptor)
|
||||||
.addInterceptor(logging)
|
.addInterceptor(logging)
|
||||||
.authenticator(TokenRefreshAuthenticator)
|
.authenticator(TokenRefreshAuthenticator)
|
||||||
.certificatePinner(certificatePinner)
|
.certificatePinner(certificatePinner)
|
||||||
|
|||||||
@@ -0,0 +1,77 @@
|
|||||||
|
package com.m57.hermescontrol.data.remote
|
||||||
|
|
||||||
|
import com.m57.hermescontrol.data.local.AuthManager
|
||||||
|
import okhttp3.Interceptor
|
||||||
|
import okhttp3.Response
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Appends `?profile=<activeProfile>` to profile-scoped management endpoints,
|
||||||
|
* mirroring the desktop/web dashboard contract
|
||||||
|
* (`web/src/lib/api.ts` → `PROFILE_SCOPED_PREFIXES` + `withManagementProfile`).
|
||||||
|
*
|
||||||
|
* Without this, mobile fired all management REST calls with NO profile scope,
|
||||||
|
* so every profile read/wrote the backend's implicit default profile —
|
||||||
|
* multi-profile setups silently corrupted config/skills/toolsets/mcp/model/env
|
||||||
|
* (issue #528).
|
||||||
|
*
|
||||||
|
* Rules (copied from the desktop contract):
|
||||||
|
* - If no profile is active, pass through unchanged (legacy "default" behavior).
|
||||||
|
* - If the URL already carries an explicit `profile=` query, leave it
|
||||||
|
* untouched — explicit beats global.
|
||||||
|
* - Only the endpoint families below honor `?profile=` on the backend;
|
||||||
|
* everything else (ops, pairing, cron, profiles themselves) is
|
||||||
|
* machine-global or self-scoped and must NOT be rewritten.
|
||||||
|
*/
|
||||||
|
object ProfileScopeInterceptor : Interceptor {
|
||||||
|
private val PROFILE_SCOPED_PREFIXES =
|
||||||
|
listOf(
|
||||||
|
"/api/status",
|
||||||
|
"/api/gateway",
|
||||||
|
"/api/analytics",
|
||||||
|
"/api/skills",
|
||||||
|
"/api/tools/toolsets",
|
||||||
|
"/api/config",
|
||||||
|
"/api/env",
|
||||||
|
"/api/mcp",
|
||||||
|
"/api/messaging/platforms",
|
||||||
|
"/api/messaging/telegram/onboarding",
|
||||||
|
"/api/messaging/whatsapp/onboarding",
|
||||||
|
"/api/model/info",
|
||||||
|
"/api/model/set",
|
||||||
|
"/api/model/auxiliary",
|
||||||
|
"/api/model/moa",
|
||||||
|
"/api/model/options",
|
||||||
|
)
|
||||||
|
|
||||||
|
/**
|
||||||
|
* True only when [path] matches a scoped prefix as a whole path segment —
|
||||||
|
* e.g. `/api/status` or `/api/status/health`, but NOT `/api/statusXYZ`
|
||||||
|
* or `/api/gatewayExtra` (Sourcery review, PR #540).
|
||||||
|
*/
|
||||||
|
private fun isProfileScopedPath(path: String): Boolean =
|
||||||
|
PROFILE_SCOPED_PREFIXES.any { prefix -> path == prefix || path.startsWith("$prefix/") }
|
||||||
|
|
||||||
|
override fun intercept(chain: Interceptor.Chain): Response {
|
||||||
|
val request = chain.request()
|
||||||
|
val profile =
|
||||||
|
AuthManager.getSelectedProfileId()
|
||||||
|
?: return chain.proceed(request)
|
||||||
|
|
||||||
|
val url = request.url
|
||||||
|
if (url.queryParameter("profile") != null) {
|
||||||
|
return chain.proceed(request) // explicit param wins
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!isProfileScopedPath(url.encodedPath)) {
|
||||||
|
return chain.proceed(request)
|
||||||
|
}
|
||||||
|
|
||||||
|
val scopedUrl =
|
||||||
|
url
|
||||||
|
.newBuilder()
|
||||||
|
.addQueryParameter("profile", profile)
|
||||||
|
.build()
|
||||||
|
|
||||||
|
return chain.proceed(request.newBuilder().url(scopedUrl).build())
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -43,6 +43,11 @@ class ApiClientTest {
|
|||||||
every { AuthManager.baseUrl() } returns mockWebServer.url("/").toString()
|
every { AuthManager.baseUrl() } returns mockWebServer.url("/").toString()
|
||||||
every { AuthManager.getHost() } returns "127.0.0.1"
|
every { AuthManager.getHost() } returns "127.0.0.1"
|
||||||
every { AuthManager.getPort() } returns 9119
|
every { AuthManager.getPort() } returns 9119
|
||||||
|
|
||||||
|
// PR #540 registers ProfileScopeInterceptor in ApiClient.buildService(),
|
||||||
|
// which calls getSelectedProfileId(). Stub it to null so the interceptor
|
||||||
|
// short-circuits (no profile scope) and these auth tests stay focused.
|
||||||
|
every { AuthManager.getSelectedProfileId() } returns null
|
||||||
}
|
}
|
||||||
|
|
||||||
@AfterEach
|
@AfterEach
|
||||||
|
|||||||
@@ -0,0 +1,129 @@
|
|||||||
|
package com.m57.hermescontrol.data.remote
|
||||||
|
|
||||||
|
import com.m57.hermescontrol.data.local.AuthManager
|
||||||
|
import io.mockk.every
|
||||||
|
import io.mockk.mockkObject
|
||||||
|
import io.mockk.unmockkAll
|
||||||
|
import okhttp3.HttpUrl
|
||||||
|
import okhttp3.OkHttpClient
|
||||||
|
import okhttp3.Request
|
||||||
|
import okhttp3.mockwebserver.MockResponse
|
||||||
|
import okhttp3.mockwebserver.MockWebServer
|
||||||
|
import org.junit.After
|
||||||
|
import org.junit.Assert.assertEquals
|
||||||
|
import org.junit.Assert.assertNull
|
||||||
|
import org.junit.Before
|
||||||
|
import org.junit.Test
|
||||||
|
|
||||||
|
class ProfileScopeInterceptorTest {
|
||||||
|
private lateinit var server: MockWebServer
|
||||||
|
|
||||||
|
@Before
|
||||||
|
fun setUp() {
|
||||||
|
mockkObject(AuthManager)
|
||||||
|
server = MockWebServer()
|
||||||
|
server.start()
|
||||||
|
}
|
||||||
|
|
||||||
|
@After
|
||||||
|
fun tearDown() {
|
||||||
|
server.shutdown()
|
||||||
|
unmockkAll()
|
||||||
|
}
|
||||||
|
|
||||||
|
/** Builds a real client with the interceptor + a stubbed AuthManager profile. */
|
||||||
|
private fun clientFor(profile: String?): OkHttpClient {
|
||||||
|
every { AuthManager.getSelectedProfileId() } returns profile
|
||||||
|
return OkHttpClient
|
||||||
|
.Builder()
|
||||||
|
.addInterceptor(ProfileScopeInterceptor)
|
||||||
|
.build()
|
||||||
|
}
|
||||||
|
|
||||||
|
private fun lastRequestedPath(client: OkHttpClient): HttpUrl {
|
||||||
|
server.enqueue(MockResponse().setResponseCode(200).setBody("{}"))
|
||||||
|
val req = Request.Builder().url(server.url("api/config")).build()
|
||||||
|
client.newCall(req).execute().close()
|
||||||
|
return server.takeRequest().requestUrl!!
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun scopedEndpoint_appendsProfile() {
|
||||||
|
val client = clientFor("work")
|
||||||
|
val url = lastRequestedPath(client)
|
||||||
|
assertEquals("work", url.queryParameter("profile"))
|
||||||
|
assertEquals("/api/config", url.encodedPath)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun noActiveProfile_passesThrough() {
|
||||||
|
val client = clientFor(null)
|
||||||
|
val url = lastRequestedPath(client)
|
||||||
|
assertNull(url.queryParameter("profile"))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun explicitProfileParam_wins() {
|
||||||
|
every { AuthManager.getSelectedProfileId() } returns "work"
|
||||||
|
val client = OkHttpClient.Builder().addInterceptor(ProfileScopeInterceptor).build()
|
||||||
|
server.enqueue(MockResponse().setResponseCode(200).setBody("{}"))
|
||||||
|
val req =
|
||||||
|
Request
|
||||||
|
.Builder()
|
||||||
|
.url(server.url("api/config?profile=other"))
|
||||||
|
.build()
|
||||||
|
client.newCall(req).execute().close()
|
||||||
|
val url = server.takeRequest().requestUrl!!
|
||||||
|
assertEquals("other", url.queryParameter("profile"))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun nonScopedEndpoint_untouched() {
|
||||||
|
every { AuthManager.getSelectedProfileId() } returns "work"
|
||||||
|
val client = OkHttpClient.Builder().addInterceptor(ProfileScopeInterceptor).build()
|
||||||
|
server.enqueue(MockResponse().setResponseCode(200).setBody("{}"))
|
||||||
|
val req = Request.Builder().url(server.url("api/cron/jobs")).build()
|
||||||
|
client.newCall(req).execute().close()
|
||||||
|
val url = server.takeRequest().requestUrl!!
|
||||||
|
assertNull(url.queryParameter("profile"))
|
||||||
|
assertEquals("/api/cron/jobs", url.encodedPath)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun skillsEndpoint_isScoped() {
|
||||||
|
val client = clientFor("alpha")
|
||||||
|
server.enqueue(MockResponse().setResponseCode(200).setBody("{}"))
|
||||||
|
val req = Request.Builder().url(server.url("api/skills")).build()
|
||||||
|
client.newCall(req).execute().close()
|
||||||
|
val url = server.takeRequest().requestUrl!!
|
||||||
|
assertEquals("alpha", url.queryParameter("profile"))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun lookalikePath_notScoped() {
|
||||||
|
// Sourcery review (PR #540): `startsWith` must not match non-segment
|
||||||
|
// suffixes like /api/statusXYZ or /api/gatewayExtra.
|
||||||
|
every { AuthManager.getSelectedProfileId() } returns "work"
|
||||||
|
val client = OkHttpClient.Builder().addInterceptor(ProfileScopeInterceptor).build()
|
||||||
|
server.enqueue(MockResponse().setResponseCode(200).setBody("{}"))
|
||||||
|
val req = Request.Builder().url(server.url("api/statusXYZ")).build()
|
||||||
|
client.newCall(req).execute().close()
|
||||||
|
val url = server.takeRequest().requestUrl!!
|
||||||
|
assertNull(url.queryParameter("profile"))
|
||||||
|
assertEquals("/api/statusXYZ", url.encodedPath)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun scopedSubPath_isScoped() {
|
||||||
|
// A scoped prefix with a trailing segment (/api/status/health) MUST
|
||||||
|
// still receive the profile param.
|
||||||
|
every { AuthManager.getSelectedProfileId() } returns "work"
|
||||||
|
val client = OkHttpClient.Builder().addInterceptor(ProfileScopeInterceptor).build()
|
||||||
|
server.enqueue(MockResponse().setResponseCode(200).setBody("{}"))
|
||||||
|
val req = Request.Builder().url(server.url("api/status/health")).build()
|
||||||
|
client.newCall(req).execute().close()
|
||||||
|
val url = server.takeRequest().requestUrl!!
|
||||||
|
assertEquals("work", url.queryParameter("profile"))
|
||||||
|
assertEquals("/api/status/health", url.encodedPath)
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in new issue
Block a user