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

task/mac-eventing-form #62999

Merged
merged 12 commits into from
Apr 10, 2020
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ export const generatePolicy = (): PolicyConfig => {
mac: {
events: {
process: true,
file: true,
network: true,
},
malware: {
mode: ProtectionModes.detect,
Expand All @@ -67,6 +69,8 @@ export const generatePolicy = (): PolicyConfig => {
linux: {
events: {
process: true,
file: true,
network: true,
},
logging: {
stdout: 'debug',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,33 @@ export function clone(policyDetailsConfig: UIPolicyConfig): UIPolicyConfig {
*/
return clonedConfig as UIPolicyConfig;
}

/**
* Returns value from `configuration`
*/
export const getIn = (a: UIPolicyConfig) => <Key extends keyof UIPolicyConfig>(key: Key) => <
Copy link
Contributor

Choose a reason for hiding this comment

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

this works to keep us type compliant when we have different combinations of keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this was to make sure the types were still getting passed through correctly

subKey extends keyof UIPolicyConfig[Key]
>(
subKey: subKey
) => <LeafKey extends keyof UIPolicyConfig[Key][subKey]>(
leafKey: LeafKey
): UIPolicyConfig[Key][subKey][LeafKey] => {
return a[key][subKey][leafKey];
};

/**
* Returns cloned `configuration` with `value` set by the `keyPath`.
*/
export const setIn = (a: UIPolicyConfig) => <Key extends keyof UIPolicyConfig>(key: Key) => <
subKey extends keyof UIPolicyConfig[Key]
>(
subKey: subKey
) => <LeafKey extends keyof UIPolicyConfig[Key][subKey]>(leafKey: LeafKey) => <
V extends UIPolicyConfig[Key][subKey][LeafKey]
>(
v: V
): UIPolicyConfig => {
const c = clone(a);
c[key][subKey][leafKey] = v;
return c;
};
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { PolicyDetailsState } from '../../types';
import { createStore, Dispatch, Store } from 'redux';
import { policyDetailsReducer, PolicyDetailsAction } from './index';
import { policyConfig, windowsEventing } from './selectors';
import { policyConfig } from './selectors';
import { clone } from '../../models/policy_details_config';
import { generatePolicy } from '../../models/policy';

Expand Down Expand Up @@ -55,7 +55,7 @@ describe('policy details: ', () => {
});
});

describe('when the user has enabled windows process eventing', () => {
describe('when the user has enabled windows process events', () => {
beforeEach(() => {
const config = policyConfig(getState());
if (!config) {
Expand All @@ -71,8 +71,31 @@ describe('policy details: ', () => {
});
});

it('windows process eventing is enabled', async () => {
expect(windowsEventing(getState())!.process).toEqual(true);
it('windows process events is enabled', () => {
const config = policyConfig(getState());
expect(config!.windows.events.process).toEqual(true);
});

Choose a reason for hiding this comment

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

@parkiino - Looks good! Will we also be adding a test here for the network eventing for Windows. This PR says mac eventing, are we also going to add tests for Mac here? Or did you want me to execute those manually for now until we can automate them?

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 can add a test for mac as well

});

describe('when the user has enabled mac file events', () => {
beforeEach(() => {
const config = policyConfig(getState());
if (!config) {
throw new Error();
}

const newPayload1 = clone(config);
newPayload1.mac.events.file = true;

dispatch({
type: 'userChangedPolicyConfig',
payload: { policyConfig: newPayload1 },
});
});

it('mac file events is enabled', () => {
const config = policyConfig(getState());

Choose a reason for hiding this comment

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

@parkiino - can we update the it to say "mac" process instead of windows?

Choose a reason for hiding this comment

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

@parkiino - also the eventing options in the AC are different from the mocks and implementation. Let's chat at stand up to see which ones we need to write tests for.

expect(config!.mac.events.file).toEqual(true);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { Reducer } from 'redux';
import { PolicyData, PolicyDetailsState, UIPolicyConfig } from '../../types';
import { PolicyDetailsState, UIPolicyConfig } from '../../types';
import { AppAction } from '../action';
import { fullPolicy, isOnPolicyDetailsPage } from './selectors';

Expand Down Expand Up @@ -89,10 +89,12 @@ export const policyDetailsReducer: Reducer<PolicyDetailsState, AppAction> = (
}

if (action.type === 'userChangedPolicyConfig') {
const newState = { ...state, policyItem: { ...(state.policyItem as PolicyData) } };
const newPolicy = (newState.policyItem.inputs[0].config.policy.value = {
...fullPolicy(state),
});
if (!state.policyItem) {
return state;
}
const newState = { ...state, policyItem: { ...state.policyItem } };
const newPolicy: any = { ...fullPolicy(state) };
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for some reason typecheck complains on lines 99-102 because it doesn't like how windows has a different number of events (process, network) than mac and linux (process, network and file). so any resolves that for now

newState.policyItem.inputs[0].config.policy.value = newPolicy;

Object.entries(action.payload.policyConfig).forEach(([section, newSettings]) => {
newPolicy[section as keyof UIPolicyConfig] = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,8 @@ export const policyConfig: (s: PolicyDetailsState) => UIPolicyConfig = createSel
}
);

/** Returns an object of all the windows eventing configuration */
export const windowsEventing = (state: PolicyDetailsState) => {
const config = policyConfig(state);
return config && config.windows.events;
};

/** Returns the total number of possible windows eventing configurations */
export const totalWindowsEventing = (state: PolicyDetailsState): number => {
export const totalWindowsEvents = (state: PolicyDetailsState): number => {
const config = policyConfig(state);
if (config) {
return Object.keys(config.windows.events).length;
Expand All @@ -95,7 +89,7 @@ export const totalWindowsEventing = (state: PolicyDetailsState): number => {
};

/** Returns the number of selected windows eventing configurations */
export const selectedWindowsEventing = (state: PolicyDetailsState): number => {
export const selectedWindowsEvents = (state: PolicyDetailsState): number => {
const config = policyConfig(state);
if (config) {
return Object.values(config.windows.events).reduce((count, event) => {
Expand All @@ -105,6 +99,26 @@ export const selectedWindowsEventing = (state: PolicyDetailsState): number => {
return 0;
};

/** Returns the total number of possible mac eventing configurations */
export const totalMacEvents = (state: PolicyDetailsState): number => {
const config = policyConfig(state);
if (config) {
return Object.keys(config.mac.events).length;
}
return 0;
};

/** Returns the number of selected mac eventing configurations */
export const selectedMacEvents = (state: PolicyDetailsState): number => {
const config = policyConfig(state);
if (config) {
return Object.values(config.mac.events).reduce((count, event) => {
return event === true ? count + 1 : count;
}, 0);
}
return 0;
};

/** is there an api call in flight */
export const isLoading = (state: PolicyDetailsState) => state.isLoading;

Expand Down
70 changes: 34 additions & 36 deletions x-pack/plugins/endpoint/public/applications/endpoint/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,34 +118,21 @@ export interface PolicyDetailsState {
* Endpoint Policy configuration
*/
export interface PolicyConfig {
windows: {
events: {
process: boolean;
network: boolean;
};
/** malware mode can be off, detect, prevent or prevent and notify user */
malware: MalwareFields;
windows: UIPolicyConfig['windows'] & {
Copy link
Contributor

Choose a reason for hiding this comment

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

curious to know why this was needed

Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying random stuff. Don't think the change is needed

logging: {
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 you use this same format 3 times here, could be a candidate to abstract

{
  logging: {
     stdout: string;
     file: string;
  }
  advanced: PolicyConfigAdvancedOptions;
}

stdout: string;
file: string;
};
advanced: PolicyConfigAdvancedOptions;
};
mac: {
events: {
process: boolean;
};
malware: MalwareFields;
mac: UIPolicyConfig['mac'] & {
logging: {
stdout: string;
file: string;
};
advanced: PolicyConfigAdvancedOptions;
};
linux: {
events: {
process: boolean;
};
linux: UIPolicyConfig['linux'] & {
logging: {
stdout: string;
file: string;
Expand All @@ -168,29 +155,39 @@ interface PolicyConfigAdvancedOptions {
};
}

/**
* Windows-specific policy configuration that is supported via the UI
*/
type WindowsPolicyConfig = Pick<PolicyConfig['windows'], 'events' | 'malware'>;

/**
* Mac-specific policy configuration that is supported via the UI
*/
type MacPolicyConfig = Pick<PolicyConfig['mac'], 'malware' | 'events'>;

/**
* Linux-specific policy configuration that is supported via the UI
*/
type LinuxPolicyConfig = Pick<PolicyConfig['linux'], 'events'>;

/**
* The set of Policy configuration settings that are show/edited via the UI
*/
export interface UIPolicyConfig {
windows: WindowsPolicyConfig;
mac: MacPolicyConfig;
linux: LinuxPolicyConfig;
}
/* eslint-disable @typescript-eslint/consistent-type-definitions */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why disable this here?

Copy link
Contributor

Choose a reason for hiding this comment

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

not needed. was just trying random stuff because typescript was beating us up

export type UIPolicyConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you ever use these fields all together and not just selecting one type from the UIPolicyConfig (e.g. UIPolicyConfig['linux'])? What's the benefit of doing it this way as opposed to just having 3 separate types?

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 as of now, there is one form that changes malware protections for both windows and mac. so it grabs the entire policyconfig and returns back the changed malware protection for both mac and windows. There will probably be other protections like this in the future

windows: {
events: {
process: boolean;
network: boolean;
};
/** malware mode can be off, detect, prevent or prevent and notify user */
malware: MalwareFields;
};
mac: {
events: {
file: boolean;
process: boolean;
network: boolean;
};
malware: MalwareFields;
};

/**
* Linux-specific policy configuration that is supported via the UI
*/
linux: {
events: {
file: boolean;
process: boolean;
network: boolean;
};
};
};

/** OS used in Policy */
export enum OS {
Expand All @@ -203,6 +200,7 @@ export enum OS {
export enum EventingFields {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is used anymore

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 believe you are right!

process = 'process',
network = 'network',
file = 'file',
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,12 @@ import {
isLoading,
apiError,
} from '../../store/policy_details/selectors';
import { WindowsEventing } from './policy_forms/eventing/windows';
import { PageView, PageViewHeaderTitle } from '../../components/page_view';
import { AppAction } from '../../types';
import { useKibana } from '../../../../../../../../src/plugins/kibana_react/public';
import { AgentsSummary } from './agents_summary';
import { VerticalDivider } from './vertical_divider';
import { WindowsEvents, MacEvents } from './policy_forms/events';
import { MalwareProtections } from './policy_forms/protections/malware';

export const PolicyDetails = React.memo(() => {
Expand Down Expand Up @@ -206,7 +206,9 @@ export const PolicyDetails = React.memo(() => {
</h4>
</EuiText>
<EuiSpacer size="xs" />
<WindowsEventing />
<WindowsEvents />
<EuiSpacer size="l" />
<MacEvents />
</PageView>
</>
);
Expand Down

This file was deleted.

Loading