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

[SECURITY_SOLUTION] delete advanced Policy fields when they are empty #84368

Merged
merged 13 commits into from
Dec 3, 2020
Merged

[SECURITY_SOLUTION] delete advanced Policy fields when they are empty #84368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4307a9f
delete advanced fields when they are empty
kevinlog Nov 25, 2020
d56f0e3
fix types
kevinlog Nov 30, 2020
e982777
clean up logic
kevinlog Nov 30, 2020
9ac26da
clean up
kevinlog Nov 30, 2020
ee92d62
Merge branch 'master' into bug/delete-empty-advanced-policy-fields
kibanamachine Nov 30, 2020
b620c6e
more comments and test
kevinlog Dec 1, 2020
a488234
Merge branch 'bug/delete-empty-advanced-policy-fields' of github.com:…
kevinlog Dec 1, 2020
c746fc8
fix test
kevinlog Dec 1, 2020
fd115dd
adjust test
kevinlog Dec 1, 2020
58ee34b
fix test
kevinlog Dec 1, 2020
1b3d631
fix typo
kevinlog Dec 2, 2020
c601ea8
Merge branch 'master' into bug/delete-empty-advanced-policy-fields
kibanamachine Dec 2, 2020
b943055
Merge branch 'master' into bug/delete-empty-advanced-policy-fields
kibanamachine Dec 3, 2020
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
32 changes: 32 additions & 0 deletions x-pack/plugins/security_solution/public/management/pages/policy/view/policy_advanced.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,45 @@ import { AdvancedPolicySchema } from '../models/advanced_policy_schema';

function setValue(obj: Record<string, unknown>, value: string, path: string[]) {
let newPolicyConfig = obj;

// First set the value.
for (let i = 0; i < path.length - 1; i++) {
if (!newPolicyConfig[path[i]]) {
newPolicyConfig[path[i]] = {} as Record<string, unknown>;
}
newPolicyConfig = newPolicyConfig[path[i]] as Record<string, unknown>;
}
newPolicyConfig[path[path.length - 1]] = value;

// Then, if the user is deleting the value, we need to ensure we clean up the config.
// We delete any sections that are empty, whether that be an empty string, empty object, or undefined.
if (value === '' || value === undefined) {
newPolicyConfig = obj;
for (let k = path.length; k >= 0; k--) {
const nextPath = path.slice(0, k);
for (let i = 0; i < nextPath.length - 1; i++) {
// Traverse and find the next section
newPolicyConfig = newPolicyConfig[nextPath[i]] as Record<string, unknown>;
}
if (
newPolicyConfig[nextPath[nextPath.length - 1]] === undefined ||
newPolicyConfig[nextPath[nextPath.length - 1]] === '' ||
Object.keys(newPolicyConfig[nextPath[nextPath.length - 1]] as object).length === 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming this casting here is safe at runtime? will it always be an object if defined at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

Copy link
Contributor

Choose a reason for hiding this comment

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

'as object' ? Shouldn't it be 'as Object' ?

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 think both work. The type error I had on this went away after I added the cast

) {
// If we're looking at the `advanced` field, we leave it undefined as opposed to deleting it.
// This is because the UI looks for this field to begin rendering.
if (nextPath[nextPath.length - 1] === 'advanced') {
newPolicyConfig[nextPath[nextPath.length - 1]] = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading this right, it seems that if the key name is advanced we keep it, but set it to undefined, but any other key we delete it. Should the advanced key also be deleted? Or does that happen when you walk the path back to the prior level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the UI looks for an advanced key which will be initialized to undefined to start making decisions on how to render the form. This is essentially resetting if it's completely emptied out

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm also a little confused about the if/else logic here; maybe more comments would be clarifying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added more comments

// In all other cases, if field is empty, we'll delete it to clean up.
} else {
delete newPolicyConfig[nextPath[nextPath.length - 1]];
}
newPolicyConfig = obj;
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this assignment multiple times, what does it do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it updates the reference to the area in the config as we follow the given path. Say our path is linux.advanced.config - we start at linux and access advanced, then we need to update the reference to access config in the next iteration.

} else {
break; // We are looking at a non-empty section, so we can terminate.
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be covered by testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I can write a test case for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed up a test

}

function getValue(obj: Record<string, unknown>, path: string[]) {
Expand Down
281 changes: 281 additions & 0 deletions x-pack/test/security_solution_endpoint/apps/endpoint/policy_details.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,287 @@ export default function ({ getPageObjects, getService }: FtrProviderContext) {
},
]);
});

