Merge pull request #2313 from zowe/fix/convert-v1-profiles

Fix Config API bugs that affect extenders converting V1 profiles

packages/imperative/CHANGELOG.md

@@ -2,6 +2,15 @@
 
 All notable changes to the Imperative package will be documented in this file.
 
+## Recent Changes
+
+- BugFix: Fixed an issue where the `ProfileInfo.profileManagerWillLoad` method failed if profiles were not yet read from disk. [#2284](https://github.com/zowe/zowe-cli/issues/2284)
+- BugFix: Fixed an issue where the `ProfileInfo.onlyV1ProfilesExist` method could wrongly return true when V2 profiles exist. [#2311](https://github.com/zowe/zowe-cli/issues/2311)
+  - Deprecated the static method `ProfileInfo.onlyV1ProfilesExist` and replaced it with an `onlyV1ProfilesExist` instance method on the `ProfileInfo` class.
+- BugFix: Fixed an issue where the `ConvertV1Profiles.convert` method may create team configuration files in the wrong directory if the environment variable `ZOWE_CLI_HOME` is set. [#2312](https://github.com/zowe/zowe-cli/issues/2312)
+- BugFix: Fixed an issue where the Imperative Event Emitter would fire event callbacks for the same app that triggered the event. [#2279](https://github.com/zowe/zowe-cli/issues/2279)
+- BugFix: Fixed an issue where the `ProfileInfo.updateKnownProperty` method could rewrite team config file to disk without any changes when storing secure value. [#2324](https://github.com/zowe/zowe-cli/issues/2324)
+
 ## `8.8.0`
 
 - BugFix: Enabled commands with either the `--help` or `--version` flags to still display their information despite any configuration file issues. [#2301](https://github.com/zowe/zowe-cli/pull/2301)

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

@@ -13,6 +13,7 @@ import * as fs from "fs";
 import * as path from "path";
 import * as os from "os";
 import * as jsonfile from "jsonfile";
+import * as glob from "fast-glob";
 import { ConfigUtils } from "../../config/src/ConfigUtils";
 import { CredentialManagerFactory } from "../../security";
 import { ImperativeConfig } from "../../utilities";
@@ -132,10 +133,29 @@ describe("Config Utils", () => {
                 })
             } as any);
 
-            const fsExistsSyncSpy = jest.spyOn(fs, "existsSync").mockReturnValueOnce(false);
+            const globSyncSpy = jest.spyOn(glob, "sync").mockReturnValueOnce([]);
 
             expect(ConfigUtils.onlyV1ProfilesExist).toBe(false);
-            expect(fsExistsSyncSpy).toHaveBeenCalledTimes(1);
+            expect(globSyncSpy).toHaveBeenCalledTimes(1);
+        });
+
+        it("should return false when only V1 profile meta files exist", () => {
+            jest.spyOn(ImperativeConfig, "instance", "get").mockReturnValue({
+                config: {
+                    exists: false
+                },
+                cliHome: "/fake/cli/home/dir",
+                loadedConfig: jest.fn(() => {
+                    return {
+                        envVariablePrefix: "Fake_cli_prefix"
+                    };
+                })
+            } as any);
+
+            const globSyncSpy = jest.spyOn(glob, "sync").mockReturnValueOnce(["profiles/zosmf/zosmf_meta.yaml"]);
+
+            expect(ConfigUtils.onlyV1ProfilesExist).toBe(false);
+            expect(globSyncSpy).toHaveBeenCalledTimes(1);
         });
 
         it("should return true when only V1 profiles exist", () => {
@@ -151,10 +171,10 @@ describe("Config Utils", () => {
                 })
             } as any);
 
-            const fsExistsSyncSpy = jest.spyOn(fs, "existsSync").mockReturnValueOnce(true);
+            const globSyncSpy = jest.spyOn(glob, "sync").mockReturnValueOnce(["profiles/zosmf/lpar1.yaml"]);
 
             expect(ConfigUtils.onlyV1ProfilesExist).toBe(true);
-            expect(fsExistsSyncSpy).toHaveBeenCalledTimes(1);
+            expect(globSyncSpy).toHaveBeenCalledTimes(1);
         });
     });
 

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

@@ -14,7 +14,7 @@ jest.mock("jsonfile");
 import * as fs from "fs";
 import * as jsonfile from "jsonfile";
 import { CredentialManagerFactory } from "../..";
-import { ConvertV1Profiles } from "../";
+import { Config, ConvertV1Profiles } from "../";
 import { ConvertMsgFmt } from "../src/doc/IConvertV1Profiles";
 import { ImperativeConfig } from "../../utilities/src/ImperativeConfig";
 import { ImperativeError } from "../../error/src/ImperativeError";
