fix: address Copilot PR review comments

- Fix UI labels to distinguish plain text vs Key Storage password fields
- Reverse precedence: Key Storage password now takes precedence over plain text
- Add null safety check before setting password from Key Storage
- Fix NullPointerException by reusing storageTree variable
- Add error handling with try-catch and logging for Key Storage access
- Add JavaDoc documentation to getFromKeyStorage method
- Use explicit UTF-8 encoding for password strings
- Update README with Key Storage authentication documentation
- Clean up test file whitespace

All changes maintain backwards compatibility - no breaking changes to property names.

Author: fdevans

README.md

@@ -54,11 +54,23 @@ You need to set up the following options to use the plugin:
 
 ### Authentication
 
-* **Git Password**: Password to authenticate remotely
+The plugin supports multiple authentication methods:
+
+#### Password Authentication
+* **Git Password (Plain Text)**: Password to authenticate remotely (plain text - less secure)
+* **Git Password Storage Path**: Key storage path for Git password (more secure - recommended)
+
+If both password fields are configured, the Key Storage path takes precedence for security.
+
+#### SSH Key Authentication
 * **SSH: Strict Host Key Checking**: Use strict host key checking.
 If `yes`, require remote host SSH key is defined in the `~/.ssh/known_hosts` file, otherwise do not verify.
 * **SSH Key Path**: SSH Key Path to authenticate
 
+**Recommended:** Use Key Storage for passwords instead of plain text. To use Key Storage:
+1. Store your password in Rundeck Key Storage (under `keys/` path)
+2. Select the password path using the Key Storage browser in the plugin configuration
+
 ### Limitations
 
 * The plugin needs to clone the full repo on the local directory path (Base Directory option) to get the file that will be added to the resource model.

src/main/groovy/com/rundeck/plugin/GitResourceModel.groovy

@@ -58,18 +58,23 @@ class GitResourceModel implements ResourceModelSource , WriteableModelSource{
             gitManager = new GitManager(configuration)
         }
 