it('should have cleared the advanced section when the user deletes the value', async () => {
const advancedPolicyButton = await pageObjects.policy.findAdvancedPolicyButton();
await advancedPolicyButton.click();

const advancedPolicyField = await pageObjects.policy.findAdvancedPolicyField();
await advancedPolicyField.clearValue();
await advancedPolicyField.click();
await advancedPolicyField.type('true');
await pageObjects.policy.confirmAndSave();

await testSubjects.existOrFail('policyDetailsSuccessMessage');

const agentFullPolicy = await policyTestResources.getFullAgentPolicy(
policyInfo.agentPolicy.id
);

expect(agentFullPolicy.inputs).to.eql([
{
id: policyInfo.packagePolicy.id,
revision: 2,
data_stream: { namespace: 'default' },
name: 'Protect East Coast',
meta: {
package: {
name: 'endpoint',
version: policyInfo.packageInfo.version,
},
},
artifact_manifest: {
artifacts: {
'endpoint-exceptionlist-macos-v1': {
compression_algorithm: 'zlib',
decoded_sha256:
'd801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
decoded_size: 14,
encoded_sha256:
'f8e6afa1d5662f5b37f83337af774b5785b5b7f1daee08b7b00c2d6813874cda',
encoded_size: 22,
encryption_algorithm: 'none',
relative_url:
'/api/endpoint/artifacts/download/endpoint-exceptionlist-macos-v1/d801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
},
'endpoint-exceptionlist-windows-v1': {
compression_algorithm: 'zlib',
decoded_sha256:
'd801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
decoded_size: 14,
encoded_sha256:
'f8e6afa1d5662f5b37f83337af774b5785b5b7f1daee08b7b00c2d6813874cda',
encoded_size: 22,
encryption_algorithm: 'none',
relative_url:
'/api/endpoint/artifacts/download/endpoint-exceptionlist-windows-v1/d801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
},
'endpoint-trustlist-linux-v1': {
compression_algorithm: 'zlib',
decoded_sha256:
'd801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
decoded_size: 14,
encoded_sha256:
'f8e6afa1d5662f5b37f83337af774b5785b5b7f1daee08b7b00c2d6813874cda',
encoded_size: 22,
encryption_algorithm: 'none',
relative_url:
'/api/endpoint/artifacts/download/endpoint-trustlist-linux-v1/d801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
},
'endpoint-trustlist-macos-v1': {
compression_algorithm: 'zlib',
decoded_sha256:
'd801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
decoded_size: 14,
encoded_sha256:
'f8e6afa1d5662f5b37f83337af774b5785b5b7f1daee08b7b00c2d6813874cda',
encoded_size: 22,
encryption_algorithm: 'none',
relative_url:
'/api/endpoint/artifacts/download/endpoint-trustlist-macos-v1/d801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
},
'endpoint-trustlist-windows-v1': {
compression_algorithm: 'zlib',
decoded_sha256:
'd801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
decoded_size: 14,
encoded_sha256:
'f8e6afa1d5662f5b37f83337af774b5785b5b7f1daee08b7b00c2d6813874cda',
encoded_size: 22,
encryption_algorithm: 'none',
relative_url:
'/api/endpoint/artifacts/download/endpoint-trustlist-windows-v1/d801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
},
},
// The manifest version could have changed when the Policy was updated because the
// policy details page ensures that a save action applies the udpated policy on top
// of the latest Package Policy. So we just ignore the check against this value by
// forcing it to be the same as the value returned in the full agent policy.
manifest_version: agentFullPolicy.inputs[0].artifact_manifest.manifest_version,
schema_version: 'v1',
},
policy: {
linux: {
events: { file: true, network: true, process: true },
logging: { file: 'info' },
advanced: { agent: { connection_delay: 'true' } },
},
mac: {
events: { file: true, network: true, process: true },
logging: { file: 'info' },
malware: { mode: 'prevent' },
popup: {
malware: {
enabled: true,
message: 'Elastic Security { action } { filename }',
},
},
},
windows: {
events: {
dll_and_driver_load: true,
dns: true,
file: true,
network: true,
process: true,
registry: true,
security: true,
},
logging: { file: 'info' },
malware: { mode: 'prevent' },
popup: {
malware: {
enabled: true,
message: 'Elastic Security { action } { filename }',
},
},
antivirus_registration: {
enabled: false,
},
},
},
type: 'endpoint',
use_output: 'default',
},
]);

// Clear the value
await advancedPolicyField.click();
await advancedPolicyField.clearValueWithKeyboard();
await pageObjects.policy.confirmAndSave();

await testSubjects.existOrFail('policyDetailsSuccessMessage');

const agentFullPolicyUpdated = await policyTestResources.getFullAgentPolicy(
policyInfo.agentPolicy.id
);

expect(agentFullPolicyUpdated.inputs).to.eql([
{
id: policyInfo.packagePolicy.id,
revision: 3,
data_stream: { namespace: 'default' },
name: 'Protect East Coast',
meta: {
package: {
name: 'endpoint',
version: policyInfo.packageInfo.version,
},
},
artifact_manifest: {
artifacts: {
'endpoint-exceptionlist-macos-v1': {
compression_algorithm: 'zlib',
decoded_sha256:
'd801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
decoded_size: 14,
encoded_sha256:
'f8e6afa1d5662f5b37f83337af774b5785b5b7f1daee08b7b00c2d6813874cda',
encoded_size: 22,
encryption_algorithm: 'none',
relative_url:
'/api/endpoint/artifacts/download/endpoint-exceptionlist-macos-v1/d801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
},
'endpoint-exceptionlist-windows-v1': {
compression_algorithm: 'zlib',
decoded_sha256:
'd801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
decoded_size: 14,
encoded_sha256:
'f8e6afa1d5662f5b37f83337af774b5785b5b7f1daee08b7b00c2d6813874cda',
encoded_size: 22,
encryption_algorithm: 'none',
relative_url:
'/api/endpoint/artifacts/download/endpoint-exceptionlist-windows-v1/d801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
},
'endpoint-trustlist-linux-v1': {
compression_algorithm: 'zlib',
decoded_sha256:
'd801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
decoded_size: 14,
encoded_sha256:
'f8e6afa1d5662f5b37f83337af774b5785b5b7f1daee08b7b00c2d6813874cda',
encoded_size: 22,
encryption_algorithm: 'none',
relative_url:
'/api/endpoint/artifacts/download/endpoint-trustlist-linux-v1/d801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
},
'endpoint-trustlist-macos-v1': {
compression_algorithm: 'zlib',
decoded_sha256:
'd801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
decoded_size: 14,
encoded_sha256:
'f8e6afa1d5662f5b37f83337af774b5785b5b7f1daee08b7b00c2d6813874cda',
encoded_size: 22,
encryption_algorithm: 'none',
relative_url:
'/api/endpoint/artifacts/download/endpoint-trustlist-macos-v1/d801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
},
'endpoint-trustlist-windows-v1': {
compression_algorithm: 'zlib',
decoded_sha256:
'd801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
decoded_size: 14,
encoded_sha256:
'f8e6afa1d5662f5b37f83337af774b5785b5b7f1daee08b7b00c2d6813874cda',
encoded_size: 22,
encryption_algorithm: 'none',
relative_url:
'/api/endpoint/artifacts/download/endpoint-trustlist-windows-v1/d801aa1fb7ddcc330a5e3173372ea6af4a3d08ec58074478e85aa5603e926658',
},
},
// The manifest version could have changed when the Policy was updated because the
// policy details page ensures that a save action applies the udpated policy on top
// of the latest Package Policy. So we just ignore the check against this value by
// forcing it to be the same as the value returned in the full agent policy.
manifest_version: agentFullPolicy.inputs[0].artifact_manifest.manifest_version,
schema_version: 'v1',
},
policy: {
linux: {
events: { file: true, network: true, process: true },
logging: { file: 'info' },
},
mac: {
events: { file: true, network: true, process: true },
logging: { file: 'info' },
malware: { mode: 'prevent' },
popup: {
malware: {
enabled: true,
message: 'Elastic Security { action } { filename }',
},
},
},
windows: {
events: {
dll_and_driver_load: true,
dns: true,
file: true,
network: true,
process: true,
registry: true,
security: true,
},
logging: { file: 'info' },
malware: { mode: 'prevent' },
popup: {
malware: {
enabled: true,
message: 'Elastic Security { action } { filename }',
},
},
antivirus_registration: {
enabled: false,
},
},
},
type: 'endpoint',
use_output: 'default',
},
]);
});
});

describe('when on Ingest Policy Edit Package Policy page', async () => {
Expand Down