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 - don't coerce JSON input to an Object #5271

Merged
merged 2 commits into from
Sep 5, 2018
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
11 changes: 9 additions & 2 deletions ui/app/components/secret-edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export default Ember.Component.extend(FocusOnInsertMixin, {
// use a named action here so we don't have to pass one in
// this will bubble to the route
toggleAdvancedEdit: 'toggleAdvancedEdit',
error: null,

codemirrorString: null,

Expand Down Expand Up @@ -79,7 +80,8 @@ export default Ember.Component.extend(FocusOnInsertMixin, {
'key.isFolder',
'key.isError',
'key.flagsIsInvalid',
'hasLintError'
'hasLintError',
'error'
),

basicModeDisabled: computed('secretDataIsAdvanced', 'showAdvancedMode', function() {
Expand Down Expand Up @@ -242,10 +244,15 @@ export default Ember.Component.extend(FocusOnInsertMixin, {
},

codemirrorUpdated(val, codemirror) {
this.set('error', null);
codemirror.performLint();
const noErrors = codemirror.state.lint.marked.length === 0;
if (noErrors) {
this.get('secretData').fromJSONString(val);
try {
this.get('secretData').fromJSONString(val);
} catch (e) {
this.set('error', e.message);
}
}
this.set('hasLintError', !noErrors);
this.set('codemirrorString', val);
Expand Down
9 changes: 7 additions & 2 deletions ui/app/lib/kv-object.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import Ember from 'ember';

const { typeOf, guidFor } = Ember;

export default Ember.ArrayProxy.extend({
fromJSON(json) {
const contents = Object.keys(json || []).map(key => {
if (json && typeOf(json) !== 'object') {
throw new Error('Vault expects data to be formatted as an JSON object.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we explain more than this? I'm afraid they're going to alter it enough so it gets accepted and then run into the same issue now where the keys are array indexes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking this is already the advanced mode so there would be some idea of what they're doing. The index thing was happening because we were using Object.keys on an array, but now we short circuit that, so it would take quite a bit of fiddling to get back there.

What sort of extra explanation were you thinking? Maybe: "The input is valid JSON, but Vault requires the top-level structure to be a JSON Object and currently it is not." Or something like that?

}
let contents = Object.keys(json || []).map(key => {
let obj = {
name: key,
value: json[key],
};
Ember.guidFor(obj);
guidFor(obj);
return obj;
});
this.setObjects(
Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/partials/secret-form-create.hbs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<form class="{{if showAdvancedMode 'advanced-edit' 'simple-edit'}}" onsubmit={{action "createOrUpdateKey" "create"}} onchange={{action "handleChange"}}>
<div class="field box is-fullwidth is-sideless is-marginless">
<NamespaceReminder @mode="create" @noun="secret" />
{{message-error model=key}}
{{message-error model=key errorMessage=error}}
<label class="label is-font-weight-normal" for="kv-key">Path for this secret</label>
<div class="field has-addons">
{{#if (not-eq key.initialParentKey '') }}
Expand Down
2 changes: 1 addition & 1 deletion ui/app/templates/partials/secret-form-edit.hbs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<form onsubmit={{action "createOrUpdateKey" "update"}} onchange={{action "handleChange"}}>
<div class="box is-sideless is-fullwidth is-marginless">
{{message-error model=key}}
{{message-error model=key errorMessage=error}}
<NamespaceReminder @mode="edit" @noun="secret" />
{{#unless showAdvancedMode}}
<div class="table info-table-row-header">
Expand Down
40 changes: 39 additions & 1 deletion ui/tests/integration/components/secret-edit-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ import hbs from 'htmlbars-inline-precompile';

moduleForComponent('secret-edit', 'Integration | Component | secret edit', {
integration: true,
beforeEach() {
this.inject.service('code-mirror', { as: 'codeMirror' });
},
});

test('it disables JSON toggle in show mode when is an advanced format', function(assert) {
Expand All @@ -19,7 +22,7 @@ test('it disables JSON toggle in show mode when is an advanced format', function
assert.dom('[data-test-secret-json-toggle]').isDisabled();
});

test('it does JSON toggle in show mode when is', function(assert) {
test('it does JSON toggle in show mode when showing string data', function(assert) {
this.set('mode', 'show');
this.set('key', {
secretData: {
Expand All @@ -32,3 +35,38 @@ test('it does JSON toggle in show mode when is', function(assert) {
this.render(hbs`{{secret-edit mode=mode key=key }}`);
assert.dom('[data-test-secret-json-toggle]').isNotDisabled();
});

test('it shows an error when creating and data is not an object', function(assert) {
this.set('mode', 'create');
this.set('key', {
secretData: {
int: '2',
null: 'null',
float: '1.234',
},
});

this.render(hbs`{{secret-edit mode=mode key=key preferAdvancedEdit=true }}`);
let instance = this.codeMirror.instanceFor(this.$('[data-test-component=json-editor]').attr('id'));
instance.setValue(JSON.stringify([{ foo: 'bar' }]));
assert.dom('[data-test-error]').includesText('Vault expects data to be formatted as an JSON object');
});

test('it shows an error when editing and the data is not an object', function(assert) {
this.set('mode', 'edit');
this.set('capabilities', {
canUpdate: true,
});
this.set('key', {
secretData: {
int: '2',
null: 'null',
float: '1.234',
},
});

this.render(hbs`{{secret-edit capabilities=capabilities mode=mode key=key preferAdvancedEdit=true }}`);
let instance = this.codeMirror.instanceFor(this.$('[data-test-component=json-editor]').attr('id'));
instance.setValue(JSON.stringify([{ foo: 'bar' }]));
assert.dom('[data-test-error]').includesText('Vault expects data to be formatted as an JSON object');
});
11 changes: 11 additions & 0 deletions ui/tests/unit/lib/kv-object-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,17 @@ fromJSONTests.forEach(function([name, input, content]) {
});
});

test(`fromJSON: non-object input`, function(assert) {
let input = [{ foo: 'bar' }];
assert.throws(
() => {
KVObject.create({ content: [] }).fromJSON(input);
},
/Vault expects data to be formatted as an JSON object/,
'throws when non-object input is used to construct the KVObject'
);
});

fromJSONTests.forEach(function([name, input, content]) {
test(`fromJSONString: ${name}`, function(assert) {
let inputString = JSON.stringify(input, null, 2);
Expand Down