diff --git a/changes/12751-cannot-save-invalid-sql b/changes/12751-cannot-save-invalid-sql new file mode 100644 index 0000000000..d9cadcbf42 --- /dev/null +++ b/changes/12751-cannot-save-invalid-sql @@ -0,0 +1 @@ +- Disable save button for invalid query or policy sql & missing name diff --git a/frontend/__mocks__/queryMock.ts b/frontend/__mocks__/queryMock.ts index e0ccc839bb..df90eae93a 100644 --- a/frontend/__mocks__/queryMock.ts +++ b/frontend/__mocks__/queryMock.ts @@ -1,6 +1,6 @@ -import { IQuery } from "interfaces/query"; +import { ISchedulableQuery } from "interfaces/schedulable_query"; -const DEFAULT_QUERY_MOCK: IQuery = { +const DEFAULT_QUERY_MOCK: ISchedulableQuery = { created_at: "2022-11-03T17:22:14Z", updated_at: "2022-11-03T17:22:14Z", id: 1, @@ -12,10 +12,25 @@ const DEFAULT_QUERY_MOCK: IQuery = { author_name: "Test User", author_email: "test@example.com", observer_can_run: false, + interval: 300, packs: [], + team_id: null, + platform: "", + min_osquery_version: "", + automations_enabled: false, + logging: "snapshot", + stats: { + user_time_p50: 0, + user_time_p95: 2, + system_time_p50: 0, + system_time_p95: 1, + total_executions: 6, + }, }; -const createMockQuery = (overrides?: Partial): IQuery => { +const createMockQuery = ( + overrides?: Partial +): ISchedulableQuery => { return { ...DEFAULT_QUERY_MOCK, ...overrides }; }; diff --git a/frontend/context/policy.tsx b/frontend/context/policy.tsx index ad8c1f43b7..1e1bf424dc 100644 --- a/frontend/context/policy.tsx +++ b/frontend/context/policy.tsx @@ -75,6 +75,8 @@ const initTable = osqueryTables.find((table) => table.name === "users") || DEFAULT_OSQUERY_TABLE; +export type IPolicyContext = InitialStateType; + const initialState = { lastEditedQueryId: null, lastEditedQueryName: "", diff --git a/frontend/context/query.tsx b/frontend/context/query.tsx index 9c3b401c70..f5778a9973 100644 --- a/frontend/context/query.tsx +++ b/frontend/context/query.tsx @@ -34,6 +34,8 @@ type InitialStateType = { setSelectedOsqueryTable: (tableName: string) => void; }; +export type IQueryContext = InitialStateType; + const initialState = { selectedOsqueryTable: find(osqueryTables, { name: "users" }) || DEFAULT_OSQUERY_TABLE, diff --git a/frontend/pages/policies/PolicyPage/components/PolicyForm/PolicyForm.tests.tsx b/frontend/pages/policies/PolicyPage/components/PolicyForm/PolicyForm.tests.tsx new file mode 100644 index 0000000000..e2b53d3d9b --- /dev/null +++ b/frontend/pages/policies/PolicyPage/components/PolicyForm/PolicyForm.tests.tsx @@ -0,0 +1,132 @@ +import React from "react"; +import { screen } from "@testing-library/react"; +import { createCustomRenderer } from "test/test-utils"; + +import createMockPolicy from "__mocks__/policyMock"; +import createMockUser from "__mocks__/userMock"; + +import PolicyForm from "./PolicyForm"; + +const mockPolicy = createMockPolicy(); + +describe("PolicyForm - component", () => { + it("disables save button for missing policy name", async () => { + const render = createCustomRenderer({ + context: { + policy: { + policyTeamId: undefined, + lastEditedQueryId: mockPolicy.id, + lastEditedQueryName: "", // missing policy name + lastEditedQueryDescription: mockPolicy.description, + lastEditedQueryBody: mockPolicy.query, + lastEditedQueryResolution: mockPolicy.resolution, + lastEditedQueryCritical: mockPolicy.critical, + lastEditedQueryPlatform: mockPolicy.platform, + defaultPolicy: false, + setLastEditedQueryName: jest.fn(), + setLastEditedQueryDescription: jest.fn(), + setLastEditedQueryBody: jest.fn(), + setLastEditedQueryResolution: jest.fn(), + setLastEditedQueryCritical: jest.fn(), + setLastEditedQueryPlatform: jest.fn(), + }, + app: { + currentUser: createMockUser(), + isGlobalObserver: false, + isGlobalAdmin: true, + isGlobalMaintainer: false, + isOnGlobalTeam: true, + isPremiumTier: true, + isSandboxMode: false, + }, + }, + }); + + render( + + ); + + expect(screen.getByRole("button", { name: "Save" })).toBeDisabled(); + }); + + it("disables save and run button with tooltip for missing policy platforms", async () => { + const render = createCustomRenderer({ + context: { + policy: { + policyTeamId: undefined, + lastEditedQueryId: mockPolicy.id, + lastEditedQueryName: mockPolicy.name, + lastEditedQueryDescription: mockPolicy.description, + lastEditedQueryBody: mockPolicy.query, + lastEditedQueryResolution: mockPolicy.resolution, + lastEditedQueryCritical: mockPolicy.critical, + lastEditedQueryPlatform: undefined, // missing policy platforms + defaultPolicy: false, + setLastEditedQueryName: jest.fn(), + setLastEditedQueryDescription: jest.fn(), + setLastEditedQueryBody: jest.fn(), + setLastEditedQueryResolution: jest.fn(), + setLastEditedQueryCritical: jest.fn(), + setLastEditedQueryPlatform: jest.fn(), + }, + app: { + currentUser: createMockUser(), + isGlobalObserver: false, + isGlobalAdmin: true, + isGlobalMaintainer: false, + isOnGlobalTeam: true, + isPremiumTier: true, + isSandboxMode: false, + }, + }, + }); + + const { container, user } = render( + + ); + + expect(screen.getByRole("button", { name: "Save" })).toBeDisabled(); + expect(screen.getByRole("button", { name: "Run" })).toBeDisabled(); + + await user.hover(screen.getByRole("button", { name: "Save" })); + + expect( + container.querySelector("#policy-form__button-wrap--tooltip") + ).toHaveTextContent(/to save or run the policy/i); + }); + + // TODO: Consider testing save button is disabled for a sql error + // Trickiness is in modifying react-ace using react-testing library +}); diff --git a/frontend/pages/policies/PolicyPage/components/PolicyForm/PolicyForm.tsx b/frontend/pages/policies/PolicyPage/components/PolicyForm/PolicyForm.tsx index 1f8a9a2948..24fd4ebe9e 100644 --- a/frontend/pages/policies/PolicyPage/components/PolicyForm/PolicyForm.tsx +++ b/frontend/pages/policies/PolicyPage/components/PolicyForm/PolicyForm.tsx @@ -466,7 +466,8 @@ const PolicyForm = ({ ); }; - const renderRunForObserver = ( + // Observers and observer+ of existing query, team role viewing inherited policy + const renderNonEditableForm = (
@@ -499,127 +500,139 @@ const PolicyForm = ({ ); - const renderForGlobalAdminOrAnyMaintainer = ( - <> -
-
-
- {renderName()} - {renderDescription()} - {renderResolution()} + // Admin or maintainer + const renderEditableQueryForm = () => { + // Save disabled for no platforms selected, query name blank on existing query, or sql errors + const disableSaveFormErrors = + (isEditMode && !isAnyPlatformSelected) || + (lastEditedQueryName === "" && !!lastEditedQueryId) || + !!size(errors); + + return ( + <> + +
+
+ {renderName()} + {renderDescription()} + {renderResolution()} +
+
{isEditMode && renderAuthor()}
-
{isEditMode && renderAuthor()}
-
- - - {renderPlatformCompatibility()} - - {(isEditMode || defaultPolicy) && platformSelector.render()} - {isEditMode && isPremiumTier && renderCriticalPolicy()} - {renderLiveQueryWarning()} -
- {hasSavePermissions && ( - <> - - - - - Select the platform(s) this -
- policy will be checked on -
- to save or run the policy. -
- - )} - - + + + {renderPlatformCompatibility()} - - Select the platform(s) this -
- policy will be checked on -
- to save or run the policy. -
-
- - {isSaveNewPolicyModalOpen && ( - - )} - - ); + {(isEditMode || defaultPolicy) && platformSelector.render()} + {isEditMode && isPremiumTier && renderCriticalPolicy()} + {renderLiveQueryWarning()} +
+ {hasSavePermissions && ( + <> + + + + + Select the platform(s) this +
+ policy will be checked on +
+ to save or run the policy. +
+ + )} + + + + + Select the platform(s) this +
+ policy will be checked on +
+ to save or run the policy. +
+
+ + {isSaveNewPolicyModalOpen && ( + + )} + + ); + }; if (isStoredPolicyLoading) { return ; } - if ( + const noEditPermissions = isTeamObserver || isGlobalObserver || - (policyTeamId === 0 && !isOnGlobalTeam) - ) { - return renderRunForObserver; + (policyTeamId === 0 && !isOnGlobalTeam); // Team user viewing inherited policy + + // Render non-editable form only + if (noEditPermissions) { + return renderNonEditableForm; } - return renderForGlobalAdminOrAnyMaintainer; + // Render default editable form + return renderEditableQueryForm(); }; export default PolicyForm; diff --git a/frontend/pages/queries/QueryPage/components/QueryForm/QueryForm.tests.tsx b/frontend/pages/queries/QueryPage/components/QueryForm/QueryForm.tests.tsx new file mode 100644 index 0000000000..87bc29c8fd --- /dev/null +++ b/frontend/pages/queries/QueryPage/components/QueryForm/QueryForm.tests.tsx @@ -0,0 +1,84 @@ +import React from "react"; +import { screen } from "@testing-library/react"; +import { createCustomRenderer } from "test/test-utils"; + +import createMockQuery from "__mocks__/queryMock"; +import createMockUser from "__mocks__/userMock"; + +import QueryForm from "./QueryForm"; + +const mockQuery = createMockQuery(); +const mockRouter = { + push: jest.fn(), + replace: jest.fn(), + goBack: jest.fn(), + goForward: jest.fn(), + go: jest.fn(), + setRouteLeaveHook: jest.fn(), + isActive: jest.fn(), + createHref: jest.fn(), + createPath: jest.fn(), +}; + +describe("QueryForm - component", () => { + it("disables save button for missing query name", async () => { + const render = createCustomRenderer({ + context: { + query: { + lastEditedQueryId: mockQuery.id, + lastEditedQueryName: "", // missing query name + lastEditedQueryDescription: mockQuery.description, + lastEditedQueryBody: mockQuery.query, + lastEditedQueryObserverCanRun: mockQuery.observer_can_run, + lastEditedQueryFrequency: mockQuery.interval, + lastEditedQueryPlatforms: mockQuery.platform, + lastEditedQueryMinOsqueryVersion: mockQuery.min_osquery_version, + lastEditedQueryLoggingType: mockQuery.logging, + setLastEditedQueryName: jest.fn(), + setLastEditedQueryDescription: jest.fn(), + setLastEditedQueryBody: jest.fn(), + setLastEditedQueryObserverCanRun: jest.fn(), + setLastEditedQueryFrequency: jest.fn(), + setLastEditedQueryPlatforms: jest.fn(), + setLastEditedQueryMinOsqueryVersion: jest.fn(), + setLastEditedQueryLoggingType: jest.fn(), + }, + app: { + currentUser: createMockUser(), + isGlobalObserver: false, + isGlobalAdmin: true, + isGlobalMaintainer: false, + isOnGlobalTeam: true, + isPremiumTier: true, + isSandboxMode: false, + }, + }, + }); + + render( + + ); + + expect(screen.getByRole("button", { name: "Save" })).toBeDisabled(); + }); + + // TODO: Consider testing save button is disabled for a sql error + // Trickiness is in modifying react-ace using react-testing library +}); diff --git a/frontend/pages/queries/QueryPage/components/QueryForm/QueryForm.tsx b/frontend/pages/queries/QueryPage/components/QueryForm/QueryForm.tsx index 7b77ad0642..2d22ee251c 100644 --- a/frontend/pages/queries/QueryPage/components/QueryForm/QueryForm.tsx +++ b/frontend/pages/queries/QueryPage/components/QueryForm/QueryForm.tsx @@ -533,6 +533,7 @@ const QueryForm = ({ } labelActionComponent={isObserverPlus && renderLabelComponent()} wrapEnabled + data-testid="ace-editor" /> )} @@ -560,182 +561,187 @@ const QueryForm = ({ const hasSavePermissions = isGlobalAdmin || isGlobalMaintainer; // Global admin, any maintainer, any observer+ on new query - const renderEditableQueryForm = ( - <> -
-
-
- {renderName()} - {renderDescription()} + const renderEditableQueryForm = () => { + // Save disabled for team maintainer/admins viewing global queries + const disableSavePermissionDenied = + isAnyTeamMaintainerOrTeamAdmin && + !storedQuery?.team_id && + !!queryIdForEdit; + + // Save and save as new disabled for query name blank on existing query or sql errors + const disableSaveFormErrors = + (lastEditedQueryName === "" && !!lastEditedQueryId) || !!size(errors); + + return ( + <> + +
+
+ {renderName()} + {renderDescription()} +
+
{savedQueryMode && renderAuthor()}
-
{savedQueryMode && renderAuthor()}
-
- - - {renderPlatformCompatibility()} - - {savedQueryMode && ( -
-
- - If automations are on, this is how often your query collects data. -
-
- - setLastEditedQueryObserverCanRun(value) - } - wrapperClassName={`${baseClass}__query-observer-can-run-wrapper`} - > - Observers can run - -

- Users with the observer role will be able to run this query on - hosts where they have access. -

-
- - {showAdvancedOptions && ( -
+ + + {renderPlatformCompatibility()} + + {savedQueryMode && ( +
+
- - + If automations are on, this is how often your query collects + data.
- )} -
- )} - {renderLiveQueryWarning()} -
- {(hasSavePermissions || isAnyTeamMaintainerOrTeamAdmin) && ( - <> - {savedQueryMode && ( - - )} -
-
+ + setLastEditedQueryObserverCanRun(value) } + wrapperClassName={`${baseClass}__query-observer-can-run-wrapper`} > - -
{" "} - - <> - You can only save changes -
to a team level query. - -
+ Observers can run + +

+ Users with the observer role will be able to run this query on + hosts where they have access. +

- + + {showAdvancedOptions && ( +
+ + + +
+ )} +
)} - -
- - {showSaveQueryModal && ( - - )} - - ); + {(hasSavePermissions || isAnyTeamMaintainerOrTeamAdmin) && ( + <> + {savedQueryMode && ( + + )} +
+
+ +
{" "} + + <> + You can only save changes +
to a team level query. + +
+
+ + )} + +
+ + {showSaveQueryModal && ( + + )} + + ); + }; if (isStoredQueryLoading) { return ; @@ -755,7 +761,7 @@ const QueryForm = ({ } // Render default editable form - return renderEditableQueryForm; + return renderEditableQueryForm(); }; export default QueryForm; diff --git a/frontend/test/test-utils.tsx b/frontend/test/test-utils.tsx index 3409523a3f..6c8cfb61e6 100644 --- a/frontend/test/test-utils.tsx +++ b/frontend/test/test-utils.tsx @@ -9,6 +9,8 @@ import { INotificationContext, NotificationContext, } from "context/notification"; +import { IPolicyContext, PolicyContext } from "context/policy"; +import { IQueryContext, QueryContext } from "context/query"; export const baseUrl = (path: string) => { return `/api/latest/fleet${path}`; @@ -36,6 +38,8 @@ export const renderWithAppContext = ( interface IContextOptions { app?: Partial; notification?: Partial; + policy?: Partial; + query?: Partial; } interface ICustomRenderOptions { @@ -55,6 +59,8 @@ interface ICustomRenderOptions { const CONTEXT_PROVIDER_MAP = { app: AppContext, notification: NotificationContext, + policy: PolicyContext, + query: QueryContext, }; type ContextProviderKeys = keyof typeof CONTEXT_PROVIDER_MAP;