@@ -425,6 +425,18 @@ describe("ConvertV1Profiles tests", () => {
                 expect(getOldProfileCountSpy).toHaveBeenCalled();
                 expect(convNeeded).toEqual(true);
             });
+
+            it("should create a client config instance if it does not exist yet", async () => {
+                delete (ImperativeConfig.instance as any).config;
+                const configLoadSpy = jest.spyOn(Config, "load").mockResolvedValueOnce({ exists: true } as any);
+
+                // call the function that we want to test
+                // using class["name"] notation because it is a private static function
+                const convNeeded = await ConvertV1Profiles["isConversionNeeded"]();
+
+                expect(configLoadSpy).toHaveBeenCalledWith("zowe", { homeDir: ImperativeConfig.instance.cliHome });
+                expect(convNeeded).toEqual(false);
+            });
         }); // end isConversionNeeded
 
         describe("moveV1ProfilesToConfigFile", () => {

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

@@ -137,16 +137,25 @@ describe("TeamConfig ProfileInfo tests", () => {
                 expect(profLoaded.profile.profLoc.jsonLoc).toBe(profAttrs.profLoc.jsonLoc);
                 expect(profLoaded.profile.isDefaultProfile).toBe(profAttrs.isDefaultProfile);
             });
+        });
+
+        describe("onlyV1ProfilesExist", () => {
+            it("should detect that V2 profiles exist", async () => {
+                const profInfo = createNewProfInfo(teamProjDir);
+                const v2ExistsSpy = jest.spyOn(profInfo, "getTeamConfig").mockReturnValue({ exists: true } as any);
+                const v1ExistsSpy = jest.spyOn(ConfigUtils, "onlyV1ProfilesExist", "get");
+                expect(profInfo.onlyV1ProfilesExist).toBe(false);
+                expect(v2ExistsSpy).toHaveBeenCalledTimes(1);
+                expect(v1ExistsSpy).not.toHaveBeenCalled();
+            });
 
             it("should detect that only V1 profiles exist", async () => {
-                // onlyV1ProfilesExist is a getter of a property, so mock the property
-                Object.defineProperty(ConfigUtils, "onlyV1ProfilesExist", {
-                    configurable: true,
-                    get: jest.fn(() => {
-                        return true;
-                    })
-                });
-                expect(ProfileInfo.onlyV1ProfilesExist).toBe(true);
+                const profInfo = createNewProfInfo(teamProjDir);
+                const v2ExistsSpy = jest.spyOn(profInfo, "getTeamConfig").mockReturnValue({ exists: false } as any);
+                const v1ExistsSpy = jest.spyOn(ConfigUtils, "onlyV1ProfilesExist", "get").mockReturnValueOnce(true);
+                expect(profInfo.onlyV1ProfilesExist).toBe(true);
+                expect(v2ExistsSpy).toHaveBeenCalledTimes(1);
+                expect(v1ExistsSpy).toHaveBeenCalledTimes(1);
             });
         });
 
