Get rid of explicit Git server protocol (#1054)

* Double check Git server protocol

Ensure that the Git server protocol is not at odds with the URL scheme.

Also move the Protocol switches below the URL to make it clear that the
URL should be entered first.

* Remove protocol selection from server config

The protocol is now extracted from the URL, and the authentication mode selection is validated by GitSettings

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

* Don't use pref values for auth modes

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

* Apply suggestions from code review

Remove now unused protocol mismatch result type

Co-authored-by: Fabian Henneke <FabianHenneke@users.noreply.github.com>

* Simplify migration logic and fix tests

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

* Revert "Simplify migration logic and fix tests"

This reverts commit 1c4c4ba5fbc212843cb6b74dd29ac858eaea7582.

* Detect URLs with null scheme as ssh

* Add changelog entry

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>

Co-authored-by: Harsh Shandilya <me@msfjarvis.dev>
Co-authored-by: Harsh Shandilya <msfjarvis@gmail.com>

CHANGELOG.md

@@ -12,6 +12,7 @@ All notable changes to this project will be documented in this file.
 ### Changed
 
 -   A descriptive error message is shown if no username is specified in the Git server settings
+-   Remove explicit protocol choice from Git server settings, it is now inferred from your URL
 
 ### Fixed
 

app/src/androidTest/java/com/zeapo/pwdstore/MigrationsTest.kt

@@ -24,6 +24,7 @@ class MigrationsTest {
         assertNull(getString(PreferenceKeys.GIT_REMOTE_USERNAME))
         assertNull(getString(PreferenceKeys.GIT_REMOTE_SERVER))
         assertNull(getString(PreferenceKeys.GIT_REMOTE_LOCATION))
+        assertNull(getString(PreferenceKeys.GIT_REMOTE_PROTOCOL))
     }
 
     @Test

app/src/main/java/com/zeapo/pwdstore/Migrations.kt

@@ -75,13 +75,12 @@ private fun migrateToGitUrlBasedConfig(context: Context) {
         remove(PreferenceKeys.GIT_REMOTE_PORT)
         remove(PreferenceKeys.GIT_REMOTE_SERVER)
         remove(PreferenceKeys.GIT_REMOTE_USERNAME)
+        remove(PreferenceKeys.GIT_REMOTE_PROTOCOL)
     }
     if (url == null || GitSettings.updateConnectionSettingsIfValid(
-            newProtocol = protocol,
             newAuthMode = GitSettings.authMode,
             newUrl = url,
             newBranch = GitSettings.branch) != GitSettings.UpdateConnectionSettingsResult.Valid) {
         e { "Failed to migrate to URL-based Git config, generated URL is invalid" }
     }
 }
-

app/src/main/java/com/zeapo/pwdstore/git/GitServerConfigActivity.kt

