Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/imperative/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions packages/imperative/src/cmd/src/CommandProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 "../..";
Expand Down Expand Up @@ -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
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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",
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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 () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,21 @@
"properties": {
"color": "orange"
}
},
"grape": {
"type": "fruit",
"properties": {
"color": "red"
},
"secure": [
"secret2"
]
},
"grapefruit": {
"type": "fruit",
"properties": {
"color": "pink"
}
}
},
"secure": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion packages/imperative/src/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the way in which we will have to eventually deal with the Imperative package.
Little by little, and as non-breaking changes. 😋
Thanks for moving the functions into a class 😋

export * from "./src/ConfigUtils";
export * from "./src/ProfileCredentials";
export * from "./src/ProfileInfo";
export * from "./src/ProfInfoErr";
Expand Down
4 changes: 2 additions & 2 deletions packages/imperative/src/config/src/Config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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])) {
Expand Down
2 changes: 1 addition & 1 deletion packages/imperative/src/config/src/ConfigAutoStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
109 changes: 60 additions & 49 deletions packages/imperative/src/config/src/ConfigUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
});
}
5 changes: 3 additions & 2 deletions packages/imperative/src/config/src/ProfileInfo.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing these edge cases for the ProfileInfo APIs

Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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}`;
}
}
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 7 additions & 5 deletions packages/imperative/src/config/src/api/ConfigSecure.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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];
}

/**
Expand Down
Loading