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

UI: remove references to comma separation for string array edit types #20163

Merged
merged 21 commits into from
Apr 19, 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
10 changes: 0 additions & 10 deletions ui/app/components/generated-item.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,16 +53,6 @@ export default Component.extend({
),
init() {
this._super(...arguments);
hellobontempo marked this conversation as resolved.
Show resolved Hide resolved
this.model.fieldGroups.forEach((element) => {
// overwriting the helpText for Token Polices.
// HelpText from the backend says add a comma separated list, which works on the CLI but not here on the UI.
// This effects TLS Certificates, Userpass, and Kubernetes. https://github.com/hashicorp/vault/issues/10346
if (element.Tokens) {
element.Tokens.find((attr) => attr.name === 'tokenPolicies').options.helpText =
'Add policies that will apply to the generated token for this user. One policy per row.';
}
});

if (this.mode === 'edit') {
// For validation to work in edit mode,
// reconstruct the model values from field group
Expand Down
10 changes: 4 additions & 6 deletions ui/app/models/pki/certificate/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,28 +59,26 @@ export default class PkiCertificateBaseModel extends Model {
@attr('string', {
label: 'Subject Alternative Names (SANs)',
subText:
'The requested Subject Alternative Names; if email protection is enabled for the role, this may contain email addresses. Add one per row.',
'The requested Subject Alternative Names; if email protection is enabled for the role, this may contain email addresses.',
editType: 'stringArray',
})
altNames;

// SANs below are editType: stringArray from openApi
@attr('string', {
label: 'IP Subject Alternative Names (IP SANs)',
subText: 'Only valid if the role allows IP SANs (which is the default). Add one per row.',
subText: 'Only valid if the role allows IP SANs (which is the default).',
})
ipSans;

@attr('string', {
label: 'URI Subject Alternative Names (URI SANs)',
subText:
'If any requested URIs do not match role policy, the entire request will be denied. Add one per row.',
subText: 'If any requested URIs do not match role policy, the entire request will be denied.',
})
uriSans;

@attr('string', {
subText:
'Requested other SANs with the format <oid>;UTF8:<utf8 string value> for each entry. Add one per row.',
subText: 'Requested other SANs with the format <oid>;UTF8:<utf8 string value> for each entry.',
})
otherSans;

Expand Down
2 changes: 1 addition & 1 deletion ui/app/models/pki/issuer.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ export default class PkiIssuerModel extends Model {

@attr('string', {
subText:
'The URL values for the Issuing Certificate field. These are different URLs for the same resource, and should be added individually, not in a comma-separated list.',
'The URL values for the Issuing Certificate field; these are different URLs for the same resource.',
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure how often and for what cases we would use a semicolon. This comes from my experience making language changes and Ivana correcting them :) . From what I can remember, she avoided them and stuck with a period. (Note: trying to search for text with semicolons is a challenge given how much we use them for all other things...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since they're related independent clauses a semicolon felt appropriate here, the super short sentences seemed really choppy. But I can defer to design!

I also thought about removing the second part all together, since it felt like it was only there so add the part about adding one per row

editType: 'stringArray',
})
issuingCertificates;
Expand Down
8 changes: 4 additions & 4 deletions ui/app/models/pki/role.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ export default class PkiRoleModel extends Model {
/* Overriding OpenApi Domain handling options */
@attr({
label: 'Allowed domains',
subText: 'Specifies the domains this role is allowed to issue certificates for. Add one item per row.',
subText: 'Specifies the domains this role is allowed to issue certificates for.',
editType: 'stringArray',
})
allowedDomains;
Expand Down Expand Up @@ -196,7 +196,7 @@ export default class PkiRoleModel extends Model {
/* Overriding API Policy identifier option */
@attr({
label: 'Policy identifiers',
subText: 'A comma-separated string or list of policy object identifiers (OIDs). Add one per row. ',
subText: 'A list of policy object identifiers (OIDs).',
editType: 'stringArray',
})
policyIdentifiers;
Expand All @@ -213,7 +213,7 @@ export default class PkiRoleModel extends Model {

@attr({
label: 'URI Subject Alternative Names (URI SANs)',
subText: 'Defines allowed URI Subject Alternative Names. Add one item per row',
subText: 'Defines allowed URI Subject Alternative Names.',
editType: 'stringArray',
docLink: '/vault/docs/concepts/policies',
})
Expand All @@ -229,7 +229,7 @@ export default class PkiRoleModel extends Model {

@attr({
label: 'Other SANs',
subText: 'Defines allowed custom OID/UTF8-string SANs. Add one item per row.',
subText: 'Defines allowed custom OID/UTF8-string SANs.',
editType: 'stringArray',
})
allowedOtherSans;
Expand Down
2 changes: 1 addition & 1 deletion ui/app/models/pki/urls.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export default class PkiUrlsModel extends Model {
@attr({
label: 'Issuing certificates',
subText:
'The URL values for the Issuing Certificate field. These are different URLs for the same resource, and should be added individually, not in a comma-separated list.',
'The URL values for the Issuing Certificate field; these are different URLs for the same resource.',
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment here re: ;

showHelpText: false,
editType: 'stringArray',
})
Expand Down
17 changes: 17 additions & 0 deletions ui/app/utils/openapi-to-attrs.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,23 @@ export const expandOpenApiProps = function (props) {
editType = items.type + capitalize(type);
}

// remove references to comma separated strings from tooltips
if (editType === 'stringArray') {
if (description?.includes('Comma separated string or')) {
description = description.replace(/\bComma\b \bseparated\b \bstring\b \bor\b |\bJSON\b /g, '');
description = description.replace(/\blist\b|\barray\b/, 'List');
} else if (description?.includes('Comma separated list')) {
description = description.replace(/\bComma\b \bseparated\b \blist\b/, 'List');
} else if (description?.includes('Comma-separated list')) {
description = description.replace(/\bComma\b-\bseparated\b \blist\b/, 'List');
} else if (description?.includes('comma-separated list')) {
description = description.replace(/\bcomma\b-\bseparated\b /, '');
} else if (description?.includes('comma-separated string or')) {
description = description.replace(/\bcomma\b-\bseparated\b \bstring\b \bor\b /, '');
description = description.replace(/\barray\b/, 'list');
}
}

hellobontempo marked this conversation as resolved.
Show resolved Hide resolved
const attrDefn = {
editType,
helpText: description,
Expand Down
2 changes: 1 addition & 1 deletion ui/lib/core/addon/components/form-field.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@
@inputValue={{get @model this.valuePath}}
@onChange={{this.setAndBroadcast}}
@attrName={{@attr.name}}
@subText={{@attr.options.subText}}
@subText="{{@attr.options.subText}} Add one item per row."
/>
{{else if (eq @attr.options.sensitive true)}}
{{! Masked Input }}
Expand Down
4 changes: 2 additions & 2 deletions ui/lib/core/addon/components/string-list.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@
{{#if @label}}
<label class="is-label" data-test-string-list-label="true">
{{@label}}
{{#if this.helpText}}
<InfoTooltip>{{this.helpText}}</InfoTooltip>
{{#if @helpText}}
<InfoTooltip>{{@helpText}}</InfoTooltip>
{{/if}}
</label>
{{#if @subText}}
Expand Down
8 changes: 0 additions & 8 deletions ui/lib/core/addon/components/string-list.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,6 @@ export default class StringList extends Component {
return inputs;
}

get helpText() {
if (this.args.attrName === 'tokenBoundCidrs') {
return 'Specifies the blocks of IP addresses which are allowed to use the generated token. One entry per row.';
} else {
return this.args.helpText;
}
}

@action
autoSize(element) {
autosize(element.querySelector('textarea'));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
{{else if (eq group "Additional subject fields")}}
These fields provide more information about the client to which the certificate belongs.
{{/if}}
Add one item per row.
Copy link
Contributor Author

@hellobontempo hellobontempo Apr 13, 2023

Choose a reason for hiding this comment

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

Removed here since it now renders above each input.
Screenshot 2023-04-13 at 4 30 54 PM

</p>
{{#each fields as |fieldName|}}
{{#let (find-by "name" fieldName @model.allFields) as |attr|}}
Expand Down
4 changes: 2 additions & 2 deletions ui/lib/pki/addon/components/pki-key-usage.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@
<StringList
class="is-box-shadowless"
data-test-input="extKeyUsageOids"
@label="Extended key usage oids"
@label="Extended key usage OIDs"
@inputValue={{get @model "extKeyUsageOids"}}
@onChange={{this.onStringListChange}}
@attrName="extKeyUsageOids"
@subText="A list of extended key usage oids. Add one item per row."
@subText="A list of extended key usage OIDs. Add one item per row."
@showHelpText={{false}}
/>
</div>
5 changes: 1 addition & 4 deletions ui/tests/acceptance/auth-list-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,7 @@ module('Acceptance | auth backend list', function (hooks) {
await triggerEvent(tooltipTrigger, 'mouseenter');
assert
.dom('[data-test-info-tooltip-content]')
.hasText(
'Add policies that will apply to the generated token for this user. One policy per row.',
'Overwritten tooltip text displays in token form field.'
);
.hasText('List of policies', 'Overwritten tooltip text displays in token form field.');

await click('[data-test-save-config="true"]');

Expand Down
14 changes: 1 addition & 13 deletions ui/tests/integration/components/string-list-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import { module, test } from 'qunit';
import { setupRenderingTest } from 'ember-qunit';
import { render, click, fillIn, triggerKeyEvent, triggerEvent } from '@ember/test-helpers';
import { render, click, fillIn, triggerKeyEvent } from '@ember/test-helpers';
import sinon from 'sinon';
import hbs from 'htmlbars-inline-precompile';

Expand Down Expand Up @@ -137,16 +137,4 @@ module('Integration | Component | string list', function (hooks) {
assert.dom('[data-test-string-list-input="0"]').hasValue('bar');
assert.dom('[data-test-string-list-input="1"]').hasValue('');
});

test('it replaces helpText if name is tokenBoundCidrs', async function (assert) {
assert.expect(1);
await render(hbs`<StringList @label={{'blah'}} @helpText={{'blah'}} @attrName={{'tokenBoundCidrs'}} />`);
const tooltipTrigger = document.querySelector('[data-test-tool-tip-trigger]');
await triggerEvent(tooltipTrigger, 'mouseenter');
assert
.dom('[data-test-info-tooltip-content]')
.hasText(
'Specifies the blocks of IP addresses which are allowed to use the generated token. One entry per row.'
);
});
});
127 changes: 127 additions & 0 deletions ui/tests/unit/utils/openapi-to-attrs-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import { attr } from '@ember-data/model';
import { expandOpenApiProps, combineAttributes, combineFieldGroups } from 'vault/utils/openapi-to-attrs';
import { module, test } from 'qunit';
import { camelize } from '@ember/string';

module('Unit | Util | OpenAPI Data Utilities', function () {
const OPENAPI_RESPONSE_PROPS = {
Expand Down Expand Up @@ -145,6 +146,120 @@ module('Unit | Util | OpenAPI Data Utilities', function () {

const NEW_FIELDS = ['one', 'two', 'three'];

const OPENAPI_DESCRIPTIONS = {
token_bound_cidrs: {
type: 'array',
description: `Comma separated string or JSON list of CIDR blocks. If set, specifies the blocks of IP addresses which are allowed to use the generated token.`,
items: {
type: 'string',
},
'x-vault-displayAttrs': {
name: "Generated Token's Bound CIDRs",
group: 'Tokens',
},
},
token_policies: {
type: 'array',
description: `Comma-separated list of policies`,
items: {
type: 'string',
},
'x-vault-displayAttrs': {
name: "Generated Token's Policies",
group: 'Tokens',
},
},
secret_id_bound_cidrs: {
type: 'array',
description: `Comma separated string or list of CIDR blocks. If set, specifies the blocks of IP addresses which can perform the login operation.`,
items: {
type: 'string',
},
},
bound_cidr_list: {
type: 'array',
description: `Deprecated: Please use "secret_id_bound_cidrs" instead. Comma separated string or list of CIDR blocks. If set, specifies the blocks of IP addresses which can perform the login operation.`,
items: {
type: 'string',
},
},
allowed_roles: {
type: 'array',
description: `Comma separated string or array of the role names allowed to get creds from this database connection. If empty no roles are allowed. If "*" all roles are allowed.`,
items: {
type: 'string',
},
},
cidr_list: {
type: 'array',
description: `[Optional for OTP type] [Not applicable for CA type] Comma separated list of CIDR blocks for which the role is applicable for. CIDR blocks can belong to more than one role.`,
items: {
type: 'string',
},
},
ocsp_servers_override: {
type: 'array',
description: `A comma-separated list of OCSP server addresses. If unset, the OCSP server is determined from the AuthorityInformationAccess extension on the certificate being inspected.`,
items: {
type: 'string',
},
},
key_usage: {
type: 'array',
description: `A comma-separated string or list of key usages (not extended key usages). Valid values can be found at https://golang.org/pkg/crypto/x509/#KeyUsage -- simply drop the "KeyUsage" part of the name. To remove all key usages from being set, set this value to an empty list.`,
items: {
type: 'string',
},
},
required_extensions: {
type: 'array',
description: `A comma-separated string or array of extensions formatted as "oid:value". Expects the extension value to be some type of ASN1 encoded string. All values much match. Supports globbing on "value".`,
items: {
type: 'string',
},
},
crl_distribution_points: {
type: 'array',
description: `Comma-separated list of URLs to be used for the CRL distribution points attribute. See also RFC 5280 Section 4.2.1.13.`,
items: {
type: 'string',
},
},
};

const STRING_ARRAY_DESCRIPTIONS = {
token_bound_cidrs: {
helpText: `List of CIDR blocks. If set, specifies the blocks of IP addresses which are allowed to use the generated token.`,
},
token_policies: {
helpText: `List of policies`,
},
secret_id_bound_cidrs: {
helpText: `List of CIDR blocks. If set, specifies the blocks of IP addresses which can perform the login operation.`,
},
bound_cidr_list: {
helpText: `Deprecated: Please use "secret_id_bound_cidrs" instead. List of CIDR blocks. If set, specifies the blocks of IP addresses which can perform the login operation.`,
},
allowed_roles: {
helpText: `List of the role names allowed to get creds from this database connection. If empty no roles are allowed. If "*" all roles are allowed.`,
},
cidr_list: {
helpText: `[Optional for OTP type] [Not applicable for CA type] List of CIDR blocks for which the role is applicable for. CIDR blocks can belong to more than one role.`,
},
ocsp_servers_override: {
helpText: `A list of OCSP server addresses. If unset, the OCSP server is determined from the AuthorityInformationAccess extension on the certificate being inspected.`,
},
key_usage: {
helpText: `A list of key usages (not extended key usages). Valid values can be found at https://golang.org/pkg/crypto/x509/#KeyUsage -- simply drop the "KeyUsage" part of the name. To remove all key usages from being set, set this value to an empty list.`,
},
required_extensions: {
helpText: `A list of extensions formatted as "oid:value". Expects the extension value to be some type of ASN1 encoded string. All values much match. Supports globbing on "value".`,
},
crl_distribution_points: {
helpText: `List of URLs to be used for the CRL distribution points attribute. See also RFC 5280 Section 4.2.1.13.`,
},
};

test('it creates objects from OpenAPI schema props', function (assert) {
assert.expect(6);
const generatedProps = expandOpenApiProps(OPENAPI_RESPONSE_PROPS);
Expand Down Expand Up @@ -229,4 +344,16 @@ module('Unit | Util | OpenAPI Data Utilities', function () {
assert.deepEqual(fieldGroups[groupName], expectedGroups[groupName], 'it incorporates all new fields');
}
});

test('it removes references to comma separation in help text for string array attrs', async function (assert) {
assert.expect(10);
const generatedProps = expandOpenApiProps(OPENAPI_DESCRIPTIONS);
for (const propName in STRING_ARRAY_DESCRIPTIONS) {
assert.strictEqual(
generatedProps[camelize(propName)].helpText,
STRING_ARRAY_DESCRIPTIONS[propName].helpText,
`correctly updates helpText for ${propName}`
);
}
});
});