Improve Git server config activity (#1051)

6 files changed, 94 insertions, 44 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c3e4118c..52236278 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,9 +9,14 @@ All notable changes to this project will be documented in this file.
 -   Allow sorting by recently used
 -   Add [Bromite](https://www.bromite.org/) and [Ungoogled Chromium](https://git.droidware.info/wchen342/ungoogled-chromium-android) to supported browsers list for Autofill
 
+### Changed
+
+-   A descriptive error message is shown if no username is specified in the Git server settings
+
 ### Fixed
 
 -   Password creation UI will scroll if it does not fit on the screen
+-   Git server protocol and authentication mode are only updated when explicitly saved
 
 ## [1.11.2] - 2020-08-24
 
diff --git a/app/src/androidTest/java/com/zeapo/pwdstore/MigrationsTest.kt b/app/src/androidTest/java/com/zeapo/pwdstore/MigrationsTest.kt
index d1b04fc3..c87bdae3 100644
--- a/app/src/androidTest/java/com/zeapo/pwdstore/MigrationsTest.kt
+++ b/app/src/androidTest/java/com/zeapo/pwdstore/MigrationsTest.kt
@@ -8,6 +8,8 @@ package com.zeapo.pwdstore
 
 import android.content.Context
 import androidx.core.content.edit
+import com.zeapo.pwdstore.git.config.ConnectionMode
+import com.zeapo.pwdstore.git.config.Protocol
 import com.zeapo.pwdstore.utils.PreferenceKeys
 import com.zeapo.pwdstore.utils.getString
 import com.zeapo.pwdstore.utils.sharedPrefs
@@ -33,7 +35,8 @@ class MigrationsTest {
             putString(PreferenceKeys.GIT_REMOTE_USERNAME, "msfjarvis")
             putString(PreferenceKeys.GIT_REMOTE_LOCATION, "/mnt/disk3/pass-repo")
             putString(PreferenceKeys.GIT_REMOTE_SERVER, "192.168.0.102")
-            putString(PreferenceKeys.GIT_REMOTE_PROTOCOL, "ssh://")
+            putString(PreferenceKeys.GIT_REMOTE_PROTOCOL, Protocol.Ssh.pref)
+            putString(PreferenceKeys.GIT_REMOTE_AUTH, ConnectionMode.Password.pref)
         }
         runMigrations(context)
         checkOldKeysAreRemoved(context)
@@ -51,7 +54,8 @@ class MigrationsTest {
             putString(PreferenceKeys.GIT_REMOTE_USERNAME, "msfjarvis")
             putString(PreferenceKeys.GIT_REMOTE_LOCATION, "/mnt/disk3/pass-repo")
             putString(PreferenceKeys.GIT_REMOTE_SERVER, "192.168.0.102")
-            putString(PreferenceKeys.GIT_REMOTE_PROTOCOL, "ssh://")
+            putString(PreferenceKeys.GIT_REMOTE_PROTOCOL, Protocol.Ssh.pref)
+            putString(PreferenceKeys.GIT_REMOTE_AUTH, ConnectionMode.SshKey.pref)
         }
         runMigrations(context)
         checkOldKeysAreRemoved(context)
@@ -69,7 +73,8 @@ class MigrationsTest {
             putString(PreferenceKeys.GIT_REMOTE_USERNAME, "msfjarvis")
             putString(PreferenceKeys.GIT_REMOTE_LOCATION, "Android-Password-Store/pass-test")
             putString(PreferenceKeys.GIT_REMOTE_SERVER, "github.com")
-            putString(PreferenceKeys.GIT_REMOTE_PROTOCOL, "https://")
+            putString(PreferenceKeys.GIT_REMOTE_PROTOCOL, Protocol.Https.pref)
+            putString(PreferenceKeys.GIT_REMOTE_AUTH, ConnectionMode.None.pref)
         }
         runMigrations(context)
         checkOldKeysAreRemoved(context)
diff --git a/app/src/main/java/com/zeapo/pwdstore/Migrations.kt b/app/src/main/java/com/zeapo/pwdstore/Migrations.kt
index 1e1943d8..d0221d9e 100644
--- a/app/src/main/java/com/zeapo/pwdstore/Migrations.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/Migrations.kt
@@ -76,7 +76,11 @@ private fun migrateToGitUrlBasedConfig(context: Context) {
         remove(PreferenceKeys.GIT_REMOTE_SERVER)
         remove(PreferenceKeys.GIT_REMOTE_USERNAME)
     }
-    if (url == null || !GitSettings.updateUrlIfValid(url)) {
+    if (url == null || GitSettings.updateConnectionSettingsIfValid(
+            newProtocol = protocol,
+            newConnectionMode = GitSettings.connectionMode,
+            newUrl = url,
+            newBranch = GitSettings.branch) != GitSettings.UpdateConnectionSettingsResult.Valid) {
         e { "Failed to migrate to URL-based Git config, generated URL is invalid" }
     }
 }
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/GitServerConfigActivity.kt b/app/src/main/java/com/zeapo/pwdstore/git/GitServerConfigActivity.kt
index 82e97e17..bb991449 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/GitServerConfigActivity.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/GitServerConfigActivity.kt
@@ -32,6 +32,9 @@ class GitServerConfigActivity : BaseGitActivity() {
 
     private val binding by viewBinding(ActivityGitCloneBinding::inflate)
 
+    private lateinit var newProtocol: Protocol
+    private lateinit var newConnectionMode: ConnectionMode
+
     override fun onCreate(savedInstanceState: Bundle?) {
         super.onCreate(savedInstanceState)
         val isClone = intent?.extras?.getInt(REQUEST_ARG_OP) ?: -1 == REQUEST_CLONE
@@ -41,22 +44,26 @@ class GitServerConfigActivity : BaseGitActivity() {
         setContentView(binding.root)
         supportActionBar?.setDisplayHomeAsUpEnabled(true)
 
-        binding.cloneProtocolGroup.check(when (GitSettings.protocol) {
-            Protocol.Ssh -> R.id.clone_protocol_ssh
-            Protocol.Https -> R.id.clone_protocol_https
-        })
-        binding.cloneProtocolGroup.addOnButtonCheckedListener { _, checkedId, checked ->
-            if (checked) {
-                when (checkedId) {
-                    R.id.clone_protocol_https -> GitSettings.protocol = Protocol.Https
-                    R.id.clone_protocol_ssh -> GitSettings.protocol = Protocol.Ssh
+        newProtocol = GitSettings.protocol
+        binding.cloneProtocolGroup.apply {
+            when (newProtocol) {
+                Protocol.Ssh -> check(R.id.clone_protocol_ssh)
+                Protocol.Https -> check(R.id.clone_protocol_https)
+            }
+            addOnButtonCheckedListener { _, checkedId, checked ->
+                if (checked) {
+                    when (checkedId) {
+                        R.id.clone_protocol_https -> newProtocol = Protocol.Https
+                        R.id.clone_protocol_ssh -> newProtocol = Protocol.Ssh
+                    }
+                    updateConnectionModeToggleGroup()
                 }
-                updateConnectionModeToggleGroup()
             }
         }
 
+        newConnectionMode = GitSettings.connectionMode
         binding.connectionModeGroup.apply {
-            when (GitSettings.connectionMode) {
+            when (newConnectionMode) {
                 ConnectionMode.SshKey -> check(R.id.connection_mode_ssh_key)
                 ConnectionMode.Password -> check(R.id.connection_mode_password)
                 ConnectionMode.OpenKeychain -> check(R.id.connection_mode_open_keychain)
@@ -64,10 +71,10 @@ class GitServerConfigActivity : BaseGitActivity() {
             }
             addOnButtonCheckedListener { _, _, _ ->
                 when (checkedButtonId) {
-                    R.id.connection_mode_ssh_key -> GitSettings.connectionMode = ConnectionMode.SshKey
-                    R.id.connection_mode_open_keychain -> GitSettings.connectionMode = ConnectionMode.OpenKeychain
-                    R.id.connection_mode_password -> GitSettings.connectionMode = ConnectionMode.Password
-                    View.NO_ID -> GitSettings.connectionMode = ConnectionMode.None
+                    R.id.connection_mode_ssh_key -> newConnectionMode = ConnectionMode.SshKey
+                    R.id.connection_mode_open_keychain -> newConnectionMode = ConnectionMode.OpenKeychain
+                    R.id.connection_mode_password -> newConnectionMode = ConnectionMode.Password
+                    View.NO_ID -> newConnectionMode = ConnectionMode.None
                 }
             }
         }
@@ -77,36 +84,50 @@ class GitServerConfigActivity : BaseGitActivity() {
         binding.serverBranch.setText(GitSettings.branch)
 
         binding.saveButton.setOnClickListener {
-            if (isClone && PasswordRepository.getRepository(null) == null)
-                PasswordRepository.initialize()
-            GitSettings.branch = binding.serverBranch.text.toString().trim()
-            if (GitSettings.updateUrlIfValid(binding.serverUrl.text.toString().trim())) {
-                if (!isClone) {
-                    Snackbar.make(binding.root, getString(R.string.git_server_config_save_success), Snackbar.LENGTH_SHORT).show()
-                    Handler().postDelayed(500) { finish() }
-                } else {
-                    cloneRepository()
+            when (GitSettings.updateConnectionSettingsIfValid(
+                newProtocol = newProtocol,
+                newConnectionMode = newConnectionMode,
+                newUrl = binding.serverUrl.text.toString().trim(),
+                newBranch = binding.serverBranch.text.toString().trim())) {
+                GitSettings.UpdateConnectionSettingsResult.FailedToParseUrl -> {
+                    Snackbar.make(binding.root, getString(R.string.git_server_config_save_error), Snackbar.LENGTH_LONG).show()
+                }
+                GitSettings.UpdateConnectionSettingsResult.MissingUsername -> {
+                    when (newProtocol) {
+                        Protocol.Https -> Snackbar.make(binding.root, getString(R.string.git_server_config_save_missing_username_https), Snackbar.LENGTH_LONG).show()
+                        Protocol.Ssh -> Snackbar.make(binding.root, getString(R.string.git_server_config_save_missing_username_ssh), Snackbar.LENGTH_LONG).show()
+                    }
+                }
+                else -> {
+                    if (isClone && PasswordRepository.getRepository(null) == null)
+                        PasswordRepository.initialize()
+                    if (!isClone) {
+                        Snackbar.make(binding.root, getString(R.string.git_server_config_save_success), Snackbar.LENGTH_SHORT).show()
+                        Handler().postDelayed(500) { finish() }
+                    } else {
+                        cloneRepository()
+                    }
                 }
-            } else {
-                Snackbar.make(binding.root, getString(R.string.git_server_config_save_error), Snackbar.LENGTH_LONG).show()
             }
         }
     }
 
     private fun updateConnectionModeToggleGroup() {
-        if (GitSettings.protocol == Protocol.Ssh) {
-            // Reset connection mode to SSH key if the current value (none) is not valid for SSH
-            if (binding.connectionModeGroup.checkedButtonIds.isEmpty())
-                binding.connectionModeGroup.check(R.id.connection_mode_ssh_key)
+        if (newProtocol == Protocol.Ssh) {
             binding.connectionModeSshKey.isEnabled = true
             binding.connectionModeOpenKeychain.isEnabled = true
+            // Reset connection mode to SSH key if the current value (none) is not valid for SSH.
+            // Important note: This has to happen after enabling the other toggle buttons or they
+            // won't check.
+            if (binding.connectionModeGroup.checkedButtonIds.isEmpty())
+                binding.connectionModeGroup.check(R.id.connection_mode_ssh_key)
             binding.connectionModeGroup.isSelectionRequired = true
         } else {
             binding.connectionModeGroup.isSelectionRequired = false
             // Reset connection mode to password if the current value is not valid for HTTPS
             // Important note: This has to happen before disabling the other toggle buttons or they
             // won't uncheck.
-            if (GitSettings.connectionMode !in listOf(ConnectionMode.None, ConnectionMode.Password))
+            if (newConnectionMode !in listOf(ConnectionMode.None, ConnectionMode.Password))
                 binding.connectionModeGroup.check(R.id.connection_mode_password)
             binding.connectionModeSshKey.isEnabled = false
             binding.connectionModeOpenKeychain.isEnabled = false
diff --git a/app/src/main/java/com/zeapo/pwdstore/git/config/GitSettings.kt b/app/src/main/java/com/zeapo/pwdstore/git/config/GitSettings.kt
index b81a8f8f..2efae40c 100644
--- a/app/src/main/java/com/zeapo/pwdstore/git/config/GitSettings.kt
+++ b/app/src/main/java/com/zeapo/pwdstore/git/config/GitSettings.kt
@@ -55,14 +55,14 @@ object GitSettings {
 
     var protocol
         get() = Protocol.fromString(settings.getString(PreferenceKeys.GIT_REMOTE_PROTOCOL))
-        set(value) {
+        private set(value) {
             settings.edit {
                 putString(PreferenceKeys.GIT_REMOTE_PROTOCOL, value.pref)
             }
         }
     var connectionMode
         get() = ConnectionMode.fromString(settings.getString(PreferenceKeys.GIT_REMOTE_AUTH))
-        set(value) {
+        private set(value) {
             settings.edit {
                 putString(PreferenceKeys.GIT_REMOTE_AUTH, value.pref)
             }
@@ -71,6 +71,8 @@ object GitSettings {
         get() = settings.getString(PreferenceKeys.GIT_REMOTE_URL)
         private set(value) {
             require(value != null)
+            if (value == url)
+                return
             settings.edit {
                 putString(PreferenceKeys.GIT_REMOTE_URL, value)
             }
@@ -96,20 +98,31 @@ object GitSettings {
         }
     var branch
         get() = settings.getString(PreferenceKeys.GIT_BRANCH_NAME) ?: DEFAULT_BRANCH
-        set(value) {
+        private set(value) {
             settings.edit {
                 putString(PreferenceKeys.GIT_BRANCH_NAME, value)
             }
         }
 
-    fun updateUrlIfValid(newUrl: String): Boolean {
-        try {
+    enum class UpdateConnectionSettingsResult {
+        Valid,
+        FailedToParseUrl,
+        MissingUsername,
+    }
+
+    fun updateConnectionSettingsIfValid(newProtocol: Protocol, newConnectionMode: ConnectionMode, newUrl: String, newBranch: String): UpdateConnectionSettingsResult {
+        val parsedUrl = try {
             URIish(newUrl)
         } catch (_: Exception) {
-            return false
+            return UpdateConnectionSettingsResult.FailedToParseUrl
         }
-        if (newUrl != url)
-            url = newUrl
-        return true
+        if (newConnectionMode != ConnectionMode.None && parsedUrl.user.isNullOrBlank())
+            return UpdateConnectionSettingsResult.MissingUsername
+
+        url = newUrl
+        protocol = newProtocol
+        connectionMode = newConnectionMode
+        branch = newBranch
+        return UpdateConnectionSettingsResult.Valid
     }
 }
diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml
index 1b32e29c..86e1c722 100644
--- a/app/src/main/res/values/strings.xml
+++ b/app/src/main/res/values/strings.xml
@@ -328,6 +328,8 @@
     <string name="connection_mode_openkeychain" translatable="false">OpenKeychain</string>
     <string name="git_server_config_save_success">Successfully saved configuration</string>
     <string name="git_server_config_save_error">The provided repository URL is not valid</string>
+    <string name="git_server_config_save_missing_username_https">Please specify the HTTPS username in the form https://username@example.com/…</string>
+    <string name="git_server_config_save_missing_username_ssh">Please specify the SSH username in the form username@example.com:…</string>
     <string name="git_config_error_hostname_empty">empty hostname</string>
     <string name="git_config_error_generic">please verify your settings and try again</string>
     <string name="git_config_error_nonnumeric_port">port must be numeric</string>