+        // Plain text password (less secure, checked first)
+        // Support old property name for backwards compatibility
+        if(configuration.getProperty(GitResourceModelFactory.GIT_PASSWORD_STORAGE)) {
+            gitManager.setGitPassword(configuration.getProperty(GitResourceModelFactory.GIT_PASSWORD_STORAGE))
+        }
+
+        // Key Storage password (more secure, takes precedence if both are set)
         if(services && configuration.getProperty(GitResourceModelFactory.GIT_PASSWORD_STORAGE_PATH)){
             ExecutionContext context = new ExecutionContextImpl.Builder()
                     .framework(framework)
                     .storageTree(services.getService(KeyStorageTree.class))
                     .build();
 
             def password = GitPluginUtil.getFromKeyStorage(configuration.getProperty(GitResourceModelFactory.GIT_PASSWORD_STORAGE_PATH), context)
-            gitManager.setGitPassword(password)
-        }
-
-        if(configuration.getProperty(GitResourceModelFactory.GIT_PASSWORD_STORAGE)) {
-            gitManager.setGitPassword(configuration.getProperty(GitResourceModelFactory.GIT_PASSWORD_STORAGE))
+            if (password != null) {
+                gitManager.setGitPassword(password)
+            }
         }
 
         if(configuration.getProperty(GitResourceModelFactory.GIT_KEY_STORAGE)) {

src/main/groovy/com/rundeck/plugin/GitResourceModelFactory.groovy

@@ -80,9 +80,9 @@ Some examples:
             .property(PropertyUtil.bool(WRITABLE, "Writable",
             "Allow to write the remote file.",
             false,"false",null,renderingOptionsConfig))
-            .property(PropertyUtil.string(GIT_PASSWORD_STORAGE, "Git Password", 'Password to authenticate remotely', false,
+            .property(PropertyUtil.string(GIT_PASSWORD_STORAGE, "Git Password (Plain Text)", 'Password to authenticate remotely (plain text)', false,
             null,null,null, renderingOptionsAuthenticationPassword))
-            .property(PropertyUtil.string(GIT_PASSWORD_STORAGE_PATH, "Git Password", 'Password Storage to authenticate remotely', false,
+            .property(PropertyUtil.string(GIT_PASSWORD_STORAGE_PATH, "Git Password Storage Path", 'Key storage path for Git password to authenticate remotely', false,
                     null,null,null, renderingOptionsAuthenticationPasswordStorage))
             .property(PropertyUtil.select(GIT_HOSTKEY_CHECKING, "SSH: Strict Host Key Checking", '''Use strict host key checking.
 If `yes`, require remote host SSH key is defined in the `~/.ssh/known_hosts` file, otherwise do not verify.''', false,

src/main/groovy/com/rundeck/plugin/util/GitPluginUtil.groovy

@@ -7,6 +7,7 @@ import com.dtolabs.rundeck.core.execution.ExecutionContextImpl
 import com.dtolabs.rundeck.core.storage.keys.KeyStorageTree;
 import com.dtolabs.rundeck.core.execution.ExecutionListener
 import groovy.transform.CompileStatic
+import java.nio.charset.StandardCharsets
 
 /**
  * Created by luistoledo on 12/18/17.
@@ -40,27 +41,45 @@ class GitPluginUtil {
         ResourceMeta contents = context.getExecutionContext().getStorageTree().getResource(path).getContents();
         ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
         contents.writeContent(byteArrayOutputStream);
-        String password = new String(byteArrayOutputStream.toByteArray());
+        String password = new String(byteArrayOutputStream.toByteArray(), StandardCharsets.UTF_8);
 
         return password;
 
     }
 
-     static String getFromKeyStorage(String path, ExecutionContextImpl context){
+    /**
+     * Retrieves the contents of a resource from the key storage using the provided path and execution context.
+     * <p>
+     * If the storage tree is available, this method attempts to read the resource at the given path and
+     * returns its contents as a String. If the storage tree is null or an error occurs, it logs a message and returns null.
+     *
+     * @param path    the path to the resource in the key storage
+     * @param context the Rundeck execution context
+     * @return the contents of the resource as a String, or null if the storage tree is null or an error occurs
+     */
+    static String getFromKeyStorage(String path, ExecutionContextImpl context){
         KeyStorageTree storageTree = (KeyStorageTree)context.getStorageTree()
 
-        if (storageTree!=null){
-            ResourceMeta contents = context.getStorageTree().getResource(path).getContents();
-            ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
-            contents.writeContent(byteArrayOutputStream);
-            String password = new String(byteArrayOutputStream.toByteArray());
-
-            return password;
-        } else {
+        if (storageTree == null){
             ExecutionListener logger = context.getExecutionListener()
-            logger.log(1, "storageTree is null. Cannot retrieve password");
+            logger.log(1, "storageTree is null. Cannot retrieve password from Key Storage.");
             return null
         }
 
+        try {
+            ResourceMeta contents = storageTree.getResource(path).getContents();
+            ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
+            try {
+                contents.writeContent(byteArrayOutputStream);
+                String password = new String(byteArrayOutputStream.toByteArray(), StandardCharsets.UTF_8);
+                return password;
+            } finally {
+                byteArrayOutputStream.close();
+            }
+        } catch (Exception e) {
+            ExecutionListener logger = context.getExecutionListener()
+            logger.log(1, "Failed to retrieve password from Key Storage at path '${path}': ${e.message}");
+            return null
+        }
     }
 }

src/test/groovy/com/rundeck/plugin/GitResourceModelSpec.groovy

@@ -170,8 +170,6 @@ class GitResourceModelSpec  extends Specification{
         def nodeSet = Mock(INodeSet)
         def framework = getFramework(nodeSet)
 
-
-
         String path = "resources"
         String fileName = "resources.xml"
         String format = "xml"
@@ -185,7 +183,7 @@ class GitResourceModelSpec  extends Specification{
                 gitBaseDirectory:path,
                 gitFormatFile:format,
                 gitFile:fileName,
-                gitPasswordPathStorage:"gitPassword",
+                gitPasswordPathStorage:"keys/git/password",
         ]
 
         def gitManager = Mock(GitManager)
@@ -216,9 +214,6 @@ class GitResourceModelSpec  extends Specification{
         then:
         1 * gitManager.getFile(path) >> inputStream
         result == nodeSet
-
-
-
     }