* Fix #647: resolve Default profile URL from top-level login URL ConnectionProfile.resolvedBaseUrl (used directly by ConnectionSection for the profile list display) silently fell back to the hardcoded loopback 127.0.0.1:9119 whenever a profile's own baseUrl was null, instead of using the URL the user actually authenticated against. - Add ConnectionProfile.resolveBaseUrl(topLevelBaseUrl) that prefers the profile's own baseUrl, then the top-level login URL, then legacy loopback. - Store-level ServerCollectionOps.resolvedBaseUrl now applies the same fallback so a null-baseUrl profile still shows the real gateway. - ConnectionSection, SettingsViewModel and ConnectViewModel resolve against AuthManager.getBaseUrl() so the loopback fallback can never surface. - Add Issue647ProfileUrlTest covering login-stamp, null-baseUrl fallback, own-URL precedence, fresh-install loopback, and store-level resolution. - Update existing AuthManagerTest assertions to the new property form. * Fix #647 (root cause): cache login URL at top-level baseUrl The previous change only made profile resolution prefer the top-level login URL, but setBaseUrl() never actually wrote the top-level baseUrl field — so it stayed DEFAULT_BASE_URL (https://127.0.0.1:9119/) forever. A Default profile seeded with the loopback default (or whose own baseUrl was never stamped) therefore still displayed the loopback URL after logging in via a custom LAN address. setBaseUrl() now also stamps the top-level baseUrl, making it the authoritative "last successful login URL" cache. getBaseUrl() and the store-level resolvedBaseUrl (profile-list display) now reflect the URL actually authenticated against. Add regression tests mirroring the exact AuthLoginViewModel.connect() sequence (seed with loopback -> setBaseUrl(loginUrl) -> token) asserting the Default profile, top-level baseUrl and store display all resolve to the login URL, never loopback. --------- Co-authored-by: M57 <Hy4ri@users.noreply.github.com>
This commit is contained in:
1 parent
c8a5055a84
commit
e44a53366b
7 files changed
+301
-17
No files matched your search
@@ -14,16 +14,44 @@ data class ConnectionProfile(
|
||||
val port: Int = 9119,
|
||||
val baseUrl: String? = null,
|
||||
) {
|
||||
/**
|
||||
* Resolves this profile's own base URL, falling back to the legacy loopback
|
||||
* default when [baseUrl] was never stamped. Prefer [resolveBaseUrl] at the
|
||||
* store/UI level so a real gateway URL used to log in takes precedence over
|
||||
* the hardcoded loopback (issue #647).
|
||||
*/
|
||||
val resolvedBaseUrl: String
|
||||
get() =
|
||||
baseUrl
|
||||
?.let {
|
||||
ServerEndpoint
|
||||
.parse(
|
||||
it,
|
||||
CleartextPolicy.ALLOW_WITH_WARNING,
|
||||
).baseUrl
|
||||
.toString()
|
||||
}
|
||||
?: ServerEndpoint.fromLegacy(host, port).baseUrl.toString()
|
||||
get() = resolveBaseUrl(null)
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolves the base URL actually used to talk to this profile's server.
|
||||
*
|
||||
* Prefers the profile's own [ConnectionProfile.baseUrl]; if that was never
|
||||
* stamped (legacy or a freshly-seeded Default profile) it falls back to
|
||||
* [topLevelBaseUrl] — the URL the user last authenticated against — and only
|
||||
* then to the legacy loopback default. This prevents the connection-profile
|
||||
* list from showing the hardcoded `127.0.0.1:9119` when a real gateway URL was
|
||||
* used to log in (issue #647).
|
||||
*/
|
||||
fun ConnectionProfile.resolveBaseUrl(topLevelBaseUrl: String?): String {
|
||||
baseUrl
|
||||
?.let {
|
||||
return ServerEndpoint
|
||||
.parse(
|
||||
it,
|
||||
CleartextPolicy.ALLOW_WITH_WARNING,
|
||||
).baseUrl
|
||||
.toString()
|
||||
}
|
||||
return topLevelBaseUrl
|
||||
?.let {
|
||||
ServerEndpoint
|
||||
.parse(
|
||||
it,
|
||||
CleartextPolicy.ALLOW_WITH_WARNING,
|
||||
).baseUrl
|
||||
.toString()
|
||||
}
|
||||
?: ServerEndpoint.fromLegacy(host, port).baseUrl.toString()
|
||||
}
|
||||
@@ -37,7 +37,13 @@ fun ServerStoreState.selfHealed(): ServerStoreState {
|
||||
val ServerStoreState.resolvedBaseUrl: String
|
||||
get() {
|
||||
val selected = connectionProfiles.firstOrNull { it.id == selectedProfileId }
|
||||
if (selected != null) return selected.resolvedBaseUrl
|
||||
if (selected != null) {
|
||||
// Prefer the selected profile's own URL, but fall back to the
|
||||
// top-level login URL (and only then to legacy loopback) so a profile
|
||||
// whose baseUrl was never stamped still shows the gateway used to
|
||||
// authenticate, not the hardcoded 127.0.0.1:9119 (issue #647).
|
||||
return selected.resolveBaseUrl(baseUrl)
|
||||
}
|
||||
return baseUrl
|
||||
?.let {
|
||||
ServerEndpoint
|
||||
|
||||
@@ -441,7 +441,16 @@ object AuthManager {
|
||||
profile
|
||||
}
|
||||
}
|
||||
state.copy(connectionProfiles = profiles)
|
||||
// Also cache the login URL at the top level so that getBaseUrl() and
|
||||
// the store-level resolvedBaseUrl (used by the profile list display)
|
||||
// reflect the URL actually authenticated against, never the hardcoded
|
||||
// loopback default. Fixes issue #647: a Default profile whose own
|
||||
// baseUrl was never stamped must still resolve to this URL, not
|
||||
// 127.0.0.1:9119.
|
||||
state.copy(
|
||||
baseUrl = normalized,
|
||||
connectionProfiles = profiles,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -6,6 +6,7 @@ import androidx.lifecycle.ViewModelProvider
|
||||
import androidx.lifecycle.viewModelScope
|
||||
import com.m57.hermescontrol.R
|
||||
import com.m57.hermescontrol.data.config.ConnectionProfile
|
||||
import com.m57.hermescontrol.data.config.resolveBaseUrl
|
||||
import com.m57.hermescontrol.data.local.AuthManager
|
||||
import com.m57.hermescontrol.data.remote.ApiClient
|
||||
import com.m57.hermescontrol.data.remote.CleartextPolicy
|
||||
@@ -53,7 +54,7 @@ class ConnectViewModel(
|
||||
_uiState.update {
|
||||
it.copy(
|
||||
token = savedToken,
|
||||
baseUrl = selectedProfile?.resolvedBaseUrl ?: savedBaseUrl,
|
||||
baseUrl = selectedProfile?.resolveBaseUrl(savedBaseUrl) ?: savedBaseUrl,
|
||||
profiles = profiles,
|
||||
selectedProfile = selectedProfile,
|
||||
profileName = selectedProfile?.name ?: "",
|
||||
@@ -76,7 +77,7 @@ class ConnectViewModel(
|
||||
it.copy(
|
||||
selectedProfile = profile,
|
||||
profileName = profile.name,
|
||||
baseUrl = profile.resolvedBaseUrl,
|
||||
baseUrl = profile.resolveBaseUrl(AuthManager.getBaseUrl()),
|
||||
token = token,
|
||||
)
|
||||
}
|
||||
|
||||
@@ -4,6 +4,7 @@ import androidx.lifecycle.ViewModel
|
||||
import androidx.lifecycle.viewModelScope
|
||||
import com.m57.hermescontrol.ScreenRegistry
|
||||
import com.m57.hermescontrol.data.config.ConnectionProfile
|
||||
import com.m57.hermescontrol.data.config.resolveBaseUrl
|
||||
import com.m57.hermescontrol.data.local.AuthManager
|
||||
import com.m57.hermescontrol.data.remote.ApiClient
|
||||
import com.m57.hermescontrol.data.remote.CleartextPolicy
|
||||
@@ -170,7 +171,7 @@ class SettingsViewModel(
|
||||
showProfileDialog = true,
|
||||
editingProfileId = profileId,
|
||||
dialogProfileName = profile.name,
|
||||
dialogProfileBaseUrl = profile.resolvedBaseUrl,
|
||||
dialogProfileBaseUrl = profile.resolveBaseUrl(AuthManager.getBaseUrl()),
|
||||
dialogProfileToken = token,
|
||||
dialogProfileError = null,
|
||||
)
|
||||
|
||||
@@ -35,6 +35,7 @@ import androidx.compose.ui.text.font.FontWeight
|
||||
import androidx.compose.ui.unit.dp
|
||||
import androidx.lifecycle.viewmodel.compose.viewModel
|
||||
import com.m57.hermescontrol.R
|
||||
import com.m57.hermescontrol.data.config.resolveBaseUrl
|
||||
import com.m57.hermescontrol.ui.settings.SectionCard
|
||||
import com.m57.hermescontrol.ui.settings.SettingsUiState
|
||||
import com.m57.hermescontrol.ui.settings.SettingsViewModel
|
||||
@@ -85,7 +86,7 @@ internal fun ConnectionSection(
|
||||
style = MaterialTheme.typography.bodyMedium.copy(fontWeight = FontWeight.Medium),
|
||||
)
|
||||
Text(
|
||||
text = profile.resolvedBaseUrl,
|
||||
text = profile.resolveBaseUrl(state.baseUrl),
|
||||
style =
|
||||
MaterialTheme.typography.bodySmall.copy(
|
||||
color = MaterialTheme.colorScheme.onSurfaceVariant,
|
||||
|
||||
@@ -0,0 +1,238 @@
|
||||
package com.m57.hermescontrol.data.local
|
||||
|
||||
import android.content.Context
|
||||
import android.content.SharedPreferences
|
||||
import androidx.security.crypto.EncryptedSharedPreferences
|
||||
import androidx.security.crypto.MasterKeys
|
||||
import com.m57.hermescontrol.data.config.ConnectionProfile
|
||||
import com.m57.hermescontrol.data.config.ServerStoreState
|
||||
import com.m57.hermescontrol.data.config.resolveBaseUrl
|
||||
import com.m57.hermescontrol.data.config.resolvedBaseUrl
|
||||
import io.mockk.every
|
||||
import io.mockk.mockk
|
||||
import io.mockk.mockkStatic
|
||||
import io.mockk.unmockkAll
|
||||
import org.junit.After
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertNotEquals
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
|
||||
class ReproContext(
|
||||
private val tempDir: java.io.File,
|
||||
baseContext: Context,
|
||||
) : android.content.ContextWrapper(baseContext) {
|
||||
override fun getFilesDir(): java.io.File = tempDir
|
||||
|
||||
override fun getApplicationContext(): Context = this
|
||||
}
|
||||
|
||||
/**
|
||||
* Regression tests for issue #647: the Default connection profile must show the
|
||||
* URL actually used to log in, never the hardcoded `127.0.0.1:9119` loopback.
|
||||
*
|
||||
* Two layers are covered:
|
||||
* 1. The login path stamps the selected profile's `baseUrl` with the login URL
|
||||
* (so the profile carries the real URL on its own).
|
||||
* 2. Defensive layer: even when a profile's own `baseUrl` is null, resolution
|
||||
* prefers the top-level login URL over the legacy loopback fallback.
|
||||
*/
|
||||
class Issue647ProfileUrlTest {
|
||||
private lateinit var mockPrefs: SharedPreferences
|
||||
private lateinit var mockEditor: SharedPreferences.Editor
|
||||
private lateinit var mockContext: Context
|
||||
private lateinit var testContext: Context
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
mockPrefs = mockk(relaxed = true)
|
||||
mockEditor = mockk(relaxed = true)
|
||||
mockContext = mockk(relaxed = true)
|
||||
|
||||
every { mockPrefs.edit() } returns mockEditor
|
||||
every { mockEditor.putString(any(), any()) } returns mockEditor
|
||||
every { mockEditor.putInt(any(), any()) } returns mockEditor
|
||||
every { mockEditor.putBoolean(any(), any()) } returns mockEditor
|
||||
every { mockPrefs.getBoolean("migrated_to_datastore", any()) } returns true
|
||||
|
||||
mockkStatic(android.util.Log::class)
|
||||
every { android.util.Log.d(any(), any()) } returns 0
|
||||
every { android.util.Log.e(any(), any()) } returns 0
|
||||
every { android.util.Log.e(any(), any(), any()) } returns 0
|
||||
every { android.util.Log.w(any(), any<String>()) } returns 0
|
||||
every { android.util.Log.i(any(), any()) } returns 0
|
||||
|
||||
val tempDir = java.io.File(System.getProperty("java.io.tmpdir") ?: "/tmp")
|
||||
testContext = ReproContext(tempDir, mockContext)
|
||||
|
||||
val tempFile = java.io.File(tempDir, "server_store.json")
|
||||
if (tempFile.exists()) tempFile.delete()
|
||||
|
||||
mockkStatic(EncryptedSharedPreferences::class)
|
||||
every {
|
||||
EncryptedSharedPreferences.create(
|
||||
any<String>(),
|
||||
any<String>(),
|
||||
any<Context>(),
|
||||
any<EncryptedSharedPreferences.PrefKeyEncryptionScheme>(),
|
||||
any<EncryptedSharedPreferences.PrefValueEncryptionScheme>(),
|
||||
)
|
||||
} returns mockPrefs
|
||||
|
||||
mockkStatic(MasterKeys::class)
|
||||
every { MasterKeys.getOrCreate(any()) } returns "mockMasterKey"
|
||||
|
||||
val field = AuthManager::class.java.getDeclaredField("prefsDeferred")
|
||||
field.isAccessible = true
|
||||
field.set(AuthManager, null)
|
||||
|
||||
val storeField = AuthManager::class.java.getDeclaredField("_serverStore")
|
||||
storeField.isAccessible = true
|
||||
storeField.set(AuthManager, null)
|
||||
|
||||
AuthManager.resetAuthStateForTest()
|
||||
AuthManager.init(testContext)
|
||||
|
||||
kotlinx.coroutines.runBlocking {
|
||||
val deferred = field.get(AuthManager) as? kotlinx.coroutines.Deferred<*>
|
||||
deferred?.await()
|
||||
}
|
||||
AuthManager.serverStore.getLatestState()
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() {
|
||||
val tempDir = java.io.File(System.getProperty("java.io.tmpdir") ?: "/tmp")
|
||||
val tempFile = java.io.File(tempDir, "server_store.json")
|
||||
if (tempFile.exists()) tempFile.delete()
|
||||
unmockkAll()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun defaultProfile_afterLogin_reflectsLoginUrl() {
|
||||
// Mirror ConnectViewModel.connect() success path (no saveProfile branch).
|
||||
val customUrl = "http://192.1.1.190:9119/"
|
||||
AuthManager.ensureDefaultProfile()
|
||||
AuthManager.setSelectedProfileId(AuthManager.DEFAULT_PROFILE_ID)
|
||||
AuthManager.setBaseUrl(customUrl)
|
||||
AuthManager.setProfileToken(AuthManager.DEFAULT_PROFILE_ID, "tok")
|
||||
|
||||
val default =
|
||||
AuthManager.getConnectionProfiles().first {
|
||||
it.id == AuthManager.DEFAULT_PROFILE_ID
|
||||
}
|
||||
assertEquals(customUrl, default.resolveBaseUrl(null))
|
||||
// Top-level + store-level (the profile-list display) must match too.
|
||||
assertEquals(customUrl, AuthManager.getBaseUrl())
|
||||
assertEquals(customUrl, AuthManager.serverStore.getLatestState().resolvedBaseUrl)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun authLogin_connect_sequence_resolvesToLoginUrl_notLoopback() {
|
||||
// Mirror the exact AuthLoginViewModel.connect() sequence the user hits:
|
||||
// the Default profile is seeded (with the loopback default) and the user
|
||||
// logs in via a custom LAN URL. After connect the Default profile, the
|
||||
// top-level base URL, and the store-level display must all reflect the
|
||||
// login URL — never the hardcoded 127.0.0.1:9119 (issue #647).
|
||||
val loginUrl = "http://192.168.1.57:9119/"
|
||||
val loopback = "https://127.0.0.1:9119/"
|
||||
|
||||
// Seed the Default profile the way the app does (host/port -> loopback).
|
||||
AuthManager.ensureDefaultProfile()
|
||||
// Confirm the initial seed really is the loopback default.
|
||||
val seeded =
|
||||
AuthManager.getConnectionProfiles().first {
|
||||
it.id == AuthManager.DEFAULT_PROFILE_ID
|
||||
}
|
||||
assertEquals(loopback, seeded.resolveBaseUrl(null))
|
||||
|
||||
// Replicate AuthLoginViewModel.connect(): select -> setBaseUrl(login) -> token.
|
||||
AuthManager.setSelectedProfileId(AuthManager.DEFAULT_PROFILE_ID)
|
||||
AuthManager.setBaseUrl(loginUrl)
|
||||
AuthManager.setToken("ws-cred")
|
||||
|
||||
val default =
|
||||
AuthManager.getConnectionProfiles().first {
|
||||
it.id == AuthManager.DEFAULT_PROFILE_ID
|
||||
}
|
||||
assertEquals(loginUrl, default.resolveBaseUrl(null))
|
||||
assertEquals(loginUrl, AuthManager.getBaseUrl())
|
||||
assertEquals(loginUrl, AuthManager.serverStore.getLatestState().resolvedBaseUrl)
|
||||
assertNotEquals(loopback, AuthManager.getBaseUrl())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun defaultProfile_withNullBaseUrl_fallsBackToLoginUrlNotLoopback() {
|
||||
// A Default profile whose own baseUrl was never stamped must still show
|
||||
// the top-level login URL, never the hardcoded loopback (issue #647).
|
||||
val customUrl = "http://192.1.1.190:9119/"
|
||||
val defaultNoUrl =
|
||||
ConnectionProfile(
|
||||
id = AuthManager.DEFAULT_PROFILE_ID,
|
||||
name = AuthManager.DEFAULT_PROFILE_NAME,
|
||||
baseUrl = null,
|
||||
)
|
||||
AuthManager.saveConnectionProfiles(listOf(defaultNoUrl))
|
||||
AuthManager.setSelectedProfileId(AuthManager.DEFAULT_PROFILE_ID)
|
||||
// Top-level login URL is set, but the profile's own baseUrl remains null.
|
||||
AuthManager.setBaseUrl(customUrl)
|
||||
|
||||
assertEquals(customUrl, defaultNoUrl.resolveBaseUrl(customUrl))
|
||||
assertEquals(customUrl, AuthManager.getBaseUrl())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun profileWithBaseUrl_ignoresTopLevelLoginUrl() {
|
||||
// When the profile has its own URL, that wins — no leakage from the
|
||||
// top-level field.
|
||||
val own = "http://10.0.0.5:9119/"
|
||||
val top = "http://192.1.1.190:9119/"
|
||||
val profile =
|
||||
ConnectionProfile(
|
||||
id = "prof-x",
|
||||
name = "X",
|
||||
baseUrl = own,
|
||||
)
|
||||
assertEquals(own, profile.resolveBaseUrl(top))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun freshInstall_withoutSavedUrl_defaultsToLoopback() {
|
||||
// No login URL anywhere: a null-baseUrl profile falls back to the
|
||||
// legacy loopback (http://127.0.0.1:9119/), preserving the fresh-install
|
||||
// default (issue #647 acceptance criteria). In practice the Default
|
||||
// profile is seeded from the top-level https default, but the pure
|
||||
// fallback must remain loopback.
|
||||
val fresh =
|
||||
ConnectionProfile(
|
||||
id = AuthManager.DEFAULT_PROFILE_ID,
|
||||
name = AuthManager.DEFAULT_PROFILE_NAME,
|
||||
baseUrl = null,
|
||||
)
|
||||
assertEquals(
|
||||
"http://127.0.0.1:9119/",
|
||||
fresh.resolveBaseUrl(null),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun serverStoreState_selectedProfileNullBaseUrl_usesTopLevel() {
|
||||
// Store-level resolution: selected profile with null baseUrl uses the
|
||||
// top-level login URL, not loopback.
|
||||
val customUrl = "http://192.1.1.190:9119/"
|
||||
val state =
|
||||
ServerStoreState(
|
||||
baseUrl = customUrl,
|
||||
connectionProfiles =
|
||||
listOf(
|
||||
ConnectionProfile(
|
||||
id = AuthManager.DEFAULT_PROFILE_ID,
|
||||
name = AuthManager.DEFAULT_PROFILE_NAME,
|
||||
baseUrl = null,
|
||||
),
|
||||
),
|
||||
selectedProfileId = AuthManager.DEFAULT_PROFILE_ID,
|
||||
)
|
||||
assertEquals(customUrl, state.resolvedBaseUrl)
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user