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.

Sign up for GitHub

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: Fix id fields not allowing update #19117

Merged
merged 19 commits into from
Feb 10, 2023
Merged

UI: Fix id fields not allowing update #19117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
d05f2ff
Remove fieldValue:id from transform models
hashishaw Feb 9, 2023
c642301
Add name to response for transform models
hashishaw Feb 9, 2023
795be3f
use fieldvalue as valuePath only if it is not id
hashishaw Feb 9, 2023
7e29bc5
fix role-aws
hashishaw Feb 9, 2023
acdbf07
fix role-ssh
hashishaw Feb 9, 2023
d862eb0
Make it consistent
hashishaw Feb 9, 2023
5c594d9
Rollback formField changes
hashishaw Feb 9, 2023
24aa2bd
remove fieldValue where openApi is false, override if true
hashishaw Feb 9, 2023
94e725a
fix transform model items
hashishaw Feb 10, 2023
3df2f28
Fix readonly field on create role
hashishaw Feb 10, 2023
a0911fa
Simplify transform serializers with primaryKey
hashishaw Feb 10, 2023
63ff1d7
Use primaryKey in serializer for role-aws
hashishaw Feb 10, 2023
d4fde7c
Fix pki-role and role-ssh
hashishaw Feb 10, 2023
25ba93d
Add check in form-field for modifying id
hashishaw Feb 10, 2023
c9df033
Fix assertion, add back aws adapter response
hashishaw Feb 10, 2023
546a5cd
Add back custom response on adapter for role-aws and role-ssh
hashishaw Feb 10, 2023
ced3f6e
Add response on adapter for transform/base
hashishaw Feb 10, 2023
88eba9f
Remove done todo
hashishaw Feb 10, 2023
4ee89b3
Revert backportable fix
hashishaw Feb 10, 2023
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: 2 additions & 2 deletions ui/app/adapters/pki/pki-role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ export default ApplicationAdapter.extend({
namespace: 'v1',

createOrUpdate(store, type, snapshot, requestType) {
const { name, backend } = snapshot.record;
const serializer = store.serializerFor(type.modelName);
const data = serializer.serialize(snapshot, requestType);
const { id } = snapshot;
const url = this.urlForRole(snapshot.record.get('backend'), id);
const url = this.urlForRole(backend, name);

return this.ajax(url, 'POST', { data });
},
Expand Down
15 changes: 10 additions & 5 deletions ui/app/adapters/role-aws.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
import { assign } from '@ember/polyfills';
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

import ApplicationAdapter from './application';
import { encodePath } from 'vault/utils/path-encoding-helpers';

export default ApplicationAdapter.extend({
namespace: 'v1',

createOrUpdate(store, type, snapshot, requestType) {
const { name, backend } = snapshot.record;
const serializer = store.serializerFor(type.modelName);
const data = serializer.serialize(snapshot, requestType);
const { id } = snapshot;
const url = this.urlForRole(snapshot.record.get('backend'), id);
const url = this.urlForRole(backend, name);

return this.ajax(url, 'POST', { data });
return this.ajax(url, 'POST', { data }).then((resp) => {
// Ember data doesn't like 204 responses except for DELETE method
const response = resp || { data: {} };
response.data.name = name;
response.data.backend = name;
return response;
});
},

createRecord() {
Expand Down Expand Up @@ -56,7 +61,7 @@ export default ApplicationAdapter.extend({
backend,
};

return assign({}, resp, data);
return { ...resp, ...data };
});
},

Expand Down
11 changes: 8 additions & 3 deletions ui/app/adapters/role-ssh.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,17 @@ export default ApplicationAdapter.extend({
namespace: 'v1',

createOrUpdate(store, type, snapshot, requestType) {
const { name, backend } = snapshot.record;
const serializer = store.serializerFor(type.modelName);
const data = serializer.serialize(snapshot, requestType);
const { id } = snapshot;
const url = this.urlForRole(snapshot.record.get('backend'), id);
const url = this.urlForRole(backend, name);

return this.ajax(url, 'POST', { data });
return this.ajax(url, 'POST', { data }).then((resp) => {
// Ember data doesn't like 204 responses except for DELETE method
const response = resp || { data: {} };
response.data.name = name;
return response;
});
},

createRecord() {
Expand Down
10 changes: 7 additions & 3 deletions ui/app/adapters/transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@ export default ApplicationAdapter.extend({
namespace: 'v1',

createOrUpdate(store, type, snapshot) {
const { backend, name } = snapshot.record;
const serializer = store.serializerFor(type.modelName);
const data = serializer.serialize(snapshot);
const { id } = snapshot;
const url = this.urlForTransformations(snapshot.record.get('backend'), id);
const url = this.urlForTransformations(backend, name);

return this.ajax(url, 'POST', { data });
return this.ajax(url, 'POST', { data }).then((resp) => {
const response = resp || {};
response.id = name;
return response;
});
},

createRecord() {
Expand Down
17 changes: 12 additions & 5 deletions ui/app/adapters/transform/base.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,16 @@ export default ApplicationAdapter.extend({
},

createOrUpdate(store, type, snapshot) {
const { backend, name } = snapshot.record;
const serializer = store.serializerFor(type.modelName);
const data = serializer.serialize(snapshot);
const { id } = snapshot;
const url = this.url(snapshot.record.get('backend'), type.modelName, id);

return this.ajax(url, 'POST', { data });
const url = this.url(backend, type.modelName, name);
return this.ajax(url, 'POST', { data }).then((resp) => {
// Ember data doesn't like 204 responses except for DELETE method
const response = resp || { data: {} };
response.data.name = name;
return response;
});
},

createRecord() {
Expand Down Expand Up @@ -42,9 +46,12 @@ export default ApplicationAdapter.extend({
fetchByQuery(query) {
const { backend, modelName, id } = query;
return this.ajax(this.url(backend, modelName, id), 'GET').then((resp) => {
// The API response doesn't explicitly include the name/id, so add it here
return {
...resp,
backend,
id,
name: id,
};
});
},
Expand All @@ -55,9 +62,9 @@ export default ApplicationAdapter.extend({

queryRecord(store, type, query) {
return this.ajax(this.url(query.backend, type.modelName, query.id), 'GET').then((result) => {
// CBS TODO: Add name to response and unmap name <> id on models
return {
id: query.id,
name: query.id,
...result,
};
});
Expand Down
3 changes: 2 additions & 1 deletion ui/app/components/role-aws-edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ export default RoleEdit.extend({
createOrUpdate(type, event) {
event.preventDefault();

const modelId = this.model.id;
// all of the attributes with fieldValue:'id' are called `name`
const modelId = this.model.id || this.model.name;
// prevent from submitting if there's no key
// maybe do something fancier later
if (type === 'create' && isBlank(modelId)) {
Expand Down
3 changes: 2 additions & 1 deletion ui/app/components/role-edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ export default Component.extend(FocusOnInsertMixin, {
createOrUpdate(type, event) {
event.preventDefault();

const modelId = this.model.id;
// all of the attributes with fieldValue:'id' are called `name`
const modelId = this.model.id || this.model.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason to keep this.model.id here? In case somehow the id differs from name on edit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ID does exist if we're editing, so I figured we'd keep it as the authoritative source of truth if it exists

// prevent from submitting if there's no key
// maybe do something fancier later
if (type === 'create' && isBlank(modelId)) {
Expand Down
2 changes: 1 addition & 1 deletion ui/app/components/transformation-edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export default TransformBase.extend({
event.preventDefault();

this.applyChanges('save', () => {
const transformationId = this.model.id;
const transformationId = this.model.id || this.model.name;
const newModelRoles = this.model.allowed_roles || [];
const initialRoles = this.initialRoles || [];

Expand Down
2 changes: 1 addition & 1 deletion ui/app/models/pki/pki-role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default Model.extend({
}),
name: attr('string', {
label: 'Role name',
fieldValue: 'id',
fieldValue: 'name',
readOnly: true,
}),
useOpenAPI: true,
Expand Down
3 changes: 1 addition & 2 deletions ui/app/models/role-aws.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,8 @@ export default Model.extend({
}),
name: attr('string', {
label: 'Role name',
fieldValue: 'id',
readOnly: true,
}),
useOpenAPI: false,
// credentialTypes are for backwards compatibility.
// we use this to populate "credentialType" in
// the serializer. if there is more than one, the
Expand All @@ -51,6 +49,7 @@ export default Model.extend({
editType: 'json',
helpText:
'A policy is an object in AWS that, when associated with an identity or resource, defines their permissions.',
defaultValue: '{\n}',
}),
fields: computed('credentialType', function () {
const credentialType = this.credentialType;
Expand Down
2 changes: 1 addition & 1 deletion ui/app/models/role-ssh.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export default Model.extend({
}),
name: attr('string', {
label: 'Role Name',
fieldValue: 'id',
fieldValue: 'name',
readOnly: true,
}),
keyType: attr('string', {
Expand Down
2 changes: 0 additions & 2 deletions ui/app/models/transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,9 @@ const TWEAK_SOURCE = [
];

const ModelExport = Model.extend({
useOpenAPI: false,
name: attr('string', {
// CBS TODO: make this required for making a transformation
label: 'Name',
fieldValue: 'id',
readOnly: true,
subText: 'The name for your transformation. This cannot be edited later.',
}),
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/transform/alphabet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const M = Model.extend({
}),

name: attr('string', {
fieldValue: 'id',
readOnly: true,
subText: 'The alphabet name. Keep in mind that spaces are not allowed and this cannot be edited later.',
}),
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/transform/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ const ModelExport = Model.extend({
name: attr('string', {
// TODO: make this required for making a transformation
label: 'Name',
fieldValue: 'id',
readOnly: true,
subText: 'The name for your role. This cannot be edited later.',
}),
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/transform/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const M = Model.extend({
}),

name: attr('string', {
fieldValue: 'id',
readOnly: true,
subText:
'Templates allow Vault to determine what and how to capture the value to be transformed. This cannot be edited later.',
Expand Down
1 change: 0 additions & 1 deletion ui/app/models/transit-key.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ export default Model.extend({
}),
name: attr('string', {
label: 'Name',
fieldValue: 'id',
readOnly: true,
}),
autoRotatePeriod: attr({
Expand Down
5 changes: 4 additions & 1 deletion ui/app/serializers/role-aws.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import ApplicationSerializer from './application';

export default ApplicationSerializer.extend({
primaryKey: 'name',
Copy link
Contributor

Choose a reason for hiding this comment

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

👏


extractLazyPaginatedData(payload) {
return payload.data.keys.map((key) => {
const model = {
id: key,
name: key,
};
if (payload.backend) {
model.backend = payload.backend;
Expand Down
7 changes: 5 additions & 2 deletions ui/app/serializers/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import ApplicationSerializer from './application';

export default ApplicationSerializer.extend({
primaryKey: 'name',

// Used for both pki-role (soon to be deprecated) and role-ssh
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for this comment!

extractLazyPaginatedData(payload) {
if (payload.zero_address_roles) {
payload.zero_address_roles.forEach((role) => {
Expand All @@ -11,7 +14,7 @@ export default ApplicationSerializer.extend({
if (!payload.data.key_info) {
return payload.data.keys.map((key) => {
const model = {
id: key,
name: key,
};
if (payload.backend) {
model.backend = payload.backend;
Expand All @@ -22,7 +25,7 @@ export default ApplicationSerializer.extend({

const ret = payload.data.keys.map((key) => {
const model = {
id: key,
name: key,
key_type: payload.data.key_info[key].key_type,
zero_address: payload.data.key_info[key].zero_address,
};
Expand Down
3 changes: 2 additions & 1 deletion ui/app/serializers/transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import ApplicationSerializer from './application';

export default ApplicationSerializer.extend({
normalizeResponse(store, primaryModelClass, payload, id, requestType) {
if (payload.data.masking_character) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this change, attempting to save a new template (or any of the other items) would throw this mysterious error in the console:

Uncaught (in promise) TypeError: e is undefined
    persist transform-edit-base.js:87
    invokeCallback rsvp.js:495
    publish rsvp.js:478
    publishRejection rsvp.js:414
    <anonymous> Ember

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks like a fun error to track down 😬

if (payload.data?.masking_character) {
payload.data.masking_character = String.fromCharCode(payload.data.masking_character);
}
return this._super(store, primaryModelClass, payload, id, requestType);
Expand All @@ -21,6 +21,7 @@ export default ApplicationSerializer.extend({
return payload.data.keys.map((key) => {
const model = {
id: key,
name: key,
};
if (payload.backend) {
model.backend = payload.backend;
Expand Down
6 changes: 2 additions & 4 deletions ui/app/serializers/transform/alphabet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
import ApplicationSerializer from '../application';

export default ApplicationSerializer.extend({
normalizeResponse(store, primaryModelClass, payload, id, requestType) {
payload.data.name = payload.id;
return this._super(store, primaryModelClass, payload, id, requestType);
},
primaryKey: 'name',

extractLazyPaginatedData(payload) {
return payload.data.keys.map((key) => {
const model = {
id: key,
name: key,
};
if (payload.backend) {
model.backend = payload.backend;
Expand Down
2 changes: 2 additions & 0 deletions ui/app/serializers/transform/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import ApplicationSerializer from '../application';

export default ApplicationSerializer.extend({
primaryKey: 'name',
extractLazyPaginatedData(payload) {
return payload.data.keys.map((key) => {
const model = {
id: key,
name: key,
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed we kept id: key here when adding name - is that something we should also do in ui/app/serializers/role.js on line 17?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question, I went back and forth between keeping both and replacing it. I reasoned to add to this one because extractLazyPaginatedData seems to be vault-specific, so there's no guarantee that we're using it in a conventional way which would take advantage of primaryKey (which I believe is extracted via extractId)
I also figured by removing it from the other serializers where we use primaryKey there may be fewer places we need to track down in case we need to update the ID in the future. But, I'm happy to add back id to the others if the team thinks it makes the most sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining your reasoning! I think this makes sense, and ideally decreases the difficulty in tracking down future ember data issues

};
if (payload.backend) {
model.backend = payload.backend;
Expand Down
8 changes: 5 additions & 3 deletions ui/app/serializers/transform/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import ApplicationSerializer from '../application';

export default ApplicationSerializer.extend({
primaryKey: 'name',

normalizeResponse(store, primaryModelClass, payload, id, requestType) {
payload.data.name = payload.id;
if (payload.data.alphabet) {
if (payload.data?.alphabet) {
payload.data.alphabet = [payload.data.alphabet];
}
// strip out P character from any named capture groups
if (payload.data.pattern) {
if (payload.data?.pattern) {
this._formatNamedCaptureGroups(payload.data, '?P', '?');
}
return this._super(store, primaryModelClass, payload, id, requestType);
Expand Down Expand Up @@ -43,6 +44,7 @@ export default ApplicationSerializer.extend({
return payload.data.keys.map((key) => {
const model = {
id: key,
name: key,
};
if (payload.backend) {
model.backend = payload.backend;
Expand Down
5 changes: 5 additions & 0 deletions ui/lib/core/addon/components/form-field.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { action } from '@ember/object';
import { capitalize } from 'vault/helpers/capitalize';
import { humanize } from 'vault/helpers/humanize';
import { dasherize } from 'vault/helpers/dasherize';
import { assert } from '@ember/debug';
/**
* @module FormField
* `FormField` components are field elements associated with a particular model.
Expand Down Expand Up @@ -61,6 +62,10 @@ export default class FormFieldComponent extends Component {
super(...arguments);
const { attr, model } = this.args;
const valuePath = attr.options?.fieldValue || attr.name;
assert(
'Form is attempting to modify an ID. Ember-data does not allow this.',
valuePath.toLowerCase() !== 'id'
);
Comment on lines +65 to +68
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

const modelValue = model[valuePath];
this.showInput = !!modelValue;
}
Expand Down