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 18 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
3 changes: 3 additions & 0 deletions builtin/credential/aws/path_role.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ auth_type is ec2 or inferred_entity_type is ec2_instance.`,
given instance IDs. Can be a list or comma-separated string of EC2 instance
IDs. This is only applicable when auth_type is ec2 or inferred_entity_type is
ec2_instance.`,
DisplayAttrs: &framework.DisplayAttributes{
Description: "If set, defines a constraint on the EC2 instances to have one of the given instance IDs. A list of EC2 instance IDs. This is only applicable when auth_type is ec2 or inferred_entity_type is ec2_instance.",
},
},
"resolve_aws_unique_ids": {
Type: framework.TypeBool,
Expand Down
15 changes: 15 additions & 0 deletions builtin/credential/cert/path_certs.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ Must be x509 PEM encoded.`,
Type: framework.TypeCommaStringSlice,
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.`,
DisplayAttrs: &framework.DisplayAttributes{
Description: "A list of OCSP server addresses. If unset, the OCSP server is determined from the AuthorityInformationAccess extension on the certificate being inspected.",
},
},
"ocsp_fail_open": {
Type: framework.TypeBool,
Expand All @@ -96,6 +99,7 @@ This parameter is deprecated, please use allowed_common_names, allowed_dns_sans,
allowed_email_sans, allowed_uri_sans.`,
DisplayAttrs: &framework.DisplayAttributes{
Group: "Constraints",
Description: "A list of names. At least one must exist in either the Common Name or SANs. Supports globbing. This parameter is deprecated, please use allowed_common_names, allowed_dns_sans, allowed_email_sans, allowed_uri_sans.",
},
},

Expand All @@ -105,6 +109,7 @@ allowed_email_sans, allowed_uri_sans.`,
At least one must exist in the Common Name. Supports globbing.`,
DisplayAttrs: &framework.DisplayAttributes{
Group: "Constraints",
Description: "A list of names. At least one must exist in the Common Name. Supports globbing.",
},
},

Expand All @@ -115,6 +120,7 @@ At least one must exist in the SANs. Supports globbing.`,
DisplayAttrs: &framework.DisplayAttributes{
Name: "Allowed DNS SANs",
Group: "Constraints",
Description: "A list of DNS names. At least one must exist in the SANs. Supports globbing.",
},
},

Expand All @@ -125,6 +131,7 @@ At least one must exist in the SANs. Supports globbing.`,
DisplayAttrs: &framework.DisplayAttributes{
Name: "Allowed Email SANs",
Group: "Constraints",
Description: "A list of Email Addresses. At least one must exist in the SANs. Supports globbing.",
},
},

Expand All @@ -135,6 +142,7 @@ At least one must exist in the SANs. Supports globbing.`,
DisplayAttrs: &framework.DisplayAttributes{
Name: "Allowed URI SANs",
Group: "Constraints",
Description: "A list of URIs. At least one must exist in the SANs. Supports globbing.",
},
},

Expand All @@ -144,6 +152,7 @@ At least one must exist in the SANs. Supports globbing.`,
At least one must exist in the OU field.`,
DisplayAttrs: &framework.DisplayAttributes{
Group: "Constraints",
Description: "A list of Organizational Units names. At least one must exist in the OU field.",
},
},

Expand All @@ -152,6 +161,9 @@ At least one must exist in the OU field.`,
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".`,
DisplayAttrs: &framework.DisplayAttributes{
Description: "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'.",
},
},

"allowed_metadata_extensions": {
Expand All @@ -160,6 +172,9 @@ All values much match. Supports globbing on "value".`,
Upon successful authentication, these extensions will be added as metadata if they are present
in the certificate. The metadata key will be the string consisting of the oid numbers
separated by a dash (-) instead of a dot (.) to allow usage in ACL templates.`,
DisplayAttrs: &framework.DisplayAttributes{
Description: "A list of OID extensions. Upon successful authentication, these extensions will be added as metadata if they are present in the certificate. The metadata key will be the string consisting of the OID numbers separated by a dash (-) instead of a dot (.) to allow usage in ACL templates.",
},
},