@@ -32,7 +32,6 @@ class GitServerConfigActivity : BaseGitActivity() {
 
     private val binding by viewBinding(ActivityGitCloneBinding::inflate)
 
-    private lateinit var newProtocol: Protocol
     private lateinit var newAuthMode: AuthMode
 
     override fun onCreate(savedInstanceState: Bundle?) {
@@ -44,24 +43,8 @@ class GitServerConfigActivity : BaseGitActivity() {
         setContentView(binding.root)
         supportActionBar?.setDisplayHomeAsUpEnabled(true)
 
-        newProtocol = GitSettings.protocol
-        binding.protocolGroup.apply {
-            when (newProtocol) {
-                Protocol.Ssh -> check(R.id.protocol_ssh)
-                Protocol.Https -> check(R.id.protocol_https)
-            }
-            addOnButtonCheckedListener { _, checkedId, checked ->
-                if (checked) {
-                    when (checkedId) {
-                        R.id.protocol_https -> newProtocol = Protocol.Https
-                        R.id.protocol_ssh -> newProtocol = Protocol.Ssh
-                    }
-                    updateAuthModeToggleGroup()
-                }
-            }
-        }
-
         newAuthMode = GitSettings.authMode
+
         binding.authModeGroup.apply {
             when (newAuthMode) {
                 AuthMode.SshKey -> check(R.id.auth_mode_ssh_key)
@@ -78,27 +61,25 @@ class GitServerConfigActivity : BaseGitActivity() {
                 }
             }
         }
-        updateAuthModeToggleGroup()
 
         binding.serverUrl.setText(GitSettings.url)
         binding.serverBranch.setText(GitSettings.branch)
 
         binding.saveButton.setOnClickListener {
-            when (GitSettings.updateConnectionSettingsIfValid(
-                newProtocol = newProtocol,
+            when (val updateResult = GitSettings.updateConnectionSettingsIfValid(
                 newAuthMode = newAuthMode,
                 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) {
+                is GitSettings.UpdateConnectionSettingsResult.MissingUsername -> {
+                    when (updateResult.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 -> {
+                GitSettings.UpdateConnectionSettingsResult.Valid -> {
                     if (isClone && PasswordRepository.getRepository(null) == null)
                         PasswordRepository.initialize()
                     if (!isClone) {
@@ -108,32 +89,18 @@ class GitServerConfigActivity : BaseGitActivity() {
                         cloneRepository()
                     }
                 }
+                is GitSettings.UpdateConnectionSettingsResult.AuthModeMismatch -> {
+                    val message = getString(
+                        R.string.git_server_config_save_auth_mode_mismatch,
+                        updateResult.newProtocol,
+                        updateResult.validModes.joinToString(", "),
+                    )
+                    Snackbar.make(binding.root, message, Snackbar.LENGTH_LONG).show()
+                }
             }
         }
     }
 
-    private fun updateAuthModeToggleGroup() {
-        if (newProtocol == Protocol.Ssh) {
-            binding.authModeSshKey.isEnabled = true
-            binding.authModeOpenKeychain.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.authModeGroup.checkedButtonIds.isEmpty())
-                binding.authModeGroup.check(R.id.auth_mode_ssh_key)
-            binding.authModeGroup.isSelectionRequired = true
-        } else {
-            binding.authModeGroup.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 (newAuthMode !in listOf(AuthMode.None, AuthMode.Password))
-                binding.authModeGroup.check(R.id.auth_mode_password)
-            binding.authModeSshKey.isEnabled = false
-            binding.authModeOpenKeychain.isEnabled = false
-        }
-    }
-
     /**
      * Clones the repository, the directory exists, deletes it
      */

app/src/main/java/com/zeapo/pwdstore/git/config/GitSettings.kt

@@ -53,13 +53,6 @@ object GitSettings {
     private val settings by lazy { Application.instance.sharedPrefs }
     private val encryptedSettings by lazy { Application.instance.getEncryptedPrefs("git_operation") }
 
-    var protocol
-        get() = Protocol.fromString(settings.getString(PreferenceKeys.GIT_REMOTE_PROTOCOL))
-        private set(value) {
-            settings.edit {
-                putString(PreferenceKeys.GIT_REMOTE_PROTOCOL, value.pref)
-            }
-        }
     var authMode
         get() = AuthMode.fromString(settings.getString(PreferenceKeys.GIT_REMOTE_AUTH))
         private set(value) {
@@ -104,23 +97,39 @@ object GitSettings {
             }
         }
 
-    enum class UpdateConnectionSettingsResult {
-        Valid,
-        FailedToParseUrl,
-        MissingUsername,
+    sealed class UpdateConnectionSettingsResult {
+        class MissingUsername(val newProtocol: Protocol) : UpdateConnectionSettingsResult()
+        class AuthModeMismatch(val newProtocol: Protocol, val validModes: List<AuthMode>) : UpdateConnectionSettingsResult()
+        object Valid : UpdateConnectionSettingsResult()
+        object FailedToParseUrl : UpdateConnectionSettingsResult()
     }
 
-    fun updateConnectionSettingsIfValid(newProtocol: Protocol, newAuthMode: AuthMode, newUrl: String, newBranch: String): UpdateConnectionSettingsResult {
+    fun updateConnectionSettingsIfValid(newAuthMode: AuthMode, newUrl: String, newBranch: String): UpdateConnectionSettingsResult {
         val parsedUrl = try {
             URIish(newUrl)
         } catch (_: Exception) {
             return UpdateConnectionSettingsResult.FailedToParseUrl
         }
+        val newProtocol = when (parsedUrl.scheme) {
+            in listOf("http", "https") -> Protocol.Https
+            in listOf("ssh", null) -> Protocol.Ssh
+            else -> return UpdateConnectionSettingsResult.FailedToParseUrl
+        }
         if (newAuthMode != AuthMode.None && parsedUrl.user.isNullOrBlank())
-            return UpdateConnectionSettingsResult.MissingUsername
+            return UpdateConnectionSettingsResult.MissingUsername(newProtocol)
+
+        val validHttpsAuth = listOf(AuthMode.None, AuthMode.Password)
+        val validSshAuth = listOf(AuthMode.OpenKeychain, AuthMode.Password, AuthMode.SshKey)
+        when {
+            newProtocol == Protocol.Https && newAuthMode !in validHttpsAuth -> {
+                return UpdateConnectionSettingsResult.AuthModeMismatch(newProtocol, validHttpsAuth)
+            }
+            newProtocol == Protocol.Ssh && newAuthMode !in validSshAuth -> {
+                return UpdateConnectionSettingsResult.AuthModeMismatch(newProtocol, validSshAuth)
+            }
+        }
 
         url = newUrl
-        protocol = newProtocol
         authMode = newAuthMode
         branch = newBranch
         return UpdateConnectionSettingsResult.Valid

app/src/main/java/com/zeapo/pwdstore/utils/PreferenceKeys.kt

@@ -32,6 +32,7 @@ object PreferenceKeys {
     const val GIT_REMOTE_LOCATION = "git_remote_location"
     @Deprecated("Use GIT_REMOTE_URL instead")
     const val GIT_REMOTE_PORT = "git_remote_port"
+    @Deprecated("Use GIT_REMOTE_URL instead")
     const val GIT_REMOTE_PROTOCOL = "git_remote_protocol"
     const val GIT_DELETE_REPO = "git_delete_repo"
     @Deprecated("Use GIT_REMOTE_URL instead")

app/src/main/res/layout/activity_git_clone.xml

@@ -29,42 +29,6 @@
             app:layout_constraintStart_toStartOf="parent"
             app:layout_constraintTop_toTopOf="parent" />
 
-        <androidx.appcompat.widget.AppCompatTextView
-            android:id="@+id/label_server_protocol"
-            style="@style/TextAppearance.MaterialComponents.Headline6"
-            android:layout_width="0dp"
-            android:layout_height="wrap_content"
-            android:layout_margin="8dp"
-            android:text="@string/server_protocol"
-            app:layout_constraintStart_toStartOf="parent"
-            app:layout_constraintTop_toBottomOf="@id/server_label" />
-
-        <com.google.android.material.button.MaterialButtonToggleGroup
-            android:id="@+id/protocol_group"
-            style="@style/TextAppearance.MaterialComponents.Headline1"
-            android:layout_width="0dp"
-            android:layout_height="wrap_content"
-            android:layout_margin="8dp"
-            app:layout_constraintStart_toStartOf="parent"
-            app:layout_constraintTop_toBottomOf="@id/label_server_protocol"
-            app:selectionRequired="true"
-            app:singleSelection="true">
-
-            <com.google.android.material.button.MaterialButton
-                android:id="@+id/protocol_ssh"
-                style="?attr/materialButtonOutlinedStyle"
-                android:layout_width="wrap_content"
-                android:layout_height="wrap_content"
-                android:text="@string/clone_protocol_ssh" />
-
-            <com.google.android.material.button.MaterialButton
-                android:id="@+id/protocol_https"
-                style="?attr/materialButtonOutlinedStyle"
-                android:layout_width="wrap_content"
-                android:layout_height="wrap_content"
-                android:text="@string/clone_protocol_https" />
-        </com.google.android.material.button.MaterialButtonToggleGroup>
-
         <com.google.android.material.textfield.TextInputLayout
             android:id="@+id/label_server_url"
             android:layout_width="0dp"
@@ -73,15 +37,15 @@
             android:hint="@string/server_url"
             app:layout_constraintEnd_toEndOf="parent"
             app:layout_constraintStart_toStartOf="parent"
-            app:layout_constraintTop_toBottomOf="@id/protocol_group">
+            app:layout_constraintTop_toBottomOf="@id/server_label">
 
             <com.google.android.material.textfield.TextInputEditText
                 android:id="@+id/server_url"
                 android:layout_width="match_parent"
                 android:layout_height="wrap_content"
                 android:imeOptions="actionNext"
-                android:nextFocusForward="@id/server_branch"
-                android:inputType="textWebEmailAddress" />
+                android:inputType="textWebEmailAddress"
+                android:nextFocusForward="@id/server_branch" />
 
         </com.google.android.material.textfield.TextInputLayout>
 
@@ -91,21 +55,21 @@
             android:layout_height="wrap_content"
             android:layout_margin="8dp"
             android:hint="@string/server_branch"
-            app:layout_constraintStart_toStartOf="parent"
             app:layout_constraintEnd_toEndOf="parent"
+            app:layout_constraintStart_toStartOf="parent"
             app:layout_constraintTop_toBottomOf="@id/label_server_url">
 
             <com.google.android.material.textfield.TextInputEditText
                 android:id="@+id/server_branch"
-                android:imeOptions="actionDone"
                 android:layout_width="match_parent"
-                android:inputType="textNoSuggestions"
-                android:layout_height="wrap_content" />
+                android:layout_height="wrap_content"
+                android:imeOptions="actionDone"
+                android:inputType="textNoSuggestions" />
 
         </com.google.android.material.textfield.TextInputLayout>
 
         <androidx.appcompat.widget.AppCompatTextView
-            android:id="@+id/label_connection_mode"
+            android:id="@+id/label_auth_mode"
             style="@style/TextAppearance.MaterialComponents.Headline6"
             android:layout_width="wrap_content"
             android:layout_height="wrap_content"
@@ -123,7 +87,7 @@
             android:layout_height="wrap_content"
             android:layout_margin="8dp"
             app:layout_constraintStart_toStartOf="parent"
-            app:layout_constraintTop_toBottomOf="@id/label_connection_mode"
+            app:layout_constraintTop_toBottomOf="@id/label_auth_mode"
             app:singleSelection="true">
 
             <com.google.android.material.button.MaterialButton

app/src/main/res/values/strings.xml

@@ -330,6 +330,7 @@
     <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_server_config_save_auth_mode_mismatch">Valid authentication modes for %1$s: %2$s</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>