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

[SIEM] [Detection Engine] Remove has manage api keys requirement #62446

Merged
merged 9 commits into from
Apr 4, 2020
Merged

[SIEM] [Detection Engine] Remove has manage api keys requirement #62446

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
03b0e2f
removes hasManageApiKeys since alerting is using the internal user ap…
dhurley14 Apr 2, 2020
d1ee771
linting error
dhurley14 Apr 2, 2020
48785aa
fixes types and removes a test for manage api keys
dhurley14 Apr 3, 2020
fd83c50
removes manage api key reducer and updates leftover tests
dhurley14 Apr 3, 2020
c6858d9
Merge branch 'master' into remove-has-manage-api-keys-req
elasticmachine Apr 3, 2020
6507f72
moves userHasNoPermissions repeated code into a function in helpers, …
dhurley14 Apr 3, 2020
ad80e2d
fix test title
dhurley14 Apr 3, 2020
93912e7
remove userHasNoPermissions function and remove tests, replace with j…
dhurley14 Apr 3, 2020
4b0b8f0
Revert "remove userHasNoPermissions function and remove tests, replac…
dhurley14 Apr 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
31 changes: 0 additions & 31 deletions ...acy/plugins/siem/public/containers/detection_engine/rules/use_pre_packaged_rules.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ describe('usePersistRule', () => {
usePrePackagedRules({
canUserCRUD: null,
hasIndexWrite: null,
hasManageApiKey: null,
isAuthenticated: null,
hasEncryptionKey: null,
isSignalIndexExists: null,
Expand Down Expand Up @@ -50,7 +49,6 @@ describe('usePersistRule', () => {
usePrePackagedRules({
canUserCRUD: null,
hasIndexWrite: null,
hasManageApiKey: null,
isAuthenticated: null,
hasEncryptionKey: null,
isSignalIndexExists: null,
Expand Down Expand Up @@ -79,7 +77,6 @@ describe('usePersistRule', () => {
usePrePackagedRules({
canUserCRUD: true,
hasIndexWrite: true,
hasManageApiKey: true,
isAuthenticated: true,
hasEncryptionKey: true,
isSignalIndexExists: true,
Expand Down Expand Up @@ -116,7 +113,6 @@ describe('usePersistRule', () => {
usePrePackagedRules({
canUserCRUD: true,
hasIndexWrite: true,
hasManageApiKey: true,
isAuthenticated: true,
hasEncryptionKey: true,
isSignalIndexExists: true,
Expand All @@ -139,7 +135,6 @@ describe('usePersistRule', () => {
usePrePackagedRules({
canUserCRUD: false,
hasIndexWrite: true,
hasManageApiKey: true,
isAuthenticated: true,
hasEncryptionKey: true,
isSignalIndexExists: true,
Expand All @@ -161,29 +156,6 @@ describe('usePersistRule', () => {
usePrePackagedRules({
canUserCRUD: true,
hasIndexWrite: false,
hasManageApiKey: true,
isAuthenticated: true,
hasEncryptionKey: true,
isSignalIndexExists: true,
})
);
await waitForNextUpdate();
await waitForNextUpdate();
let resp = null;
if (result.current.createPrePackagedRules) {
resp = await result.current.createPrePackagedRules();
}
expect(resp).toEqual(false);
});
});

test('can NOT createPrePackagedRules because hasManageApiKey === false', async () => {
await act(async () => {
const { result, waitForNextUpdate } = renderHook<unknown, ReturnPrePackagedRules>(() =>
usePrePackagedRules({
canUserCRUD: true,
hasIndexWrite: true,
hasManageApiKey: false,
isAuthenticated: true,
hasEncryptionKey: true,
isSignalIndexExists: true,
Expand All @@ -205,7 +177,6 @@ describe('usePersistRule', () => {
usePrePackagedRules({
canUserCRUD: true,
hasIndexWrite: true,
hasManageApiKey: true,
isAuthenticated: false,
hasEncryptionKey: true,
isSignalIndexExists: true,
Expand All @@ -227,7 +198,6 @@ describe('usePersistRule', () => {
usePrePackagedRules({
canUserCRUD: true,
hasIndexWrite: true,
hasManageApiKey: true,
isAuthenticated: true,
hasEncryptionKey: false,
isSignalIndexExists: true,
Expand All @@ -249,7 +219,6 @@ describe('usePersistRule', () => {
usePrePackagedRules({
canUserCRUD: true,
hasIndexWrite: true,
hasManageApiKey: true,
isAuthenticated: true,
hasEncryptionKey: true,
isSignalIndexExists: false,
Expand Down
13 changes: 1 addition & 12 deletions ...k/legacy/plugins/siem/public/containers/detection_engine/rules/use_pre_packaged_rules.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ export interface ReturnPrePackagedRules {
interface UsePrePackagedRuleProps {
canUserCRUD: boolean | null;
hasIndexWrite: boolean | null;
hasManageApiKey: boolean | null;
isAuthenticated: boolean | null;
hasEncryptionKey: boolean | null;
isSignalIndexExists: boolean | null;
Expand All @@ -36,7 +35,6 @@ interface UsePrePackagedRuleProps {
* Hook for using to get status about pre-packaged Rules from the Detection Engine API
*
* @param hasIndexWrite boolean
* @param hasManageApiKey boolean
* @param isAuthenticated boolean
* @param hasEncryptionKey boolean
* @param isSignalIndexExists boolean
Expand All @@ -45,7 +43,6 @@ interface UsePrePackagedRuleProps {
export const usePrePackagedRules = ({
canUserCRUD,
hasIndexWrite,
hasManageApiKey,
isAuthenticated,
hasEncryptionKey,
isSignalIndexExists,
Expand Down Expand Up @@ -117,7 +114,6 @@ export const usePrePackagedRules = ({
if (
canUserCRUD &&
hasIndexWrite &&
hasManageApiKey &&
isAuthenticated &&
hasEncryptionKey &&
isSignalIndexExists
Expand Down Expand Up @@ -185,14 +181,7 @@ export const usePrePackagedRules = ({
isSubscribed = false;
abortCtrl.abort();
};
}, [
canUserCRUD,
hasIndexWrite,
hasManageApiKey,
isAuthenticated,
hasEncryptionKey,
isSignalIndexExists,
]);
}, [canUserCRUD, hasIndexWrite, isAuthenticated, hasEncryptionKey, isSignalIndexExists]);

return {
loading,
Expand Down
1 change: 0 additions & 1 deletion x-pack/legacy/plugins/siem/public/containers/detection_engine/signals/mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,7 +992,6 @@ export const mockUserPrivilege: Privilege = {
monitor_watcher: true,
monitor_transform: true,
read_ilm: true,
manage_api_key: true,
manage_security: true,
manage_own_api_key: false,
manage_saml: true,
Expand Down
1 change: 0 additions & 1 deletion x-pack/legacy/plugins/siem/public/containers/detection_engine/signals/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ export interface Privilege {
monitor_watcher: boolean;
monitor_transform: boolean;
read_ilm: boolean;
manage_api_key: boolean;
manage_security: boolean;
manage_own_api_key: boolean;
manage_saml: boolean;
Expand Down
3 changes: 0 additions & 3 deletions ...egacy/plugins/siem/public/containers/detection_engine/signals/use_privilege_user.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ describe('usePrivilegeUser', () => {
hasEncryptionKey: null,
hasIndexManage: null,
hasIndexWrite: null,
hasManageApiKey: null,
isAuthenticated: null,
loading: true,
});
Expand All @@ -39,7 +38,6 @@ describe('usePrivilegeUser', () => {
hasEncryptionKey: true,
hasIndexManage: true,
hasIndexWrite: true,
hasManageApiKey: true,
isAuthenticated: true,
loading: false,
});
Expand All @@ -61,7 +59,6 @@ describe('usePrivilegeUser', () => {
hasEncryptionKey: false,
hasIndexManage: false,
hasIndexWrite: false,
hasManageApiKey: false,
isAuthenticated: false,
loading: false,
});
Expand Down
13 changes: 1 addition & 12 deletions x-pack/legacy/plugins/siem/public/containers/detection_engine/signals/use_privilege_user.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ export interface ReturnPrivilegeUser {
isAuthenticated: boolean | null;
hasEncryptionKey: boolean | null;
hasIndexManage: boolean | null;
hasManageApiKey: boolean | null;
hasIndexWrite: boolean | null;
}
/**
Expand All @@ -27,17 +26,12 @@ export const usePrivilegeUser = (): ReturnPrivilegeUser => {
const [privilegeUser, setPrivilegeUser] = useState<
Pick<
ReturnPrivilegeUser,
| 'isAuthenticated'
| 'hasEncryptionKey'
| 'hasIndexManage'
| 'hasManageApiKey'
| 'hasIndexWrite'
'isAuthenticated' | 'hasEncryptionKey' | 'hasIndexManage' | 'hasIndexWrite'
>
>({
isAuthenticated: null,
hasEncryptionKey: null,
hasIndexManage: null,
hasManageApiKey: null,
hasIndexWrite: null,
});
const [, dispatchToaster] = useStateToaster();
Expand Down Expand Up @@ -65,10 +59,6 @@ export const usePrivilegeUser = (): ReturnPrivilegeUser => {
privilege.index[indexName].create_doc ||
privilege.index[indexName].index ||
privilege.index[indexName].write,
hasManageApiKey:
privilege.cluster.manage_security ||
privilege.cluster.manage_api_key ||
privilege.cluster.manage_own_api_key,
});
}
}
Expand All @@ -78,7 +68,6 @@ export const usePrivilegeUser = (): ReturnPrivilegeUser => {
isAuthenticated: false,
hasEncryptionKey: false,
hasIndexManage: false,
hasManageApiKey: false,
hasIndexWrite: false,
});
errorToToaster({ title: i18n.PRIVILEGE_FETCH_FAILURE, error, dispatchToaster });
Expand Down
21 changes: 0 additions & 21 deletions x-pack/legacy/plugins/siem/public/pages/detection_engine/components/user_info/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ export interface State {
canUserCRUD: boolean | null;
hasIndexManage: boolean | null;
hasIndexWrite: boolean | null;
hasManageApiKey: boolean | null;
isSignalIndexExists: boolean | null;
isAuthenticated: boolean | null;
hasEncryptionKey: boolean | null;
Expand All @@ -27,7 +26,6 @@ const initialState: State = {
canUserCRUD: null,
hasIndexManage: null,
hasIndexWrite: null,
hasManageApiKey: null,
isSignalIndexExists: null,
isAuthenticated: null,
hasEncryptionKey: null,
Expand All @@ -37,10 +35,6 @@ const initialState: State = {

export type Action =
| { type: 'updateLoading'; loading: boolean }
| {
type: 'updateHasManageApiKey';
hasManageApiKey: boolean | null;
}
| {
type: 'updateHasIndexManage';
hasIndexManage: boolean | null;
Expand Down Expand Up @@ -90,12 +84,6 @@ export const userInfoReducer = (state: State, action: Action): State => {
hasIndexWrite: action.hasIndexWrite,
};
}
case 'updateHasManageApiKey': {
return {
...state,
hasManageApiKey: action.hasManageApiKey,
};
}
case 'updateIsSignalIndexExists': {
return {
...state,
Expand Down Expand Up @@ -151,7 +139,6 @@ export const useUserInfo = (): State => {
canUserCRUD,
hasIndexManage,
hasIndexWrite,
hasManageApiKey,
isSignalIndexExists,
isAuthenticated,
hasEncryptionKey,
Expand All @@ -166,7 +153,6 @@ export const useUserInfo = (): State => {
hasEncryptionKey: isApiEncryptionKey,
hasIndexManage: hasApiIndexManage,
hasIndexWrite: hasApiIndexWrite,
hasManageApiKey: hasApiManageApiKey,
} = usePrivilegeUser();
const {
loading: indexNameLoading,
Expand Down Expand Up @@ -197,12 +183,6 @@ export const useUserInfo = (): State => {
}
}, [loading, hasIndexWrite, hasApiIndexWrite]);

useEffect(() => {
if (!loading && hasManageApiKey !== hasApiManageApiKey && hasApiManageApiKey != null) {
dispatch({ type: 'updateHasManageApiKey', hasManageApiKey: hasApiManageApiKey });
}
}, [loading, hasManageApiKey, hasApiManageApiKey]);

useEffect(() => {
if (
!loading &&
Expand Down Expand Up @@ -258,7 +238,6 @@ export const useUserInfo = (): State => {
canUserCRUD,
hasIndexManage,
hasIndexWrite,
hasManageApiKey,
signalIndexName,
};
};
4 changes: 1 addition & 3 deletions x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/create/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ const CreateRulePageComponent: React.FC = () => {
isAuthenticated,
hasEncryptionKey,
canUserCRUD,
hasManageApiKey,
} = useUserInfo();
const [, dispatchToaster] = useStateToaster();
const [openAccordionId, setOpenAccordionId] = useState<RuleStep>(RuleStep.defineRule);
Expand Down Expand Up @@ -117,8 +116,7 @@ const CreateRulePageComponent: React.FC = () => {
getActionMessageParams((stepsData.current['define-rule'].data as DefineStepRule).ruleType),
[stepsData.current['define-rule'].data]
);
const userHasNoPermissions =
canUserCRUD != null && hasManageApiKey != null ? !canUserCRUD || !hasManageApiKey : false;
const userHasNoPermissions = canUserCRUD != null ? !canUserCRUD : false;
Copy link
Contributor

@FrankHassanabad FrankHassanabad Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This small chunk of code should have test coverage and this should be potentially a pure function.

I see this chunk being copied in 4 places which is bad. I would pull this out into a pure function in a utils/helper, add tests and then call it here and below.

Copy link
Contributor

@XavierM XavierM Apr 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

what about just using !canUserCRUD now and you already have test for that!
Bad idea because the UX will be worst, always follow @FrankHassanabad suggestions


const setStepData = useCallback(
(step: RuleStep, data: unknown, isValid: boolean) => {
Expand Down
4 changes: 1 addition & 3 deletions x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/details/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ const RuleDetailsPageComponent: FC<PropsFromRedux> = ({
isAuthenticated,
hasEncryptionKey,
canUserCRUD,
hasManageApiKey,
hasIndexWrite,
signalIndexName,
} = useUserInfo();
Expand All @@ -115,8 +114,7 @@ const RuleDetailsPageComponent: FC<PropsFromRedux> = ({
scheduleRuleData: null,
};
const [lastSignals] = useSignalInfo({ ruleId });
const userHasNoPermissions =
canUserCRUD != null && hasManageApiKey != null ? !canUserCRUD || !hasManageApiKey : false;
const userHasNoPermissions = canUserCRUD != null ? !canUserCRUD : false;

const title = isLoading === true || rule === null ? <EuiLoadingSpinner size="m" /> : rule.name;
const subTitle = useMemo(
Expand Down
4 changes: 1 addition & 3 deletions x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/edit/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,11 @@ const EditRulePageComponent: FC = () => {
isAuthenticated,
hasEncryptionKey,
canUserCRUD,
hasManageApiKey,
} = useUserInfo();
const { detailName: ruleId } = useParams();
const [loading, rule] = useRule(ruleId);

const userHasNoPermissions =
canUserCRUD != null && hasManageApiKey != null ? !canUserCRUD || !hasManageApiKey : false;
const userHasNoPermissions = canUserCRUD != null ? !canUserCRUD : false;

const [initForm, setInitForm] = useState(false);
const [myAboutRuleForm, setMyAboutRuleForm] = useState<AboutStepRuleForm>({
Expand Down
5 changes: 1 addition & 4 deletions x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ const RulesPageComponent: React.FC = () => {
hasEncryptionKey,
canUserCRUD,
hasIndexWrite,
hasManageApiKey,
} = useUserInfo();
const {
createPrePackagedRules,
Expand All @@ -52,7 +51,6 @@ const RulesPageComponent: React.FC = () => {
} = usePrePackagedRules({
canUserCRUD,
hasIndexWrite,
hasManageApiKey,
isSignalIndexExists,
isAuthenticated,
hasEncryptionKey,
Expand All @@ -63,8 +61,7 @@ const RulesPageComponent: React.FC = () => {
rulesNotUpdated
);

const userHasNoPermissions =
canUserCRUD != null && hasManageApiKey != null ? !canUserCRUD || !hasManageApiKey : false;
const userHasNoPermissions = canUserCRUD != null ? !canUserCRUD : false;

const handleRefreshRules = useCallback(async () => {
if (refreshRulesData.current != null) {
Expand Down