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

feat: add param noSensitiveData to Plugin.load #665

Merged
merged 7 commits into from
Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
18 changes: 9 additions & 9 deletions src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -813,11 +813,11 @@ export class Config implements IConfig {
}

// when no manifest exists, the default is calculated. This may throw, so we need to catch it
const defaultFlagToCached = async (flag: CompletableOptionFlag<any>, suppressSensitiveData = false) => {
const defaultFlagToCached = async (flag: CompletableOptionFlag<any>, isWritingManifest = false) => {
// Prefer the helpDefaultValue function (returns a friendly string for complex types)
if (typeof flag.defaultHelp === 'function') {
try {
return await flag.defaultHelp({options: flag, flags: {}}, suppressSensitiveData)
return await flag.defaultHelp({options: flag, flags: {}}, isWritingManifest)
} catch {
return
}
Expand All @@ -826,18 +826,18 @@ const defaultFlagToCached = async (flag: CompletableOptionFlag<any>, suppressSen
// if not specified, try the default function
if (typeof flag.default === 'function') {
try {
return await flag.default({options: {}, flags: {}}, suppressSensitiveData)
return await flag.default({options: {}, flags: {}}, isWritingManifest)
} catch {}
} else {
return flag.default
}
}

const defaultArgToCached = async (arg: Arg<any>, suppressSensitiveData = false): Promise<any> => {
const defaultArgToCached = async (arg: Arg<any>, isWritingManifest = false): Promise<any> => {
// Prefer the helpDefaultValue function (returns a friendly string for complex types)
if (typeof arg.defaultHelp === 'function') {
try {
return await arg.defaultHelp({options: arg, flags: {}}, suppressSensitiveData)
return await arg.defaultHelp({options: arg, flags: {}}, isWritingManifest)
} catch {
return
}
Expand All @@ -846,14 +846,14 @@ const defaultArgToCached = async (arg: Arg<any>, suppressSensitiveData = false):
// if not specified, try the default function
if (typeof arg.default === 'function') {
try {
return await arg.default({options: arg, flags: {}}, suppressSensitiveData)
return await arg.default({options: arg, flags: {}}, isWritingManifest)
} catch {}
} else {
return arg.default
}
}

export async function toCached(c: Command.Class, plugin?: IPlugin | undefined, suppressSensitiveData?: boolean): Promise<Command.Cached> {
export async function toCached(c: Command.Class, plugin?: IPlugin | undefined, isWritingManifest?: boolean): Promise<Command.Cached> {
const flags = {} as {[k: string]: Command.Flag.Cached}

for (const [name, flag] of Object.entries(c.flags || {})) {
Expand Down Expand Up @@ -894,7 +894,7 @@ export async function toCached(c: Command.Class, plugin?: IPlugin | undefined, s
dependsOn: flag.dependsOn,
relationships: flag.relationships,
exclusive: flag.exclusive,
default: await defaultFlagToCached(flag, suppressSensitiveData),
default: await defaultFlagToCached(flag, isWritingManifest),
deprecated: flag.deprecated,
deprecateAliases: c.deprecateAliases,
aliases: flag.aliases,
Expand All @@ -914,7 +914,7 @@ export async function toCached(c: Command.Class, plugin?: IPlugin | undefined, s
description: arg.description,
required: arg.required,
options: arg.options,
default: await defaultArgToCached(arg, suppressSensitiveData),
default: await defaultArgToCached(arg, isWritingManifest),
hidden: arg.hidden,
}
}
Expand Down
10 changes: 5 additions & 5 deletions src/config/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,11 @@ export class Plugin implements IPlugin {

/**
* Loads a plugin
* @param suppressSensitiveData - if true, suppress sensitive data from being included in cached data
* @param isWritingManifest - if true, exclude selected data from manifest
* default is false to maintain backwards compatibility
* @returns Promise<void>
*/
public async load(suppressSensitiveData?: boolean): Promise<void> {
public async load(isWritingManifest?: boolean): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered adding isWritingManifest as a new field in the options object here?
https://github.com/oclif/core/blob/main/src/config/plugin.ts#L137

That way we could load plugins in oclif manifest like this:

const plugin = new Plugin({root: fullPath, isWritingManifest: true})
await plugin.load()

instead of adding a new arg to Plugin.load

this.type = this.options.type || 'core'
this.tag = this.options.tag
const root = await findRoot(this.options.name, this.options.root)
Expand All @@ -166,7 +166,7 @@ export class Plugin implements IPlugin {

this.hooks = mapValues(this.pjson.oclif.hooks || {}, i => Array.isArray(i) ? i : [i])

this.manifest = await this._manifest(Boolean(this.options.ignoreManifest), Boolean(this.options.errorOnManifestCreate), suppressSensitiveData)
this.manifest = await this._manifest(Boolean(this.options.ignoreManifest), Boolean(this.options.errorOnManifestCreate), isWritingManifest)
this.commands = Object
.entries(this.manifest.commands)
.map(([id, c]) => ({
Expand Down Expand Up @@ -250,7 +250,7 @@ export class Plugin implements IPlugin {
return cmd
}

protected async _manifest(ignoreManifest: boolean, errorOnManifestCreate = false, suppressSensitiveData = false): Promise<Manifest> {
protected async _manifest(ignoreManifest: boolean, errorOnManifestCreate = false, isWritingManifest = false): Promise<Manifest> {
const readManifest = async (dotfile = false): Promise<Manifest | undefined> => {
try {
const p = path.join(this.root, `${dotfile ? '.' : ''}oclif.manifest.json`)
Expand Down Expand Up @@ -285,7 +285,7 @@ export class Plugin implements IPlugin {
version: this.version,
commands: (await Promise.all(this.commandIDs.map(async id => {
try {
return [id, await toCached(await this.findCommand(id, {must: true}), this, suppressSensitiveData)]
return [id, await toCached(await this.findCommand(id, {must: true}), this, isWritingManifest)]
} catch (error: any) {
const scope = 'toCached'
if (Boolean(errorOnManifestCreate) === false) this.warn(error, scope)
Expand Down
96 changes: 90 additions & 6 deletions src/interfaces/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export type Metadata = {

type MetadataFlag = {
setFromDefault?: boolean;
defaultHelp?: unknown;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to retain value from defaultHelp to be used in later steps of parse/validation.

}

export type ListItem = [string, string | undefined]
Expand All @@ -55,11 +56,94 @@ export type DefaultContext<T> = {
flags: Record<string, string>;
}

export type FlagDefault<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>, suppressSensitiveData?: boolean) => Promise<T>)
export type FlagDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>, suppressSensitiveData?: boolean) => Promise<string | undefined>)

export type ArgDefault<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>, suppressSensitiveData?: boolean) => Promise<T>)
export type ArgDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>, suppressSensitiveData?: boolean) => Promise<string | undefined>)
/**
* Type to define a default value for a flag.
* @param context The context of the flag.
* @param isWritingManifest Informs the function that a manifest file is being written.
* The manifest file is used to store the flag definitions, with a default value if present, for a command and is published to npm.
* When a manifest file is being written, the default value may contain data that should not be included in the manifest.
* The plugin developer can use isWritingManifest to determine if the default value should be omitted from the manifest.
* in the function's implementation.
* @example
* static flags = {
* foo: flags.string({
* defaultHelp: async (context, isWritingManifest) => {
* if (isWritingManifest) {
* return undefined
* }
* return 'value that is used outside a manifest'
* },
* }),
* }
*/
export type FlagDefault<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>, isWritingManifest?: boolean) => Promise<T>)

/**
* Type to define a defaultHelp value for a flag.
* The defaultHelp value is used in the help output for the flag and when writing a manifest.
* It is also can be used to provide a value for the flag when issuing certain error messages.
*
* @param context The context of the flag.
* @param isWritingManifest Informs the function that a manifest file is being written.
* The manifest file is used to store the flag definitions, with a default value if present via defaultHelp, for a command and is published to npm.
* When a manifest file is being written, the default value may contain data that should not be included in the manifest.
* The plugin developer can use isWritingManifest to determine if the defaultHelp value should be omitted from the manifest.
* in the function's implementation.
* @example
* static flags = {
* foo: flags.string({
* defaultHelp: async (context, isWritingManifest) => {
* if (isWritingManifest) {
* return undefined
* }
* return 'value that is used outside a manifest'
* },
* }),
* }
*/
export type FlagDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<P & OptionFlag<T, P>>, isWritingManifest?: boolean) => Promise<string | undefined>)

/**
* Type to define a default value for an arg.
* @param context The context of the arg.
* @param isWritingManifest Informs the function that a manifest file is being written.
* The manifest file is used to store the arg definitions, with a default value if present, for a command and is published to npm.
* When a manifest file is being written, the default value may contain data that should not be included in the manifest.
* The plugin developer can use isWritingManifest to determine if the default value should be omitted from the manifest.
* in the function's implementation.
* @example
* public static readonly args = {
* one: Args.string({
* default: async (context, isWritingManifest) => {
* if (isWritingManifest) {
* return undefined
* }
* return 'value that is used outside a manifest'
* }),
* };
*/
export type ArgDefault<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>, isWritingManifest?: boolean) => Promise<T>)

/**
* Type to define a defaultHelp value for an arg.
* @param context The context of the arg.
* @param isWritingManifest Informs the function that a manifest file is being written.
* The manifest file is used to store the arg definitions, with a default value if present via defaultHelp, for a command and is published to npm.
* When a manifest file is being written, the default value may contain data that should not be included in the manifest.
* The plugin developer can use isWritingManifest to determine if the default value should be omitted from the manifest.
* in the function's implementation.
* @example
* public static readonly args = {
* one: Args.string({
* defaultHelp: async (context, isWritingManifest) => {
* if (isWritingManifest) {
* return undefined
* }
* return 'value that is used outside a manifest'
* }),
* };
*/
export type ArgDefaultHelp<T, P = CustomOptions> = T | ((context: DefaultContext<Arg<T, P>>, isWritingManifest?: boolean) => Promise<string | undefined>)

export type FlagRelationship = string | {name: string; when: (flags: Record<string, unknown>) => Promise<boolean>};
export type Relationship = {
Expand Down Expand Up @@ -208,7 +292,7 @@ export type BooleanFlag<T> = FlagProps & BooleanFlagProps & {

export type OptionFlagDefaults<T, P = CustomOptions, M = false> = FlagProps & OptionFlagProps & {
parse: FlagParser<T, string, P>
defaultHelp?: T | FlagDefaultHelp<T>;
defaultHelp?: FlagDefaultHelp<T>;
input: string[];
default?: M extends true ? FlagDefault<T[] | undefined, P> : FlagDefault<T | undefined, P>;
}
Expand Down
2 changes: 1 addition & 1 deletion src/interfaces/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,5 @@ export interface Plugin {

findCommand(id: string, opts: { must: true }): Promise<Command.Class>;
findCommand(id: string, opts?: { must: boolean }): Promise<Command.Class> | undefined;
load(suppressSensitiveData: boolean): Promise<void>;
load(isWritingManifest: boolean): Promise<void>;
}
12 changes: 10 additions & 2 deletions src/parser/parse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,14 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags']
const flag = this.input.flags[token.flag]

if (!flag) throw new CLIError(`Unexpected flag ${token.flag}`)

// if flag has defaultHelp, capture its value into metadata
if (Reflect.has(flag, 'defaultHelp')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

check to see if defaultHelp is present on the flag. If so capture either constant or results of function call for later use.

const defaultHelpProperty = Reflect.get(flag, 'defaultHelp')
const defaultHelp = (typeof defaultHelpProperty === 'function' ? await defaultHelpProperty({options: flag, flags, ...this.context}) : defaultHelpProperty)
this.metaData.flags[token.flag] = {...this.metaData.flags[token.flag], defaultHelp}
}

if (flag.type === 'boolean') {
if (token.input === `--no-${flag.name}`) {
flags[token.flag] = false
Expand Down Expand Up @@ -256,7 +264,7 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags']
for (const k of Object.keys(this.input.flags)) {
const flag = this.input.flags[k]
if (flags[k]) continue
if (flag.env && Object.prototype.hasOwnProperty.call(process.env, flag.env)) {
if (flag.env && Reflect.has(process.env, flag.env)) {
const input = process.env[flag.env]
if (flag.type === 'option') {
if (input) {
Expand All @@ -271,7 +279,7 @@ export class Parser<T extends ParserInput, TFlags extends OutputFlags<T['flags']
}

if (!(k in flags) && flag.default !== undefined) {
this.metaData.flags[k] = {setFromDefault: true}
this.metaData.flags[k] = {...this.metaData.flags[k], setFromDefault: true}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sure all metadata captured are retained

const defaultValue = (typeof flag.default === 'function' ? await flag.default({options: flag, flags, ...this.context}) : flag.default)
flags[k] = defaultValue
}
Expand Down
3 changes: 2 additions & 1 deletion src/parser/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,8 @@ export async function validate(parse: {
if (parse.output.metadata.flags && parse.output.metadata.flags[name]?.setFromDefault)
continue
if (parse.output.flags[flag] !== undefined) {
return {...base, status: 'failed', reason: `--${flag}=${parse.output.flags[flag]} cannot also be provided when using --${name}`}
const flagValue = parse.output.metadata.flags?.[flag]?.defaultHelp ?? parse.output.flags[flag]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exclusive use error to be created here. To avoid --target-org=[object Object] cannot also be provided when using --all, see if defaultHelp value is present and if so use that in message.

return {...base, status: 'failed', reason: `--${flag}=${flagValue} cannot also be provided when using --${name}`}
}
}

Expand Down
41 changes: 41 additions & 0 deletions test/parser/parse.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,10 @@ See more help with --help`)
constructor(input: string) {
this.prop = input
}

public toString(): string {
return this.prop
}
}
it('uses default via value', async () => {
const out = await parse([], {
Expand All @@ -707,6 +711,43 @@ See more help with --help`)
})
expect(out.flags.foo?.prop).to.equal('baz')
})
it('should error with exclusive flag violation', async () => {
try {
const out = await parse(['--foo', 'baz', '--bar'], {
flags: {
foo: Flags.custom<TestClass>({
parse: async input => new TestClass(input),
defaultHelp: new TestClass('bar'),
})(),
bar: Flags.boolean({
exclusive: ['foo'],
}),
},
})
expect.fail(`Should have thrown an error ${JSON.stringify(out)}`)
} catch (error) {
assert(error instanceof Error)
expect(error.message).to.include('--foo=bar cannot also be provided when using --bar')
}
})
it('should error with exclusive flag violation and defaultHelp value', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

nice!

try {
const out = await parse(['--foo', 'baz', '--bar'], {
flags: {
foo: Flags.custom<TestClass>({
parse: async input => new TestClass(input),
})(),
bar: Flags.boolean({
exclusive: ['foo'],
}),
},
})
expect.fail(`Should have thrown an error ${JSON.stringify(out)}`)
} catch (error) {
assert(error instanceof Error)
expect(error.message).to.include('--foo=baz cannot also be provided when using --bar')
}
})
it('uses parser when value provided', async () => {
const out = await parse(['--foo=bar'], {
flags: {
Expand Down