Skip to content

[Security Solution][Detections] Loosen lists permissions #75378

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

Merged
merged 3 commits into from
Aug 19, 2020
Merged

[Security Solution][Detections] Loosen lists permissions #75378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
06e554e
Allow read-only lists users to use Detections if lists indexes exist
rylnd Aug 18, 2020
a119d84
Disable the Value Lists modal when the user cannot write lists
rylnd Aug 18, 2020
f856762
style: Remove unnecessary useCallbacks
rylnd Aug 18, 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
2 changes: 1 addition & 1 deletion ...ecurity_solution/public/detections/containers/detection_engine/lists/use_lists_config.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export const useListsConfig = (): UseListsConfigReturn => {
const hasIndexError = indexError != null;
const needsIndexConfiguration =
needsIndex && (canManageIndex === false || (canManageIndex === true && hasIndexError));
const needsConfiguration = !enabled || canWriteIndex === false || needsIndexConfiguration;
const needsConfiguration = !enabled || needsIndexConfiguration;
Copy link
Contributor

@FrankHassanabad FrankHassanabad Aug 19, 2020

Choose a reason for hiding this comment

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

Only suggestions I would have would be if we can have tests around this area that would be pretty nice to add to the documentation and help future maintainers.

return { canManageIndex, canWriteIndex, enabled, loading, needsConfiguration };

This looks really good in that later we can push this to the "needs permissions" and make actionable UI's to tell the user what they're missing exactly and how to fix it. So this was really nice to see here.

Copy link
Contributor Author

@rylnd rylnd Aug 19, 2020

Choose a reason for hiding this comment

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

I wholeheartedly agree on the tests. I had been waiting to write a functional test for "read-only lists user can use Detections," but in the interim I will add some unit tests around these hooks in a followup PR 👍


useEffect(() => {
if (needsIndex && canManageIndex) {
Expand Down
14 changes: 8 additions & 6 deletions x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@ type Func = (refreshPrePackagedRule?: boolean) => void;
const RulesPageComponent: React.FC = () => {
const history = useHistory();
const [showImportModal, setShowImportModal] = useState(false);
const [isValueListsModalShown, setIsValueListsModalShown] = useState(false);
const showValueListsModal = useCallback(() => setIsValueListsModalShown(true), []);
const hideValueListsModal = useCallback(() => setIsValueListsModalShown(false), []);
const [showValueListsModal, setShowValueListsModal] = useState(false);
const refreshRulesData = useRef<null | Func>(null);
const {
loading: userInfoLoading,
Expand All @@ -54,6 +52,7 @@ const RulesPageComponent: React.FC = () => {
} = useUserInfo();
const {
loading: listsConfigLoading,
canWriteIndex: canWriteListsIndex,
needsConfiguration: needsListsConfiguration,
} = useListsConfig();
const loading = userInfoLoading || listsConfigLoading;
Expand Down Expand Up @@ -147,7 +146,10 @@ const RulesPageComponent: React.FC = () => {
return (
<>
{userHasNoPermissions(canUserCRUD) && <ReadOnlyCallOut />}
<ValueListsModal showModal={isValueListsModalShown} onClose={hideValueListsModal} />
<ValueListsModal
showModal={showValueListsModal}
onClose={() => setShowValueListsModal(false)}
/>
<ImportDataModal
checkBoxLabel={i18n.OVERWRITE_WITH_SAME_NAME}
closeModal={() => setShowImportModal(false)}
Expand Down Expand Up @@ -208,8 +210,8 @@ const RulesPageComponent: React.FC = () => {
<EuiButton
data-test-subj="open-value-lists-modal-button"
iconType="importAction"
isDisabled={userHasNoPermissions(canUserCRUD) || loading}
onClick={showValueListsModal}
isDisabled={!canWriteListsIndex || loading}
onClick={() => setShowValueListsModal(true)}
>
{i18n.UPLOAD_VALUE_LISTS}
</EuiButton>
Expand Down