@@ -311,8 +320,8 @@ describe("TeamConfig ProfileInfo tests", () => {
     describe("profileManagerWillLoad", () => {
         it("should return false if secure credentials fail to load", async () => {
             const profInfo = createNewProfInfo(teamProjDir);
-            jest.spyOn((profInfo as any).mCredentials, "isSecured", "get").mockReturnValueOnce(true);
-            jest.spyOn((profInfo as any).mCredentials, "loadManager").mockImplementationOnce(async () => {
+            jest.spyOn((profInfo as any).mCredentials, "isCredentialManagerInAppSettings").mockReturnValueOnce(true);
+            jest.spyOn((profInfo as any).mCredentials, "activateCredMgrOverride").mockImplementationOnce(async () => {
                 throw new Error("bad credential manager");
             });
 
@@ -327,10 +336,10 @@ describe("TeamConfig ProfileInfo tests", () => {
             expect(response).toEqual(true);
         });
 
-        it("should return true if credentials are not secure", async () => {
+        it("should return true if there is no credential manager", async () => {
             // ensure that we are not in the team project directory
             const profInfo = createNewProfInfo(origDir);
-            (profInfo as any).mCredentials = { isSecured: false };
+            jest.spyOn((profInfo as any).mCredentials, "isCredentialManagerInAppSettings").mockReturnValueOnce(false);
             const response = await profInfo.profileManagerWillLoad();
             expect(response).toEqual(true);
         });
@@ -1297,7 +1306,7 @@ describe("TeamConfig ProfileInfo tests", () => {
     });
 
     describe("updateKnownProperty", () => {
-        it("should throw and error if the property location type is invalid", async () => {
+        it("should throw an error if the property location type is invalid", async () => {
             const profInfo = createNewProfInfo(teamProjDir);
             await profInfo.readProfilesFromDisk();
             let caughtError;
@@ -1336,6 +1345,7 @@ describe("TeamConfig ProfileInfo tests", () => {
             const profInfo = createNewProfInfo(teamProjDir);
             await profInfo.readProfilesFromDisk();
             const jsonPathMatchesSpy = jest.spyOn(ConfigUtils, "jsonPathMatches");
+            const configSaveSpy = jest.spyOn(profInfo.getTeamConfig(), "save");
 
             const prof = profInfo.mergeArgsForProfile(profInfo.getAllProfiles("dummy")[0]);
             const ret = await profInfo.updateKnownProperty({ mergedArgs: prof, property: "host", value: "example.com" });
@@ -1344,6 +1354,26 @@ describe("TeamConfig ProfileInfo tests", () => {
             expect(newHost).toEqual("example.com");
             expect(ret).toBe(true);
             expect(jsonPathMatchesSpy).toHaveBeenCalled(); // Verify that profile names are matched correctly
+            expect(configSaveSpy).toHaveBeenCalled();
+        });
+
+        it("should update the given property in the vault and return true", async () => {
+            const profInfo = createNewProfInfo(teamProjDir);
+            await profInfo.readProfilesFromDisk();
+            const jsonPathMatchesSpy = jest.spyOn(ConfigUtils, "jsonPathMatches");
+            jest.spyOn(profInfo.getTeamConfig().api.secure, "secureFields").mockReturnValue(["profiles.LPAR4.properties.host"]);
+            const configSaveSpy = jest.spyOn(profInfo.getTeamConfig(), "save");
+            const configSecureSaveSpy = jest.spyOn(profInfo.getTeamConfig().api.secure, "save");
+
+            const prof = profInfo.mergeArgsForProfile(profInfo.getAllProfiles("dummy")[0]);
+            const ret = await profInfo.updateKnownProperty({ mergedArgs: prof, property: "host", value: "example.com", setSecure: true });
+            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
+            expect(configSaveSpy).not.toHaveBeenCalled();
+            expect(configSecureSaveSpy).toHaveBeenCalled();
         });
 
         it("should remove the given property if the value specified if undefined", async () => {

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

@@ -9,9 +9,10 @@
 *
 */
 
-import { homedir as osHomedir } from "os";
-import { normalize as pathNormalize, join as pathJoin } from "path";
-import { existsSync as fsExistsSync } from "fs";
+import * as fs from "fs";
+import * as os from "os";
+import * as path from "path";
+import * as glob from "fast-glob";
 import * as jsonfile from "jsonfile";
 
 import { CredentialManagerFactory } from "../../security/src/CredentialManagerFactory";
@@ -32,7 +33,7 @@ export class ConfigUtils {
      * @returns {string} - Returns the Zowe home directory
      */
     public static getZoweDir(): string {
-        const defaultHome = pathJoin(osHomedir(), ".zowe");
+        const defaultHome = path.join(os.homedir(), ".zowe");
         if (ImperativeConfig.instance.loadedConfig?.defaultHome !== defaultHome) {
             ImperativeConfig.instance.loadedConfig = {
                 name: "zowe",
@@ -52,8 +53,8 @@ export class ConfigUtils {
      */
     public static readExtendersJson(): IExtendersJsonOpts {
         const cliHome = ImperativeConfig.instance.loadedConfig != null ? ImperativeConfig.instance.cliHome : ConfigUtils.getZoweDir();
-        const extenderJsonPath = pathJoin(cliHome, "extenders.json");
-        if (!fsExistsSync(extenderJsonPath)) {
+        const extenderJsonPath = path.join(cliHome, "extenders.json");
+        if (!fs.existsSync(extenderJsonPath)) {
             jsonfile.writeFileSync(extenderJsonPath, {
                 profileTypes: {}
             }, { spaces: 4 });
@@ -70,7 +71,7 @@ export class ConfigUtils {
      */
     public static writeExtendersJson(obj: IExtendersJsonOpts): boolean {
         try {
-            const extenderJsonPath = pathJoin(ConfigUtils.getZoweDir(), "extenders.json");
+            const extenderJsonPath = path.join(ConfigUtils.getZoweDir(), "extenders.json");
             jsonfile.writeFileSync(extenderJsonPath, obj, { spaces: 4 });
         } catch (err) {
             return false;
@@ -144,20 +145,14 @@ export class ConfigUtils {
             return false;
         }
 
-        let v1ZosmfProfileFileNm: string;
-        try {
-            v1ZosmfProfileFileNm = pathNormalize(ImperativeConfig.instance.cliHome + "/profiles/zosmf/zosmf_meta.yaml");
-        } catch (_thrownErr) {
-            // We failed to get the CLI home directory. So, we definitely have no V1 profiles.
-            return false;
-        }
-
-        if (fsExistsSync(v1ZosmfProfileFileNm)) {
-            // we found V1 profiles
-            return true;
-        }
-
-        return false;
+        // look for V1 profiles in the CLI home directory
+        const v1ProfilePaths = glob.sync("profiles/**/*.yaml", { cwd: ImperativeConfig.instance.cliHome })
+            .filter(filename => {
+                // ignore meta yaml files
+                const { dir, name } = path.parse(filename);
+                return name !== path.basename(dir) + "_meta";
+            });
+        return v1ProfilePaths.length > 0;
     }
 
     /**
@@ -190,10 +185,10 @@ export class ConfigUtils {
             const envVarNm = envVarPrefix + EnvironmentalVariableSettings.CLI_HOME_SUFFIX;
             if (process.env[envVarNm] === undefined) {
                 // use OS home directory
-                homeDir = pathJoin(osHomedir(), "." + appName.toLowerCase());
+                homeDir = path.join(os.homedir(), "." + appName.toLowerCase());
             } else {
                 // use the available environment variable
-                homeDir = pathNormalize(process.env[envVarNm]);
+                homeDir = path.normalize(process.env[envVarNm]);
             }
             ImperativeConfig.instance.loadedConfig = {
                 name: appName,
@@ -300,8 +295,9 @@ export class ConfigUtils {
      * Checks if the given token has expired. Supports JSON web tokens only.
      *
      * @param {string} token - The JSON web token to check
-     * @returns {boolean} Whether the token has expired.
-     * Returns `false` if the token cannot be decoded or an expire time is not specified in the payload.
+     * @returns {boolean}
+     *     Whether the token has expired. Returns `false` if the token cannot
+     *     be decoded or an expire time is not specified in the payload.
      */
     public static hasTokenExpired(token: string): boolean {
         // JWT format: [header].[payload].[signature]

packages/imperative/src/config/src/ConvertV1Profiles.ts

@@ -139,9 +139,7 @@ export class ConvertV1Profiles {
             // Initialization for VSCode extensions does not create the config property, so create it now.
             ImperativeConfig.instance.config = await Config.load(
                 ImperativeConfig.instance.loadedConfig.name,
-                {
-                    homeDir: ImperativeConfig.instance.loadedConfig.defaultHome
-                }
+                { homeDir: ImperativeConfig.instance.cliHome }
             );
         }
 

packages/imperative/src/config/src/ProfileCredentials.ts

@@ -54,9 +54,9 @@ export class ProfileCredentials {
 
     /**
      * Check if secure credentials will be encrypted or stored in plain text.
-     * If using team config, this will always return true. If using classic
-     * profiles, this will check whether a custom CredentialManager is defined
-     * in the Imperative settings.json file.
+     * This will return true if the team configuration files contain secure
+     * fields, or if a custom CredentialManager is defined in the Imperative
+     * settings.json file.
      */
     public get isSecured(): boolean {
         this.mSecured = this.isTeamConfigSecure() || this.isCredentialManagerInAppSettings();
@@ -74,6 +74,22 @@ export class ProfileCredentials {
             throw new ImperativeError({ msg: "Secure credential storage is not enabled" });
         }
 
+        await this.activateCredMgrOverride();
+        await this.mProfileInfo.getTeamConfig().api.secure.load({
+            load: (key: string): Promise<string> => {
+                return CredentialManagerFactory.manager.load(key, true);
+            },
+            save: (key: string, value: any): Promise<void> => {
+                return CredentialManagerFactory.manager.save(key, value);
+            }
+        });
+    }
+
+    /**
+     * Attempt to initialize `CredentialManagerFactory` with the specified override.
+     * @internal
+     */
+    public async activateCredMgrOverride(): Promise<void> {
         if (!CredentialManagerFactory.initialized) {
             try {
                 // TODO? Make CredentialManagerFactory.initialize params optional
@@ -86,15 +102,6 @@ export class ProfileCredentials {
                 });
             }
         }
-
-        await this.mProfileInfo.getTeamConfig().api.secure.load({
-            load: (key: string): Promise<string> => {
-                return CredentialManagerFactory.manager.load(key, true);
-            },
-            save: (key: string, value: any): Promise<void> => {
-                return CredentialManagerFactory.manager.save(key, value);
-            }
-        });
     }
 
     /**
@@ -109,8 +116,9 @@ export class ProfileCredentials {
     /**
      * Check whether a custom CredentialManager is defined in the Imperative
      * settings.json file.
+     * @internal
      */
-    private isCredentialManagerInAppSettings(): boolean {
+    public isCredentialManagerInAppSettings(): boolean {
         try {
             const fileName = path.join(ImperativeConfig.instance.cliHome, "settings", "imperative.json");
             let settings: any;