Merge pull request #2016 from zowe/fix/config-secure-props

fix(profInfo): Fix secure property names being returned for wrong profile

packages/imperative/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 All notable changes to the Imperative package will be documented in this file.
 
+## Recent Changes
+
+- BugFix: Fixed issue where secure property names could be returned for the wrong profile. [zowe-explorer#2633](https://github.com/zowe/vscode-extension-for-zowe/issues/2633)
+
 ## `5.20.2`
 
 - BugFix: Fixed issue when a property is not found in `ProfileInfo.updateProperty({forceUpdate: true})`. [zowe-explorer#2493](https://github.com/zowe/vscode-extension-for-zowe/issues/2493)

packages/imperative/src/cmd/src/CommandProcessor.ts

@@ -43,7 +43,7 @@ import { CliUtils } from "../../utilities/src/CliUtils";
 import { WebHelpManager } from "./help/WebHelpManager";
 import { ICommandProfile } from "./doc/profiles/definition/ICommandProfile";
 import { Config } from "../../config/src/Config";
-import { getActiveProfileName } from "../../config/src/ConfigUtils";
+import { ConfigUtils } from "../../config/src/ConfigUtils";
 import { ConfigConstants } from "../../config/src/ConfigConstants";
 import { IDaemonContext } from "../../imperative/src/doc/IDaemonContext";
 import { IHandlerResponseApi } from "../..";
@@ -820,7 +820,7 @@ export class CommandProcessor {
 
             const combinedProfiles = [ ...showInputsOnly.requiredProfiles ?? [], ...showInputsOnly.optionalProfiles ?? [] ];
             combinedProfiles.forEach((profile) => {
-                const name = getActiveProfileName(profile, commandParameters.arguments); // get profile name
+                const name = ConfigUtils.getActiveProfileName(profile, commandParameters.arguments); // get profile name
                 const props = this.mConfig.api.secure.securePropsForProfile(name); // get secure props
                 configSecureProps.push(...props); // add to list
             });

packages/imperative/src/config/__tests__/Config.secure.unit.test.ts

@@ -161,7 +161,10 @@ describe("Config secure tests", () => {
             .mockReturnValueOnce(false);    // Global layer
         jest.spyOn(fs, "readFileSync");
         const config = await Config.load(MY_APP);
-        expect(config.api.secure.secureFields()).toEqual(["profiles.fruit.properties.secret"]);
+        expect(config.api.secure.secureFields()).toEqual([
+            "profiles.fruit.properties.secret",
+            "profiles.fruit.profiles.grape.properties.secret2"
+        ]);
     });
 
     it("should list all secure fields for a profile", async () => {
@@ -176,6 +179,19 @@ describe("Config secure tests", () => {
         expect(config.api.secure.securePropsForProfile("fruit.apple")).toEqual(["secret"]);
     });
 
+    it("should not list secure fields for a profile with partial name match", async () => {
+        jest.spyOn(Config, "search").mockReturnValue(projectConfigPath);
+        jest.spyOn(fs, "existsSync")
+            .mockReturnValueOnce(false)     // Project user layer
+            .mockReturnValueOnce(true)      // Project layer
+            .mockReturnValueOnce(false)     // User layer
+            .mockReturnValueOnce(false);    // Global layer
+        jest.spyOn(fs, "readFileSync");
+        const config = await Config.load(MY_APP);
+        expect(config.api.secure.securePropsForProfile("fruit.grape")).toEqual(["secret", "secret2"]);
+        expect(config.api.secure.securePropsForProfile("fruit.grapefruit")).toEqual(["secret"]);
+    });
+
     describe("secureInfoForProp", () => {
         const configProperties: IConfig = {
             profiles: {

packages/imperative/src/config/__tests__/ConfigUtils.unit.test.ts

@@ -9,7 +9,7 @@
 *
 */
 
-import * as ConfigUtils from "../../config/src/ConfigUtils";
+import { ConfigUtils } from "../../config/src/ConfigUtils";
 import { CredentialManagerFactory } from "../../security";
 import { ImperativeConfig } from "../../utilities";
 

packages/imperative/src/config/__tests__/ProfileInfo.TeamConfig.unit.test.ts

@@ -26,6 +26,7 @@ import { ConfigAutoStore } from "../src/ConfigAutoStore";
 import { ImperativeConfig } from "../../utilities/src/ImperativeConfig";
 import { ImperativeError } from "../../error";
 import { IProfInfoUpdatePropOpts } from "../src/doc/IProfInfoUpdatePropOpts";
+import { ConfigUtils } from "../src/ConfigUtils";
 
 const testAppNm = "ProfInfoApp";
 const testEnvPrefix = testAppNm.toUpperCase();
@@ -1021,6 +1022,7 @@ describe("TeamConfig ProfileInfo tests", () => {
             };
             jest.spyOn(profInfo as any, "mergeArgsForProfile").mockReturnValue(mergedArgs);
             const updateKnownPropertySpy = jest.spyOn(profInfo as any, "updateKnownProperty").mockResolvedValue(true);
+            const jsonPathMatchesSpy = jest.spyOn(ConfigUtils, "jsonPathMatches");
             const profileOptions: IProfInfoUpdatePropOpts = {
                 profileName: "LPAR4",
                 profileType: "dummy",
@@ -1038,6 +1040,7 @@ describe("TeamConfig ProfileInfo tests", () => {
             expect(mergedArgs.knownArgs[0].argLoc.jsonLoc).toEqual("profiles.LPAR4.properties.rejectUnauthorized");
             const osLocInfo = { global: false, user: false, name: "LPAR4", path: path.join(teamProjDir, `${testAppNm}.config.json`) };
             expect(updateKnownPropertySpy).toHaveBeenCalledWith({ ...profileOptions, mergedArgs, osLocInfo });
+            expect(jsonPathMatchesSpy).toHaveBeenCalledTimes(1); // Verify that profile names are matched correctly
         });
 
         it("should succeed forceUpdating a property even if the property doesn't exist", async () => {
@@ -1161,13 +1164,15 @@ describe("TeamConfig ProfileInfo tests", () => {
         it("should update the given property and return true", async () => {
             const profInfo = createNewProfInfo(teamProjDir);
             await profInfo.readProfilesFromDisk();
+            const jsonPathMatchesSpy = jest.spyOn(ConfigUtils, "jsonPathMatches");
 
             const prof = profInfo.mergeArgsForProfile(profInfo.getAllProfiles("dummy")[0]);
             const ret = await profInfo.updateKnownProperty({ mergedArgs: prof, property: "host", value: "example.com" });
             const newHost = profInfo.getTeamConfig().api.layers.get().properties.profiles.LPAR4.properties.host;
 
             expect(newHost).toEqual("example.com");
             expect(ret).toBe(true);
+            expect(jsonPathMatchesSpy).toHaveBeenCalled(); // Verify that profile names are matched correctly
         });
 
         it("should remove the given property if the value specified if undefined", async () => {

packages/imperative/src/config/__tests__/__resources__/project.config.json

@@ -16,6 +16,21 @@
                     "properties": {
                         "color": "orange"
                     }
+                },
+                "grape": {
+                    "type": "fruit",
+                    "properties": {
+                        "color": "red"
+                    },
+                    "secure": [
+                        "secret2"
+                    ]
+                },
+                "grapefruit": {
+                    "type": "fruit",
+                    "properties": {
+                        "color": "pink"
+                    }
                 }
             },
             "secure": [

packages/imperative/src/config/__tests__/__snapshots__/Config.unit.test.ts.snap

@@ -67,6 +67,21 @@ Object {
           },
           "type": "fruit",
         },
+        "grape": Object {
+          "properties": Object {
+            "color": "red",
+          },
+          "secure": Array [
+            "secret2",
+          ],
+          "type": "fruit",
+        },
+        "grapefruit": Object {
+          "properties": Object {
+            "color": "pink",
+          },
+          "type": "fruit",
+        },
         "orange": Object {
           "properties": Object {
             "color": "orange",
@@ -183,6 +198,21 @@ Object {
           },
           "type": "fruit",
         },
+        "grape": Object {
+          "properties": Object {
+            "color": "red",
+          },
+          "secure": Array [
+            "secret2",
+          ],
+          "type": "fruit",
+        },
+        "grapefruit": Object {
+          "properties": Object {
+            "color": "pink",
+          },
+          "type": "fruit",
+        },
         "orange": Object {
           "properties": Object {
             "color": "orange",

packages/imperative/src/config/index.ts

@@ -14,7 +14,7 @@ export * from "./src/ConfigAutoStore";
 export * from "./src/ConfigConstants";
 export * from "./src/ConfigSchema";
 export * from "./src/ConfigBuilder";
-export * as ConfigUtils from "./src/ConfigUtils";
+export * from "./src/ConfigUtils";
 export * from "./src/ProfileCredentials";
 export * from "./src/ProfileInfo";
 export * from "./src/ProfInfoErr";

packages/imperative/src/config/src/Config.ts

@@ -27,7 +27,7 @@ import { IConfigOpts } from "./doc/IConfigOpts";
 import { IConfigSecure } from "./doc/IConfigSecure";
 import { IConfigVault } from "./doc/IConfigVault";
 import { ConfigLayers, ConfigPlugins, ConfigProfiles, ConfigSecure } from "./api";
-import { coercePropValue } from "./ConfigUtils";
+import { ConfigUtils } from "./ConfigUtils";
 import { IConfigSchemaInfo } from "./doc/IConfigSchema";
 import { JsUtils } from "../../utilities/src/JsUtils";
 import { IConfigMergeOpts } from "./doc/IConfigMergeOpts";
@@ -443,7 +443,7 @@ export class Config {
                 obj = obj[segment];
             } else if (segments.indexOf(segment) === segments.length - 1) {
                 if (opts?.parseString) {
-                    value = coercePropValue(value);
+                    value = ConfigUtils.coercePropValue(value);
                 }
 
                 if (opts?.parseString && Array.isArray(obj[segment])) {

packages/imperative/src/config/src/ConfigAutoStore.ts

@@ -13,7 +13,7 @@ import * as lodash from "lodash";
 import { ICommandArguments, IHandlerParameters } from "../../cmd";
 import { ICommandHandlerRequire } from "../../cmd/src/doc/handler/ICommandHandlerRequire";
 import { ICommandProfileAuthConfig } from "../../cmd/src/doc/profiles/definition/ICommandProfileAuthConfig";
-import * as ConfigUtils from "./ConfigUtils";
+import { ConfigUtils } from "./ConfigUtils";
 import { AbstractAuthHandler } from "../../imperative/src/auth/handlers/AbstractAuthHandler";
 import { ImperativeConfig } from "../../utilities";
 import { ISession } from "../../rest/src/session/doc/ISession";

packages/imperative/src/config/src/ConfigUtils.ts

@@ -14,59 +14,70 @@ import { ICommandArguments } from "../../cmd";
 import { ImperativeConfig } from "../../utilities";
 import { ImperativeError } from "../../error";
 
-/**
- * Coeerces string property value to a boolean or number type.
- * @param value String value
- * @param type Property type defined in the schema
- * @returns Boolean, number, or string
- */
-export function coercePropValue(value: any, type?: string) {
-    if (type === "boolean" || type === "number") {
-        // For boolean or number, parse the string and throw on failure
-        return JSON.parse(value);
-    } else if (type == null) {
-        // For unknown type, try to parse the string and ignore failure
-        try {
+export class ConfigUtils {
+    /**
+     * Coeerces string property value to a boolean or number type.
+     * @param value String value
+     * @param type Property type defined in the schema
+     * @returns Boolean, number, or string
+     */
+    public static coercePropValue(value: any, type?: string) {
+        if (type === "boolean" || type === "number") {
+            // For boolean or number, parse the string and throw on failure
             return JSON.parse(value);
-        } catch {
-            return value;
+        } else if (type == null) {
+            // For unknown type, try to parse the string and ignore failure
+            try {
+                return JSON.parse(value);
+            } catch {
+                return value;
+            }
+        } else {
+            // For string or other type, don't do any parsing
+            return value.toString();
         }
-    } else {
-        // For string or other type, don't do any parsing
-        return value.toString();
     }
-}
 
-/**
- * Retrieves the name of the active profile for the given type. If no such
- * profile exists, returns the default name which can be used to create a new profile.
- * @param profileType The type of CLI profile
- * @param cmdArguments CLI arguments which may specify a profile
- * @param defaultProfileName Name to fall back to if profile doesn't exist. If
- *                           not specified, the profile type will be used.
- * @returns The profile name
- */
-export function getActiveProfileName(profileType: string, cmdArguments?: ICommandArguments, defaultProfileName?: string): string {
-    // Look for profile name first in command line arguments, second in
-    // default profiles defined in config, and finally fall back to using
-    // the profile type as the profile name.
-    return cmdArguments?.[`${profileType}-profile`] ||
-        ImperativeConfig.instance.config?.properties.defaults[profileType] ||
-        defaultProfileName || profileType;
-}
+    /**
+     * Retrieves the name of the active profile for the given type. If no such
+     * profile exists, returns the default name which can be used to create a new profile.
+     * @param profileType The type of CLI profile
+     * @param cmdArguments CLI arguments which may specify a profile
+     * @param defaultProfileName Name to fall back to if profile doesn't exist. If
+     *                           not specified, the profile type will be used.
+     * @returns The profile name
+     */
+    public static getActiveProfileName(profileType: string, cmdArguments?: ICommandArguments, defaultProfileName?: string): string {
+        // Look for profile name first in command line arguments, second in
+        // default profiles defined in config, and finally fall back to using
+        // the profile type as the profile name.
+        return cmdArguments?.[`${profileType}-profile`] ||
+            ImperativeConfig.instance.config?.properties.defaults[profileType] ||
+            defaultProfileName || profileType;
+    }
 
-/**
- * Form an error message for failures to securely save a value.
- * @param solution Text that our caller can supply for a solution.
- * @returns ImperativeError to be thrown
- */
-export function secureSaveError(solution?: string): ImperativeError {
-    let details = CredentialManagerFactory.manager.secureErrorDetails();
-    if (solution != null) {
-        details = (details != null) ? (details + `\n - ${solution}`) : solution;
+    /**
+     * Checks if partial path is equal to or nested inside full path
+     * @param fullPath JSON path to profile 1
+     * @param partialPath JSON path to profile 2
+     */
+    public static jsonPathMatches(fullPath: string, partialPath: string): boolean {
+        return fullPath === partialPath || fullPath.startsWith(partialPath + ".profiles.");
+    }
+
+    /**
+     * Form an error message for failures to securely save a value.
+     * @param solution Text that our caller can supply for a solution.
+     * @returns ImperativeError to be thrown
+     */
+    public static secureSaveError(solution?: string): ImperativeError {
+        let details = CredentialManagerFactory.manager.secureErrorDetails();
+        if (solution != null) {
+            details = (details != null) ? (details + `\n - ${solution}`) : solution;
+        }
+        return new ImperativeError({
+            msg: "Unable to securely save credentials.",
+            additionalDetails: details
+        });
     }
-    return new ImperativeError({
-        msg: "Unable to securely save credentials.",
-        additionalDetails: details
-    });
 }

packages/imperative/src/config/src/ProfileInfo.ts

@@ -51,6 +51,7 @@ import { ConfigAutoStore } from "./ConfigAutoStore";
 import { IGetAllProfilesOptions } from "./doc/IProfInfoProps";
 import { IConfig } from "./doc/IConfig";
 import { IProfInfoRemoveKnownPropOpts } from "./doc/IProfInfoRemoveKnownPropOpts";
+import { ConfigUtils } from "./ConfigUtils";
 
 /**
  * This class provides functions to retrieve profile-related information.
@@ -205,7 +206,7 @@ export class ProfileInfo {
             const knownProperty = mergedArgs.knownArgs.find((v => v.argName === options.property));
             if (knownProperty != null) {
                 const profPath = this.getTeamConfig().api.profiles.getProfilePathFromName(options.profileName);
-                if (!knownProperty.argLoc.jsonLoc.startsWith(profPath)) {
+                if (!ConfigUtils.jsonPathMatches(knownProperty.argLoc.jsonLoc, profPath)) {
                     knownProperty.argLoc.jsonLoc = `${profPath}.properties.${options.property}`;
                 }
             }
@@ -293,7 +294,7 @@ export class ProfileInfo {
                 let oldLayer: IProfLocOsLocLayer;
                 const layer = this.getTeamConfig().layerActive();
                 const osLoc = options.osLocInfo ?? this.getOsLocInfo(
-                    this.getAllProfiles().find(p => toUpdate.argLoc.jsonLoc.startsWith(p.profLoc.jsonLoc)))?.[0];
+                    this.getAllProfiles().find(p => ConfigUtils.jsonPathMatches(toUpdate.argLoc.jsonLoc, p.profLoc.jsonLoc)))?.[0];
                 if (osLoc && (layer.user !== osLoc.user || layer.global !== osLoc.global)) {
                     oldLayer = { user: layer.user, global: layer.global };
                     this.getTeamConfig().api.layers.activate(osLoc.user, osLoc.global);

packages/imperative/src/config/src/api/ConfigSecure.ts

@@ -19,6 +19,7 @@ import { IConfigSecureProperties } from "../doc/IConfigSecure";
 import { ConfigConstants } from "../ConfigConstants";
 import { IConfigProfile } from "../doc/IConfigProfile";
 import { CredentialManagerFactory } from "../../../security";
+import { ConfigUtils } from "../ConfigUtils";
 
 /**
  * API Class for manipulating config layers.
@@ -200,17 +201,18 @@ export class ConfigSecure extends ConfigApi {
      * @param profileName Profile name to search for
      * @returns Array of secure property names
      */
-    public securePropsForProfile(profileName: string) {
+    public securePropsForProfile(profileName: string): string[] {
         const profilePath = this.mConfig.api.profiles.getProfilePathFromName(profileName);
-        const secureProps = [];
+        const secureProps = new Set<string>();
         for (const propPath of this.secureFields()) {
             const pathSegments = propPath.split(".");  // profiles.XXX.properties.YYY
             // eslint-disable-next-line @typescript-eslint/no-magic-numbers
-            if (profilePath.startsWith(pathSegments.slice(0, -2).join("."))) {
-                secureProps.push(pathSegments.pop());
+            const propProfilePath = pathSegments.slice(0, -2).join(".");
+            if (ConfigUtils.jsonPathMatches(profilePath, propProfilePath)) {
+                secureProps.add(pathSegments.pop());
             }
         }
-        return secureProps;
+        return [...secureProps];
     }
 
     /**