-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
task/mac-eventing-form #62999
Changes from 11 commits
e11e19f
43c6a10
3d9e34c
d9c77ac
c12e150
d413a05
9fdfe77
0e8aa04
817c4bb
bf65411
47ac630
2e636ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
|
@@ -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); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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('windows process eventing is enabled', () => { | ||
const config = policyConfig(getState()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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'; | ||
|
||
|
@@ -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) }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. instead of any can't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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] = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'] & { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. curious to know why this was needed There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
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; | ||
|
@@ -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 */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why disable this here? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -203,6 +200,7 @@ export enum OS { | |
export enum EventingFields { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is used anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i believe you are right! |
||
process = 'process', | ||
network = 'network', | ||
file = 'file', | ||
} | ||
|
||
/** | ||
|
This file was deleted.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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