"display_name": {
Expand Down
3 changes: 3 additions & 0 deletions builtin/credential/ldap/path_groups.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ func pathGroups(b *backend) *framework.Path {
"policies": {
Type: framework.TypeCommaStringSlice,
Description: "Comma-separated list of policies associated to the group.",
DisplayAttrs: &framework.DisplayAttributes{
Description: "A list of policies associated to the group.",
},
},
},

Expand Down
6 changes: 6 additions & 0 deletions builtin/credential/ldap/path_users.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,17 @@ func pathUsers(b *backend) *framework.Path {
"groups": {
Type: framework.TypeCommaStringSlice,
Description: "Comma-separated list of additional groups associated with the user.",
DisplayAttrs: &framework.DisplayAttributes{
Description: "A list of additional groups associated with the user.",
},
},

"policies": {
Type: framework.TypeCommaStringSlice,
Description: "Comma-separated list of policies associated with the user.",
DisplayAttrs: &framework.DisplayAttributes{
Description: "A list of policies associated with the user.",
},
},
},

Expand Down
3 changes: 3 additions & 0 deletions builtin/credential/okta/path_groups.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ func pathGroups(b *backend) *framework.Path {
"policies": {
Type: framework.TypeCommaStringSlice,
Description: "Comma-separated list of policies associated to the group.",
DisplayAttrs: &framework.DisplayAttributes{
Description: "A list of policies associated to the group.",
},
},
},

Expand Down
3 changes: 2 additions & 1 deletion builtin/credential/radius/path_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,10 @@ func pathConfig(b *backend) *framework.Path {
"unregistered_user_policies": {
Type: framework.TypeString,
Default: "",
Description: "Comma-separated list of policies to grant upon successful RADIUS authentication of an unregisted user (default: empty)",
Description: "Comma-separated list of policies to grant upon successful RADIUS authentication of an unregistered user (default: empty)",
DisplayAttrs: &framework.DisplayAttributes{
Name: "Policies for unregistered users",
Description: "List of policies to grant upon successful RADIUS authentication of an unregistered user (default: empty)",
},
},
"dial_timeout": {
Expand Down
3 changes: 3 additions & 0 deletions builtin/credential/radius/path_users.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ func pathUsers(b *backend) *framework.Path {
"policies": {
Type: framework.TypeCommaStringSlice,
Description: "Comma-separated list of policies associated to the user.",
DisplayAttrs: &framework.DisplayAttributes{
Description: "A list of policies associated to the user.",
},
},
},

Expand Down
3 changes: 3 additions & 0 deletions builtin/credential/userpass/path_user_policies.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ func pathUserPolicies(b *backend) *framework.Path {
"token_policies": {
Type: framework.TypeCommaStringSlice,
Description: "Comma-separated list of policies",
DisplayAttrs: &framework.DisplayAttributes{
Description: "A list of policies that will apply to the generated token for this user.",
},
},
},

Expand Down
3 changes: 3 additions & 0 deletions changelog/20163.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
ui: adds warning for commas in stringArray inputs and updates tooltip help text to remove references to comma separation
```
5 changes: 5 additions & 0 deletions sdk/framework/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,11 @@ type DisplayAttributes struct {
// Name is the name of the field suitable as a label or documentation heading.
Name string `json:"name,omitempty"`

// Description of the field that renders as tooltip help text beside the label (name) in the UI.
// This may be used to replace descriptions that reference comma separation but correspond
// to UI inputs where only arrays are valid. For example params with Type: framework.TypeCommaStringSlice
Description string `json:"description,omitempty"`

// Value is a sample value to display for this field. This may be used
// to indicate a default value, but it is for display only and completely separate
// from any Default member handling.
Expand Down
2 changes: 2 additions & 0 deletions sdk/helper/tokenutil/tokenutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ func TokenFields() map[string]*framework.FieldSchema {
DisplayAttrs: &framework.DisplayAttributes{
Name: "Generated Token's Bound CIDRs",
Group: "Tokens",
Description: "A list of CIDR blocks. If set, specifies the blocks of IP addresses which are allowed to use the generated token.",
},
},

Expand Down Expand Up @@ -125,6 +126,7 @@ func TokenFields() map[string]*framework.FieldSchema {
DisplayAttrs: &framework.DisplayAttributes{
Name: "Generated Token's Policies",
Group: "Tokens",
Description: "A list of policies that will apply to the generated token for this user.",
},
},

Expand Down
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
4 changes: 4 additions & 0 deletions ui/app/styles/core/forms.scss
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,10 @@ fieldset.form-fieldset {
border: none;
}

.has-warning-border {
border: 1px solid $yellow-500;
}

.has-error-border,
select.has-error-border {
border: 1px solid $red-500;
Expand Down
8 changes: 6 additions & 2 deletions ui/app/utils/openapi-to-attrs.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ export const expandOpenApiProps = function (props) {
type = 'number';
}

if (prop['x-vault-displayAttrs']?.description) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we not add description above on line 19? Would make this a bit clearer. If we needed to rename it to something so a falsey value doesn't override the previous definition of description you could rename it like { ..., description: openApiDescription } = prop[...]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea!

description = prop['x-vault-displayAttrs'].description;
}

editType = editType || type;

if (format === 'seconds') {
Expand Down Expand Up @@ -50,8 +54,8 @@ export const expandOpenApiProps = function (props) {
attrDefn.sensitive = true;
}

//only set a label if we have one from OpenAPI
//otherwise the propName will be humanized by the form-field component
// only set a label if we have one from OpenAPI
// otherwise the propName will be humanized by the form-field component
if (name) {
attrDefn.label = name;
}
Expand Down
3 changes: 1 addition & 2 deletions ui/lib/core/addon/components/form-field.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,11 @@
<StringList
data-test-input={{@attr.name}}
@label={{this.labelString}}
@warning={{@attr.options.warning}}
@helpText={{if this.showHelpText @attr.options.helpText}}
@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
19 changes: 13 additions & 6 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 All @@ -19,15 +19,12 @@
</p>
{{/if}}
{{/if}}
{{#if @warning}}
<AlertBanner @type="warning" @message={{@warning}} />
{{/if}}
{{#each this.inputList as |data index|}}
<div class="field is-grouped" data-test-string-list-row={{index}}>
<div class="control is-expanded">
<Textarea
data-test-string-list-input={{index}}
class="input"
class="input {{if (includes index this.indicesWithComma) 'has-warning-border'}}"
@value={{data.value}}
name={{concat this.elementId "-" index}}
id={{concat this.elementId "-" index}}
Expand Down Expand Up @@ -56,6 +53,16 @@
</button>
{{/if}}
</div>
{{#if (includes index this.indicesWithComma)}}
<Icon class="is-flex-v-centered has-text-highlight" @name="alert-triangle-fill" />
{{/if}}
</div>
{{/each}}
{{#if this.indicesWithComma}}
<AlertInline
@type="warning"
@message="Input contains a comma. Please separate values into individual rows."
@isMarginless={{true}}
/>
{{/if}}
</div>
Loading